Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
David Aguilar dav...@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 14farr...@gmail.com
Signed-off-by: David Aguilar dav...@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, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
Mikael Magnusson mika...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
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 dav...@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 14farr...@gmail.com ...to the commit message trailer since it looks like it's happy. Thanks for testing! cheers, David - Forwarded message from Adri Farr 14farr...@gmail.com - Date: Sat, 15 Nov 2014 00:10:12 +0100 From: Adri Farr 14farr...@gmail.com To: David Aguilar dav...@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 dav...@gmail.com: [snip] - End forwarded message - -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano gits...@pobox.com wrote:
David Aguilar dav...@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 14farr...@gmail.com
Signed-off-by: David Aguilar dav...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
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 gits...@pobox.com wrote:
David Aguilar dav...@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 14farr...@gmail.com
Signed-off-by: David Aguilar dav...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
David Aguilar dav...@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 14farr...@gmail.com
Signed-off-by: David Aguilar dav...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
On Fri, Nov 14, 2014 at 01:51:39PM -0800, Junio C Hamano wrote:
David Aguilar dav...@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 14farr...@gmail.com
Signed-off-by: David Aguilar dav...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] difftool: honor --trust-exit-code for builtin tools
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 14farr...@gmail.com ...to the commit message trailer since it looks like it's happy. Thanks for testing! cheers, David - Forwarded message from Adri Farr 14farr...@gmail.com - Date: Sat, 15 Nov 2014 00:10:12 +0100 From: Adri Farr 14farr...@gmail.com To: David Aguilar dav...@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 dav...@gmail.com: [snip] - End forwarded message - -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
