public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: David Aguilar <davvid@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/3] mergetool--lib: remove use of $status global
Date: Fri, 21 Nov 2014 11:16:49 -0800	[thread overview]
Message-ID: <xmqqsihcz7mm.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1416532829-68662-2-git-send-email-davvid@gmail.com> (David Aguilar's message of "Thu, 20 Nov 2014 17:20:27 -0800")

David Aguilar <davvid@gmail•com> writes:

> Remove return statements and rework check_unchanged() so that the exit
> status from the last evaluated expression bubbles up to the callers.
>
> Signed-off-by: David Aguilar <davvid@gmail•com>
> ---
>  git-mergetool--lib.sh | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 2b66351..fe61e89 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -92,7 +92,7 @@ translate_merge_tool_path () {
>  check_unchanged () {
>  	if test "$MERGED" -nt "$BACKUP"
>  	then
> -		status=0
> +		return 0
>  	else
>  		while true
>  		do
> @@ -100,8 +100,8 @@ check_unchanged () {
>  			printf "Was the merge successful? [y/n] "
>  			read answer || return 1
>  			case "$answer" in
> -			y*|Y*) status=0; break ;;
> -			n*|N*) status=1; break ;;
> +			y*|Y*) return 0 ;;
> +			n*|N*) return 1 ;;
>  			esac
>  		done
>  	fi

Note: The above left in the response not because I have any comment
on or objection to it but because it is relevant to the comment on
the next hunk.

> @@ -130,13 +128,10 @@ setup_user_tool () {
>  		then
>  			touch "$BACKUP"
>  			( eval $merge_tool_cmd )
> -			status=$?
>  			check_unchanged
>  		else
>  			( eval $merge_tool_cmd )
> -			status=$?
>  		fi
> -		return $status
>  	}
>  }

The caller of this funciton used to get the status from running
$merge_tool_cmd, but now it gets the result from check_unchanged.

Maybe that is more correct thing to report, but this does change the
behaviour, no?

    ... goes and looks ...

Ahh, the assignment to $status before running check_unchanged was not
doing anything useful, because check_unchanged stomped on $status
before it returned anyway.

So the net effect of this hunk to the caller's is unchanged.  It is
a bit tricky but the end result looks correct.

next prev parent reply	other threads:[~2014-11-21 19:17 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 [this message]
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 ` [PATCH 0/3] mergetool/difftool cleanup Junio C Hamano
2014-11-22  0:20   ` 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=xmqqsihcz7mm.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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox