{"thread":{"id":"31644","subject":"[PATCH] mergetool--lib: Allow custom commands to override built-ins","startedAt":"2012-09-25T07:48:11Z","lastAt":"2012-09-26T18:31:45Z","messageCount":4,"participants":["David Aguilar","Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199887","messageId":"1348559291-71739-1-git-send-email-davvid@gmail.com","threadId":"31644","inReplyTo":null,"subject":"[PATCH] mergetool--lib: Allow custom commands to override built-ins","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-09-25T07:48:11Z","receivedAt":"2012-09-25T07:48:11Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Allow users to override the default commands provided by the\nmergetools/* scriptlets.\n\nUsers occasionally run into problems where they expect to be\nable to override the built-in tool names.  The documentation\ndoes not explicitly mention that built-ins cannot be overridden,\nso it's easy to assume that it should work.\n\nLift this restriction so that built-in tools are handled the\nsame way as user-configured tools.  Add tests to guarantee this\nbehavior.\n\nA nice benefit of this change is that it protects users from\nhaving future versions of git trump their custom configuration\nwith a new built-in tool.\n\nC.f.:\n\nhttp://stackoverflow.com/questions/7435002/mergetool-from-gitconfig-being-ignored\nhttp://thread.gmane.org/gmane.comp.version-control.msysgit/13188\nhttp://thread.gmane.org/gmane.comp.version-control.git/148267\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThe Cc: list includes folks who were involved in the above threads.\nHopefully we can do without the workarounds now ;-)\n\n git-mergetool--lib.sh | 40 ++++++++++++++++++++++++++++++++++++++--\n mergetools/defaults   | 28 ++--------------------------\n t/t7610-mergetool.sh  | 13 +++++++++++++\n t/t7800-difftool.sh   | 11 +++++++++++\n 4 files changed, 64 insertions(+), 28 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 54cb708..f013a03 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -104,13 +104,49 @@ run_merge_tool () {\n \n \tif merge_mode\n \tthen\n-\t\tmerge_cmd \"$1\"\n+\t\trun_merge_cmd \"$1\"\n \telse\n-\t\tdiff_cmd \"$1\"\n+\t\trun_diff_cmd \"$1\"\n \tfi\n \treturn $status\n }\n \n+# Run a either a configured or built-in diff tool\n+run_diff_cmd () {\n+\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n+\tif test -n \"$merge_tool_cmd\"\n+\tthen\n+\t\t( eval $merge_tool_cmd )\n+\t\tstatus=$?\n+\t\treturn $status\n+\telse\n+\t\tdiff_cmd \"$1\"\n+\tfi\n+}\n+\n+# Run a either a configured or built-in merge tool\n+run_merge_cmd () {\n+\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n+\tif test -n \"$merge_tool_cmd\"\n+\tthen\n+\t\ttrust_exit_code=\"$(git config --bool \\\n+\t\t\tmergetool.\"$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\tstatus=$?\n+\t\t\tcheck_unchanged\n+\t\telse\n+\t\t\t( eval $merge_tool_cmd )\n+\t\t\tstatus=$?\n+\t\tfi\n+\t\treturn $status\n+\telse\n+\t\tmerge_cmd \"$1\"\n+\tfi\n+}\n+\n list_merge_tool_candidates () {\n \tif merge_mode\n \tthen\ndiff --git a/mergetools/defaults b/mergetools/defaults\nindex 1d8f2a3..21e63ec 100644\n--- a/mergetools/defaults\n+++ b/mergetools/defaults\n@@ -8,36 +8,12 @@ can_diff () {\n }\n \n diff_cmd () {\n-\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n-\tif test -z \"$merge_tool_cmd\"\n-\tthen\n-\t\tstatus=1\n-\t\tbreak\n-\tfi\n-\t( eval $merge_tool_cmd )\n-\tstatus=$?\n+\tstatus=1\n \treturn $status\n }\n \n merge_cmd () {\n-\tmerge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n-\tif test -z \"$merge_tool_cmd\"\n-\tthen\n-\t\tstatus=1\n-\t\tbreak\n-\tfi\n-\ttrust_exit_code=\"$(git config --bool \\\n-\t\tmergetool.\"$1\".trustExitCode || echo false)\"\n-\tif test \"$trust_exit_code\" = \"false\"\n-\tthen\n-\t\ttouch \"$BACKUP\"\n-\t\t( eval $merge_tool_cmd )\n-\t\tstatus=$?\n-\t\tcheck_unchanged\n-\telse\n-\t\t( eval $merge_tool_cmd )\n-\t\tstatus=$?\n-\tfi\n+\tstatus=1\n \treturn $status\n }\n \ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 6fa0c70..bc38737 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -509,4 +509,17 @@ test_expect_success 'file with no base' '\n     git reset --hard master >/dev/null 2>&1\n '\n \n+test_expect_success 'custom commands override built-ins' '\n+    git checkout -b test14 branch1 &&\n+    git config mergetool.defaults.cmd \"cat \\\"\\$REMOTE\\\" >\\\"\\$MERGED\\\"\" &&\n+    git config mergetool.defaults.trustExitCode true &&\n+    test_must_fail git merge master &&\n+    git mergetool --no-prompt --tool defaults -- both &&\n+    echo master both added >expected &&\n+    test_cmp both expected &&\n+    git config --unset mergetool.defaults.cmd &&\n+    git config --unset mergetool.defaults.trustExitCode &&\n+    git reset --hard master >/dev/null 2>&1\n+'\n+\n test_done\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 9c3e997..eb1d3f8 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -76,6 +76,17 @@ test_expect_success PERL 'custom commands' '\n \ttest \"$diff\" = \"branch\"\n '\n \n+# Ensures that a custom difftool.<tool>.cmd overrides built-ins\n+test_expect_success PERL 'custom commands override built-ins' '\n+\trestore_test_defaults &&\n+\tgit config difftool.defaults.cmd \"cat \\$REMOTE\" &&\n+\n+\tdiff=$(git difftool --tool defaults --no-prompt branch) &&\n+\ttest \"$diff\" = \"master\" &&\n+\n+\tgit config --unset difftool.defaults.cmd\n+'\n+\n # Ensures that git-difftool ignores bogus --tool values\n test_expect_success PERL 'difftool ignores bad --tool values' '\n \tdiff=$(git difftool --no-prompt --tool=bad-tool branch)\n-- \n1.7.12.1.402.g28d36ee\n"},{"id":"199933","messageId":"CALkWK0naJt840LfMNBM7EtdyLE5nmJeAsxG-Wttj8c84O5X7jA@mail.gmail.com","threadId":"31644","inReplyTo":"1348559291-71739-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-09-26T10:03:18Z","receivedAt":"2012-09-26T10:03:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi David,\n\nDavid Aguilar wrote:\n>  diff_cmd () {\n> -       merge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n> -       if test -z \"$merge_tool_cmd\"\n> -       then\n> -               status=1\n> -               break\n> -       fi\n> -       ( eval $merge_tool_cmd )\n> -       status=$?\n> +       status=1\n>         return $status\n>  }\n\nNit: Why not return 1, instead of setting $status and returning it?\n\nRam\n"},{"id":"199946","messageId":"7vtxukubds.fsf@alter.siamese.dyndns.org","threadId":"31644","inReplyTo":"CALkWK0naJt840LfMNBM7EtdyLE5nmJeAsxG-Wttj8c84O5X7jA@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-26T14:06:23Z","receivedAt":"2012-09-26T14:06:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Hi David,\n>\n> David Aguilar wrote:\n>>  diff_cmd () {\n>> -       merge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n>> -       if test -z \"$merge_tool_cmd\"\n>> -       then\n>> -               status=1\n>> -               break\n>> -       fi\n>> -       ( eval $merge_tool_cmd )\n>> -       status=$?\n>> +       status=1\n>>         return $status\n>>  }\n>\n> Nit: Why not return 1, instead of setting $status and returning it?\n\nPerhaps because the caller \"run_merge_tool\" pays attention to\n$status that is a global variable?\n\nHave you traced the call chain?\n"},{"id":"199957","messageId":"CAJDDKr4OkvAvRQFjp0NuzYkE2DSdvXh1vvTdKeaCq+bvQA4kuQ@mail.gmail.com","threadId":"31644","inReplyTo":"7vtxukubds.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetool--lib: Allow custom commands to override built-ins","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-09-26T18:31:45Z","receivedAt":"2012-09-26T18:31:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Sep 26, 2012 at 7:06 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> Hi David,\n>>\n>> David Aguilar wrote:\n>>>  diff_cmd () {\n>>> -       merge_tool_cmd=\"$(get_merge_tool_cmd \"$1\")\"\n>>> -       if test -z \"$merge_tool_cmd\"\n>>> -       then\n>>> -               status=1\n>>> -               break\n>>> -       fi\n>>> -       ( eval $merge_tool_cmd )\n>>> -       status=$?\n>>> +       status=1\n>>>         return $status\n>>>  }\n>>\n>> Nit: Why not return 1, instead of setting $status and returning it?\n>\n> Perhaps because the caller \"run_merge_tool\" pays attention to\n> $status that is a global variable?\n>\n> Have you traced the call chain?\n\nExactly.  I would like to eliminate globals whenever possible,\nbut this particular topic involved refactoring which aimed to keep\nexisting behavior w.r.t these variables unchanged.\n-- \nDavid\n"}]}