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

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

[David Aguilar]

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

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

[Junio C Hamano]

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.

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

[David Aguilar]

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

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

[Mikael Magnusson]

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.

-- 
Mikael Magnusson

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

[David Aguilar]

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

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

[Junio C Hamano]

Mikael Magnusson <mikachu@gmail.com> writes:

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

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

[Aaron Schrab]

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.

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

[Andreas Schwab]

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

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

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

[David Aguilar]

Adri sent me this directly but I think it should have gone to the list.

Adri, if you don't mind, Junio can add:

Tested-by: Adri Farr <14farresa@gmail.com>

...to the commit message trailer since it looks like it's happy.

Thanks for testing!

cheers,
David

----- Forwarded message from Adri Farr <14farresa@gmail.com> -----

Date: Sat, 15 Nov 2014 00:10:12 +0100
From: Adri Farr <14farresa@gmail.com>
To: David Aguilar <davvid@gmail.com>
Subject: Re: [PATCH] difftool: honor --trust-exit-code for builtin tools

I have tested this patch both in vim and meld and it works
wonderfully. Thank you for the time put into this. I should have
provided feedback back when the patch was proposed. I guess it's never
too late :).

2014-11-14 22:57 GMT+01:00 David Aguilar <davvid@gmail.com>:
> [snip]

----- End forwarded message -----

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

[Adri Farr]

Oh, sorry about that. I didn't realize I was directly responding to
you. Apologizes. Hopefully I'm doing it right this time. I don't have
much experience with mailing lists, and Gmail doesn't seem to help.
You don't need to add the 'Tested-by' line. Testing is the least I can
do. If you still want to add that line, my full name is 'Adria
Farres'. Thank you!

2014-11-15 1:27 GMT+01:00 David Aguilar <davvid@gmail.com>:
>
> Adri sent me this directly but I think it should have gone to the list.
>
> Adri, if you don't mind, Junio can add:
>
> Tested-by: Adri Farr <14farresa@gmail.com>
>
> ...to the commit message trailer since it looks like it's happy.
>
> Thanks for testing!
>
> cheers,
> David
>
> ----- Forwarded message from Adri Farr <14farresa@gmail.com> -----
>
> Date: Sat, 15 Nov 2014 00:10:12 +0100
> From: Adri Farr <14farresa@gmail.com>
> To: David Aguilar <davvid@gmail.com>
> Subject: Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
>
> I have tested this patch both in vim and meld and it works
> wonderfully. Thank you for the time put into this. I should have
> provided feedback back when the patch was proposed. I guess it's never
> too late :).
>
> 2014-11-14 22:57 GMT+01:00 David Aguilar <davvid@gmail.com>:
>> [snip]
>
> ----- End forwarded message -----