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

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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.

Hi,

On Tue, 29 Apr 2008, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

quoted

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