Re: [PATCH] difftool: always honor "command not found" exit code

Re: [PATCH] difftool: always honor "command not found" exit code

[Tom Tanner (BLOOMBERG/ LONDON)]

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)

----- Original Message -----
From: gitster@pobox.com
To: john@keeping.me.uk
Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org
At: 08/14/16 04:21:18

John Keeping <john@keeping.me.uk> writes:

> At the moment difftool's "trust exit code" logic always suppresses the
> exit status of the diff utility we invoke.  This is useful because we
> don't want to exit just because diff returned "1" because the files
> differ, but it's confusing if the shell returns an error because the
> selected diff utility is not found.
>
> POSIX specifies 127 as the exit status for "command not found" and 126
> for "command found but is not executable" [1] and at least bash and dash
> follow this specification, while diff utilities generally use "1" for
> the exit status we want to ignore.
>
> Handle 126 and 127 as special values, assuming that they always mean
> that the command could not be executed.

Sounds like a reasonable thing to do.  Will queue; thanks.

Re: [PATCH] difftool: always honor "command not found" exit code

[Junio C Hamano]

"Tom Tanner (BLOOMBERG/ LONDON)" <ttanner2@bloomberg.net> writes:

> From: gitster@pobox.com
> To: john@keeping.me.uk
> Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org
> At: 08/14/16 04:21:18
>
> John Keeping <john@keeping.me.uk> writes:
> ...
>> POSIX specifies 127 as the exit status for "command not found" and 126
>> for "command found but is not executable" [1] and at least bash and dash
>> follow this specification, while diff utilities generally use "1" for
>> the exit status we want to ignore.
>>
>> Handle 126 and 127 as special values, assuming that they always mean
>> that the command could not be executed.
>
> Sounds like a reasonable thing to do.  Will queue; thanks.

> 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)

Certainly, it feels safer to notice an unusual exit status code and
error out to force the user to take notice, but that reasoning
assumes that "128 and above" are noteworthy exceptions.

I do not have a strong opinion on that part.

Re: [PATCH] difftool: always honor "command not found" exit code

[John Keeping]

On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> "Tom Tanner (BLOOMBERG/ LONDON)" <ttanner2@bloomberg.net> writes:
> 
> > From: gitster@pobox.com
> > To: john@keeping.me.uk
> > Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org
> > At: 08/14/16 04:21:18
> >
> > John Keeping <john@keeping.me.uk> writes:
> > ...
> >> POSIX specifies 127 as the exit status for "command not found" and 126
> >> for "command found but is not executable" [1] and at least bash and dash
> >> follow this specification, while diff utilities generally use "1" for
> >> the exit status we want to ignore.
> >>
> >> Handle 126 and 127 as special values, assuming that they always mean
> >> that the command could not be executed.
> >
> > Sounds like a reasonable thing to do.  Will queue; thanks.
> 
> > 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)
> 
> Certainly, it feels safer to notice an unusual exit status code and
> error out to force the user to take notice, but that reasoning
> assumes that "128 and above" are noteworthy exceptions.

Reading further in POSIX:

	The exit status of a command that terminated because it received
	a signal shall be reported as greater than 128.

I think if we accept the argument above about diff utilities generally
using low numbers for the status values we're ignoring intentionally,
then we can just treat any value above 125 as a fatal error.

[PATCH v2] difftool: always honor fatal error exit codes

[John Keeping]

At the moment difftool's "trust exit code" logic always suppresses the
exit status of the diff utility we invoke.  This is useful because we
don't want to exit just because diff returned "1" because the files
differ, but it's confusing if the shell returns an error because the
selected diff utility is not found.

POSIX specifies 127 as the exit status for "command not found", 126 for
"command found but is not executable" and values greater than 128 if the
command terminated because it received a signal [1] and at least bash
and dash follow this specification, while diff utilities generally use
"1" for the exit status we want to ignore.

Handle any value of 126 or greater as a special value indicating that
some form of fatal error occurred.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Mon, Aug 15, 2016 at 10:35:26PM +0100, John Keeping wrote:
> On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> > "Tom Tanner (BLOOMBERG/ LONDON)" <ttanner2@bloomberg.net> writes:
> > 
> > > From: gitster@pobox.com
> > > To: john@keeping.me.uk
> > > Cc: Tom Tanner (BLOOMBERG/ LONDON), davvid@gmail.com, git@vger.kernel.org
> > > At: 08/14/16 04:21:18
> > >
> > > John Keeping <john@keeping.me.uk> writes:
> > > ...
> > >> POSIX specifies 127 as the exit status for "command not found" and 126
> > >> for "command found but is not executable" [1] and at least bash and dash
> > >> follow this specification, while diff utilities generally use "1" for
> > >> the exit status we want to ignore.
> > >>
> > >> Handle 126 and 127 as special values, assuming that they always mean
> > >> that the command could not be executed.
> > >
> > > Sounds like a reasonable thing to do.  Will queue; thanks.
> > 
> > > 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)
> > 
> > Certainly, it feels safer to notice an unusual exit status code and
> > error out to force the user to take notice, but that reasoning
> > assumes that "128 and above" are noteworthy exceptions.
> 
> Reading further in POSIX:
> 
> 	The exit status of a command that terminated because it received
> 	a signal shall be reported as greater than 128.
> 
> I think if we accept the argument above about diff utilities generally
> using low numbers for the status values we're ignoring intentionally,
> then we can just treat any value above 125 as a fatal error.

