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@xxxxxxxxx>
---
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