Re: [PATCH 1/3] mergetool--lib: remove use of $status global

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2014, 19:16 UTC
Message-ID: <xmqqsihcz7mm.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416532829-68662-2-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 32 quoted lines

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

Show 13 quoted lines

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