trustExitCode doesn't apply to vimdiff mergetool

trustExitCode doesn't apply to vimdiff mergetool

[Dun Peal]

I'm using vimdiff as my mergetool, and have the following lines in ~/.gitconfig:

[merge]
    tool = vimdiff
[mergetool "vimdiff"]
    trustExitCode = true


My understanding from the docs is that this sets
mergetool.vimdiff.trustExitCode to true, thereby concluding that a
merge hasn't been successful if vimdiff's exit code is non-zero.

Unfortunately, when I exit Vim using `:cq` - which returns code 1 -
the merge is still presumed to have succeeded.

Is there a way to accomplish the desired effect, such that exiting
vimdiff with a non-zero code would prevent git from resolving the
conflict in the merged file?

Re: trustExitCode doesn't apply to vimdiff mergetool

[Jeff King]

On Sat, Nov 26, 2016 at 09:44:36PM -0500, Dun Peal wrote:

> I'm using vimdiff as my mergetool, and have the following lines in
> ~/.gitconfig:
> 
> [merge]
>     tool = vimdiff
> [mergetool "vimdiff"]
>     trustExitCode = true
> 
> 
> My understanding from the docs is that this sets
> mergetool.vimdiff.trustExitCode to true, thereby concluding that a
> merge hasn't been successful if vimdiff's exit code is non-zero.
> 
> Unfortunately, when I exit Vim using `:cq` - which returns code 1 -
> the merge is still presumed to have succeeded.
> 
> Is there a way to accomplish the desired effect, such that exiting
> vimdiff with a non-zero code would prevent git from resolving the
> conflict in the merged file?

I don't use mergetool myself, but peeking at the code, it looks like
trustExitCode is used only for a "user" tool, not for the builtin tool
profiles. That sounds kind of confusing to me, but I'll leave discussion
of that to people more interested in mergetool.

However, I think you can work around it by defining your own tool that
runs vimdiff:

  git config merge.tool foo
  git config mergetool.foo.cmd 'vimdiff "$LOCAL" "$BASE" "$REMOTE" "$MERGED"'
  git config mergetool.foo.trustExitCode true

Though note that the builtin vimdiff invocation is a little more
complicated than that. You may want to adapt what is in git.git's
mergetools/vimdiff to your liking.

-Peff

Re: trustExitCode doesn't apply to vimdiff mergetool

[Dun Peal]

Thanks, Jeff.

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.

If your merge tool quit with an error, it is more sensible to assume
that the resolution you were working on has not been successfully
concluded.

In the rare case where one did successfully conclude the resolution,
you can always quickly mark the file resolved. I'm not even sure how
to do that short of `git checkout -m -- file`, which would lose any
work you've already done towards the merge.

Long story short, I hope the developers change this default, or at
least let us override it for the builtin invocations.

Finally, if you're not using mergetools, how do you resolve conflicts?

On Sun, Nov 27, 2016 at 12:08 AM, Jeff King <peff@peff.net> wrote:
> On Sat, Nov 26, 2016 at 09:44:36PM -0500, Dun Peal wrote:
>
>> I'm using vimdiff as my mergetool, and have the following lines in
>> ~/.gitconfig:
>>
>> [merge]
>>     tool = vimdiff
>> [mergetool "vimdiff"]
>>     trustExitCode = true
>>
>>
>> My understanding from the docs is that this sets
>> mergetool.vimdiff.trustExitCode to true, thereby concluding that a
>> merge hasn't been successful if vimdiff's exit code is non-zero.
>>
>> Unfortunately, when I exit Vim using `:cq` - which returns code 1 -
>> the merge is still presumed to have succeeded.
>>
>> Is there a way to accomplish the desired effect, such that exiting
>> vimdiff with a non-zero code would prevent git from resolving the
>> conflict in the merged file?
>
> I don't use mergetool myself, but peeking at the code, it looks like
> trustExitCode is used only for a "user" tool, not for the builtin tool
> profiles. That sounds kind of confusing to me, but I'll leave discussion
> of that to people more interested in mergetool.
>
> However, I think you can work around it by defining your own tool that
> runs vimdiff:
>
>   git config merge.tool foo
>   git config mergetool.foo.cmd 'vimdiff "$LOCAL" "$BASE" "$REMOTE" "$MERGED"'
>   git config mergetool.foo.trustExitCode true
>
> Though note that the builtin vimdiff invocation is a little more
> complicated than that. You may want to adapt what is in git.git's
> mergetools/vimdiff to your liking.
>
> -Peff

Re: trustExitCode doesn't apply to vimdiff mergetool

[Jeff King]

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.

> Finally, if you're not using mergetools, how do you resolve conflicts?

I just edit the conflicted sections in vim. I do use git-jump (see
contrib/git-jump), but that's just to get to them quickly.

-Peff

Re: trustExitCode doesn't apply to vimdiff mergetool

[David Aguilar]

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?

--- 8< ---
Date: Sun, 27 Nov 2016 17:26:55 -0800
Subject: [PATCH] mergetool: honor mergetool.<tool>.trustExitCode for all tools

The built-in mergetools originally required that each tool scriptlet
opt-in to the trustExitCode behavior, based on whether or not the tool
called check_unchanged() itself.

