{"thread":{"id":"13321","subject":"bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","startedAt":"2008-04-29T20:04:32Z","lastAt":"2008-04-30T08:34:51Z","messageCount":7,"participants":["Mike Coleman","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"75565","messageId":"3c6c07c20804291304n36976417wf3c2a13303aa3133@mail.gmail.com","threadId":"13321","inReplyTo":null,"subject":"bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Mike Coleman","fromEmail":"tutufan@gmail.com","sentAt":"2008-04-29T20:04:32Z","receivedAt":"2008-04-29T20:04:32Z","isPatch":false,"sender":{"key":"tutufan@gmail.com","avatar":null},"body":"At least in version 1.5.4.2, git-diff silently fails when not run\ninside a repository.  It should give an error diagnostic, especially\nsince \"no output\" would otherwise be a meaningful response.\n\nI think there are other git programs that have this problem as well.\n\nMike\n"},{"id":"75604","messageId":"7vabjc5l3r.fsf@gitster.siamese.dyndns.org","threadId":"13321","inReplyTo":"3c6c07c20804291304n36976417wf3c2a13303aa3133@mail.gmail.com","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-29T22:53:28Z","receivedAt":"2008-04-29T22:53:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mike Coleman\" <tutufan@gmail.com> writes:\n\n> At least in version 1.5.4.2, git-diff silently fails when not run\n> inside a repository.  It should give an error diagnostic, especially\n> since \"no output\" would otherwise be a meaningful response.\n\nUnfortunately this does not have enough information to go by, as unlike\nmany other programs, \"git diff\" contains a hack to be usable as a better\n(for certain definition of \"better\" I may not necessarily agree with) GNU\ndiff replacement when run outside a repository.\n\ni.e.\n\n\tmkdir -p /var/tmp/junk\n        cd /var/tmp/junk\n        rm -fr .git ;# make sure it is not a repository\n\techo >a hello\n        echo >b world\n\tgit diff --color a b\n\nis supposed to work.\n"},{"id":"75607","messageId":"3c6c07c20804291603q4fbe957eq3e3da39d4a2e29c0@mail.gmail.com","threadId":"13321","inReplyTo":"7vabjc5l3r.fsf@gitster.siamese.dyndns.org","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Mike Coleman","fromEmail":"tutufan@gmail.com","sentAt":"2008-04-29T23:03:08Z","receivedAt":"2008-04-29T23:03:08Z","isPatch":false,"sender":{"key":"tutufan@gmail.com","avatar":null},"body":"Oh, I didn't realize that.  It doesn't seem to be mentioned on the man\npage, though I can't necessarily claim that I would have seen it if it\nhad.\n\nEven so, this seems like a bug.  If I do this:\n\n    $ cd /\n    $ git-diff\n\nthere is no error message and no error status.  A diagnostic would be\nvery helpful.\n\nMike\n\n\n\nOn Tue, Apr 29, 2008 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Mike Coleman\" <tutufan@gmail.com> writes:\n>\n>  > At least in version 1.5.4.2, git-diff silently fails when not run\n>  > inside a repository.  It should give an error diagnostic, especially\n>  > since \"no output\" would otherwise be a meaningful response.\n>\n>  Unfortunately this does not have enough information to go by, as unlike\n>  many other programs, \"git diff\" contains a hack to be usable as a better\n>  (for certain definition of \"better\" I may not necessarily agree with) GNU\n>  diff replacement when run outside a repository.\n>\n>  i.e.\n>\n>         mkdir -p /var/tmp/junk\n>         cd /var/tmp/junk\n>         rm -fr .git ;# make sure it is not a repository\n>         echo >a hello\n>         echo >b world\n>         git diff --color a b\n>\n>  is supposed to work.\n>\n"},{"id":"75614","messageId":"7vskx444h5.fsf@gitster.siamese.dyndns.org","threadId":"13321","inReplyTo":"3c6c07c20804291603q4fbe957eq3e3da39d4a2e29c0@mail.gmail.com","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-29T23:37:58Z","receivedAt":"2008-04-29T23:37:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mike Coleman\" <tutufan@gmail.com> writes:\n\n> On Tue, Apr 29, 2008 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> \"Mike Coleman\" <tutufan@gmail.com> writes:\n>>\n>>  > At least in version 1.5.4.2, git-diff silently fails when not run\n>>  > inside a repository.  It should give an error diagnostic, especially\n>>  > since \"no output\" would otherwise be a meaningful response.\n>>\n>>  Unfortunately this does not have enough information to go by, as unlike\n>>  many other programs, \"git diff\" contains a hack to be usable as a better\n>>  (for certain definition of \"better\" I may not necessarily agree with) GNU\n>>  diff replacement when run outside a repository.\n>>\n>>  i.e.\n>>\n>>         mkdir -p /var/tmp/junk\n>>         cd /var/tmp/junk\n>>         rm -fr .git ;# make sure it is not a repository\n>>         echo >a hello\n>>         echo >b world\n>>         git diff --color a b\n>>\n>>  is supposed to work.\n\n> Oh, I didn't realize that.  It doesn't seem to be mentioned on the man\n> page, though I can't necessarily claim that I would have seen it if it\n> had.\n>\n> Even so, this seems like a bug.  If I do this:\n>\n>     $ cd /\n>     $ git-diff\n>\n> there is no error message and no error status.  A diagnostic would be\n> very helpful.\n\nAh, that indeed is not very helpful.\n\nUnfortunately, every time I look at this hack, I seem to find an unrelated\nbug in it.  Here is today's.\n\n\t$ for i in 1 2 3; do >/var/tmp/$i; done\n        $ cd /\n        $ git diff /var/tmp/1\n        Segmentation Fault\n\nWhen nongit is true, we know the user has to be asking --no-index diff, so\nperhaps we can fix it by doing something like this?\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 069e450..cfd629d 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,\n \t\t\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n \t\t\tbreak;\n \t\t}\n+\tif (nongit && argc != i + 2)\n+\t\tdie(\"git diff [--no-index] takes two paths\");\n+\n \tif (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&\n \t\t\t\t!is_outside_repo(argv[i], nongit, prefix)))\n \t\treturn -1;\n"},{"id":"75619","messageId":"alpine.DEB.1.00.0804300155320.17469@eeepc-johanness","threadId":"13321","inReplyTo":"7vskx444h5.fsf@gitster.siamese.dyndns.org","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-04-30T00:56:39Z","receivedAt":"2008-04-30T00:56:39Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 29 Apr 2008, Junio C Hamano wrote:\n\n> \"Mike Coleman\" <tutufan@gmail.com> writes:\n> \n> > On Tue, Apr 29, 2008 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> \"Mike Coleman\" <tutufan@gmail.com> writes:\n> >>\n> >>  > At least in version 1.5.4.2, git-diff silently fails when not run\n> >>  > inside a repository.  It should give an error diagnostic, especially\n> >>  > since \"no output\" would otherwise be a meaningful response.\n> >>\n> >>  Unfortunately this does not have enough information to go by, as unlike\n> >>  many other programs, \"git diff\" contains a hack to be usable as a better\n> >>  (for certain definition of \"better\" I may not necessarily agree with) GNU\n> >>  diff replacement when run outside a repository.\n> >>\n> >>  i.e.\n> >>\n> >>         mkdir -p /var/tmp/junk\n> >>         cd /var/tmp/junk\n> >>         rm -fr .git ;# make sure it is not a repository\n> >>         echo >a hello\n> >>         echo >b world\n> >>         git diff --color a b\n> >>\n> >>  is supposed to work.\n> \n> > Oh, I didn't realize that.  It doesn't seem to be mentioned on the man\n> > page, though I can't necessarily claim that I would have seen it if it\n> > had.\n> >\n> > Even so, this seems like a bug.  If I do this:\n> >\n> >     $ cd /\n> >     $ git-diff\n> >\n> > there is no error message and no error status.  A diagnostic would be\n> > very helpful.\n> \n> Ah, that indeed is not very helpful.\n> \n> Unfortunately, every time I look at this hack, I seem to find an unrelated\n> bug in it.  Here is today's.\n> \n> \t$ for i in 1 2 3; do >/var/tmp/$i; done\n>         $ cd /\n>         $ git diff /var/tmp/1\n>         Segmentation Fault\n> \n> When nongit is true, we know the user has to be asking --no-index diff, so\n> perhaps we can fix it by doing something like this?\n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index 069e450..cfd629d 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,\n>  \t\t\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n>  \t\t\tbreak;\n>  \t\t}\n> +\tif (nongit && argc != i + 2)\n> +\t\tdie(\"git diff [--no-index] takes two paths\");\n> +\n>  \tif (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&\n>  \t\t\t\t!is_outside_repo(argv[i], nongit, prefix)))\n>  \t\treturn -1;\n\nThat looks to me as if the second if() should have triggered, and the \ncaller of setup_diff_no_index() should have errored out.\n\nCiao,\nDscho \"who has too many issues with git-submodule right now\"\n"},{"id":"75622","messageId":"7vwsmg2ks7.fsf@gitster.siamese.dyndns.org","threadId":"13321","inReplyTo":"alpine.DEB.1.00.0804300155320.17469@eeepc-johanness","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-30T01:28:40Z","receivedAt":"2008-04-30T01:28:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> > Even so, this seems like a bug.  If I do this:\n>> >\n>> >     $ cd /\n>> >     $ git-diff\n>> >\n>> > there is no error message and no error status.  A diagnostic would be\n>> > very helpful.\n>> \n>> Ah, that indeed is not very helpful.\n>> \n>> Unfortunately, every time I look at this hack, I seem to find an unrelated\n>> bug in it.  Here is today's.\n>> \n>> \t$ for i in 1 2 3; do >/var/tmp/$i; done\n>>         $ cd /\n>>         $ git diff /var/tmp/1\n>>         Segmentation Fault\n>> \n>> When nongit is true, we know the user has to be asking --no-index diff, so\n>> perhaps we can fix it by doing something like this?\n>> \n>> diff --git a/diff-lib.c b/diff-lib.c\n>> index 069e450..cfd629d 100644\n>> --- a/diff-lib.c\n>> +++ b/diff-lib.c\n>> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,\n>>  \t\t\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n>>  \t\t\tbreak;\n>>  \t\t}\n>> +\tif (nongit && argc != i + 2)\n>> +\t\tdie(\"git diff [--no-index] takes two paths\");\n>> +\n>>  \tif (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&\n>>  \t\t\t\t!is_outside_repo(argv[i], nongit, prefix)))\n>>  \t\treturn -1;\n>\n> That looks to me as if the second if() should have triggered, and the \n> caller of setup_diff_no_index() should have errored out.\n\nI think the above three-liner fix is something we should have done when we\nadded --no-index codepath.  Before the --no-index hack was introduced, we\ndid not even got this far to the place the caller of this function is, if\nwe are outside a repository.  By returning -1 from here instead of dying,\nthis code is driving the codepath that has always expected to already be\nin a repository into a nonrepository, causing them to segfault because\nthere is no git-dir or work-tree set up done yet as they expect.\n"},{"id":"75652","messageId":"alpine.DEB.1.00.0804300933570.17469@eeepc-johanness","threadId":"13321","inReplyTo":"7vwsmg2ks7.fsf@gitster.siamese.dyndns.org","subject":"Re: bug: git-diff silently fails when run outside of a repository (v1.5.4.2)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-04-30T08:34:51Z","receivedAt":"2008-04-30T08:34:51Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 29 Apr 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> > Even so, this seems like a bug.  If I do this:\n> >> >\n> >> >     $ cd /\n> >> >     $ git-diff\n> >> >\n> >> > there is no error message and no error status.  A diagnostic would be\n> >> > very helpful.\n> >> \n> >> Ah, that indeed is not very helpful.\n> >> \n> >> Unfortunately, every time I look at this hack, I seem to find an unrelated\n> >> bug in it.  Here is today's.\n> >> \n> >> \t$ for i in 1 2 3; do >/var/tmp/$i; done\n> >>         $ cd /\n> >>         $ git diff /var/tmp/1\n> >>         Segmentation Fault\n> >> \n> >> When nongit is true, we know the user has to be asking --no-index diff, so\n> >> perhaps we can fix it by doing something like this?\n> >> \n> >> diff --git a/diff-lib.c b/diff-lib.c\n> >> index 069e450..cfd629d 100644\n> >> --- a/diff-lib.c\n> >> +++ b/diff-lib.c\n> >> @@ -264,6 +264,9 @@ int setup_diff_no_index(struct rev_info *revs,\n> >>  \t\t\tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n> >>  \t\t\tbreak;\n> >>  \t\t}\n> >> +\tif (nongit && argc != i + 2)\n> >> +\t\tdie(\"git diff [--no-index] takes two paths\");\n> >> +\n> >>  \tif (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&\n> >>  \t\t\t\t!is_outside_repo(argv[i], nongit, prefix)))\n> >>  \t\treturn -1;\n> >\n> > That looks to me as if the second if() should have triggered, and the \n> > caller of setup_diff_no_index() should have errored out.\n> \n> I think the above three-liner fix is something we should have done when we\n> added --no-index codepath.  Before the --no-index hack was introduced, we\n> did not even got this far to the place the caller of this function is, if\n> we are outside a repository.  By returning -1 from here instead of dying,\n> this code is driving the codepath that has always expected to already be\n> in a repository into a nonrepository, causing them to segfault because\n> there is no git-dir or work-tree set up done yet as they expect.\n\nFair enough.\n\nCiao,\nDscho\n"}]}