Re: [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Adam Spiers <git@xxxxxxxxxxxxxx> writes:

> Fix a corner case where check-ignore would segfault when run with the
> '.' argument from the top level of a repository, due to prefix_path()
> converting '.' into the empty string.  It doesn't make much sense to
> call check-ignore from the top level with '.' as a parameter, since
> the top-level directory would never typically be ignored, but of
> course it should not segfault in this case.
>
> Signed-off-by: Adam Spiers <git@xxxxxxxxxxxxxx>
> ---

Please step back a bit and explain why the original had check for
path[0] in the first place?

If the answer is "the code wanted to special case the question 'is
the top-level excluded?', but used a wrong variable to implement the
check, and this patch is a fix to that", then the proposed commit
log message looks incomplete.  The cause of the segv is not that
prefix_path() returns an empty string, but because the function
called inside the "if" block was written without expecting to be fed
the path that refers to the top-level of the working tree, no?

While this change certainly will prevent the "check the top-level"
request to last-exclude-matching-path, I have to wonder if it is a
good idea to force the caller of the l-e-m-p function to even care.

In other words, would it be a cleaner approach to fix the l-e-m-p
function so that the caller can ask "check the top-level" and give a
sensible answer (perhaps the answer may be "nothing matches"), and
remove the "&& path[0]" (or "&& full_path[0]") special case from
this call site?

The last sentence "It doesn't make much sense..." in the proposed
log message would become a good justification for such a special
case at the beginning of l-e-m-p function, I would think.

>  builtin/check-ignore.c | 2 +-
>  t/t0008-ignores.sh     | 5 +++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
> index 709535c..b0dd7c2 100644
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)
>  					? strlen(prefix) : 0, path);
>  		full_path = check_path_for_gitlink(full_path);
>  		die_if_path_beyond_symlink(full_path, prefix);
> -		if (!seen[i] && path[0]) {
> +		if (!seen[i] && full_path[0]) {
>  			exclude = last_exclude_matching_path(&check, full_path,
>  							     -1, &dtype);
>  			if (exclude) {
> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> index ebe7c70..9c1bde1 100755
> --- a/t/t0008-ignores.sh
> +++ b/t/t0008-ignores.sh
> @@ -138,6 +138,7 @@ test_expect_success 'setup' '
>  	cat <<-\EOF >.gitignore &&
>  		one
>  		ignored-*
> +		top-level-dir/
>  	EOF
>  	for dir in . a
>  	do
> @@ -177,6 +178,10 @@ test_expect_success 'setup' '
>  #
>  # test invalid inputs
>  
> +test_expect_success_multi '. corner-case' '' '
> +	test_check_ignore . 1
> +'
> +
>  test_expect_success_multi 'empty command line' '' '
>  	test_check_ignore "" 128 &&
>  	stderr_contains "fatal: no path specified"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]