threads / patch / 38021

patch, 3 partsmergetool/difftool cleanup

Subject: [PATCH 0/3] mergetool/difftool cleanup

## tl;dr

8 messages between Nov 21, 2014 and Nov 22, 2014. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

David Aguilar· Nov 21, 2014, 01:20 UTC · lore

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(-)
-- 
2.2.0.rc2.26.g3e3388f
David Aguilar· Nov 21, 2014, 01:20 UTC · re: David Aguilar · lore

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

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(-)
Show changes to git-mergetool--lib.sh +5 −15
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
@@ -119,8 +119,6 @@ setup_user_tool () {
 
 	diff_cmd () {
 		( eval $merge_tool_cmd )
-		status=$?
-		return $status
 	}
 
 	merge_cmd () {
@@ -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
 	}
 }
 
@@ -153,13 +148,11 @@ setup_tool () {
 	}
 
 	diff_cmd () {
-		status=1
-		return $status
+		return 1
 	}
 
 	merge_cmd () {
-		status=1
-		return $status
+		return 1
 	}
 
 	translate_merge_tool_path () {
@@ -210,7 +203,6 @@ run_merge_tool () {
 
 	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
-	status=0
 
 	# Bring tool-specific functions into scope
 	setup_tool "$1" || return 1
@@ -221,8 +213,6 @@ run_merge_tool () {
 	else
 		run_diff_cmd "$1"
 	fi
-	status=$?
-	return $status
 }
 
 # Run a either a configured or built-in diff tool

-- 
2.2.0.rc2.26.g3e3388f
Junio C Hamano· Nov 21, 2014, 19:16 UTC · re: David Aguilar · lore

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

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.

David Aguilar· Nov 21, 2014, 01:20 UTC · re: David Aguilar · lore

[PATCH 2/3] difftool--helper: add explicit exit statement

git-difftool--helper returns a zero exit status unless --trust-exit-code is in effect. Add an explicit exit statement to make this clearer.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool--helper.sh | 2 ++
 1 file changed, 2 insertions(+)
Show changes to git-difftool--helper.sh +2 −0
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index d4fb6df..2b11b1d 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -94,3 +94,5 @@ else
 		shift 7
 	done
 fi
+
+exit 0

-- 
2.2.0.rc2.26.g3e3388f
David Aguilar· Nov 21, 2014, 01:20 UTC · re: David Aguilar · lore

[PATCH 3/3] mergetool: simplify conditionals

Combine the $last_status checks into a single conditional. Replace $last_status and $rollup_status with a single variable.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)
Show changes to git-mergetool.sh +5 −11
diff --git a/git-mergetool.sh b/git-mergetool.sh
index ff050e5..d20581c 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -426,8 +426,6 @@ fi
 merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
 
-last_status=0
-rollup_status=0
 files=
 
 if test $# -eq 0
@@ -455,19 +453,15 @@ printf "%s\n" "$files"
 
 IFS='
 '
+rc=0
 for i in $files
 do
-	if test $last_status -ne 0
-	then
-		prompt_after_failed_merge || exit 1
-	fi
 	printf "\n"
-	merge_file "$i"
-	last_status=$?
-	if test $last_status -ne 0
+	if ! merge_file "$i"
 	then
-		rollup_status=1
+		rc=1
+		prompt_after_failed_merge || exit 1
 	fi
 done
 
-exit $rollup_status
+exit $rc

-- 
2.2.0.rc2.26.g3e3388f
David Aguilar· Nov 21, 2014, 09:03 UTC · re: David Aguilar · lore

[PATCH] mergetools: stop setting $status in merge_cmd()

No callers rely on $status so there's don't need to set it during merge_cmd() for diffmerge, emerge, and kdiff3.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is based on the mergetool/difftool cleanup patches.
 mergetools/diffmerge | 1 -
 mergetools/emerge    | 1 -
 mergetools/kdiff3    | 1 -
 3 files changed, 3 deletions(-)
Show changes to 3 files +0 −3

mergetools/diffmerge, mergetools/emerge, mergetools/kdiff3

diff --git a/mergetools/diffmerge b/mergetools/diffmerge
index 85ac720..f138cb4 100644
--- a/mergetools/diffmerge
+++ b/mergetools/diffmerge
@@ -11,5 +11,4 @@ merge_cmd () {
 		"$merge_tool_path" --merge \
 			--result="$MERGED" "$LOCAL" "$REMOTE"
 	fi
-	status=$?
 }
diff --git a/mergetools/emerge b/mergetools/emerge
index f96d9e5..7b895fd 100644
--- a/mergetools/emerge
+++ b/mergetools/emerge
@@ -15,7 +15,6 @@ merge_cmd () {
 			"$LOCAL" "$REMOTE" \
 			"$(basename "$MERGED")"
 	fi
-	status=$?
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
index a30034f..793d129 100644
--- a/mergetools/kdiff3
+++ b/mergetools/kdiff3
@@ -20,5 +20,4 @@ merge_cmd () {
 			-o "$MERGED" "$LOCAL" "$REMOTE" \
 		>/dev/null 2>&1
 	fi
-	status=$?
 }

-- 
2.2.0.rc2.26.g6d3471d.dirty
Junio C Hamano· Nov 21, 2014, 19:28 UTC · re: David Aguilar · lore

Re: [PATCH 0/3] mergetool/difftool cleanup

David Aguilar <davvid@gmail.com> writes:
Show 15 quoted lines
> 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(-)
Show changes to git-mergetool--lib.sh +0 −1
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
David Aguilar· Nov 22, 2014, 00:20 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] mergetool/difftool cleanup

On Fri, Nov 21, 2014 at 11:28:03AM -0800, Junio C Hamano wrote:
Show 23 quoted lines
> 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.

That makes a lot of sense, please do insert this patch before 1/3 (actually 1/4 with the additional patch I sent after the initial submission).

Thanks, David

Show 36 quoted lines
> 
> -- >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
>

← back to recent threads