On Sun, Nov 16, 2014 at 02:51:11AM +0100, Mikael Magnusson wrote:
> On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano <gitster@xxxxxxxxx> 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.
>
> $status is an alias for $? in zsh, and so cannot be assigned to. But
> other than that I don't think it holds any meaning and should be fine
> in a .sh script.
Thanks for the heads-up ~ this is even more reason to cleanup
the script a bit.
If we still need a local variable for it in a few places then I'll
call it $rc instead, but it'll only be used for local things
rather than its current global usage.
--
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