Re: [PATCH] difftool: honor --trust-exit-code for builtin tools

[David Aguilar <davvid@gmail.com>]

On Fri, Nov 14, 2014 at 01:51:39PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > run_merge_tool() was not setting $status, which prevented the
> > exit code for builtin tools from being forwarded to the caller.
> >
> > Capture the exit status and add a test to guarantee the behavior.
> >
> > Reported-by: Adria Farres <14farresa@gmail.com>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> >  git-mergetool--lib.sh | 1 +
> >  t/t7800-difftool.sh   | 5 +++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> > index a40d3df..2b66351 100644
> > --- a/git-mergetool--lib.sh
> > +++ b/git-mergetool--lib.sh
> > @@ -221,6 +221,7 @@ run_merge_tool () {
> >  	else
> >  		run_diff_cmd "$1"
> >  	fi
> > +	status=$?
> >  	return $status
> >  }
> 
> Thanks for a quick turn-around.  As a hot-fix for what is already in
> -rc I am fine with this fix but the patch makes me wonder if $status
> as a global shell variable has any significance.
> 
> I see that this shell function in its early part does this:
> 
> 	status=0
>         setup_tool "$1" || return 1
> 
> which means that the caller of this function, instead of checking
> what is returned as the return value of the function like:
> 
> 	if run_merge_tool ...
>         then
> 		...
> 
> relies on the value of $status in its later part of the code like:
> 
> 	run_merge_tool ...
> 	...
> 	if test "$status" = 0
> 	then
> 		...
> 
> then we are already in trouble.  And the latter form, if we had such
> a flow in the code, is simply a bad taste.
> 
> A cleaner fix might be to get rid of the extra $status variable from
> this function and let the function return the result of its last
> command, either run_merge_cmd or run_diff_cmd, by either explicitly
> having "return $?" at the end, or not having that "return $status"
> line.  But that relies on us not having any caller that relies on
> the $status carried as a global variable around, so it will be more
> work to convince ourselves that such a fix is correctly done.  From
> my cursory look, what I suggested above should be safe and correct,
> but I do not want to risk an unnecessary and silly breakage this
> late in the cycle.
> 
> So I'll queue this patch as-is for upcoming 2.2, but I think we
> would want to revisit this issue after the release is done.


Thanks for the sug, I totally agree with that.
I'll put a $status audit/rework for mergetool+difftool on my
todo list.

cheers,
-- 
David