Hi,
On Tue, 29 Apr 2008, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
Fair enough.
Ciao,
Dscho
--
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
