On Fri, Nov 14, 2014 at 01:51:39PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html