{"thread":{"id":"44563","subject":"[PATCH 1/2] mergetool: honor mergetool.$tool.trustExitCode for built-in tools","startedAt":"2016-11-29T09:40:08Z","lastAt":"2016-11-29T18:58:39Z","messageCount":2,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"306537","messageId":"20161129093807.24637-1-davvid@gmail.com","threadId":"44563","inReplyTo":null,"subject":"[PATCH 1/2] mergetool: honor mergetool.$tool.trustExitCode for built-in tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-11-29T09:38:07Z","receivedAt":"2016-11-29T09:40:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Built-in merge tools contain a hard-coded assumption about\nwhether or not a tool's exit code can be trusted to determine\nthe success or failure of a merge.  Tools whose exit codes are\nnot trusted contain calls to check_unchanged() in their\nmerge_cmd() functions.\n\nA problem with this is that the trustExitCode configuration is\nnot honored for built-in tools.\n\nTeach built-in tools to honor the trustExitCode configuration.\nExtend run_merge_cmd() so that it is responsible for calling\ncheck_unchanged() when a tool's exit code cannot be trusted.\nRemove check_unchanged() calls from scriptlets since they are no\nlonger responsible for calling it.\n\nWhen no configuration is present, exit_code_trustable() is\nchecked to see whether the exit code should be trusted.\nThe default implementation returns false.\n\nTools whose exit codes can be trusted override\nexit_code_trustable() to true.\n\nReported-by: Dun Peal <dunpealer@gmail.com>\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh    | 56 ++++++++++++++++++++++++++++++++++++++----------\n mergetools/araxis        |  2 --\n mergetools/bc            |  2 --\n mergetools/codecompare   |  2 --\n mergetools/deltawalker   |  6 +++++-\n mergetools/diffmerge     |  4 ++++\n mergetools/diffuse       |  2 --\n mergetools/ecmerge       |  2 --\n mergetools/emerge        |  4 ++++\n mergetools/examdiff      |  2 --\n mergetools/kdiff3        |  4 ++++\n mergetools/kompare       |  4 ++++\n mergetools/meld          |  3 +--\n mergetools/opendiff      |  2 --\n mergetools/p4merge       |  2 --\n mergetools/tkdiff        |  4 ++++\n mergetools/tortoisemerge |  2 --\n mergetools/vimdiff       |  2 --\n mergetools/winmerge      |  2 --\n mergetools/xxdiff        |  2 --\n 20 files changed, 71 insertions(+), 38 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9abd00be2..9a8b97a2a 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@@ -162,6 +153,28 @@ setup_tool () {\n \t\techo \"$1\"\n \t}\n \n+\t# Most tools' exit codes cannot be trusted, so By default we ignore\n+\t# their exit code and check the merged file's modification time in\n+\t# check_unchanged() to determine whether or not the merge was\n+\t# successful.  The return value from run_merge_cmd, by default, is\n+\t# determined by check_unchanged().\n+\t#\n+\t# When a tool's exit code can be trusted then the return value from\n+\t# run_merge_cmd is simply the tool's exit code, and check_unchanged()\n+\t# is not called.\n+\t#\n+\t# The return value of exit_code_trustable() tells us whether or not we\n+\t# can trust the tool's exit code.\n+\t#\n+\t# User-defined and built-in tools default to false.\n+\t# Built-in tools advertise that their exit code is trustable by\n+\t# redefining exit_code_trustable() to true.\n+\n+\texit_code_trustable () {\n+\t\tfalse\n+\t}\n+\n+\n \tif ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n \t\tsetup_user_tool\n@@ -197,6 +210,19 @@ get_merge_tool_cmd () {\n \tfi\n }\n \n+trust_exit_code () {\n+\tif git config --bool \"mergetool.$1.trustExitCode\"\n+\tthen\n+\t\t:; # OK\n+\telif exit_code_trustable\n+\tthen\n+\t\techo true\n+\telse\n+\t\techo false\n+\tfi\n+}\n+\n+\n # Entry point for running tools\n run_merge_tool () {\n \t# If GIT_PREFIX is empty then we cannot use it in tools\n@@ -225,7 +251,15 @@ run_diff_cmd () {\n \n # Run a either a configured or built-in merge tool\n run_merge_cmd () {\n-\tmerge_cmd \"$1\"\n+\tmergetool_trust_exit_code=$(trust_exit_code \"$1\")\n+\tif test \"$mergetool_trust_exit_code\" = \"true\"\n+\tthen\n+\t\tmerge_cmd \"$1\"\n+\telse\n+\t\ttouch \"$BACKUP\"\n+\t\tmerge_cmd \"$1\"\n+\t\tcheck_unchanged\n+\tfi\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/deltawalker b/mergetools/deltawalker\nindex b3c71b623..ee6f374bc 100644\n--- a/mergetools/deltawalker\n+++ b/mergetools/deltawalker\n@@ -16,6 +16,10 @@ merge_cmd () {\n \tfi >/dev/null 2>&1\n }\n \n-translate_merge_tool_path() {\n+translate_merge_tool_path () {\n \techo DeltaWalker\n }\n+\n+exit_code_trustable () {\n+\ttrue\n+}\ndiff --git a/mergetools/diffmerge b/mergetools/diffmerge\nindex f138cb4e7..9b6355b98 100644\n--- a/mergetools/diffmerge\n+++ b/mergetools/diffmerge\n@@ -12,3 +12,7 @@ merge_cmd () {\n \t\t\t--result=\"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n }\n+\n+exit_code_trustable () {\n+\ttrue\n+}\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/emerge b/mergetools/emerge\nindex 7b895fdb1..d1ce513ff 100644\n--- a/mergetools/emerge\n+++ b/mergetools/emerge\n@@ -20,3 +20,7 @@ merge_cmd () {\n translate_merge_tool_path() {\n \techo emacs\n }\n+\n+exit_code_trustable () {\n+\ttrue\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/kdiff3 b/mergetools/kdiff3\nindex 793d1293b..0264ed5b2 100644\n--- a/mergetools/kdiff3\n+++ b/mergetools/kdiff3\n@@ -21,3 +21,7 @@ merge_cmd () {\n \t\t>/dev/null 2>&1\n \tfi\n }\n+\n+exit_code_trustable () {\n+\ttrue\n+}\ndiff --git a/mergetools/kompare b/mergetools/kompare\nindex 433686c12..e8c0bfa67 100644\n--- a/mergetools/kompare\n+++ b/mergetools/kompare\n@@ -5,3 +5,7 @@ can_merge () {\n diff_cmd () {\n \t\"$merge_tool_path\" \"$LOCAL\" \"$REMOTE\"\n }\n+\n+exit_code_trustable () {\n+\ttrue\n+}\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/tkdiff b/mergetools/tkdiff\nindex 618c438e8..eee5cb57e 100644\n--- a/mergetools/tkdiff\n+++ b/mergetools/tkdiff\n@@ -10,3 +10,7 @@ merge_cmd () {\n \t\t\"$merge_tool_path\" -o \"$MERGED\" \"$LOCAL\" \"$REMOTE\"\n \tfi\n }\n+\n+exit_code_trustable () {\n+\ttrue\n+}\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.6.g2e567fd\n\n"},{"id":"306556","messageId":"xmqq37iaglm1.fsf@gitster.mtv.corp.google.com","threadId":"44563","inReplyTo":"20161129093807.24637-1-davvid@gmail.com","subject":"Re: [PATCH 1/2] mergetool: honor mergetool.$tool.trustExitCode for built-in tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-29T18:57:26Z","receivedAt":"2016-11-29T18:58:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Built-in merge tools contain a hard-coded assumption about\n> whether or not a tool's exit code can be trusted to determine\n> the success or failure of a merge.  Tools whose exit codes are\n> not trusted contain calls to check_unchanged() in their\n> merge_cmd() functions.\n>\n> A problem with this is that the trustExitCode configuration is\n> not honored for built-in tools.\n>\n> Teach built-in tools to honor the trustExitCode configuration.\n> Extend run_merge_cmd() so that it is responsible for calling\n> check_unchanged() when a tool's exit code cannot be trusted.\n> Remove check_unchanged() calls from scriptlets since they are no\n> longer responsible for calling it.\n>\n> When no configuration is present, exit_code_trustable() is\n> checked to see whether the exit code should be trusted.\n> The default implementation returns false.\n>\n> Tools whose exit codes can be trusted override\n> exit_code_trustable() to true.\n>\n> Reported-by: Dun Peal <dunpealer@gmail.com>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh    | 56 ++++++++++++++++++++++++++++++++++++++----------\n>  mergetools/araxis        |  2 --\n>  mergetools/bc            |  2 --\n>  mergetools/codecompare   |  2 --\n>  mergetools/deltawalker   |  6 +++++-\n>  mergetools/diffmerge     |  4 ++++\n>  mergetools/diffuse       |  2 --\n>  mergetools/ecmerge       |  2 --\n>  mergetools/emerge        |  4 ++++\n>  mergetools/examdiff      |  2 --\n>  mergetools/kdiff3        |  4 ++++\n>  mergetools/kompare       |  4 ++++\n>  mergetools/meld          |  3 +--\n>  mergetools/opendiff      |  2 --\n>  mergetools/p4merge       |  2 --\n>  mergetools/tkdiff        |  4 ++++\n>  mergetools/tortoisemerge |  2 --\n>  mergetools/vimdiff       |  2 --\n>  mergetools/winmerge      |  2 --\n>  mergetools/xxdiff        |  2 --\n>  20 files changed, 71 insertions(+), 38 deletions(-)\n\n\nOK, so the idea is that the backends that used to touch $BACKUP and\nused to call check_unchanged stop doing these two things, while\nother backends override exit_code_trustable().  That makes the\nchange to the backends very straightforward to understand.\n\n>  # Run a either a configured or built-in merge tool\n>  run_merge_cmd () {\n> -\tmerge_cmd \"$1\"\n> +\tmergetool_trust_exit_code=$(trust_exit_code \"$1\")\n> +\tif test \"$mergetool_trust_exit_code\" = \"true\"\n> +\tthen\n> +\t\tmerge_cmd \"$1\"\n> +\telse\n> +\t\ttouch \"$BACKUP\"\n> +\t\tmerge_cmd \"$1\"\n> +\t\tcheck_unchanged\n> +\tfi\n>  }\n\nLooks sensible.  Thanks.  Will queue.\n"}]}