threads / patch / 37969

patchdifftool: honor --trust-exit-code for builtin tools

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

## tl;dr

8 messages between Nov 14, 2014 and Nov 17, 2014. Diffs are folded; open one to read it.

replies: 7people: 5as markdown or json

David Aguilar· Nov 14, 2014, 21:33 UTC · lore

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(+)
Show changes to 2 files +6 −0

git-mergetool--lib.sh, t/t7800-difftool.sh

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
 }
 
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 69bde7a..ea35a02 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -86,6 +86,11 @@ test_expect_success PERL 'difftool forwards exit code with --trust-exit-code' '
 	test_must_fail git difftool -y --trust-exit-code -t error branch
 '
 
+test_expect_success PERL 'difftool forwards exit code with --trust-exit-code for built-ins' '
+	test_config difftool.vimdiff.path false &&
+	test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
+'
+
 test_expect_success PERL 'difftool honors difftool.trustExitCode = true' '
 	test_config difftool.error.cmd false &&
 	test_config difftool.trustExitCode true &&

-- 
2.2.0.rc1.23.gf570943.dirty
Junio C Hamano· Nov 14, 2014, 21:51 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 23 quoted lines
> 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.

David Aguilar· Nov 14, 2014, 21:57 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 14, 2014 at 01:51:39PM -0800, Junio C Hamano wrote:
Show 66 quoted lines
> 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
Mikael Magnusson· Nov 16, 2014, 01:51 UTC · re: Junio C Hamano · lore

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

On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> 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.

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

-- 
Mikael Magnusson
David Aguilar· Nov 16, 2014, 02:36 UTC · re: Mikael Magnusson · lore

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

On Sun, Nov 16, 2014 at 02:51:11AM +0100, Mikael Magnusson wrote:
Show 34 quoted lines
> On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano <gitster@pobox.com> 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.
> 
> $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
Junio C Hamano· Nov 16, 2014, 18:11 UTC · re: Mikael Magnusson · lore

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

Mikael Magnusson <mikachu@gmail.com> writes:
Show 19 quoted lines
>>> 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.
That is not what I meant by "global ... significance".

The question was if the codepath in the caller depends on this setting the global variable here, or nobody looks at and depends on the global variable we are setting here after this function returns.

It does not have any significance that a random shell implementation is not POSIX compliant. That would merely mean that such a shell cannot be used to run POSIX shell scripts like our Porcelain. I would suspect that zsh has more "posixly correct" mode, with which it _can_ run POSIX shell scripts, and I would imagine that this "$status is an alias $?" business is disabled in that mode?

My quick glance across the codepaths in the callers of this funciton indicated that it should be safe not using this global variable, so my answer to my original question was "no there is no significance". I think we can safely remove any mention of status from this shell function, i.e. if we remove initial assignment to 0, remove this new assignment and then remove the "return $status" at the end, the caller would still be happy.

Aaron Schrab· Nov 17, 2014, 22:15 UTC · re: Junio C Hamano · lore

Re: difftool: honor --trust-exit-code for builtin tools

At 10:11 -0800 16 Nov 2014, Junio C Hamano <gitster@pobox.com> wrote:
>It does not have any significance that a random shell implementation
>is not POSIX compliant.  That would merely mean that such a shell
>cannot be used to run POSIX shell scripts like our Porcelain.

Right, and I suspect that it's very rare for zsh to be used as /bin/sh. I've heard of people doing it just to see what would fail, but not of anybody doing that for regular use.

>I would suspect that zsh has more "posixly correct" mode, with which
>it _can_ run POSIX shell scripts, and I would imagine that this 
>"$status is an alias $?" business is disabled in that mode?

Yes, if zsh is invoked as either "sh" or "ksh" it attempts to emulate the usual semantics of the named shell. One of the differences is that $status isn't special in the emulation modes.

Andreas Schwab· Nov 16, 2014, 08:18 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
Show 22 quoted lines
> 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

If you want to return the last exit status at the end of a function you don't need any return at all.

Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

← back to recent threads