* trustExitCode doesn't apply to vimdiff mergetool
@ 2016-11-27 2:44 Dun Peal
2016-11-27 5:08 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Dun Peal @ 2016-11-27 2:44 UTC (permalink / raw)
To: Git ML
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?
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-27 2:44 trustExitCode doesn't apply to vimdiff mergetool Dun Peal @ 2016-11-27 5:08 ` Jeff King 2016-11-27 13:46 ` Dun Peal 0 siblings, 1 reply; 8+ messages in thread From: Jeff King @ 2016-11-27 5:08 UTC (permalink / raw) To: Dun Peal; +Cc: Git ML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-27 5:08 ` Jeff King @ 2016-11-27 13:46 ` Dun Peal 2016-11-27 16:55 ` Jeff King 0 siblings, 1 reply; 8+ messages in thread From: Dun Peal @ 2016-11-27 13:46 UTC (permalink / raw) To: Jeff King; +Cc: Git ML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-27 13:46 ` Dun Peal @ 2016-11-27 16:55 ` Jeff King 2016-11-28 1:45 ` David Aguilar 0 siblings, 1 reply; 8+ messages in thread From: Jeff King @ 2016-11-27 16:55 UTC (permalink / raw) To: Dun Peal; +Cc: Git ML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-27 16:55 ` Jeff King @ 2016-11-28 1:45 ` David Aguilar 2016-11-28 2:01 ` Jeff King 2016-11-28 2:15 ` David Aguilar 0 siblings, 2 replies; 8+ messages in thread From: David Aguilar @ 2016-11-28 1:45 UTC (permalink / raw) To: Jeff King; +Cc: Dun Peal, Git ML 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-28 1:45 ` David Aguilar @ 2016-11-28 2:01 ` Jeff King 2016-11-28 2:15 ` David Aguilar 1 sibling, 0 replies; 8+ messages in thread From: Jeff King @ 2016-11-28 2:01 UTC (permalink / raw) To: David Aguilar; +Cc: Dun Peal, Git ML 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-28 1:45 ` David Aguilar 2016-11-28 2:01 ` Jeff King @ 2016-11-28 2:15 ` David Aguilar 2016-11-28 17:17 ` Junio C Hamano 1 sibling, 1 reply; 8+ messages in thread From: David Aguilar @ 2016-11-28 2:15 UTC (permalink / raw) To: Jeff King; +Cc: Dun Peal, Git ML 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: trustExitCode doesn't apply to vimdiff mergetool 2016-11-28 2:15 ` David Aguilar @ 2016-11-28 17:17 ` Junio C Hamano 0 siblings, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2016-11-28 17:17 UTC (permalink / raw) To: David Aguilar; +Cc: Jeff King, Dun Peal, Git ML 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 } ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-28 17:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-27 2:44 trustExitCode doesn't apply to vimdiff mergetool Dun Peal 2016-11-27 5:08 ` Jeff King 2016-11-27 13:46 ` Dun Peal 2016-11-27 16:55 ` Jeff King 2016-11-28 1:45 ` David Aguilar 2016-11-28 2:01 ` Jeff King 2016-11-28 2:15 ` David Aguilar 2016-11-28 17:17 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).