From: Junio C Hamano <gitster@pobox•com>
To: David Aguilar <davvid@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 0/3] mergetool/difftool cleanup
Date: Fri, 21 Nov 2014 11:28:03 -0800 [thread overview]
Message-ID: <xmqqmw7kz73w.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416532829-68662-1-git-send-email-davvid@gmail.com> (David Aguilar's message of "Thu, 20 Nov 2014 17:20:26 -0800")
David Aguilar <davvid@gmail•com> writes:
> This is a cleanup series to remove the use of the $status
> global variable in mergetool/difftool.
>
> This should wait until after the current RC series is
> over but figured I'd send it out.
>
> David Aguilar (3):
> mergetool--lib: remove use of $status global
> difftool--helper: add explicit exit statement
> mergetool: simplify conditionals
>
> git-difftool--helper.sh | 2 ++
> git-mergetool--lib.sh | 20 +++++---------------
> git-mergetool.sh | 16 +++++-----------
> 3 files changed, 12 insertions(+), 26 deletions(-)
Looked quite straight-forward from a cursory read.
I tentatively inserted the attached patch before 1/3. If the series
was done with that extra step as preliminary clean-up, I wouldn't
have had to wonder if the hunk at "@@ -130,13" was correct.
-- >8 --
From: Junio C Hamano <gitster@pobox•com>
Date: Fri, 21 Nov 2014 11:17:57 -0800
Subject: [PATCH] mergetool--lib: remove no-op assignment to $status from setup_user_tool
Even though setup_user_tool assigns the exit status from "eval
$merge_tool_cmd" to $status, the variable is overwritten by the
function it calls next, check_unchanged, without ever getting looked
at by anybody. And "return $status" at the end of this function
returns the value check_unchanged assigned to it (which is the same
as the value the function returns). Which makes the assignment a
no-op.
Remove it.
Signed-off-by: Junio C Hamano <gitster@pobox•com>
---
git-mergetool--lib.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 2b66351..3e06389 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -130,7 +130,6 @@ setup_user_tool () {
then
touch "$BACKUP"
( eval $merge_tool_cmd )
- status=$?
check_unchanged
else
( eval $merge_tool_cmd )
--
2.2.0-rc2-128-ge2b5e8e
next prev parent reply other threads:[~2014-11-21 19:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-21 1:20 [PATCH 0/3] mergetool/difftool cleanup David Aguilar
2014-11-21 1:20 ` [PATCH 1/3] mergetool--lib: remove use of $status global David Aguilar
2014-11-21 19:16 ` Junio C Hamano
2014-11-21 1:20 ` [PATCH 2/3] difftool--helper: add explicit exit statement David Aguilar
2014-11-21 1:20 ` [PATCH 3/3] mergetool: simplify conditionals David Aguilar
2014-11-21 9:03 ` [PATCH] mergetools: stop setting $status in merge_cmd() David Aguilar
2014-11-21 19:28 ` Junio C Hamano [this message]
2014-11-22 0:20 ` [PATCH 0/3] mergetool/difftool cleanup David Aguilar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqmw7kz73w.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=davvid@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox