mergetool: honor mergetool.$tool.trustExitCode for built-in tools

davvid · gitster · commit 7c10605d2ccf · 2016-11-29T10:54:03.000-08:00
Built-in merge tools contain a hard-coded assumption about
whether or not a tool's exit code can be trusted to determine
the success or failure of a merge.  Tools whose exit codes are
not trusted contain calls to check_unchanged() in their
merge_cmd() functions.

A problem with this is that the trustExitCode configuration is
not honored for built-in tools.

Teach built-in tools to honor the trustExitCode configuration.
Extend run_merge_cmd() so that it is responsible for calling
check_unchanged() when a tool's exit code cannot be trusted.
Remove check_unchanged() calls from scriptlets since they are no
longer responsible for calling it.

When no configuration is present, exit_code_trustable() is
checked to see whether the exit code should be trusted.
The default implementation returns false.

Tools whose exit codes can be trusted override
exit_code_trustable() to true.

Reported-by: Dun Peal &lt;dunpealer@gmail.com&gt;
Signed-off-by: David Aguilar &lt;davvid@gmail.com&gt;
Signed-off-by: Junio C Hamano &lt;gitster@pobox.com&gt;
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
@@ -125,16 +125,7 @@ setup_user_tool () {
 	}
 
 	merge_cmd () {
-		trust_exit_code=$(git config --bool \
-			"mergetool.$1.trustExitCode" || echo false)
-		if test "$trust_exit_code" = "false"
-		then
-			touch "$BACKUP"
-			( eval $merge_tool_cmd )
-			check_unchanged
-		else
-			( eval $merge_tool_cmd )
-		fi
+		( eval $merge_tool_cmd )
 	}
 }
 
@@ -162,6 +153,28 @@ setup_tool () {
 		echo "$1"
 	}
 
+	# Most tools' exit codes cannot be trusted, so By default we ignore
+	# their exit code and check the merged file's modification time in
+	# check_unchanged() to determine whether or not the merge was
+	# successful.  The return value from run_merge_cmd, by default, is
+	# determined by check_unchanged().
+	#
+	# When a tool's exit code can be trusted then the return value from
+	# run_merge_cmd is simply the tool's exit code, and check_unchanged()
+	# is not called.
+	#
+	# The return value of exit_code_trustable() tells us whether or not we
+	# can trust the tool's exit code.
+	#
+	# User-defined and built-in tools default to false.
+	# Built-in tools advertise that their exit code is trustable by
+	# redefining exit_code_trustable() to true.
+
+	exit_code_trustable () {
+		false
+	}
+
+
 	if ! test -f "$MERGE_TOOLS_DIR/$tool"
 	then
 		setup_user_tool
@@ -197,6 +210,19 @@ get_merge_tool_cmd () {
 	fi
 }
 