Here's what that looks like.

 git-difftool--helper.sh | 7 +++++++
 t/t7800-difftool.sh     | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 84d6cc0..7bfb673 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -86,6 +86,13 @@ else
 	do
 		launch_merge_tool "$1" "$2" "$5"
 		status=$?
+		if test $status -ge 126
+		then
+			# Command not found (127), not executable (126) or
+			# exited via a signal (>= 128).
+			exit $status
+		fi
+
 		if test "$status" != 0 &&
 			test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
 		then
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2974900..70a2de4 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
 	test_cmp expect actual
 '
 
+test_expect_success PERL 'difftool honors exit status if command not found' '
+	test_config difftool.nonexistent.cmd i-dont-exist &&
+	test_config difftool.trustExitCode false &&
+	test_must_fail git difftool -y -t nonexistent branch
+'
+
 test_expect_success PERL 'difftool honors --gui' '
 	difftool_test_setup &&
 	test_config merge.tool bogus-tool &&
-- 
2.9.3.728.g30b24b4

Re: [PATCH v2] difftool: always honor fatal error exit codes

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Here's what that looks like.

Sounds good.  It feels a bit funny to see that new mentions of
$status are unquoted (which is totally valid because we know it has
$? that cannot be anything other than a short decimal integer),
while the one in the post-context quotes it, but that's not a huge
issue.

Will queue.  Thanks.

>  git-difftool--helper.sh | 7 +++++++
>  t/t7800-difftool.sh     | 6 ++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 84d6cc0..7bfb673 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -86,6 +86,13 @@ else
>  	do
>  		launch_merge_tool "$1" "$2" "$5"
>  		status=$?
> +		if test $status -ge 126
> +		then
> +			# Command not found (127), not executable (126) or
> +			# exited via a signal (>= 128).
> +			exit $status
> +		fi
> +
>  		if test "$status" != 0 &&
>  			test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
>  		then
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 2974900..70a2de4 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success PERL 'difftool honors exit status if command not found' '
> +	test_config difftool.nonexistent.cmd i-dont-exist &&
> +	test_config difftool.trustExitCode false &&
> +	test_must_fail git difftool -y -t nonexistent branch
> +'
> +
>  test_expect_success PERL 'difftool honors --gui' '
>  	difftool_test_setup &&
>  	test_config merge.tool bogus-tool &&

git difftool and git mergetool aren't returning errors when the tool has issues

[Tom Tanner (BLOOMBERG/ LONDON)]

For instance, if you set your diff/mergetool to meld and you don't have it installed:
> git difftool

Viewing (1/1): 'blah'
Launch 'meld' [Y/n]? y
/home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> echo $?
0

> /home/ttanner/bin/meld
/home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> echo $?
127

or
> env DISPLAY=wibble git difftool

Viewing (1/1): blah'
Launch 'xxdiff' [Y/n]? y
xxdiff: cannot connect to X server wibble
> echo $?
0
> env DISPLAY=wibble xxdiff thisfile thatfile
xxdiff: cannot connect to X server wibble
> echo $?
1
and this one (with DISPLAY unset because it was being run elsewhere)
> git mergtool
Normal merge conflict for 'blah: 
{local}: modified file 
{remote}: modified file 
Could not find ':' in DISPLAY: yeah, right 
XIO: fatal IO error 73 (Connection reset by peer) on X server "server:66.0" 
after 0 requests (0 known processed) with 0 events remaining. 
> echo $?
0
which last looked like a successful merge to a script, because it returned 0.

Re: git difftool and git mergetool aren't returning errors when the tool has issues

[John Keeping]

On Fri, Aug 12, 2016 at 07:13:41AM -0000, Tom Tanner (BLOOMBERG/ LONDON) wrote:
> For instance, if you set your diff/mergetool to meld and you don't have it installed:
> > git difftool
> 
> Viewing (1/1): 'blah'
> Launch 'meld' [Y/n]? y
> /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> > echo $?
> 0
> 
> > /home/ttanner/bin/meld
> /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> > echo $?
> 127

Have you looked at the --trust-exit-code option to git-difftool?

