* bug: git-diff silently fails when run outside of a repository (v1.5.4.2) @ 2008-04-29 20:04 Mike Coleman 2008-04-29 22:53 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Mike Coleman @ 2008-04-29 20:04 UTC (permalink / raw) To: git 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. I think there are other git programs that have this problem as well. Mike ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-29 20:04 bug: git-diff silently fails when run outside of a repository (v1.5.4.2) Mike Coleman @ 2008-04-29 22:53 ` Junio C Hamano 2008-04-29 23:03 ` Mike Coleman 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2008-04-29 22:53 UTC (permalink / raw) To: Mike Coleman; +Cc: git "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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-29 22:53 ` Junio C Hamano @ 2008-04-29 23:03 ` Mike Coleman 2008-04-29 23:37 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Mike Coleman @ 2008-04-29 23:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-29 23:03 ` Mike Coleman @ 2008-04-29 23:37 ` Junio C Hamano 2008-04-30 0:56 ` Johannes Schindelin 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2008-04-29 23:37 UTC (permalink / raw) To: Mike Coleman; +Cc: Johannes Schindelin, git "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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-29 23:37 ` Junio C Hamano @ 2008-04-30 0:56 ` Johannes Schindelin 2008-04-30 1:28 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Johannes Schindelin @ 2008-04-30 0:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Mike Coleman, git Hi, On Tue, 29 Apr 2008, Junio C Hamano wrote: > "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; That looks to me as if the second if() should have triggered, and the caller of setup_diff_no_index() should have errored out. Ciao, Dscho "who has too many issues with git-submodule right now" ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-30 0:56 ` Johannes Schindelin @ 2008-04-30 1:28 ` Junio C Hamano 2008-04-30 8:34 ` Johannes Schindelin 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2008-04-30 1:28 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Mike Coleman, git 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2) 2008-04-30 1:28 ` Junio C Hamano @ 2008-04-30 8:34 ` Johannes Schindelin 0 siblings, 0 replies; 7+ messages in thread From: Johannes Schindelin @ 2008-04-30 8:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Mike Coleman, git Hi, On Tue, 29 Apr 2008, Junio C Hamano wrote: > 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. Fair enough. Ciao, Dscho ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-04-30 8:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-29 20:04 bug: git-diff silently fails when run outside of a repository (v1.5.4.2) Mike Coleman 2008-04-29 22:53 ` Junio C Hamano 2008-04-29 23:03 ` Mike Coleman 2008-04-29 23:37 ` Junio C Hamano 2008-04-30 0:56 ` Johannes Schindelin 2008-04-30 1:28 ` Junio C Hamano 2008-04-30 8:34 ` Johannes Schindelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).