Re: [PATCH] git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 20, 2024, 09:56 UTC
Message-ID: <ZdR3UlDf-Y_pokd0@tanuki>
In-Reply-To: <xmqqttm8i8hb.fsf@gitster.g>

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

> Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 40 quoted lines

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