{"thread":{"id":"43850","subject":"Re: [PATCH] difftool: always honor \"command not found\" exit code","startedAt":"2016-08-15T10:38:28Z","lastAt":"2016-08-15T22:26:56Z","messageCount":5,"participants":["Tom Tanner (BLOOMBERG/ LONDON)","Junio C Hamano","John Keeping"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"299323","messageId":"57B19B9F0205070000390238_0_29566@p057","threadId":"43850","inReplyTo":null,"subject":"Re: [PATCH] difftool: always honor \"command not found\" exit code","fromName":"Tom Tanner (BLOOMBERG/ LONDON)","fromEmail":"ttanner2@bloomberg.net","sentAt":null,"receivedAt":"2016-08-15T10:38:28Z","isPatch":true,"sender":{"key":"ttanner2@bloomberg.net","avatar":null},"body":"Would it be possible to also treat signals (128 and above) as 'special' values as well (as I've seen some merge tools self destruct like that from time to time)\n\n----- Original Message -----\nFrom: gitster@pobox.com\nTo: john@keeping.me.uk\nCc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org\nAt: 08/14/16 04:21:18\n\nJohn Keeping <john@keeping.me.uk> writes:\n\n> At the moment difftool's \"trust exit code\" logic always suppresses the\n> exit status of the diff utility we invoke.  This is useful because we\n> don't want to exit just because diff returned \"1\" because the files\n> differ, but it's confusing if the shell returns an error because the\n> selected diff utility is not found.\n>\n> POSIX specifies 127 as the exit status for \"command not found\" and 126\n> for \"command found but is not executable\" [1] and at least bash and dash\n> follow this specification, while diff utilities generally use \"1\" for\n> the exit status we want to ignore.\n>\n> Handle 126 and 127 as special values, assuming that they always mean\n> that the command could not be executed.\n\nSounds like a reasonable thing to do.  Will queue; thanks.\n\n"},{"id":"299389","messageId":"xmqqa8gdhjgd.fsf@gitster.mtv.corp.google.com","threadId":"43850","inReplyTo":"57B19B9F0205070000390238_0_29566@p057","subject":"Re: [PATCH] difftool: always honor \"command not found\" exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T20:21:22Z","receivedAt":"2016-08-15T20:21:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tom Tanner (BLOOMBERG/ LONDON)\" <ttanner2@bloomberg.net> writes:\n\n> From: gitster@pobox.com\n> To: john@keeping.me.uk\n> Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org\n> At: 08/14/16 04:21:18\n>\n> John Keeping <john@keeping.me.uk> writes:\n> ...\n>> POSIX specifies 127 as the exit status for \"command not found\" and 126\n>> for \"command found but is not executable\" [1] and at least bash and dash\n>> follow this specification, while diff utilities generally use \"1\" for\n>> the exit status we want to ignore.\n>>\n>> Handle 126 and 127 as special values, assuming that they always mean\n>> that the command could not be executed.\n>\n> Sounds like a reasonable thing to do.  Will queue; thanks.\n\n> Would it be possible to also treat signals (128 and above) as\n> 'special' values as well (as I've seen some merge tools self\n> destruct like that from time to time)\n\nCertainly, it feels safer to notice an unusual exit status code and\nerror out to force the user to take notice, but that reasoning\nassumes that \"128 and above\" are noteworthy exceptions.\n\nI do not have a strong opinion on that part.\n"},{"id":"299403","messageId":"20160815213526.6m7gu2v3fhtmx6wj@john.keeping.me.uk","threadId":"43850","inReplyTo":"xmqqa8gdhjgd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool: always honor \"command not found\" exit code","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-08-15T21:35:26Z","receivedAt":"2016-08-15T21:35:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:\n> \"Tom Tanner (BLOOMBERG/ LONDON)\" <ttanner2@bloomberg.net> writes:\n> \n> > From: gitster@pobox.com\n> > To: john@keeping.me.uk\n> > Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org\n> > At: 08/14/16 04:21:18\n> >\n> > John Keeping <john@keeping.me.uk> writes:\n> > ...\n> >> POSIX specifies 127 as the exit status for \"command not found\" and 126\n> >> for \"command found but is not executable\" [1] and at least bash and dash\n> >> follow this specification, while diff utilities generally use \"1\" for\n> >> the exit status we want to ignore.\n> >>\n> >> Handle 126 and 127 as special values, assuming that they always mean\n> >> that the command could not be executed.\n> >\n> > Sounds like a reasonable thing to do.  Will queue; thanks.\n> \n> > Would it be possible to also treat signals (128 and above) as\n> > 'special' values as well (as I've seen some merge tools self\n> > destruct like that from time to time)\n> \n> Certainly, it feels safer to notice an unusual exit status code and\n> error out to force the user to take notice, but that reasoning\n> assumes that \"128 and above\" are noteworthy exceptions.\n\nReading further in POSIX:\n\n\tThe exit status of a command that terminated because it received\n\ta signal shall be reported as greater than 128.\n\nI think if we accept the argument above about diff utilities generally\nusing low numbers for the status values we're ignoring intentionally,\nthen we can just treat any value above 125 as a fatal error.\n"},{"id":"299414","messageId":"20160815215439.kwt4jmjrtcufjmih@john.keeping.me.uk","threadId":"43850","inReplyTo":"20160815213526.6m7gu2v3fhtmx6wj@john.keeping.me.uk","subject":"[PATCH v2] difftool: always honor fatal error exit codes","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-08-15T21:54:39Z","receivedAt":"2016-08-15T21:54:50Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"At the moment difftool's \"trust exit code\" logic always suppresses the\nexit status of the diff utility we invoke.  This is useful because we\ndon't want to exit just because diff returned \"1\" because the files\ndiffer, but it's confusing if the shell returns an error because the\nselected diff utility is not found.\n\nPOSIX specifies 127 as the exit status for \"command not found\", 126 for\n\"command found but is not executable\" and values greater than 128 if the\ncommand terminated because it received a signal [1] and at least bash\nand dash follow this specification, while diff utilities generally use\n\"1\" for the exit status we want to ignore.\n\nHandle any value of 126 or greater as a special value indicating that\nsome form of fatal error occurred.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\nOn Mon, Aug 15, 2016 at 10:35:26PM +0100, John Keeping wrote:\n> On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:\n> > \"Tom Tanner (BLOOMBERG/ LONDON)\" <ttanner2@bloomberg.net> writes:\n> > \n> > > From: gitster@pobox.com\n> > > To: john@keeping.me.uk\n> > > Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org\n> > > At: 08/14/16 04:21:18\n> > >\n> > > John Keeping <john@keeping.me.uk> writes:\n> > > ...\n> > >> POSIX specifies 127 as the exit status for \"command not found\" and 126\n> > >> for \"command found but is not executable\" [1] and at least bash and dash\n> > >> follow this specification, while diff utilities generally use \"1\" for\n> > >> the exit status we want to ignore.\n> > >>\n> > >> Handle 126 and 127 as special values, assuming that they always mean\n> > >> that the command could not be executed.\n> > >\n> > > Sounds like a reasonable thing to do.  Will queue; thanks.\n> > \n> > > Would it be possible to also treat signals (128 and above) as\n> > > 'special' values as well (as I've seen some merge tools self\n> > > destruct like that from time to time)\n> > \n> > Certainly, it feels safer to notice an unusual exit status code and\n> > error out to force the user to take notice, but that reasoning\n> > assumes that \"128 and above\" are noteworthy exceptions.\n> \n> Reading further in POSIX:\n> \n> \tThe exit status of a command that terminated because it received\n> \ta signal shall be reported as greater than 128.\n> \n> I think if we accept the argument above about diff utilities generally\n> using low numbers for the status values we're ignoring intentionally,\n> then we can just treat any value above 125 as a fatal error.\n\nHere's what that looks like.\n\n git-difftool--helper.sh | 7 +++++++\n t/t7800-difftool.sh     | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 84d6cc0..7bfb673 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -86,6 +86,13 @@ else\n \tdo\n \t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n \t\tstatus=$?\n+\t\tif test $status -ge 126\n+\t\tthen\n+\t\t\t# Command not found (127), not executable (126) or\n+\t\t\t# exited via a signal (>= 128).\n+\t\t\texit $status\n+\t\tfi\n+\n \t\tif test \"$status\" != 0 &&\n \t\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n \t\tthen\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 2974900..70a2de4 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success PERL 'difftool honors exit status if command not found' '\n+\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n+\ttest_config difftool.trustExitCode false &&\n+\ttest_must_fail git difftool -y -t nonexistent branch\n+'\n+\n test_expect_success PERL 'difftool honors --gui' '\n \tdifftool_test_setup &&\n \ttest_config merge.tool bogus-tool &&\n-- \n2.9.3.728.g30b24b4\n\n"},{"id":"299416","messageId":"xmqq4m6lfz35.fsf@gitster.mtv.corp.google.com","threadId":"43850","inReplyTo":"20160815215439.kwt4jmjrtcufjmih@john.keeping.me.uk","subject":"Re: [PATCH v2] difftool: always honor fatal error exit codes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T22:26:38Z","receivedAt":"2016-08-15T22:26:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Here's what that looks like.\n\nSounds good.  It feels a bit funny to see that new mentions of\n$status are unquoted (which is totally valid because we know it has\n$? that cannot be anything other than a short decimal integer),\nwhile the one in the post-context quotes it, but that's not a huge\nissue.\n\nWill queue.  Thanks.\n\n>  git-difftool--helper.sh | 7 +++++++\n>  t/t7800-difftool.sh     | 6 ++++++\n>  2 files changed, 13 insertions(+)\n>\n> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> index 84d6cc0..7bfb673 100755\n> --- a/git-difftool--helper.sh\n> +++ b/git-difftool--helper.sh\n> @@ -86,6 +86,13 @@ else\n>  \tdo\n>  \t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n>  \t\tstatus=$?\n> +\t\tif test $status -ge 126\n> +\t\tthen\n> +\t\t\t# Command not found (127), not executable (126) or\n> +\t\t\t# exited via a signal (>= 128).\n> +\t\t\texit $status\n> +\t\tfi\n> +\n>  \t\tif test \"$status\" != 0 &&\n>  \t\t\ttest \"$GIT_DIFFTOOL_TRUST_EXIT_CODE\" = true\n>  \t\tthen\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 2974900..70a2de4 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success PERL 'difftool honors exit status if command not found' '\n> +\ttest_config difftool.nonexistent.cmd i-dont-exist &&\n> +\ttest_config difftool.trustExitCode false &&\n> +\ttest_must_fail git difftool -y -t nonexistent branch\n> +'\n> +\n>  test_expect_success PERL 'difftool honors --gui' '\n>  \tdifftool_test_setup &&\n>  \ttest_config merge.tool bogus-tool &&\n"}]}