Refactor the functions so that run_merge_cmd() (rather than merge_cmd())
takes care of calling check_unchanged() so that all tools handle
the trustExitCode behavior uniformly.

Remove the check_unchanged() calls from the scriptlets.
A subtle benefit of this change is that the responsibility of
merge_cmd() has been narrowed to running the command only,
rather than also needing to deal with the backup file and
checking for changes.

Reported-by: Dun Peal <dunpealer@gmail.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh    | 24 ++++++++++++++----------
 mergetools/araxis        |  2 --
 mergetools/bc            |  2 --
 mergetools/codecompare   |  2 --
 mergetools/diffuse       |  2 --
 mergetools/ecmerge       |  2 --
 mergetools/examdiff      |  2 --
 mergetools/meld          |  3 +--
 mergetools/opendiff      |  2 --
 mergetools/p4merge       |  2 --
 mergetools/tortoisemerge |  2 --
 mergetools/vimdiff       |  2 --
 mergetools/winmerge      |  2 --
 mergetools/xxdiff        |  2 --
 14 files changed, 15 insertions(+), 36 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9abd00be2..3d8a873ab 100644
--- 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 )
 	}
 }
 
@@ -225,7 +216,20 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
+	touch "$BACKUP"
+
 	merge_cmd "$1"
+	status=$?
+
+	trust_exit_code=$(git config --bool \
+		"mergetool.$1.trustExitCode" || echo false)
+	if test "$trust_exit_code" = "false"
+	then
+		check_unchanged
+		status=$?
+	fi
+
+	return $status
 }
 
 list_merge_tool_candidates () {
diff --git a/mergetools/araxis b/mergetools/araxis
index 64f97c5e9..e2407b65b 100644
--- 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
index b6319d206..3a69e60fa 100644
--- 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
index 3f0486bc8..9f60e8da6 100644
--- 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/diffuse b/mergetools/diffuse
index 02e0843f4..5a3ae8b56 100644
--- 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
index 13c2e439d..6c5101c4f 100644
--- 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/examdiff b/mergetools/examdiff
index 7b524d408..e72b06fc4 100644
--- 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/meld b/mergetools/meld
index 83ebdfb4c..bc178e888 100644
--- a/mergetools/meld
+++ b/mergetools/meld
@@ -7,7 +7,7 @@ merge_cmd () {
 	then
 		check_meld_for_output_version
 	fi
-	touch "$BACKUP"
+
 	if test "$meld_has_output_option" = true
 	then
 		"$merge_tool_path" --output "$MERGED" \
@@ -15,7 +15,6 @@ merge_cmd () {
 	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
index 0942b2a73..b608dd6de 100644
--- 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
index 5a608abf9..7a5b291dd 100644
--- 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/tortoisemerge b/mergetools/tortoisemerge
index 3b89f1c82..d7ab666a5 100644
--- 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
index 74ea6d547..a841ffdb4 100644
--- 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
index f3819d316..74d03259f 100644
--- 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
index 05b443394..e284811ff 100644
--- 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
 }
-- 
2.11.0.rc3.dirty

Re: trustExitCode doesn't apply to vimdiff mergetool

[Jeff King]

On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:

> 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.

FWIW, that was the refactoring that came to mind when I looked at the
code yesterday. This is the first time I've looked at the mergetool
code, though, so you can take that with the appropriate grain of salt.

Your patch looks mostly good to me. One minor comment:

>  	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 )
>  	}
>  }
>  
> @@ -225,7 +216,20 @@ run_diff_cmd () {
>  
>  # Run a either a configured or built-in merge tool
>  run_merge_cmd () {
> +	touch "$BACKUP"
> +
>  	merge_cmd "$1"
> +	status=$?
> +
> +	trust_exit_code=$(git config --bool \
> +		"mergetool.$1.trustExitCode" || echo false)
> +	if test "$trust_exit_code" = "false"
> +	then
> +		check_unchanged
> +		status=$?
> +	fi
> +

In the original, we only touch $BACKUP if we care about timestamps. I
can't think of a reason it would matter to do the touch in the
trustExitCode=true case, but you could also write it as:

  if test "$trust_exit_code" = "false"
  then
	touch "$BACKUP"
	merge_cmd "$1"
	check_unchanged
  else
	merge_cmd "$1"
  fi

  # now $? is from either merge_cmd or check_unchanged

Yours is arguably less subtle, though, which may be a good thing.

-Peff

Re: trustExitCode doesn't apply to vimdiff mergetool

[David Aguilar]

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

Re: trustExitCode doesn't apply to vimdiff mergetool

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> 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.

I think this is sensible, because the way I look at this issue is
that in an ideal world, we would want all tool backends consistently
give us usable exit codes, but some tools are known to give unusable
exit codes, so we ignore their exit codes by default.

As to the implementation, I think you can reduce the duplication by
having each tool backend 

 - export a new function that echos "true" or "false"; or
 - export a new function that returns true or false; or
 - set a variable whose value is either "true" or "false"

and use that from the trust_exit_code() in git-mergetool--lib.sh.
Something like this (for the second alternative).

    trust_exit_code () {
        if git config --bool "mergtool.$.trustExitCode"
	then
		:; # OK
	elif mergetool_exitcode_trustable
	then
		echo true
	else
		echo false
        fi
    }