Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 30, 2008, 01:28 UTC
- Message-ID
- <7vwsmg2ks7.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <alpine.DEB.1.00.0804300155320.17469@eeepc-johanness>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 38 quoted lines
>> > 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.