On Fri, Feb 16, 2024 at 10:12:32AM -0800, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

[snip]

quoted

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.

quoted

+	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