{"thread":{"id":"44549","subject":"trustExitCode doesn't apply to vimdiff mergetool","startedAt":"2016-11-27T02:44:43Z","lastAt":"2016-11-28T17:17:48Z","messageCount":8,"participants":["Dun Peal","Jeff King","David Aguilar","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"306455","messageId":"CAD03jn5PAZcFeesaq2osjo7cYd1frWZeN0odNqTh+AMxSEmLgQ@mail.gmail.com","threadId":"44549","inReplyTo":null,"subject":"trustExitCode doesn't apply to vimdiff mergetool","fromName":"Dun Peal","fromEmail":"dunpealer@gmail.com","sentAt":"2016-11-27T02:44:36Z","receivedAt":"2016-11-27T02:44:43Z","isPatch":false,"sender":{"key":"dunpealer@gmail.com","avatar":"https://gravatar.com/avatar/42f3e6a1166eb44e33f24c20ccbe809fa8591413f366dfe3755c8a8d4935ace9?d=mp&s=160"},"body":"I'm using vimdiff as my mergetool, and have the following lines in ~/.gitconfig:\n\n[merge]\n    tool = vimdiff\n[mergetool \"vimdiff\"]\n    trustExitCode = true\n\n\nMy understanding from the docs is that this sets\nmergetool.vimdiff.trustExitCode to true, thereby concluding that a\nmerge hasn't been successful if vimdiff's exit code is non-zero.\n\nUnfortunately, when I exit Vim using `:cq` - which returns code 1 -\nthe merge is still presumed to have succeeded.\n\nIs there a way to accomplish the desired effect, such that exiting\nvimdiff with a non-zero code would prevent git from resolving the\nconflict in the merged file?\n"},{"id":"306457","messageId":"20161127050818.rmjpvha64y4wosq2@sigill.intra.peff.net","threadId":"44549","inReplyTo":"CAD03jn5PAZcFeesaq2osjo7cYd1frWZeN0odNqTh+AMxSEmLgQ@mail.gmail.com","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-27T05:08:18Z","receivedAt":"2016-11-27T05:08:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 26, 2016 at 09:44:36PM -0500, Dun Peal wrote:\n\n> I'm using vimdiff as my mergetool, and have the following lines in\n> ~/.gitconfig:\n> \n> [merge]\n>     tool = vimdiff\n> [mergetool \"vimdiff\"]\n>     trustExitCode = true\n> \n> \n> My understanding from the docs is that this sets\n> mergetool.vimdiff.trustExitCode to true, thereby concluding that a\n> merge hasn't been successful if vimdiff's exit code is non-zero.\n> \n> Unfortunately, when I exit Vim using `:cq` - which returns code 1 -\n> the merge is still presumed to have succeeded.\n> \n> Is there a way to accomplish the desired effect, such that exiting\n> vimdiff with a non-zero code would prevent git from resolving the\n> conflict in the merged file?\n\nI don't use mergetool myself, but peeking at the code, it looks like\ntrustExitCode is used only for a \"user\" tool, not for the builtin tool\nprofiles. That sounds kind of confusing to me, but I'll leave discussion\nof that to people more interested in mergetool.\n\nHowever, I think you can work around it by defining your own tool that\nruns vimdiff:\n\n  git config merge.tool foo\n  git config mergetool.foo.cmd 'vimdiff \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"'\n  git config mergetool.foo.trustExitCode true\n\nThough note that the builtin vimdiff invocation is a little more\ncomplicated than that. You may want to adapt what is in git.git's\nmergetools/vimdiff to your liking.\n\n-Peff\n"},{"id":"306463","messageId":"CAD03jn7gU9g7NyDk_3wYTKsYQUtRGg6msvumZqUDs44hMOVX7w@mail.gmail.com","threadId":"44549","inReplyTo":"20161127050818.rmjpvha64y4wosq2@sigill.intra.peff.net","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"Dun Peal","fromEmail":"dunpealer@gmail.com","sentAt":"2016-11-27T13:46:40Z","receivedAt":"2016-11-27T13:46:52Z","isPatch":false,"sender":{"key":"dunpealer@gmail.com","avatar":"https://gravatar.com/avatar/42f3e6a1166eb44e33f24c20ccbe809fa8591413f366dfe3755c8a8d4935ace9?d=mp&s=160"},"body":"Thanks, Jeff.\n\nIgnoring a non-zero exit code from the merge tool, and assuming a\nsuccessful merge in that case, seems like the wrong default behavior\nto me.\n\nIf your merge tool quit with an error, it is more sensible to assume\nthat the resolution you were working on has not been successfully\nconcluded.\n\nIn the rare case where one did successfully conclude the resolution,\nyou can always quickly mark the file resolved. I'm not even sure how\nto do that short of `git checkout -m -- file`, which would lose any\nwork you've already done towards the merge.\n\nLong story short, I hope the developers change this default, or at\nleast let us override it for the builtin invocations.\n\nFinally, if you're not using mergetools, how do you resolve conflicts?\n\nOn Sun, Nov 27, 2016 at 12:08 AM, Jeff King <peff@peff.net> wrote:\n> On Sat, Nov 26, 2016 at 09:44:36PM -0500, Dun Peal wrote:\n>\n>> I'm using vimdiff as my mergetool, and have the following lines in\n>> ~/.gitconfig:\n>>\n>> [merge]\n>>     tool = vimdiff\n>> [mergetool \"vimdiff\"]\n>>     trustExitCode = true\n>>\n>>\n>> My understanding from the docs is that this sets\n>> mergetool.vimdiff.trustExitCode to true, thereby concluding that a\n>> merge hasn't been successful if vimdiff's exit code is non-zero.\n>>\n>> Unfortunately, when I exit Vim using `:cq` - which returns code 1 -\n>> the merge is still presumed to have succeeded.\n>>\n>> Is there a way to accomplish the desired effect, such that exiting\n>> vimdiff with a non-zero code would prevent git from resolving the\n>> conflict in the merged file?\n>\n> I don't use mergetool myself, but peeking at the code, it looks like\n> trustExitCode is used only for a \"user\" tool, not for the builtin tool\n> profiles. That sounds kind of confusing to me, but I'll leave discussion\n> of that to people more interested in mergetool.\n>\n> However, I think you can work around it by defining your own tool that\n> runs vimdiff:\n>\n>   git config merge.tool foo\n>   git config mergetool.foo.cmd 'vimdiff \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"'\n>   git config mergetool.foo.trustExitCode true\n>\n> Though note that the builtin vimdiff invocation is a little more\n> complicated than that. You may want to adapt what is in git.git's\n> mergetools/vimdiff to your liking.\n>\n> -Peff\n"},{"id":"306466","messageId":"20161127165559.j5okxyztwescheug@sigill.intra.peff.net","threadId":"44549","inReplyTo":"CAD03jn7gU9g7NyDk_3wYTKsYQUtRGg6msvumZqUDs44hMOVX7w@mail.gmail.com","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-27T16:55:59Z","receivedAt":"2016-11-27T16:56:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:\n\n> Ignoring a non-zero exit code from the merge tool, and assuming a\n> successful merge in that case, seems like the wrong default behavior\n> to me.\n\nYeah, I'm inclined to agree. But like I said, I'm not too familiar with\nthis area, so maybe there are subtle things I'm missing.\n\n> Finally, if you're not using mergetools, how do you resolve conflicts?\n\nI just edit the conflicted sections in vim. I do use git-jump (see\ncontrib/git-jump), but that's just to get to them quickly.\n\n-Peff\n"},{"id":"306468","messageId":"20161128014538.GA18691@gmail.com","threadId":"44549","inReplyTo":"20161127165559.j5okxyztwescheug@sigill.intra.peff.net","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-11-28T01:45:38Z","receivedAt":"2016-11-28T01:46:30Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote:\n> On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:\n> \n> > Ignoring a non-zero exit code from the merge tool, and assuming a\n> > successful merge in that case, seems like the wrong default behavior\n> > to me.\n> \n> Yeah, I'm inclined to agree. But like I said, I'm not too familiar with\n> this area, so maybe there are subtle things I'm missing.\n\nI think this may have been an oversight in how the\ntrust-exit-code feature is implemented across builtins.\n\nRight now, specific builtin tools could in theory opt-in to the\nfeature, but I think it should be handled in a central place.\nFor vimdiff, the exit code is not considered because the\nscriptlet calls check_unchanged(), which only cares about\nmodifciation time.\n\nI have a patch that makes it so that none of the tools do the\ncheck_unchanged logic themselves and instead rely on the\nlibrary code to handle it for them.  This makes the\nimplementation uniform across all tools, and allows tools to\nopt-in to trustExitCode=true.\n\nThis means that all of the builtin tools will default to\ntrustExitCode=false, and they can opt-in by setting the\nconfiguration variable.\n\nFor tkdiff and kdiff3, this is a subtle change in behavior, but\nnot one that should be problematic, and the upside is that we'll\nhave consistency across all tools.\n\nIn this scenario specifically, what happens is that the\nscriptlet is calling check_unchanged(), which checks the\nmodification time of the file, and if the file is new then it\nassumes that the merge succeeded.  check_unchanged() is clearing\nthe exit code.\n\nTry the patch below.  I tested it with vimdiff and it seems to\nprovide the desired behavior:\n- the modificaiton time behavior is the default\n- setting mergetool.vimdiff.trustExitCode = true will make it\n  honor vim's exit code via :cq\n\nOne possible idea that could avoid the subtle tkdiff/kdiff3\nchange in behavior would be to allow the scriptlets to advertise\ntheir preference for the default trustExitCode setting.  These\ntools could say, \"default to true\", and the rest can assume\nfalse.\n\nIf others feel that this is worth the extra machinery, and the\nmental burden of tools having different defaults, then that\ncould be implemented as a follow-up patch.  IMO I'd be okay with\nnot needing it and only adding it if someone notices, but if\nothers feel otherwise we can do it sooner rather than later.\n\nThoughts?\n\n--- 8< ---\nDate: Sun, 27 Nov 2016 17:26:55 -0800\nSubject: [PATCH] mergetool: honor mergetool.<tool>.trustExitCode for all tools\n\nThe built-in mergetools originally required that each tool scriptlet\nopt-in to the trustExitCode behavior, based on whether or not the tool\ncalled check_unchanged() itself.\n\nRefactor the functions so that run_merge_cmd() (rather than merge_cmd())\ntakes care of calling check_unchanged() so that all tools handle\nthe trustExitCode behavior uniformly.\n\nRemove the check_unchanged() calls from the scriptlets.\nA subtle benefit of this change is that the responsibility of\nmerge_cmd() has been narrowed to running the command only,\nrather than also needing to deal with the backup file and\nchecking for changes.\n\nReported-by: Dun Peal <dunpealer@gmail.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh    | 24 ++++++++++++++----------\n mergetools/araxis        |  2 --\n mergetools/bc            |  2 --\n mergetools/codecompare   |  2 --\n mergetools/diffuse       |  2 --\n mergetools/ecmerge       |  2 --\n mergetools/examdiff      |  2 --\n mergetools/meld          |  3 +--\n mergetools/opendiff      |  2 --\n mergetools/p4merge       |  2 --\n mergetools/tortoisemerge |  2 --\n mergetools/vimdiff       |  2 --\n mergetools/winmerge      |  2 --\n mergetools/xxdiff        |  2 --\n 14 files changed, 15 insertions(+), 36 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9abd00be2..3d8a873ab 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -125,16 +125,7 @@ setup_user_tool () {\n \t}\n \n \tmerge_cmd () {\n-\t\ttrust_exit_code=$(git config --bool \\\n-\t\t\t\"mergetool.$1.trustExitCode\" || echo false)\n-\t\tif test \"$trust_exit_code\" = \"false\"\n-\t\tthen\n-\t\t\ttouch \"$BACKUP\"\n-\t\t\t( eval $merge_tool_cmd )\n-\t\t\tcheck_unchanged\n-\t\telse\n-\t\t\t( eval $merge_tool_cmd )\n-\t\tfi\n+\t\t( eval $merge_tool_cmd )\n \t}\n }\n \n@@ -225,7 +216,20 @@ run_diff_cmd () {\n \n # Run a either a configured or built-in merge tool\n run_merge_cmd () {\n+\ttouch \"$BACKUP\"\n+\n \tmerge_cmd \"$1\"\n+\tstatus=$?\n+\n+\ttrust_exit_code=$(git config --bool \\\n+\t\t\"mergetool.$1.trustExitCode\" || echo false)\n+\tif test \"$trust_exit_code\" = \"false\"\n+\tthen\n+\t\tcheck_unchanged\n+\t\tstatus=$?\n+\tfi\n+\n+\treturn $status\n }\n \n list_merge_tool_candidates () {\ndiff --git a/mergetools/araxis b/mergetools/araxis\nindex 64f97c5e9..e2407b65b 100644\n--- a/mergetools/araxis\n+++ b/mergetools/araxis\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" -wait -merge -3 -a1 \\\n@@ -12,7 +11,6 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" -wait -2 \\\n \t\t\t\"$LOCAL\" \"$REMOTE\" \"$MERGED\" >/dev/null 2>&1\n \tfi\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/bc b/mergetools/bc\nindex b6319d206..3a69e60fa 100644\n--- a/mergetools/bc\n+++ b/mergetools/bc\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \"$BASE\" \\\n@@ -12,7 +11,6 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \\\n \t\t\t-mergeoutput=\"$MERGED\"\n \tfi\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/codecompare b/mergetools/codecompare\nindex 3f0486bc8..9f60e8da6 100644\n--- a/mergetools/codecompare\n+++ b/mergetools/codecompare\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" -MF=\"$LOCAL\" -TF=\"$REMOTE\" -BF=\"$BASE\" \\\n@@ -12,7 +11,6 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" -MF=\"$LOCAL\" -TF=\"$REMOTE\" \\\n \t\t\t-RF=\"$MERGED\"\n \tfi\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/diffuse b/mergetools/diffuse\nindex 02e0843f4..5a3ae8b56 100644\n--- a/mergetools/diffuse\n+++ b/mergetools/diffuse\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" \\\n@@ -13,5 +12,4 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" \\\n \t\t\t\"$LOCAL\" \"$MERGED\" \"$REMOTE\" | cat\n \tfi\n-\tcheck_unchanged\n }\ndiff --git a/mergetools/ecmerge b/mergetools/ecmerge\nindex 13c2e439d..6c5101c4f 100644\n--- a/mergetools/ecmerge\n+++ b/mergetools/ecmerge\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" \"$BASE\" \"$LOCAL\" \"$REMOTE\" \\\n@@ -12,5 +11,4 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \\\n \t\t\t--default --mode=merge2 --to=\"$MERGED\"\n \tfi\n-\tcheck_unchanged\n }\ndiff --git a/mergetools/examdiff b/mergetools/examdiff\nindex 7b524d408..e72b06fc4 100644\n--- a/mergetools/examdiff\n+++ b/mergetools/examdiff\n@@ -3,14 +3,12 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" -merge \"$LOCAL\" \"$BASE\" \"$REMOTE\" -o:\"$MERGED\" -nh\n \telse\n \t\t\"$merge_tool_path\" -merge \"$LOCAL\" \"$REMOTE\" -o:\"$MERGED\" -nh\n \tfi\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/meld b/mergetools/meld\nindex 83ebdfb4c..bc178e888 100644\n--- a/mergetools/meld\n+++ b/mergetools/meld\n@@ -7,7 +7,7 @@ merge_cmd () {\n \tthen\n \t\tcheck_meld_for_output_version\n \tfi\n-\ttouch \"$BACKUP\"\n+\n \tif test \"$meld_has_output_option\" = true\n \tthen\n \t\t\"$merge_tool_path\" --output \"$MERGED\" \\\n@@ -15,7 +15,6 @@ merge_cmd () {\n \telse\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$MERGED\" \"$REMOTE\"\n \tfi\n-\tcheck_unchanged\n }\n \n # Check whether we should use 'meld --output <file>'\ndiff --git a/mergetools/opendiff b/mergetools/opendiff\nindex 0942b2a73..b608dd6de 100644\n--- a/mergetools/opendiff\n+++ b/mergetools/opendiff\n@@ -3,7 +3,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \\\n@@ -12,5 +11,4 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\" \\\n \t\t\t-merge \"$MERGED\" | cat\n \tfi\n-\tcheck_unchanged\n }\ndiff --git a/mergetools/p4merge b/mergetools/p4merge\nindex 5a608abf9..7a5b291dd 100644\n--- a/mergetools/p4merge\n+++ b/mergetools/p4merge\n@@ -20,14 +20,12 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif ! $base_present\n \tthen\n \t\tcp -- \"$LOCAL\" \"$BASE\"\n \t\tcreate_virtual_base \"$BASE\" \"$REMOTE\"\n \tfi\n \t\"$merge_tool_path\" \"$BASE\" \"$REMOTE\" \"$LOCAL\" \"$MERGED\"\n-\tcheck_unchanged\n }\n \n create_empty_file () {\ndiff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge\nindex 3b89f1c82..d7ab666a5 100644\n--- a/mergetools/tortoisemerge\n+++ b/mergetools/tortoisemerge\n@@ -5,7 +5,6 @@ can_diff () {\n merge_cmd () {\n \tif $base_present\n \tthen\n-\t\ttouch \"$BACKUP\"\n \t\tbasename=\"$(basename \"$merge_tool_path\" .exe)\"\n \t\tif test \"$basename\" = \"tortoisegitmerge\"\n \t\tthen\n@@ -17,7 +16,6 @@ merge_cmd () {\n \t\t\t\t-base:\"$BASE\" -mine:\"$LOCAL\" \\\n \t\t\t\t-theirs:\"$REMOTE\" -merged:\"$MERGED\"\n \t\tfi\n-\t\tcheck_unchanged\n \telse\n \t\techo \"$merge_tool_path cannot be used without a base\" 1>&2\n \t\treturn 1\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 74ea6d547..a841ffdb4 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -4,7 +4,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tcase \"$1\" in\n \tgvimdiff|vimdiff)\n \t\tif $base_present\n@@ -31,7 +30,6 @@ merge_cmd () {\n \t\tfi\n \t\t;;\n \tesac\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/winmerge b/mergetools/winmerge\nindex f3819d316..74d03259f 100644\n--- a/mergetools/winmerge\n+++ b/mergetools/winmerge\n@@ -6,10 +6,8 @@ diff_cmd () {\n merge_cmd () {\n \t# mergetool.winmerge.trustExitCode is implicitly false.\n \t# touch $BACKUP so that we can check_unchanged.\n-\ttouch \"$BACKUP\"\n \t\"$merge_tool_path\" -u -e -dl Local -dr Remote \\\n \t\t\"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n-\tcheck_unchanged\n }\n \n translate_merge_tool_path() {\ndiff --git a/mergetools/xxdiff b/mergetools/xxdiff\nindex 05b443394..e284811ff 100644\n--- a/mergetools/xxdiff\n+++ b/mergetools/xxdiff\n@@ -6,7 +6,6 @@ diff_cmd () {\n }\n \n merge_cmd () {\n-\ttouch \"$BACKUP\"\n \tif $base_present\n \tthen\n \t\t\"$merge_tool_path\" -X --show-merged-pane \\\n@@ -21,5 +20,4 @@ merge_cmd () {\n \t\t\t-R 'Accel.SearchForward: \"Ctrl-G\"' \\\n \t\t\t--merged-file \"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n-\tcheck_unchanged\n }\n-- \n2.11.0.rc3.dirty\n"},{"id":"306469","messageId":"20161128020128.6jhdpg444xbshtzz@sigill.intra.peff.net","threadId":"44549","inReplyTo":"20161128014538.GA18691@gmail.com","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-28T02:01:28Z","receivedAt":"2016-11-28T02:01:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:\n\n> I have a patch that makes it so that none of the tools do the\n> check_unchanged logic themselves and instead rely on the\n> library code to handle it for them.  This makes the\n> implementation uniform across all tools, and allows tools to\n> opt-in to trustExitCode=true.\n> \n> This means that all of the builtin tools will default to\n> trustExitCode=false, and they can opt-in by setting the\n> configuration variable.\n\nFWIW, that was the refactoring that came to mind when I looked at the\ncode yesterday. This is the first time I've looked at the mergetool\ncode, though, so you can take that with the appropriate grain of salt.\n\nYour patch looks mostly good to me. One minor comment:\n\n>  \tmerge_cmd () {\n> -\t\ttrust_exit_code=$(git config --bool \\\n> -\t\t\t\"mergetool.$1.trustExitCode\" || echo false)\n> -\t\tif test \"$trust_exit_code\" = \"false\"\n> -\t\tthen\n> -\t\t\ttouch \"$BACKUP\"\n> -\t\t\t( eval $merge_tool_cmd )\n> -\t\t\tcheck_unchanged\n> -\t\telse\n> -\t\t\t( eval $merge_tool_cmd )\n> -\t\tfi\n> +\t\t( eval $merge_tool_cmd )\n>  \t}\n>  }\n>  \n> @@ -225,7 +216,20 @@ run_diff_cmd () {\n>  \n>  # Run a either a configured or built-in merge tool\n>  run_merge_cmd () {\n> +\ttouch \"$BACKUP\"\n> +\n>  \tmerge_cmd \"$1\"\n> +\tstatus=$?\n> +\n> +\ttrust_exit_code=$(git config --bool \\\n> +\t\t\"mergetool.$1.trustExitCode\" || echo false)\n> +\tif test \"$trust_exit_code\" = \"false\"\n> +\tthen\n> +\t\tcheck_unchanged\n> +\t\tstatus=$?\n> +\tfi\n> +\n\nIn the original, we only touch $BACKUP if we care about timestamps. I\ncan't think of a reason it would matter to do the touch in the\ntrustExitCode=true case, but you could also write it as:\n\n  if test \"$trust_exit_code\" = \"false\"\n  then\n\ttouch \"$BACKUP\"\n\tmerge_cmd \"$1\"\n\tcheck_unchanged\n  else\n\tmerge_cmd \"$1\"\n  fi\n\n  # now $? is from either merge_cmd or check_unchanged\n\nYours is arguably less subtle, though, which may be a good thing.\n\n-Peff\n"},{"id":"306470","messageId":"20161128021554.GA30863@gmail.com","threadId":"44549","inReplyTo":"20161128014538.GA18691@gmail.com","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-11-28T02:15:54Z","receivedAt":"2016-11-28T02:16:03Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Nov 27, 2016 at 05:45:38PM -0800, David Aguilar wrote:\n> On Sun, Nov 27, 2016 at 11:55:59AM -0500, Jeff King wrote:\n> > On Sun, Nov 27, 2016 at 08:46:40AM -0500, Dun Peal wrote:\n> > \n> > > Ignoring a non-zero exit code from the merge tool, and assuming a\n> > > successful merge in that case, seems like the wrong default behavior\n> > > to me.\n> > \n> > Yeah, I'm inclined to agree. But like I said, I'm not too familiar with\n> > this area, so maybe there are subtle things I'm missing.\n> \n> I think this may have been an oversight in how the\n> trust-exit-code feature is implemented across builtins.\n> \n> Right now, specific builtin tools could in theory opt-in to the\n> feature, but I think it should be handled in a central place.\n> For vimdiff, the exit code is not considered because the\n> scriptlet calls check_unchanged(), which only cares about\n> modifciation time.\n> \n> I have a patch that makes it so that none of the tools do the\n> check_unchanged logic themselves and instead rely on the\n> library code to handle it for them.  This makes the\n> implementation uniform across all tools, and allows tools to\n> opt-in to trustExitCode=true.\n> \n> This means that all of the builtin tools will default to\n> trustExitCode=false, and they can opt-in by setting the\n> configuration variable.\n> \n> For tkdiff and kdiff3, this is a subtle change in behavior, but\n> not one that should be problematic, and the upside is that we'll\n> have consistency across all tools.\n> \n> In this scenario specifically, what happens is that the\n> scriptlet is calling check_unchanged(), which checks the\n> modification time of the file, and if the file is new then it\n> assumes that the merge succeeded.  check_unchanged() is clearing\n> the exit code.\n> \n> Try the patch below.  I tested it with vimdiff and it seems to\n> provide the desired behavior:\n> - the modificaiton time behavior is the default\n> - setting mergetool.vimdiff.trustExitCode = true will make it\n>   honor vim's exit code via :cq\n> \n> One possible idea that could avoid the subtle tkdiff/kdiff3\n> change in behavior would be to allow the scriptlets to advertise\n> their preference for the default trustExitCode setting.  These\n> tools could say, \"default to true\", and the rest can assume\n> false.\n> \n> If others feel that this is worth the extra machinery, and the\n> mental burden of tools having different defaults, then that\n> could be implemented as a follow-up patch.  IMO I'd be okay with\n> not needing it and only adding it if someone notices, but if\n> others feel otherwise we can do it sooner rather than later.\n> \n> Thoughts?\n\nFor the curious, here is what that patch might look like.\nThis allows scriptlets to redefine trust_exit_code() so that\nthey can advertise that they prefer default=true.\n\nThe main benefit of this is that we're preserving the original\nbehavior before these patches.  I'll let this sit out here for\ncomments for a few days to see what others think.\n\n--- >8 ---\nDate: Sun, 27 Nov 2016 18:08:08 -0800\nSubject: [PATCH] mergetool: restore trustExitCode behavior for builtins tools\n\ndeltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally\nprovided behavior that matched trustExitCode=true.\n\nThe default for all tools is trustExitCode=false, which conflicts with\nthese tools' defaults.  Allow tools to advertise their own default value\nfor trustExitCode so that users do not need to opt-in to the original\nbehavior.\n\nWhile this makes the default inconsistent between tools, it can still be\noverridden, and it makes it consistent with the current Git behavior.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh  | 15 +++++++++++++--\n mergetools/deltawalker |  6 ++++++\n mergetools/diffmerge   |  6 ++++++\n mergetools/emerge      |  6 ++++++\n mergetools/kdiff3      |  6 ++++++\n mergetools/kompare     |  6 ++++++\n mergetools/tkdiff      |  6 ++++++\n 7 files changed, 49 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 3d8a873ab..be079723a 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -120,6 +120,12 @@ setup_user_tool () {\n \tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n \ttest -n \"$merge_tool_cmd\" || return 1\n \n+\ttrust_exit_code () {\n+\t\t# user-defined tools default to trustExitCode = false\n+\t\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\t\techo false\n+\t}\n+\n \tdiff_cmd () {\n \t\t( eval $merge_tool_cmd )\n \t}\n@@ -153,6 +159,12 @@ setup_tool () {\n \t\techo \"$1\"\n \t}\n \n+\ttrust_exit_code () {\n+\t\t# built-in tools default to trustExitCode = false\n+\t\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\t\techo false\n+\t}\n+\n \tif ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n \t\tsetup_user_tool\n@@ -221,8 +233,7 @@ run_merge_cmd () {\n \tmerge_cmd \"$1\"\n \tstatus=$?\n \n-\ttrust_exit_code=$(git config --bool \\\n-\t\t\"mergetool.$1.trustExitCode\" || echo false)\n+\ttrust_exit_code=$(trust_exit_code \"$1\")\n \tif test \"$trust_exit_code\" = \"false\"\n \tthen\n \t\tcheck_unchanged\ndiff --git a/mergetools/deltawalker b/mergetools/deltawalker\nindex b3c71b623..ad978f83d 100644\n--- a/mergetools/deltawalker\n+++ b/mergetools/deltawalker\n@@ -19,3 +19,9 @@ merge_cmd () {\n translate_merge_tool_path() {\n \techo DeltaWalker\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\ndiff --git a/mergetools/diffmerge b/mergetools/diffmerge\nindex f138cb4e7..437b34996 100644\n--- a/mergetools/diffmerge\n+++ b/mergetools/diffmerge\n@@ -12,3 +12,9 @@ merge_cmd () {\n \t\t\t--result=\"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\ndiff --git a/mergetools/emerge b/mergetools/emerge\nindex 7b895fdb1..8c950d678 100644\n--- a/mergetools/emerge\n+++ b/mergetools/emerge\n@@ -20,3 +20,9 @@ merge_cmd () {\n translate_merge_tool_path() {\n \techo emacs\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\ndiff --git a/mergetools/kdiff3 b/mergetools/kdiff3\nindex 793d1293b..9d94876b9 100644\n--- a/mergetools/kdiff3\n+++ b/mergetools/kdiff3\n@@ -21,3 +21,9 @@ merge_cmd () {\n \t\t>/dev/null 2>&1\n \tfi\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\ndiff --git a/mergetools/kompare b/mergetools/kompare\nindex 433686c12..0ae0bdc02 100644\n--- a/mergetools/kompare\n+++ b/mergetools/kompare\n@@ -5,3 +5,9 @@ can_merge () {\n diff_cmd () {\n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\ndiff --git a/mergetools/tkdiff b/mergetools/tkdiff\nindex 618c438e8..d73792a21 100644\n--- a/mergetools/tkdiff\n+++ b/mergetools/tkdiff\n@@ -10,3 +10,9 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" -o \"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n }\n+\n+trust_exit_code () {\n+\t# Default to trustExitCode = true\n+\tgit config --bool \"mergetool.$1.trustExitCode\" ||\n+\techo true\n+}\n-- \n2.11.0.rc3.1.g2633b1d.dirty\n"},{"id":"306500","messageId":"xmqq4m2rle16.fsf@gitster.mtv.corp.google.com","threadId":"44549","inReplyTo":"20161128021554.GA30863@gmail.com","subject":"Re: trustExitCode doesn't apply to vimdiff mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-28T17:17:41Z","receivedAt":"2016-11-28T17:17:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> deltawalker, diffmerge, emerge, kdiff3, kompare, and tkdiff originally\n> provided behavior that matched trustExitCode=true.\n>\n> The default for all tools is trustExitCode=false, which conflicts with\n> these tools' defaults.  Allow tools to advertise their own default value\n> for trustExitCode so that users do not need to opt-in to the original\n> behavior.\n>\n> While this makes the default inconsistent between tools, it can still be\n> overridden, and it makes it consistent with the current Git behavior.\n\nI think this is sensible, because the way I look at this issue is\nthat in an ideal world, we would want all tool backends consistently\ngive us usable exit codes, but some tools are known to give unusable\nexit codes, so we ignore their exit codes by default.\n\nAs to the implementation, I think you can reduce the duplication by\nhaving each tool backend \n\n - export a new function that echos \"true\" or \"false\"; or\n - export a new function that returns true or false; or\n - set a variable whose value is either \"true\" or \"false\"\n\nand use that from the trust_exit_code() in git-mergetool--lib.sh.\nSomething like this (for the second alternative).\n\n    trust_exit_code () {\n        if git config --bool \"mergtool.$.trustExitCode\"\n\tthen\n\t\t:; # OK\n\telif mergetool_exitcode_trustable\n\tthen\n\t\techo true\n\telse\n\t\techo false\n        fi\n    }\n"}]}