It would be nice if there was a way to differentiate between complete
failure and just the diff tool exiting with a non-zero return status
because the files differ, but I'm not sure whether we can do that
reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
run [1] so it may be sensible to to treat those as special values.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

[PATCH] difftool: always honor "command not found" exit code

[John Keeping]

At the moment difftool's "trust exit code" logic always suppresses the
exit status of the diff utility we invoke.  This is useful because we
don't want to exit just because diff returned "1" because the files
differ, but it's confusing if the shell returns an error because the
selected diff utility is not found.

POSIX specifies 127 as the exit status for "command not found" and 126
for "command found but is not executable" [1] and at least bash and dash
follow this specification, while diff utilities generally use "1" for
the exit status we want to ignore.

Handle 126 and 127 as special values, assuming that they always mean
that the command could not be executed.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Sat, Aug 13, 2016 at 11:36:39AM +0100, John Keeping wrote:
> It would be nice if there was a way to differentiate between complete
> failure and just the diff tool exiting with a non-zero return status
> because the files differ, but I'm not sure whether we can do that
> reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
> run [1] so it may be sensible to to treat those as special values.

Something like this perhaps?  I think this is probably safe, but it's
always possible that some diff utility does use 126 or 127 as a "normal"
exit status.  I'm not sure what we can do about that other than add a
"really, really don't trust the exit status" option!

 git-difftool--helper.sh | 18 ++++++++++++++----
 t/t7800-difftool.sh     |  6 ++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 84d6cc0..68877d4 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -86,11 +86,21 @@ else
 	do
 		launch_merge_tool "$1" "$2" "$5"
 		status=$?
-		if test "$status" != 0 &&
-			test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
-		then
+		case "$status" in
+		0)
+			: OK
+			;;
+		126|127)
+			# Command not found or not executable
 			exit $status
-		fi
+			;;
+		*)
+			if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
+			then
+				exit $status
+			fi
+			;;
+		esac
 		shift 7
 	done
 fi
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2974900..70a2de4 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
 	test_cmp expect actual
 '
 
+test_expect_success PERL 'difftool honors exit status if command not found' '
+	test_config difftool.nonexistent.cmd i-dont-exist &&
+	test_config difftool.trustExitCode false &&
+	test_must_fail git difftool -y -t nonexistent branch
+'
+
 test_expect_success PERL 'difftool honors --gui' '
 	difftool_test_setup &&
 	test_config merge.tool bogus-tool &&
-- 
2.9.2.639.g855ae9f

Re: [PATCH] difftool: always honor "command not found" exit code

[David Aguilar]

On Sat, Aug 13, 2016 at 12:30:28PM +0100, John Keeping wrote:
> At the moment difftool's "trust exit code" logic always suppresses the
> exit status of the diff utility we invoke.  This is useful because we
> don't want to exit just because diff returned "1" because the files
> differ, but it's confusing if the shell returns an error because the
> selected diff utility is not found.
> 
> POSIX specifies 127 as the exit status for "command not found" and 126
> for "command found but is not executable" [1] and at least bash and dash
> follow this specification, while diff utilities generally use "1" for
> the exit status we want to ignore.
> 
> Handle 126 and 127 as special values, assuming that they always mean
> that the command could not be executed.
> 
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
> 
> Signed-off-by: John Keeping <john@keeping.me.uk>

Looks good to me, thanks.

Acked-by: David Aguilar <davvid@gmail.com>

> ---
> On Sat, Aug 13, 2016 at 11:36:39AM +0100, John Keeping wrote:
> > It would be nice if there was a way to differentiate between complete
> > failure and just the diff tool exiting with a non-zero return status
> > because the files differ, but I'm not sure whether we can do that
> > reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
> > run [1] so it may be sensible to to treat those as special values.
> 
> Something like this perhaps?  I think this is probably safe, but it's
> always possible that some diff utility does use 126 or 127 as a "normal"
> exit status.  I'm not sure what we can do about that oaaaather than add a
> "really, really don't trust the exit status" option!


We can always add a mechanism for tool-specific error codes
later if we ever end up needing it, but this seems sufficient.

cheers,
-- 
David

Re: [PATCH] difftool: always honor "command not found" exit code

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> At the moment difftool's "trust exit code" logic always suppresses the
> exit status of the diff utility we invoke.  This is useful because we
> don't want to exit just because diff returned "1" because the files
> differ, but it's confusing if the shell returns an error because the
> selected diff utility is not found.
>
> POSIX specifies 127 as the exit status for "command not found" and 126
> for "command found but is not executable" [1] and at least bash and dash
> follow this specification, while diff utilities generally use "1" for
> the exit status we want to ignore.
>
> Handle 126 and 127 as special values, assuming that they always mean
> that the command could not be executed.

Sounds like a reasonable thing to do.  Will queue; thanks.