git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`

Commit Message

Patrick Steinhardt Feb. 16, 2024, 8:39 a.m. UTC

The `--trust-exit-code` option for git-diff-tool(1) was introduced via
2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).
When set, it makes us return the exit code of the invoked diff tool when
diffing multiple files. This patch didn't change the code path where
`--dir-diff` was passed because we already returned the exit code of the

Comments

Junio C Hamano Feb. 16, 2024, 6:12 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> The `--trust-exit-code` option for git-diff-tool(1) was introduced via
> 2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).
> When set, it makes us return the exit code of the invoked diff tool when
> diffing multiple files. This patch didn't change the code path where
> `--dir-diff` was passed because we already returned the exit code of the
> diff tool unconditionally in that case.
>
> This was changed a month later via c41d3fedd8 (difftool--helper: add
> explicit exit statement, 2014-11-20), where an explicit `exit 0` was
> added to the end of git-difftool--helper.sh. While the stated intent of
> that commit was merely a cleanup, it had the consequence that we now
> to ignore the exit code of the diff tool when `--dir-diff` was set. This
> change in behaviour is thus very likely an unintended side effect of
> this patch.
>
> Now there are two ways to fix this:
>
>   - We can either restore the original behaviour, which unconditionally
>     returned the exit code of the diffing tool when `--dir-diff` is
>     passed.
>
>   - Or we can make the `--dir-diff` case respect the `--trust-exit-code`
>     flag.
>
> The fact that we have been ignoring exit codes for 7 years by now makes
> me rather lean towards the latter option. Furthermore, respecting the
> flag in one case but not the other would needlessly make the user
> interface more complex.
>
> Fix the bug so that we also honor `--trust-exit-code` for dir diffs and
> adjust the documentation accordingly.
>
> Reported-by: Jean-Rémy Falleri <[e-mail]>
> Signed-off-by: Patrick Steinhardt <[e-mail]>
> ---

Sounds sensible.

The last time David was on list seems to be in April 2023; just in 
case let's CC him for an Ack (or something else).

>  Documentation/git-difftool.txt |  1 -
>  git-difftool--helper.sh        | 14 +++++
>  t/t7800-difftool.sh            | 99 ++++++++++++++++++----------------
>  3 files changed, 68 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index c05f97aca9..a616f8b2e6 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -105,7 +105,6 @@ instead.  `--no-symlinks` is the default on Windows.
>  	`merge.tool` until a tool is found.
>  
>  --[no-]trust-exit-code::
> -	'git-difftool' invokes a diff tool individually on each file.
>  	Errors reported by the diff tool are ignored by default.
>  	Use `--trust-exit-code` to make 'git-difftool' exit when an
>  	invoked diff tool returns a non-zero exit code.
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index e4e820e680..09d8542917 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -91,6 +91,20 @@ then
>  	# ignore the error from the above --- run_merge_tool
>  	# will diagnose unusable tool by itself
>  	run_merge_tool "$merge_tool" false
> +
> +	status=$?
> +	if test $status -ge 126
> +	then
> +		# Command not found (127), not executable (126) or
> +		# exited via a signal (>= 128).
> +		exit $status
> +	fi

So these errors spawning the tool backend are always reported,
regardless of the trust-exit-code settings.  OK.

> +	if test "$status" != 0 &&
> +		test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
> +	then
> +		exit $status
> +	fi

I found this somehow harder to reason about than necessary.  Just

	if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
	then
		exit $status
	fi

would have been a more straight-forward expression of what we want
to happen here, i.e. "if we are told to report the tool's exit
status, we do so, regardless of what the exit status is".

Not that the construct in your patch is wrong---we will exit with 0
at the end even when "trust-exit-code" thing is true and the tool
returned success.

Patrick Steinhardt Feb. 20, 2024, 9:56 a.m. UTC | #2

On Fri, Feb 16, 2024 at 10:12:32AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> writes:
[snip]
> > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> > index e4e820e680..09d8542917 100755
> > --- a/git-difftool--helper.sh
> > +++ b/git-difftool--helper.sh
> > @@ -91,6 +91,20 @@ then
> >  	# ignore the error from the above --- run_merge_tool
> >  	# will diagnose unusable tool by itself
> >  	run_merge_tool "$merge_tool" false
> > +
> > +	status=$?
> > +	if test $status -ge 126
> > +	then
> > +		# Command not found (127), not executable (126) or
> > +		# exited via a signal (>= 128).
> > +		exit $status
> > +	fi
> 
> So these errors spawning the tool backend are always reported,
> regardless of the trust-exit-code settings.  OK.
> 
> > +	if test "$status" != 0 &&
> > +		test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
> > +	then
> > +		exit $status
> > +	fi
> 
> I found this somehow harder to reason about than necessary.  Just
> 
> 	if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
> 	then
> 		exit $status
> 	fi
> 
> would have been a more straight-forward expression of what we want
> to happen here, i.e. "if we are told to report the tool's exit
> status, we do so, regardless of what the exit status is".
> 
> Not that the construct in your patch is wrong---we will exit with 0
> at the end even when "trust-exit-code" thing is true and the tool
> returned success.

Fair point indeed. Looks like I was a bit too lazy here by simply
copying over the construct from the non-dir-diff case. Over there we
need to special case the 0 exit code because we don't want to exit the
loop in that case. But here it's completely unnecessary.

Will adapt, thanks!

Patrick

David Aguilar Feb. 28, 2024, 1:52 a.m. UTC | #3

On Fri, Feb 16, 2024 at 10:12 AM Junio C Hamano <[e-mail]> wrote:
>
> Patrick Steinhardt <[e-mail]> writes:
>
> > The `--trust-exit-code` option for git-diff-tool(1) was introduced via
> > 2b52123fcf (difftool: add support for --trust-exit-code, 2014-10-26).
> > When set, it makes us return the exit code of the invoked diff tool when
> > diffing multiple files. This patch didn't change the code path where
> > `--dir-diff` was passed because we already returned the exit code of the
> > diff tool unconditionally in that case.
> >
> > This was changed a month later via c41d3fedd8 (difftool--helper: add
> > explicit exit statement, 2014-11-20), where an explicit `exit 0` was
> > added to the end of git-difftool--helper.sh. While the stated intent of
> > that commit was merely a cleanup, it had the consequence that we now
> > to ignore the exit code of the diff tool when `--dir-diff` was set. This
> > change in behaviour is thus very likely an unintended side effect of
> > this patch.
> >
> > Now there are two ways to fix this:
> >
> >   - We can either restore the original behaviour, which unconditionally
> >     returned the exit code of the diffing tool when `--dir-diff` is
> >     passed.
> >
> >   - Or we can make the `--dir-diff` case respect the `--trust-exit-code`
> >     flag.
> >
> > The fact that we have been ignoring exit codes for 7 years by now makes
> > me rather lean towards the latter option. Furthermore, respecting the
> > flag in one case but not the other would needlessly make the user
> > interface more complex.
> >
> > Fix the bug so that we also honor `--trust-exit-code` for dir diffs and
> > adjust the documentation accordingly.
> >
> > Reported-by: Jean-Rémy Falleri <[e-mail]>
> > Signed-off-by: Patrick Steinhardt <[e-mail]>
> > ---
>
> Sounds sensible.
>
> The last time David was on list seems to be in April 2023; just in
> case let's CC him for an Ack (or something else).

Thanks! I'm still lurking around.
I've been meaning to make some Git-adjacent announcements and
contributions soon..

Until then:

Acked-by: David Aguilar <[e-mail]>

Junio C Hamano Feb. 28, 2024, 2:15 a.m. UTC | #4

David Aguilar <[e-mail]> writes:

>> The last time David was on list seems to be in April 2023; just in
>> case let's CC him for an Ack (or something else).
>
> Thanks! I'm still lurking around.
> I've been meaning to make some Git-adjacent announcements and
> contributions soon..
>
> Until then:
>
> Acked-by: David Aguilar <[e-mail]>

Thanks.  

It always is nice to hear from old-timer project participants.

Patch

diff tool unconditionally in that case.

This was changed a month later via c41d3fedd8 (difftool--helper: add
explicit exit statement, 2014-11-20), where an explicit `exit 0` was
added to the end of git-difftool--helper.sh. While the stated intent of
that commit was merely a cleanup, it had the consequence that we now
to ignore the exit code of the diff tool when `--dir-diff` was set. This
change in behaviour is thus very likely an unintended side effect of
this patch.

Now there are two ways to fix this:

  - We can either restore the original behaviour, which unconditionally
    returned the exit code of the diffing tool when `--dir-diff` is
    passed.

  - Or we can make the `--dir-diff` case respect the `--trust-exit-code`
    flag.

The fact that we have been ignoring exit codes for 7 years by now makes
me rather lean towards the latter option. Furthermore, respecting the
flag in one case but not the other would needlessly make the user
interface more complex.

Fix the bug so that we also honor `--trust-exit-code` for dir diffs and
adjust the documentation accordingly.

Reported-by: Jean-Rémy Falleri <[e-mail]>
Signed-off-by: Patrick Steinhardt <[e-mail]>
---
 Documentation/git-difftool.txt |  1 -
 git-difftool--helper.sh        | 14 +++++
 t/t7800-difftool.sh            | 99 ++++++++++++++++++----------------
 3 files changed, 68 insertions(+), 46 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index c05f97aca9..a616f8b2e6 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,7 +105,6 @@  instead.  `--no-symlinks` is the default on Windows.
 	`merge.tool` until a tool is found.
 
 --[no-]trust-exit-code::
-	'git-difftool' invokes a diff tool individually on each file.
 	Errors reported by the diff tool are ignored by default.
 	Use `--trust-exit-code` to make 'git-difftool' exit when an
 	invoked diff tool returns a non-zero exit code.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index e4e820e680..09d8542917 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -91,6 +91,20 @@  then
 	# ignore the error from the above --- run_merge_tool
 	# will diagnose unusable tool by itself
 	run_merge_tool "$merge_tool" false
+
+	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
+		exit $status
+	fi
 else
 	# Launch the merge tool on each path provided by 'git diff'
 	while test $# -gt 6
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 6a36be1e63..96ae5d5880 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -91,58 +91,67 @@  test_expect_success 'difftool forwards arguments to diff' '
 	rm for-diff
 '
 
-test_expect_success 'difftool ignores exit code' '
-	test_config difftool.error.cmd false &&
-	git difftool -y -t error branch
-'
+for opt in '' '--dir-diff'
+do
+	test_expect_success "difftool ${opt} ignores exit code" "
+		test_config difftool.error.cmd false &&
+		git difftool ${opt} -y -t error branch
+	"
 
-test_expect_success 'difftool forwards exit code with --trust-exit-code' '
-	test_config difftool.error.cmd false &&
-	test_must_fail git difftool -y --trust-exit-code -t error branch
-'
+	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code" "
+		test_config difftool.error.cmd false &&
+		test_must_fail git difftool ${opt} -y --trust-exit-code -t error branch
+	"
 
-test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
-	test_config difftool.vimdiff.path false &&
-	test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
-'
+	test_expect_success "difftool ${opt} forwards exit code with --trust-exit-code for built-ins" "
+		test_config difftool.vimdiff.path false &&
+		test_must_fail git difftool ${opt} -y --trust-exit-code -t vimdiff branch
+	"
 
-test_expect_success 'difftool honors difftool.trustExitCode = true' '
-	test_config difftool.error.cmd false &&
-	test_config difftool.trustExitCode true &&
-	test_must_fail git difftool -y -t error branch
-'
+	test_expect_success "difftool ${opt} honors difftool.trustExitCode = true" "
+		test_config difftool.error.cmd false &&
+		test_config difftool.trustExitCode true &&
+		test_must_fail git difftool ${opt} -y -t error branch
+	"
 
-test_expect_success 'difftool honors difftool.trustExitCode = false' '
-	test_config difftool.error.cmd false &&
-	test_config difftool.trustExitCode false &&
-	git difftool -y -t error branch
-'
+	test_expect_success "difftool ${opt} honors difftool.trustExitCode = false" "
+		test_config difftool.error.cmd false &&
+		test_config difftool.trustExitCode false &&
+		git difftool ${opt} -y -t error branch
+	"
 
-test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
-	test_config difftool.error.cmd false &&
-	test_config difftool.trustExitCode true &&
-	git difftool -y --no-trust-exit-code -t error branch
-'
+	test_expect_success "difftool ${opt} ignores exit code with --no-trust-exit-code" "
+		test_config difftool.error.cmd false &&
+		test_config difftool.trustExitCode true &&
+		git difftool ${opt} -y --no-trust-exit-code -t error branch
+	"
 
-test_expect_success 'difftool stops on error with --trust-exit-code' '
-	test_when_finished "rm -f for-diff .git/fail-right-file" &&
-	test_when_finished "git reset -- for-diff" &&
-	write_script .git/fail-right-file <<-\EOF &&
-	echo failed
-	exit 1
-	EOF
-	>for-diff &&
-	git add for-diff &&
-	test_must_fail git difftool -y --trust-exit-code \
-		--extcmd .git/fail-right-file branch >actual &&
-	test_line_count = 1 actual
-'
+	test_expect_success "difftool ${opt} stops on error with --trust-exit-code" "
+		test_when_finished 'rm -f for-diff .git/fail-right-file' &&
+		test_when_finished 'git reset -- for-diff' &&
+		write_script .git/fail-right-file <<-\EOF &&
+		echo failed
+		exit 1
+		EOF
+		>for-diff &&
+		git add for-diff &&
+		test_must_fail git difftool ${opt} -y --trust-exit-code \
+			--extcmd .git/fail-right-file branch >actual &&
+		test_line_count = 1 actual
+	"
 
-test_expect_success '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 "difftool ${opt} honors exit status if command not found" "
+		test_config difftool.nonexistent.cmd i-dont-exist &&
+		test_config difftool.trustExitCode false &&
+		if test "${opt}" = '--dir-diff'
+		then
+			expected_code=127
+		else
+			expected_code=128
+		fi &&
+		test_expect_code \${expected_code} git difftool ${opt} -y -t nonexistent branch
+	"
+done
 
 test_expect_success 'difftool honors --gui' '
 	difftool_test_setup &&