bug: git-diff silently fails when run outside of a repository (v1.5.4.2)

> "Mike Coleman" <tutufan@gmail.com> writes:
>
>  > At least in version 1.5.4.2, git-diff silently fails when not run
>  > inside a repository.  It should give an error diagnostic, especially
>  > since "no output" would otherwise be a meaningful response.
>
>  Unfortunately this does not have enough information to go by, as unlike
>  many other programs, "git diff" contains a hack to be usable as a better
>  (for certain definition of "better" I may not necessarily agree with) GNU
>  diff replacement when run outside a repository.
>
>  i.e.
>
>         mkdir -p /var/tmp/junk
>         cd /var/tmp/junk
>         rm -fr .git ;# make sure it is not a repository
>         echo >a hello
>         echo >b world
>         git diff --color a b
>
>  is supposed to work.
>

> On Tue, Apr 29, 2008 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> "Mike Coleman" <tutufan@gmail.com> writes:
>>
>>  > At least in version 1.5.4.2, git-diff silently fails when not run
>>  > inside a repository.  It should give an error diagnostic, especially
>>  > since "no output" would otherwise be a meaningful response.
>>
>>  Unfortunately this does not have enough information to go by, as unlike
>>  many other programs, "git diff" contains a hack to be usable as a better
>>  (for certain definition of "better" I may not necessarily agree with) GNU
>>  diff replacement when run outside a repository.
>>
>>  i.e.
>>
>>         mkdir -p /var/tmp/junk
>>         cd /var/tmp/junk
>>         rm -fr .git ;# make sure it is not a repository
>>         echo >a hello
>>         echo >b world
>>         git diff --color a b
>>
>>  is supposed to work.

> Oh, I didn't realize that.  It doesn't seem to be mentioned on the man
> page, though I can't necessarily claim that I would have seen it if it
> had.
>
> Even so, this seems like a bug.  If I do this:
>
>     $ cd /
>     $ git-diff
>
> there is no error message and no error status.  A diagnostic would be
> very helpful.

> "Mike Coleman" <tutufan@gmail.com> writes:
> 
> > On Tue, Apr 29, 2008 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> "Mike Coleman" <tutufan@gmail.com> writes:
> >>
> >>  > At least in version 1.5.4.2, git-diff silently fails when not run
> >>  > inside a repository.  It should give an error diagnostic, especially
> >>  > since "no output" would otherwise be a meaningful response.
> >>
> >>  Unfortunately this does not have enough information to go by, as unlike
> >>  many other programs, "git diff" contains a hack to be usable as a better
> >>  (for certain definition of "better" I may not necessarily agree with) GNU
> >>  diff replacement when run outside a repository.
> >>
> >>  i.e.
> >>
> >>         mkdir -p /var/tmp/junk
> >>         cd /var/tmp/junk
> >>         rm -fr .git ;# make sure it is not a repository
> >>         echo >a hello
> >>         echo >b world
> >>         git diff --color a b
> >>
> >>  is supposed to work.
> 
> > Oh, I didn't realize that.  It doesn't seem to be mentioned on the man
> > page, though I can't necessarily claim that I would have seen it if it
> > had.
> >
> > Even so, this seems like a bug.  If I do this:
> >
> >     $ cd /
> >     $ git-diff
> >
> > there is no error message and no error status.  A diagnostic would be
> > very helpful.
> 
> Ah, that indeed is not very helpful.
> 
> Unfortunately, every time I look at this hack, I seem to find an unrelated
> bug in it.  Here is today's.
> 
> 	$ for i in 1 2 3; do >/var/tmp/$i; done
>         $ cd /
>         $ git diff /var/tmp/1
>         Segmentation Fault
> 
> When nongit is true, we know the user has to be asking --no-index diff, so
> perhaps we can fix it by doing something like this?
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index 069e450..cfd629d 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,
>  			DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
>  			break;
>  		}
> +	if (nongit && argc != i + 2)
> +		die("git diff [--no-index] takes two paths");
> +
>  	if (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&
>  				!is_outside_repo(argv[i], nongit, prefix)))
>  		return -1;

>> > Even so, this seems like a bug.  If I do this:
>> >
>> >     $ cd /
>> >     $ git-diff
>> >
>> > there is no error message and no error status.  A diagnostic would be
>> > very helpful.
>> 
>> Ah, that indeed is not very helpful.
>> 
>> Unfortunately, every time I look at this hack, I seem to find an unrelated
>> bug in it.  Here is today's.
>> 
>> 	$ for i in 1 2 3; do >/var/tmp/$i; done
>>         $ cd /
>>         $ git diff /var/tmp/1
>>         Segmentation Fault
>> 
>> When nongit is true, we know the user has to be asking --no-index diff, so
>> perhaps we can fix it by doing something like this?
>> 
>> diff --git a/diff-lib.c b/diff-lib.c
>> index 069e450..cfd629d 100644
>> --- a/diff-lib.c
>> +++ b/diff-lib.c
>> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,
>>  			DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
>>  			break;
>>  		}
>> +	if (nongit && argc != i + 2)
>> +		die("git diff [--no-index] takes two paths");
>> +
>>  	if (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&
>>  				!is_outside_repo(argv[i], nongit, prefix)))
>>  		return -1;
>
> That looks to me as if the second if() should have triggered, and the 
> caller of setup_diff_no_index() should have errored out.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > Even so, this seems like a bug.  If I do this:
> >> >
> >> >     $ cd /
> >> >     $ git-diff
> >> >
> >> > there is no error message and no error status.  A diagnostic would be
> >> > very helpful.
> >> 
> >> Ah, that indeed is not very helpful.
> >> 
> >> Unfortunately, every time I look at this hack, I seem to find an unrelated
> >> bug in it.  Here is today's.
> >> 
> >> 	$ for i in 1 2 3; do >/var/tmp/$i; done
> >>         $ cd /
> >>         $ git diff /var/tmp/1
> >>         Segmentation Fault
> >> 
> >> When nongit is true, we know the user has to be asking --no-index diff, so
> >> perhaps we can fix it by doing something like this?
> >> 
> >> diff --git a/diff-lib.c b/diff-lib.c
> >> index 069e450..cfd629d 100644
> >> --- a/diff-lib.c
> >> +++ b/diff-lib.c
> >> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,
> >>  			DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
> >>  			break;
> >>  		}
> >> +	if (nongit && argc != i + 2)
> >> +		die("git diff [--no-index] takes two paths");
> >> +
> >>  	if (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&
> >>  				!is_outside_repo(argv[i], nongit, prefix)))
> >>  		return -1;
> >
> > That looks to me as if the second if() should have triggered, and the 
> > caller of setup_diff_no_index() should have errored out.
> 
> I think the above three-liner fix is something we should have done when we
> added --no-index codepath.  Before the --no-index hack was introduced, we
> did not even got this far to the place the caller of this function is, if
> we are outside a repository.  By returning -1 from here instead of dying,
> this code is driving the codepath that has always expected to already be
> in a repository into a nonrepository, causing them to segfault because
> there is no git-dir or work-tree set up done yet as they expect.