Re: [PATCH 1/3] mergetool--lib: remove use of $status global
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:03:05
David Aguilar [off-list ref] writes:
quoted hunk
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 <redacted> --- 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.
quoted 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.