+trust_exit_code () {
+	if git config --bool "mergetool.$1.trustExitCode"
+	then
+		:; # OK
+	elif exit_code_trustable
+	then
+		echo true
+	else
+		echo false
+	fi
+}
+
+
 # Entry point for running tools
 run_merge_tool () {
 	# If GIT_PREFIX is empty then we cannot use it in tools
@@ -225,7 +251,15 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
-	merge_cmd "$1"
+	mergetool_trust_exit_code=$(trust_exit_code "$1")
+	if test "$mergetool_trust_exit_code" = "true"
+	then
+		merge_cmd "$1"
+	else
+		touch "$BACKUP"
+		merge_cmd "$1"
+		check_unchanged
+	fi
 }
 
 list_merge_tool_candidates () {
diff --git a/mergetools/araxis b/mergetools/araxis
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" -wait -merge -3 -a1 \
@@ -12,7 +11,6 @@ merge_cmd () {
 		"$merge_tool_path" -wait -2 \
 			"$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1
 	fi
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/bc b/mergetools/bc
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" \
@@ -12,7 +11,6 @@ merge_cmd () {
 		"$merge_tool_path" "$LOCAL" "$REMOTE" \
 			-mergeoutput="$MERGED"
 	fi
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/codecompare b/mergetools/codecompare
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" -MF="$LOCAL" -TF="$REMOTE" -BF="$BASE" \
@@ -12,7 +11,6 @@ merge_cmd () {
 		"$merge_tool_path" -MF="$LOCAL" -TF="$REMOTE" \
 			-RF="$MERGED"
 	fi
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/deltawalker b/mergetools/deltawalker
@@ -16,6 +16,10 @@ merge_cmd () {
 	fi >/dev/null 2>&1
 }
 
-translate_merge_tool_path() {
+translate_merge_tool_path () {
 	echo DeltaWalker
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
@@ -12,3 +12,7 @@ merge_cmd () {
 			--result="$MERGED" "$LOCAL" "$REMOTE"
 	fi
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/diffuse b/mergetools/diffuse
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" \
@@ -13,5 +12,4 @@ merge_cmd () {
 		"$merge_tool_path" \
 			"$LOCAL" "$MERGED" "$REMOTE" | cat
 	fi
-	check_unchanged
 }
diff --git a/mergetools/ecmerge b/mergetools/ecmerge
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" \
@@ -12,5 +11,4 @@ merge_cmd () {
 		"$merge_tool_path" "$LOCAL" "$REMOTE" \
 			--default --mode=merge2 --to="$MERGED"
 	fi
-	check_unchanged
 }
diff --git a/mergetools/emerge b/mergetools/emerge
@@ -20,3 +20,7 @@ merge_cmd () {
 translate_merge_tool_path() {
 	echo emacs
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/examdiff b/mergetools/examdiff
@@ -3,14 +3,12 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" -merge "$LOCAL" "$BASE" "$REMOTE" -o:"$MERGED" -nh
 	else
 		"$merge_tool_path" -merge "$LOCAL" "$REMOTE" -o:"$MERGED" -nh
 	fi
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
@@ -21,3 +21,7 @@ merge_cmd () {
 		>/dev/null 2>&1
 	fi
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/kompare b/mergetools/kompare
@@ -5,3 +5,7 @@ can_merge () {
 diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/meld b/mergetools/meld
@@ -7,15 +7,14 @@ merge_cmd () {
 	then
 		check_meld_for_output_version
 	fi
-	touch "$BACKUP"
+
 	if test "$meld_has_output_option" = true
 	then
 		"$merge_tool_path" --output "$MERGED" \
 			"$LOCAL" "$BASE" "$REMOTE"
 	else
 		"$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
 	fi
-	check_unchanged
 }
 
 # Check whether we should use 'meld --output <file>'
diff --git a/mergetools/opendiff b/mergetools/opendiff
@@ -3,7 +3,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" "$LOCAL" "$REMOTE" \
@@ -12,5 +11,4 @@ merge_cmd () {
 		"$merge_tool_path" "$LOCAL" "$REMOTE" \
 			-merge "$MERGED" | cat
 	fi
-	check_unchanged
 }
diff --git a/mergetools/p4merge b/mergetools/p4merge
@@ -20,14 +20,12 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if ! $base_present
 	then
 		cp -- "$LOCAL" "$BASE"
 		create_virtual_base "$BASE" "$REMOTE"
 	fi
 	"$merge_tool_path" "$BASE" "$REMOTE" "$LOCAL" "$MERGED"
-	check_unchanged
 }
 
 create_empty_file () {
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
@@ -10,3 +10,7 @@ merge_cmd () {
 		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
 	fi
 }
+
+exit_code_trustable () {
+	true
+}
diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
@@ -5,7 +5,6 @@ can_diff () {
 merge_cmd () {
 	if $base_present
 	then
-		touch "$BACKUP"
 		basename="$(basename "$merge_tool_path" .exe)"
 		if test "$basename" = "tortoisegitmerge"
 		then
@@ -17,7 +16,6 @@ merge_cmd () {
 				-base:"$BASE" -mine:"$LOCAL" \
 				-theirs:"$REMOTE" -merged:"$MERGED"
 		fi
-		check_unchanged
 	else
 		echo "$merge_tool_path cannot be used without a base" 1>&2
 		return 1
diff --git a/mergetools/vimdiff b/mergetools/vimdiff
@@ -4,7 +4,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	case "$1" in
 	gvimdiff|vimdiff)
 		if $base_present
@@ -31,7 +30,6 @@ merge_cmd () {
 		fi
 		;;
 	esac
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/winmerge b/mergetools/winmerge
@@ -6,10 +6,8 @@ diff_cmd () {
 merge_cmd () {
 	# mergetool.winmerge.trustExitCode is implicitly false.
 	# touch $BACKUP so that we can check_unchanged.
-	touch "$BACKUP"
 	"$merge_tool_path" -u -e -dl Local -dr Remote \
 		"$LOCAL" "$REMOTE" "$MERGED"
-	check_unchanged
 }
 
 translate_merge_tool_path() {
diff --git a/mergetools/xxdiff b/mergetools/xxdiff
@@ -6,7 +6,6 @@ diff_cmd () {
 }
 
 merge_cmd () {
-	touch "$BACKUP"
 	if $base_present
 	then
 		"$merge_tool_path" -X --show-merged-pane \
@@ -21,5 +20,4 @@ merge_cmd () {
 			-R 'Accel.SearchForward: "Ctrl-G"' \
 			--merged-file "$MERGED" "$LOCAL" "$REMOTE"
 	fi
-	check_unchanged
 }

Original file line number	Diff line number	Diff line change
`@@ -3,7 +3,6 @@ diff_cmd () {`
`3`	`3`	`}`
`4`	`4`
`5`	`5`	`merge_cmd () {`
`6`		`- touch "$BACKUP"`
`7`	`6`	`if $base_present`
`8`	`7`	`then`
`9`	`8`	`"$merge_tool_path" -wait -merge -3 -a1 \`
`@@ -12,7 +11,6 @@ merge_cmd () {`
`12`	`11`	`"$merge_tool_path" -wait -2 \`
`13`	`12`	`"$LOCAL" "$REMOTE" "$MERGED" >/dev/null 2>&1`
`14`	`13`	`fi`
`15`		`- check_unchanged`
`16`	`14`	`}`
`17`	`15`
`18`	`16`	`translate_merge_tool_path() {`
Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,10 @@ merge_cmd () {`
`16`	`16`	`fi >/dev/null 2>&1`
`17`	`17`	`}`
`18`	`18`
`19`		`-translate_merge_tool_path() {`
	`19`	`+translate_merge_tool_path () {`
`20`	`20`	`echo DeltaWalker`
`21`	`21`	`}`
	`22`	`+`
	`23`	`+exit_code_trustable () {`
	`24`	`+ true`
	`25`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -12,3 +12,7 @@ merge_cmd () {`
`12`	`12`	`--result="$MERGED" "$LOCAL" "$REMOTE"`
`13`	`13`	`fi`
`14`	`14`	`}`
	`15`	`+`
	`16`	`+exit_code_trustable () {`
	`17`	`+ true`
	`18`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -20,3 +20,7 @@ merge_cmd () {`
`20`	`20`	`translate_merge_tool_path() {`
`21`	`21`	`echo emacs`
`22`	`22`	`}`
	`23`	`+`
	`24`	`+exit_code_trustable () {`
	`25`	`+ true`
	`26`	`+}`
Original file line number	Diff line number	Diff line change
`@@ -3,14 +3,12 @@ diff_cmd () {`
`3`	`3`	`}`
`4`	`4`
`5`	`5`	`merge_cmd () {`
`6`		`- touch "$BACKUP"`
`7`	`6`	`if $base_present`
`8`	`7`	`then`
`9`	`8`	`"$merge_tool_path" -merge "$LOCAL" "$BASE" "$REMOTE" -o:"$MERGED" -nh`
`10`	`9`	`else`
`11`	`10`	`"$merge_tool_path" -merge "$LOCAL" "$REMOTE" -o:"$MERGED" -nh`
`12`	`11`	`fi`
`13`		`- check_unchanged`
`14`	`12`	`}`
`15`	`13`
`16`	`14`	`translate_merge_tool_path() {`