{"thread":{"id":"34167","subject":"[PATCH] mergetool--lib: refactor {diff,merge}_cmd logic","startedAt":"2013-06-16T17:51:22Z","lastAt":"2013-06-17T20:13:16Z","messageCount":3,"participants":["John Keeping","David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221012","messageId":"828585499e535649d14ef0ecd0f53403aefb10c2.1371405056.git.john@keeping.me.uk","threadId":"34167","inReplyTo":null,"subject":"[PATCH] mergetool--lib: refactor {diff,merge}_cmd logic","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-06-16T17:51:22Z","receivedAt":"2013-06-16T17:51:22Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Instead of needing a wrapper to call the diff/merge command, simply\nprovide the diff_cmd and merge_cmd functions for user-specified tools in\nthe same way as we do for built-in tools.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 82 ++++++++++++++++++++++-----------------------------\n 1 file changed, 35 insertions(+), 47 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex e338be5..6a72106 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -114,6 +114,33 @@ valid_tool () {\n \ttest -n \"$cmd\"\n }\n \n+setup_user_tool () {\n+\tmerge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n+\ttest -n \"$merge_tool_cmd\" || return 1\n+\n+\tdiff_cmd () {\n+\t\t( eval $merge_tool_cmd )\n+\t\tstatus=$?\n+\t\treturn $status\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\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+\t}\n+}\n+\n setup_tool () {\n \ttool=\"$1\"\n \n@@ -142,15 +169,15 @@ setup_tool () {\n \n \tif ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n-\t\t# Use a special return code for this case since we want to\n-\t\t# source \"defaults\" even when an explicit tool path is\n-\t\t# configured since the user can use that to override the\n-\t\t# default path in the scriptlet.\n-\t\treturn 2\n+\t\tsetup_user_tool\n+\t\treturn $?\n \tfi\n \n \t# Load the redefined functions\n \t. \"$MERGE_TOOLS_DIR/$tool\"\n+\t# Now let the user override the default command for the tool.  If\n+\t# they have not done so then this will return 1 which we ignore.\n+\tsetup_user_tool\n \n \tif merge_mode && ! can_merge\n \tthen\n@@ -187,20 +214,7 @@ run_merge_tool () {\n \tstatus=0\n \n \t# Bring tool-specific functions into scope\n-\tsetup_tool \"$1\"\n-\texitcode=$?\n-\tcase $exitcode in\n-\t0)\n-\t\t:\n-\t\t;;\n-\t2)\n-\t\t# The configured tool is not a built-in tool.\n-\t\ttest -n \"$merge_tool_path\" || return 1\n-\t\t;;\n-\t*)\n-\t\treturn $exitcode\n-\t\t;;\n-\tesac\n+\tsetup_tool \"$1\" || return 1\n \n \tif merge_mode\n \tthen\n@@ -213,38 +227,12 @@ run_merge_tool () {\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+\tdiff_cmd \"$1\"\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\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\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+\tmerge_cmd \"$1\"\n }\n \n list_merge_tool_candidates () {\n-- \n1.8.3.779.g691e267\n"},{"id":"221034","messageId":"CAJDDKr5ewcYgtZn-oCQ5b=rGix5EALRdiSaNMatOwtP75e0h3g@mail.gmail.com","threadId":"34167","inReplyTo":"828585499e535649d14ef0ecd0f53403aefb10c2.1371405056.git.john@keeping.me.uk","subject":"Re: [PATCH] mergetool--lib: refactor {diff,merge}_cmd logic","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-06-17T05:31:29Z","receivedAt":"2013-06-17T05:31:29Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jun 16, 2013 at 10:51 AM, John Keeping <john@keeping.me.uk> wrote:\n> Instead of needing a wrapper to call the diff/merge command, simply\n> provide the diff_cmd and merge_cmd functions for user-specified tools in\n> the same way as we do for built-in tools.\n>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n\nThis is a nice cleanup.\n\nAcked-by: David Aguilar <davvid@gmail.com>\n\n\n>  git-mergetool--lib.sh | 82 ++++++++++++++++++++++-----------------------------\n>  1 file changed, 35 insertions(+), 47 deletions(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index e338be5..6a72106 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -114,6 +114,33 @@ valid_tool () {\n>         test -n \"$cmd\"\n>  }\n>\n> +setup_user_tool () {\n> +       merge_tool_cmd=$(get_merge_tool_cmd \"$tool\")\n> +       test -n \"$merge_tool_cmd\" || return 1\n> +\n> +       diff_cmd () {\n> +               ( eval $merge_tool_cmd )\n> +               status=$?\n> +               return $status\n> +       }\n> +\n> +       merge_cmd () {\n> +               trust_exit_code=$(git config --bool \\\n> +                       \"mergetool.$1.trustExitCode\" || echo false)\n> +               if test \"$trust_exit_code\" = \"false\"\n> +               then\n> +                       touch \"$BACKUP\"\n> +                       ( eval $merge_tool_cmd )\n> +                       status=$?\n> +                       check_unchanged\n> +               else\n> +                       ( eval $merge_tool_cmd )\n> +                       status=$?\n> +               fi\n> +               return $status\n> +       }\n> +}\n> +\n>  setup_tool () {\n>         tool=\"$1\"\n>\n> @@ -142,15 +169,15 @@ setup_tool () {\n>\n>         if ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n>         then\n> -               # Use a special return code for this case since we want to\n> -               # source \"defaults\" even when an explicit tool path is\n> -               # configured since the user can use that to override the\n> -               # default path in the scriptlet.\n> -               return 2\n> +               setup_user_tool\n> +               return $?\n>         fi\n>\n>         # Load the redefined functions\n>         . \"$MERGE_TOOLS_DIR/$tool\"\n> +       # Now let the user override the default command for the tool.  If\n> +       # they have not done so then this will return 1 which we ignore.\n> +       setup_user_tool\n>\n>         if merge_mode && ! can_merge\n>         then\n> @@ -187,20 +214,7 @@ run_merge_tool () {\n>         status=0\n>\n>         # Bring tool-specific functions into scope\n> -       setup_tool \"$1\"\n> -       exitcode=$?\n> -       case $exitcode in\n> -       0)\n> -               :\n> -               ;;\n> -       2)\n> -               # The configured tool is not a built-in tool.\n> -               test -n \"$merge_tool_path\" || return 1\n> -               ;;\n> -       *)\n> -               return $exitcode\n> -               ;;\n> -       esac\n> +       setup_tool \"$1\" || return 1\n>\n>         if merge_mode\n>         then\n> @@ -213,38 +227,12 @@ run_merge_tool () {\n>\n>  # Run a either a configured or built-in diff tool\n>  run_diff_cmd () {\n> -       merge_tool_cmd=$(get_merge_tool_cmd \"$1\")\n> -       if test -n \"$merge_tool_cmd\"\n> -       then\n> -               ( eval $merge_tool_cmd )\n> -               status=$?\n> -               return $status\n> -       else\n> -               diff_cmd \"$1\"\n> -       fi\n> +       diff_cmd \"$1\"\n>  }\n>\n>  # Run a either a configured or built-in merge tool\n>  run_merge_cmd () {\n> -       merge_tool_cmd=$(get_merge_tool_cmd \"$1\")\n> -       if test -n \"$merge_tool_cmd\"\n> -       then\n> -               trust_exit_code=$(git config --bool \\\n> -                       \"mergetool.$1.trustExitCode\" || echo false)\n> -               if test \"$trust_exit_code\" = \"false\"\n> -               then\n> -                       touch \"$BACKUP\"\n> -                       ( eval $merge_tool_cmd )\n> -                       status=$?\n> -                       check_unchanged\n> -               else\n> -                       ( eval $merge_tool_cmd )\n> -                       status=$?\n> -               fi\n> -               return $status\n> -       else\n> -               merge_cmd \"$1\"\n> -       fi\n> +       merge_cmd \"$1\"\n>  }\n>\n>  list_merge_tool_candidates () {\n> --\n> 1.8.3.779.g691e267\n>\n\n\n\n--\nDavid\n"},{"id":"221098","messageId":"7vtxkwbin7.fsf@alter.siamese.dyndns.org","threadId":"34167","inReplyTo":"CAJDDKr5ewcYgtZn-oCQ5b=rGix5EALRdiSaNMatOwtP75e0h3g@mail.gmail.com","subject":"Re: [PATCH] mergetool--lib: refactor {diff,merge}_cmd logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-17T20:13:16Z","receivedAt":"2013-06-17T20:13:16Z","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> On Sun, Jun 16, 2013 at 10:51 AM, John Keeping <john@keeping.me.uk> wrote:\n>> Instead of needing a wrapper to call the diff/merge command, simply\n>> provide the diff_cmd and merge_cmd functions for user-specified tools in\n>> the same way as we do for built-in tools.\n>>\n>> Signed-off-by: John Keeping <john@keeping.me.uk>\n>> ---\n>\n> This is a nice cleanup.\n>\n> Acked-by: David Aguilar <davvid@gmail.com>\n\nThanks, both.\n"}]}