Re: trustExitCode doesn't apply to vimdiff mergetool

[David Aguilar <davvid@gmail.com>]

On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:
> On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote:
> > On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:
> > 
> > > Ignoring a non-zero exit code from the merge tool, and assuming a
> > > successful merge in that case, seems like the wrong default behavior
> > > to me.
> > 
> > Yeah, I'm inclined to agree. But like I said, I'm not too familiar with
> > this area, so maybe there are subtle things I'm missing.
> 
> I think this may have been an oversight in how the
> trust-exit-code feature is implemented across builtins.
> 
> Right now, specific builtin tools could in theory opt-in to the
> feature, but I think it should be handled in a central place.
> For vimdiff, the exit code is not considered because the
> scriptlet calls check_unchanged(), which only cares about
> modifciation time.
> 
> I have a patch that makes it so that none of the tools do the
> check_unchanged logic themselves and instead rely on the
> library code to handle it for them.  This makes the
> implementation uniform across all tools, and allows tools to
> opt-in to trustExitCode=true.
> 
> This means that all of the builtin tools will default to
> trustExitCode=false, and they can opt-in by setting the
> configuration variable.
> 
> For tkdiff and kdiff3, this is a subtle change in behavior, but
> not one that should be problematic, and the upside is that we'll
> have consistency across all tools.
> 
> In this scenario specifically, what happens is that the
> scriptlet is calling check_unchanged(), which checks the
> modification time of the file, and if the file is new then it
> assumes that the merge succeeded.  check_unchanged() is clearing
> the exit code.
> 
> Try the patch below.  I tested it with vimdiff and it seems to
> provide the desired behavior:
> - the modificaiton time behavior is the default
> - setting mergetool.vimdiff.trustExitCode = true will make it
>   honor vim's exit code via :cq
> 
> One possible idea that could avoid the subtle tkdiff/kdiff3
> change in behavior would be to allow the scriptlets to advertise
> their preference for the default trustExitCode setting.  These
> tools could say, "default to true", and the rest can assume
> false.
> 
> If others feel that this is worth the extra machinery, and the
> mental burden of tools having different defaults, then that
> could be implemented as a follow-up patch.  IMO I'd be okay with
> not needing it and only adding it if someone notices, but if
> others feel otherwise we can do it sooner rather than later.
> 
> Thoughts?

For the curious, here is what that patch might look like.
This allows scriptlets to redefine trust_exit_code() so that
they can advertise that they prefer default=true.

The main benefit of this is that we're preserving the original
behavior before these patches.  I'll let this sit out here for
comments for a few days to see what others think.

--- >8 ---
Date: Sun, 27 Nov 2016 18:08:08 -0800
Subject: [PATCH] mergetool: restore trustExitCode behavior for builtins tools

deltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally
provided behavior that matched trustExitCode=true.

The default for all tools is trustExitCode=false, which conflicts with
these tools' defaults.  Allow tools to advertise their own default value
for trustExitCode so that users do not need to opt-in to the original
behavior.

While this makes the default inconsistent between tools, it can still be
overridden, and it makes it consistent with the current Git behavior.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh  | 15 +++++++++++++--
 mergetools/deltawalker |  6 ++++++
 mergetools/diffmerge   |  6 ++++++
 mergetools/emerge      |  6 ++++++
 mergetools/kdiff3      |  6 ++++++
 mergetools/kompare     |  6 ++++++
 mergetools/tkdiff      |  6 ++++++
 7 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 3d8a873ab..be079723a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -120,6 +120,12 @@ setup_user_tool () {
 	merge_tool_cmd=$(get_merge_tool_cmd "$tool")
 	test -n "$merge_tool_cmd" || return 1
 
+	trust_exit_code () {
+		# user-defined tools default to trustExitCode = false
+		git config --bool "mergetool.$1.trustExitCode" ||
+		echo false
+	}
+
 	diff_cmd () {
 		( eval $merge_tool_cmd )
 	}
@@ -153,6 +159,12 @@ setup_tool () {
 		echo "$1"
 	}
 
+	trust_exit_code () {
+		# built-in tools default to trustExitCode = false
+		git config --bool "mergetool.$1.trustExitCode" ||
+		echo false
+	}
+
 	if ! test -f "$MERGE_TOOLS_DIR/$tool"
 	then
 		setup_user_tool
@@ -221,8 +233,7 @@ run_merge_cmd () {
 	merge_cmd "$1"
 	status=$?
 
-	trust_exit_code=$(git config --bool \
-		"mergetool.$1.trustExitCode" || echo false)
+	trust_exit_code=$(trust_exit_code "$1")
 	if test "$trust_exit_code" = "false"
 	then
 		check_unchanged
diff --git a/mergetools/deltawalker b/mergetools/deltawalker
index b3c71b623..ad978f83d 100644
--- a/mergetools/deltawalker
+++ b/mergetools/deltawalker
@@ -19,3 +19,9 @@ merge_cmd () {
 translate_merge_tool_path() {
 	echo DeltaWalker
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
diff --git a/mergetools/diffmerge b/mergetools/diffmerge
index f138cb4e7..437b34996 100644
--- a/mergetools/diffmerge
+++ b/mergetools/diffmerge
@@ -12,3 +12,9 @@ merge_cmd () {
 			--result="$MERGED" "$LOCAL" "$REMOTE"
 	fi
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
diff --git a/mergetools/emerge b/mergetools/emerge
index 7b895fdb1..8c950d678 100644
--- a/mergetools/emerge
+++ b/mergetools/emerge
@@ -20,3 +20,9 @@ merge_cmd () {
 translate_merge_tool_path() {
 	echo emacs
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
diff --git a/mergetools/kdiff3 b/mergetools/kdiff3
index 793d1293b..9d94876b9 100644
--- a/mergetools/kdiff3
+++ b/mergetools/kdiff3
@@ -21,3 +21,9 @@ merge_cmd () {
 		>/dev/null 2>&1
 	fi
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
diff --git a/mergetools/kompare b/mergetools/kompare
index 433686c12..0ae0bdc02 100644
--- a/mergetools/kompare
+++ b/mergetools/kompare
@@ -5,3 +5,9 @@ can_merge () {
 diff_cmd () {
 	"$merge_tool_path" "$LOCAL" "$REMOTE"
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
diff --git a/mergetools/tkdiff b/mergetools/tkdiff
index 618c438e8..d73792a21 100644
--- a/mergetools/tkdiff
+++ b/mergetools/tkdiff
@@ -10,3 +10,9 @@ merge_cmd () {
 		"$merge_tool_path" -o "$MERGED" "$LOCAL" "$REMOTE"
 	fi
 }
+
+trust_exit_code () {
+	# Default to trustExitCode = true
+	git config --bool "mergetool.$1.trustExitCode" ||
+	echo true
+}
-- 
2.11.0.rc3.1.g2633b1d.dirty