Re: [PATCH v2 1/4] mergetool--lib: Simplify...

Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:55:57

David Aguilar [off-list ref] writes:

On Tue, Jan 29, 2013 at 11:22 AM, John Keeping [off-list ref] wrote:

quoted

On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:

quoted

Update variable assignments to always use $(command "$arg")
in their RHS instead of "$(command "$arg")" as the latter
is harder to read.  Make get_merge_tool_cmd() simpler by
avoiding "echo" and $(command) substitutions completely.

Signed-off-by: David Aguilar <redacted>
---

@@ -300,9 +292,9 @@ get_merge_tool_path () {
      fi
      if test -z "$merge_tool_path"
      then
-             merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
+             merge_tool_path=$(translate_merge_tool_path "$merge_tool")
      fi
-     if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
+     if test -z $(get_merge_tool_cmd "$merge_tool") &&

This change should be reverted to avoid calling "test -z" without any
other arguments, as Johannes pointed out in v1.

The rest of this patch looks good to me.

You're right.  My eyes have probably been staring at it too long and I
missed this (even though I thought I had checked).

By now you (and people who were following this thread) are beginning
to see why I said "I'd feel safer with extra dq" ;-)

I'll amend locally and push the result out.

[PATCH v3 1/4] mergetool--lib: simplify command expressions

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:57

Update variable assignments to always use $(command "$arg")
in their RHS instead of "$(command "$arg")" as the latter
is harder to read.  Make get_merge_tool_cmd() simpler by
avoiding "echo" and $(command) substitutions completely.

Signed-off-by: David Aguilar <redacted>
---
This is a replacement patch for what's currently in pu to
fix the empty "test -z" expression.

 git-mergetool--lib.sh | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1d0fb12..1ff6d38 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh

@@ -32,17 +32,10 @@ check_unchanged () {
 	fi
 }
 
-valid_tool_config () {
-	if test -n "$(get_merge_tool_cmd "$1")"
-	then
-		return 0
-	else
-		return 1
-	fi
-}
-
 valid_tool () {
-	setup_tool "$1" || valid_tool_config "$1"
+	setup_tool "$1" && return 0
+	cmd=$(get_merge_tool_cmd "$1")
+	test -n "$cmd"
 }
 
 setup_tool () {

@@ -96,14 +89,13 @@ setup_tool () {
 }
 
 get_merge_tool_cmd () {
-	# Prints the custom command for a merge tool
 	merge_tool="$1"
 	if diff_mode
 	then
-		echo "$(git config difftool.$merge_tool.cmd ||
-			git config mergetool.$merge_tool.cmd)"
+		git config "difftool.$merge_tool.cmd" ||
+		git config "mergetool.$merge_tool.cmd"
 	else
-		echo "$(git config mergetool.$merge_tool.cmd)"
+		git config "mergetool.$merge_tool.cmd"
 	fi
 }

@@ -114,7 +106,7 @@ run_merge_tool () {
 	GIT_PREFIX=${GIT_PREFIX:-.}
 	export GIT_PREFIX
 
-	merge_tool_path="$(get_merge_tool_path "$1")" || exit
+	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 	status=0

@@ -145,7 +137,7 @@ run_merge_tool () {
 
 # Run a either a configured or built-in diff tool
 run_diff_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	merge_tool_cmd=$(get_merge_tool_cmd "$1")
 	if test -n "$merge_tool_cmd"
 	then
 		( eval $merge_tool_cmd )

@@ -158,11 +150,11 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	merge_tool_cmd=$(get_merge_tool_cmd "$1")
 	if test -n "$merge_tool_cmd"
 	then
-		trust_exit_code="$(git config --bool \
-			mergetool."$1".trustExitCode || echo false)"
+		trust_exit_code=$(git config --bool \
+			"mergetool.$1.trustExitCode" || echo false)
 		if test "$trust_exit_code" = "false"
 		then
 			touch "$BACKUP"

@@ -253,7 +245,7 @@ guess_merge_tool () {
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $tools
 	do
-		merge_tool_path="$(translate_merge_tool_path "$i")"
+		merge_tool_path=$(translate_merge_tool_path "$i")
 		if type "$merge_tool_path" >/dev/null 2>&1
 		then
 			echo "$i"

@@ -300,9 +292,11 @@ get_merge_tool_path () {
 	fi
 	if test -z "$merge_tool_path"
 	then
-		merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
+		merge_tool_path=$(translate_merge_tool_path "$merge_tool")
 	fi
-	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
+
+	merge_tool_cmd=$(get_merge_tool_cmd "$merge_tool")
+	if test -z "$merge_tool_cmd" &&
 		! type "$merge_tool_path" >/dev/null 2>&1
 	then
 		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\

@@ -314,11 +308,11 @@ get_merge_tool_path () {
 
 get_merge_tool () {
 	# Check if a merge tool has been configured
-	merge_tool="$(get_configured_merge_tool)"
+	merge_tool=$(get_configured_merge_tool)
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
-		merge_tool="$(guess_merge_tool)" || exit
+		merge_tool=$(guess_merge_tool) || exit
 	fi
 	echo "$merge_tool"
 }

-- 
1.8.0.9.g3370a50

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help