On Fri, Feb 16, 2024 at 10:12:32AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
