{"thread":{"id":"32731","subject":"[PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","startedAt":"2013-01-26T06:50:58Z","lastAt":"2013-01-27T06:05:29Z","messageCount":8,"participants":["David Aguilar","John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"207866","messageId":"1359183058-51835-1-git-send-email-davvid@gmail.com","threadId":"32731","inReplyTo":null,"subject":"[PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-26T06:50:58Z","receivedAt":"2013-01-26T06:50:58Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Remove the exceptions for \"vim\" and \"defaults\" in the mergetool library\nso that every filename in mergetools/ matches 1:1 with the name of a\nvalid built-in tool.\n\nMake common functions available in $MERGE_TOOLS_DIR/include/.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis should make things ok on platforms where we don't have symlinks.\n\n Makefile                                     |  2 +-\n git-mergetool--lib.sh                        | 41 +++++++++++-----------------\n mergetools/gvimdiff                          |  1 +\n mergetools/gvimdiff2                         |  1 +\n mergetools/{defaults => include/defaults.sh} |  0\n mergetools/{vim => include/vim.sh}           |  0\n mergetools/vimdiff                           |  1 +\n mergetools/vimdiff2                          |  1 +\n 8 files changed, 21 insertions(+), 26 deletions(-)\n create mode 100644 mergetools/gvimdiff\n create mode 100644 mergetools/gvimdiff2\n rename mergetools/{defaults => include/defaults.sh} (100%)\n rename mergetools/{vim => include/vim.sh} (100%)\n create mode 100644 mergetools/vimdiff\n create mode 100644 mergetools/vimdiff2\n\ndiff --git a/Makefile b/Makefile\nindex f69979e..3bc6eb5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2724,7 +2724,7 @@ install: all\n \t$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n-\t$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n+\tcp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n ifndef NO_GETTEXT\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n \t(cd po/build/locale && $(TAR) cf - .) | \\\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex aa38bd1..c866ed8 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -1,5 +1,7 @@\n #!/bin/sh\n # git-mergetool--lib is a library for common merge tool functions\n+MERGE_TOOLS_DIR=\"$(git --exec-path)/mergetools\"\n+\n diff_mode() {\n \ttest \"$TOOL_MODE\" = diff\n }\n@@ -44,25 +46,14 @@ valid_tool () {\n }\n \n setup_tool () {\n-\tcase \"$1\" in\n-\tvim*|gvim*)\n-\t\ttool=vim\n-\t\t;;\n-\t*)\n-\t\ttool=\"$1\"\n-\t\t;;\n-\tesac\n-\tmergetools=\"$(git --exec-path)/mergetools\"\n+\ttool=\"$1\"\n \n-\t# Load the default definitions\n-\t. \"$mergetools/defaults\"\n-\tif ! test -f \"$mergetools/$tool\"\n+\tif ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n \tthen\n \t\treturn 1\n \tfi\n-\n-\t# Load the redefined functions\n-\t. \"$mergetools/$tool\"\n+\t. \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n+\t. \"$MERGE_TOOLS_DIR/$tool\"\n \n \tif merge_mode && ! can_merge\n \tthen\n@@ -99,7 +90,7 @@ run_merge_tool () {\n \tbase_present=\"$2\"\n \tstatus=0\n \n-\t# Bring tool-specific functions into scope\n+\t# Bring tool specific functions into scope\n \tsetup_tool \"$1\"\n \n \tif merge_mode\n@@ -177,18 +168,17 @@ list_merge_tool_candidates () {\n show_tool_help () {\n \tunavailable= available= LF='\n '\n-\n-\tscriptlets=\"$(git --exec-path)\"/mergetools\n-\tfor i in \"$scriptlets\"/*\n+\tfor i in \"$MERGE_TOOLS_DIR\"/*\n \tdo\n-\t\t. \"$scriptlets\"/defaults\n-\t\t. \"$i\"\n-\n-\t\ttool=\"$(basename \"$i\")\"\n-\t\tif test \"$tool\" = \"defaults\"\n+\t\tif test -d \"$i\"\n \t\tthen\n \t\t\tcontinue\n-\t\telif merge_mode && ! can_merge\n+\t\tfi\n+\n+\t\t. \"$MERGE_TOOLS_DIR\"/include/defaults.sh\n+\t\t. \"$i\"\n+\n+\t\tif merge_mode && ! can_merge\n \t\tthen\n \t\t\tcontinue\n \t\telif diff_mode && ! can_diff\n@@ -196,6 +186,7 @@ show_tool_help () {\n \t\t\tcontinue\n \t\tfi\n \n+\t\ttool=$(basename \"$i\")\n \t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n \t\tthen\ndiff --git a/mergetools/gvimdiff b/mergetools/gvimdiff\nnew file mode 100644\nindex 0000000..f5890b1\n--- /dev/null\n+++ b/mergetools/gvimdiff\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/include/vim.sh\"\ndiff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2\nnew file mode 100644\nindex 0000000..f5890b1\n--- /dev/null\n+++ b/mergetools/gvimdiff2\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/include/vim.sh\"\ndiff --git a/mergetools/defaults b/mergetools/include/defaults.sh\nsimilarity index 100%\nrename from mergetools/defaults\nrename to mergetools/include/defaults.sh\ndiff --git a/mergetools/vim b/mergetools/include/vim.sh\nsimilarity index 100%\nrename from mergetools/vim\nrename to mergetools/include/vim.sh\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nnew file mode 100644\nindex 0000000..f5890b1\n--- /dev/null\n+++ b/mergetools/vimdiff\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/include/vim.sh\"\ndiff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2\nnew file mode 100644\nindex 0000000..f5890b1\n--- /dev/null\n+++ b/mergetools/vimdiff2\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n-- \n1.8.0.6.ge6f188f.dirty\n"},{"id":"207876","messageId":"20130126121202.GH7498@serenity.lan","threadId":"32731","inReplyTo":"1359183058-51835-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-26T12:12:02Z","receivedAt":"2013-01-26T12:12:02Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:\n> Remove the exceptions for \"vim\" and \"defaults\" in the mergetool library\n> so that every filename in mergetools/ matches 1:1 with the name of a\n> valid built-in tool.\n> \n> Make common functions available in $MERGE_TOOLS_DIR/include/.\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> \n> diff --git a/Makefile b/Makefile\n> index f69979e..3bc6eb5 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2724,7 +2724,7 @@ install: all\n>  \t$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n>  \t$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n>  \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> -\t$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> +\tcp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n\nShouldn't this be more like this?\n\n\t$(INSTALL) -m 644 $(subst include,,$(wildcard mergetools/*)) \\\n\t\t'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n\t$(INSTALL) -m 644 mergetools/include/* \\\n\t\t'$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n\nI'm not sure creating an 'include' directory actually buys us much over\ndeclaring that 'vimdiff' is the real script and the others just source\nit.  Either way there is a single special entry in the mergetools\ndirectory.\n\n>  ifndef NO_GETTEXT\n>  \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n>  \t(cd po/build/locale && $(TAR) cf - .) | \\\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index aa38bd1..c866ed8 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -1,5 +1,7 @@\n>  #!/bin/sh\n>  # git-mergetool--lib is a library for common merge tool functions\n> +MERGE_TOOLS_DIR=\"$(git --exec-path)/mergetools\"\n> +\n>  diff_mode() {\n>  \ttest \"$TOOL_MODE\" = diff\n>  }\n> @@ -44,25 +46,14 @@ valid_tool () {\n>  }\n>  \n>  setup_tool () {\n> -\tcase \"$1\" in\n> -\tvim*|gvim*)\n> -\t\ttool=vim\n> -\t\t;;\n> -\t*)\n> -\t\ttool=\"$1\"\n> -\t\t;;\n> -\tesac\n> -\tmergetools=\"$(git --exec-path)/mergetools\"\n> +\ttool=\"$1\"\n\nUnnecessary quoting.\n\n> -\t# Load the default definitions\n> -\t. \"$mergetools/defaults\"\n> -\tif ! test -f \"$mergetools/$tool\"\n> +\tif ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n>  \tthen\n>  \t\treturn 1\n>  \tfi\n> -\n> -\t# Load the redefined functions\n> -\t. \"$mergetools/$tool\"\n> +\t. \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n> +\t. \"$MERGE_TOOLS_DIR/$tool\"\n>  \n>  \tif merge_mode && ! can_merge\n>  \tthen\n> @@ -99,7 +90,7 @@ run_merge_tool () {\n>  \tbase_present=\"$2\"\n>  \tstatus=0\n>  \n> -\t# Bring tool-specific functions into scope\n> +\t# Bring tool specific functions into scope\n\nThis isn't related to this change (and I think tool-specific is more\ncorrect anyway).\n\n>  \tsetup_tool \"$1\"\n>  \n>  \tif merge_mode\n> @@ -177,18 +168,17 @@ list_merge_tool_candidates () {\n>  show_tool_help () {\n>  \tunavailable= available= LF='\n>  '\n> -\n> -\tscriptlets=\"$(git --exec-path)\"/mergetools\n> -\tfor i in \"$scriptlets\"/*\n> +\tfor i in \"$MERGE_TOOLS_DIR\"/*\n>  \tdo\n> -\t\t. \"$scriptlets\"/defaults\n> -\t\t. \"$i\"\n> -\n> -\t\ttool=\"$(basename \"$i\")\"\n> -\t\tif test \"$tool\" = \"defaults\"\n> +\t\tif test -d \"$i\"\n>  \t\tthen\n>  \t\t\tcontinue\n> -\t\telif merge_mode && ! can_merge\n> +\t\tfi\n> +\n> +\t\t. \"$MERGE_TOOLS_DIR\"/include/defaults.sh\n> +\t\t. \"$i\"\n> +\n> +\t\tif merge_mode && ! can_merge\n>  \t\tthen\n>  \t\t\tcontinue\n>  \t\telif diff_mode && ! can_diff\n> @@ -196,6 +186,7 @@ show_tool_help () {\n>  \t\t\tcontinue\n>  \t\tfi\n\n\nI'd prefer to see my change to setup_tool done before this so that we\ncan just use:\n\n\tsetup_tool \"$tool\" 2>/dev/null || continue\n\nfor the above block.  I'll send a fixed version in a couple of minutes.\n\n> +\t\ttool=$(basename \"$i\")\n>  \t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n>  \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n>  \t\tthen\n> diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff\n> new file mode 100644\n> index 0000000..f5890b1\n> --- /dev/null\n> +++ b/mergetools/gvimdiff\n> @@ -0,0 +1 @@\n> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n> diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2\n> new file mode 100644\n> index 0000000..f5890b1\n> --- /dev/null\n> +++ b/mergetools/gvimdiff2\n> @@ -0,0 +1 @@\n> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n> diff --git a/mergetools/defaults b/mergetools/include/defaults.sh\n> similarity index 100%\n> rename from mergetools/defaults\n> rename to mergetools/include/defaults.sh\n> diff --git a/mergetools/vim b/mergetools/include/vim.sh\n> similarity index 100%\n> rename from mergetools/vim\n> rename to mergetools/include/vim.sh\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> new file mode 100644\n> index 0000000..f5890b1\n> --- /dev/null\n> +++ b/mergetools/vimdiff\n> @@ -0,0 +1 @@\n> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n> diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2\n> new file mode 100644\n> index 0000000..f5890b1\n> --- /dev/null\n> +++ b/mergetools/vimdiff2\n> @@ -0,0 +1 @@\n> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n"},{"id":"207889","messageId":"CAJDDKr5UMyJOthSOPuChx7BCpcGwtmYcnVMh83q9kgCWSxDp4w@mail.gmail.com","threadId":"32731","inReplyTo":"20130126121202.GH7498@serenity.lan","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-26T20:40:23Z","receivedAt":"2013-01-26T20:40:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:\n>> Remove the exceptions for \"vim\" and \"defaults\" in the mergetool library\n>> so that every filename in mergetools/ matches 1:1 with the name of a\n>> valid built-in tool.\n>>\n>> Make common functions available in $MERGE_TOOLS_DIR/include/.\n>>\n>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>> ---\n>>\n>> diff --git a/Makefile b/Makefile\n>> index f69979e..3bc6eb5 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2724,7 +2724,7 @@ install: all\n>>       $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n>>       $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n>>       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>> -     $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>> +     cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>\n> Shouldn't this be more like this?\n>\n>         $(INSTALL) -m 644 $(subst include,,$(wildcard mergetools/*)) \\\n>                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n>         $(INSTALL) -m 644 mergetools/include/* \\\n>                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n>\n> I'm not sure creating an 'include' directory actually buys us much over\n> declaring that 'vimdiff' is the real script and the others just source\n> it.  Either way there is a single special entry in the mergetools\n> directory.\n>\n>>  ifndef NO_GETTEXT\n>>       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n>>       (cd po/build/locale && $(TAR) cf - .) | \\\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index aa38bd1..c866ed8 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -1,5 +1,7 @@\n>>  #!/bin/sh\n>>  # git-mergetool--lib is a library for common merge tool functions\n>> +MERGE_TOOLS_DIR=\"$(git --exec-path)/mergetools\"\n>> +\n>>  diff_mode() {\n>>       test \"$TOOL_MODE\" = diff\n>>  }\n>> @@ -44,25 +46,14 @@ valid_tool () {\n>>  }\n>>\n>>  setup_tool () {\n>> -     case \"$1\" in\n>> -     vim*|gvim*)\n>> -             tool=vim\n>> -             ;;\n>> -     *)\n>> -             tool=\"$1\"\n>> -             ;;\n>> -     esac\n>> -     mergetools=\"$(git --exec-path)/mergetools\"\n>> +     tool=\"$1\"\n>\n> Unnecessary quoting.\n>\n>> -     # Load the default definitions\n>> -     . \"$mergetools/defaults\"\n>> -     if ! test -f \"$mergetools/$tool\"\n>> +     if ! test -f \"$MERGE_TOOLS_DIR/$tool\"\n>>       then\n>>               return 1\n>>       fi\n>> -\n>> -     # Load the redefined functions\n>> -     . \"$mergetools/$tool\"\n>> +     . \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n>> +     . \"$MERGE_TOOLS_DIR/$tool\"\n>>\n>>       if merge_mode && ! can_merge\n>>       then\n>> @@ -99,7 +90,7 @@ run_merge_tool () {\n>>       base_present=\"$2\"\n>>       status=0\n>>\n>> -     # Bring tool-specific functions into scope\n>> +     # Bring tool specific functions into scope\n>\n> This isn't related to this change (and I think tool-specific is more\n> correct anyway).\n>\n>>       setup_tool \"$1\"\n>>\n>>       if merge_mode\n>> @@ -177,18 +168,17 @@ list_merge_tool_candidates () {\n>>  show_tool_help () {\n>>       unavailable= available= LF='\n>>  '\n>> -\n>> -     scriptlets=\"$(git --exec-path)\"/mergetools\n>> -     for i in \"$scriptlets\"/*\n>> +     for i in \"$MERGE_TOOLS_DIR\"/*\n>>       do\n>> -             . \"$scriptlets\"/defaults\n>> -             . \"$i\"\n>> -\n>> -             tool=\"$(basename \"$i\")\"\n>> -             if test \"$tool\" = \"defaults\"\n>> +             if test -d \"$i\"\n>>               then\n>>                       continue\n>> -             elif merge_mode && ! can_merge\n>> +             fi\n>> +\n>> +             . \"$MERGE_TOOLS_DIR\"/include/defaults.sh\n>> +             . \"$i\"\n>> +\n>> +             if merge_mode && ! can_merge\n>>               then\n>>                       continue\n>>               elif diff_mode && ! can_diff\n>> @@ -196,6 +186,7 @@ show_tool_help () {\n>>                       continue\n>>               fi\n>\n>\n> I'd prefer to see my change to setup_tool done before this so that we\n> can just use:\n>\n>         setup_tool \"$tool\" 2>/dev/null || continue\n>\n> for the above block.  I'll send a fixed version in a couple of minutes.\n\n\nI'll reroll this patch with your notes on top of your new patch later today.\n\nThanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).\n\n\n>\n>> +             tool=$(basename \"$i\")\n>>               merge_tool_path=$(translate_merge_tool_path \"$tool\")\n>>               if type \"$merge_tool_path\" >/dev/null 2>&1\n>>               then\n>> diff --git a/mergetools/gvimdiff b/mergetools/gvimdiff\n>> new file mode 100644\n>> index 0000000..f5890b1\n>> --- /dev/null\n>> +++ b/mergetools/gvimdiff\n>> @@ -0,0 +1 @@\n>> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n>> diff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2\n>> new file mode 100644\n>> index 0000000..f5890b1\n>> --- /dev/null\n>> +++ b/mergetools/gvimdiff2\n>> @@ -0,0 +1 @@\n>> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n>> diff --git a/mergetools/defaults b/mergetools/include/defaults.sh\n>> similarity index 100%\n>> rename from mergetools/defaults\n>> rename to mergetools/include/defaults.sh\n>> diff --git a/mergetools/vim b/mergetools/include/vim.sh\n>> similarity index 100%\n>> rename from mergetools/vim\n>> rename to mergetools/include/vim.sh\n>> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n>> new file mode 100644\n>> index 0000000..f5890b1\n>> --- /dev/null\n>> +++ b/mergetools/vimdiff\n>> @@ -0,0 +1 @@\n>> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n>> diff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2\n>> new file mode 100644\n>> index 0000000..f5890b1\n>> --- /dev/null\n>> +++ b/mergetools/vimdiff2\n>> @@ -0,0 +1 @@\n>> +. \"$MERGE_TOOLS_DIR/include/vim.sh\"\n\n\n\n-- \nDavid\n"},{"id":"207890","messageId":"20130126205434.GL7498@serenity.lan","threadId":"32731","inReplyTo":"CAJDDKr5UMyJOthSOPuChx7BCpcGwtmYcnVMh83q9kgCWSxDp4w@mail.gmail.com","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-26T20:54:34Z","receivedAt":"2013-01-26T20:54:34Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Jan 26, 2013 at 12:40:23PM -0800, David Aguilar wrote:\n> On Sat, Jan 26, 2013 at 4:12 AM, John Keeping <john@keeping.me.uk> wrote:\n> > On Fri, Jan 25, 2013 at 10:50:58PM -0800, David Aguilar wrote:\n> >> diff --git a/Makefile b/Makefile\n> >> index f69979e..3bc6eb5 100644\n> >> --- a/Makefile\n> >> +++ b/Makefile\n> >> @@ -2724,7 +2724,7 @@ install: all\n> >>       $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n> >>       $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n> >>       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> >> -     $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> >> +     cp -R mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> >\n> > Shouldn't this be more like this?\n> >\n> >         $(INSTALL) -m 644 $(subst include,,$(wildcard mergetools/*)) \\\n> >                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> >         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n> >         $(INSTALL) -m 644 mergetools/include/* \\\n> >                 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n> >\n> > I'm not sure creating an 'include' directory actually buys us much over\n> > declaring that 'vimdiff' is the real script and the others just source\n> > it.  Either way there is a single special entry in the mergetools\n> > directory.\n>\n> Thanks (and thanks for the Makefile hint.. I knew it was wrong! ;-).\n\nI think that version's still not right actually, the first line should\nprobably use filter-out not subst:\n\n\t$(INSTALL) -m 644 $(filter-out include,$(wildcard mergetools/*)) \\\n\t\t'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n\n\nJohn\n"},{"id":"207930","messageId":"7v8v7fz0ii.fsf@alter.siamese.dyndns.org","threadId":"32731","inReplyTo":"20130126121202.GH7498@serenity.lan","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T04:57:09Z","receivedAt":"2013-01-27T04:57:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> I'm not sure creating an 'include' directory actually buys us much over\n> declaring that 'vimdiff' is the real script and the others just source\n> it.\n\nIs 'include' really used for such a purpose?  It only houses defaults.sh\nas far as I can see.\n\nAs that defaults.sh file is used only to define trivially empty\nshell functions, I wonder if it is better to get rid of it, and\ndefine these functions in line in git-mergetool--lib.sh.  Such a\nchange would like the attached on top of the entire series.\n\n Makefile                       |  6 +-----\n git-mergetool--lib.sh          | 24 ++++++++++++++++++++++--\n mergetools/include/defaults.sh | 22 ----------------------\n 3 files changed, 23 insertions(+), 29 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 26f217f..f69979e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2724,11 +2724,7 @@ install: all\n \t$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n \t$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n-\t$(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \\\n-\t\t'$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n-\t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n-\t$(INSTALL) -m 644 mergetools/include/* \\\n-\t\t'$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n+\t$(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n ifndef NO_GETTEXT\n \t$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n \t(cd po/build/locale && $(TAR) cf - .) | \\\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 7ea7510..1d0fb12 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -57,8 +57,28 @@ setup_tool () {\n \t\treturn 2\n \tfi\n \n-\t# Load the default functions\n-\t. \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n+\t# Fallback definitions, to be overriden by tools.\n+\tcan_merge () {\n+\t\treturn 0\n+\t}\n+\n+\tcan_diff () {\n+\t\treturn 0\n+\t}\n+\n+\tdiff_cmd () {\n+\t\tstatus=1\n+\t\treturn $status\n+\t}\n+\n+\tmerge_cmd () {\n+\t\tstatus=1\n+\t\treturn $status\n+\t}\n+\n+\ttranslate_merge_tool_path () {\n+\t\techo \"$1\"\n+\t}\n \n \t# Load the redefined functions\n \t. \"$MERGE_TOOLS_DIR/$tool\"\ndiff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh\ndeleted file mode 100644\nindex 21e63ec..0000000\n--- a/mergetools/include/defaults.sh\n+++ /dev/null\n@@ -1,22 +0,0 @@\n-# Redefined by builtin tools\n-can_merge () {\n-\treturn 0\n-}\n-\n-can_diff () {\n-\treturn 0\n-}\n-\n-diff_cmd () {\n-\tstatus=1\n-\treturn $status\n-}\n-\n-merge_cmd () {\n-\tstatus=1\n-\treturn $status\n-}\n-\n-translate_merge_tool_path () {\n-\techo \"$1\"\n-}\n"},{"id":"207931","messageId":"CAJDDKr5cCbNi5q5_Ds-yohXR56ZfVs7YBTgJP3THjRx1=EgG9w@mail.gmail.com","threadId":"32731","inReplyTo":"7v8v7fz0ii.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T05:07:49Z","receivedAt":"2013-01-27T05:07:49Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> John Keeping <john@keeping.me.uk> writes:\n>\n>> I'm not sure creating an 'include' directory actually buys us much over\n>> declaring that 'vimdiff' is the real script and the others just source\n>> it.\n>\n> Is 'include' really used for such a purpose?  It only houses defaults.sh\n> as far as I can see.\n>\n> As that defaults.sh file is used only to define trivially empty\n> shell functions, I wonder if it is better to get rid of it, and\n> define these functions in line in git-mergetool--lib.sh.  Such a\n> change would like the attached on top of the entire series.\n\nI think that's much better.\n\n\n>  Makefile                       |  6 +-----\n>  git-mergetool--lib.sh          | 24 ++++++++++++++++++++++--\n>  mergetools/include/defaults.sh | 22 ----------------------\n>  3 files changed, 23 insertions(+), 29 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 26f217f..f69979e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -2724,11 +2724,7 @@ install: all\n>         $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n>         $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n>         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> -       $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \\\n> -               '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n> -       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n> -       $(INSTALL) -m 644 mergetools/include/* \\\n> -               '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n> +       $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>  ifndef NO_GETTEXT\n>         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n>         (cd po/build/locale && $(TAR) cf - .) | \\\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 7ea7510..1d0fb12 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -57,8 +57,28 @@ setup_tool () {\n>                 return 2\n>         fi\n>\n> -       # Load the default functions\n> -       . \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n> +       # Fallback definitions, to be overriden by tools.\n> +       can_merge () {\n> +               return 0\n> +       }\n> +\n> +       can_diff () {\n> +               return 0\n> +       }\n> +\n> +       diff_cmd () {\n> +               status=1\n> +               return $status\n> +       }\n> +\n> +       merge_cmd () {\n> +               status=1\n> +               return $status\n> +       }\n> +\n> +       translate_merge_tool_path () {\n> +               echo \"$1\"\n> +       }\n>\n>         # Load the redefined functions\n>         . \"$MERGE_TOOLS_DIR/$tool\"\n> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh\n> deleted file mode 100644\n> index 21e63ec..0000000\n> --- a/mergetools/include/defaults.sh\n> +++ /dev/null\n> @@ -1,22 +0,0 @@\n> -# Redefined by builtin tools\n> -can_merge () {\n> -       return 0\n> -}\n> -\n> -can_diff () {\n> -       return 0\n> -}\n> -\n> -diff_cmd () {\n> -       status=1\n> -       return $status\n> -}\n> -\n> -merge_cmd () {\n> -       status=1\n> -       return $status\n> -}\n> -\n> -translate_merge_tool_path () {\n> -       echo \"$1\"\n> -}\n\n\n\n-- \nDavid\n"},{"id":"207935","messageId":"CAJDDKr692zbg+PiFWx1y81yn=s2e=C0pFhsup4z0uTRNOTMPwg@mail.gmail.com","threadId":"32731","inReplyTo":"CAJDDKr5cCbNi5q5_Ds-yohXR56ZfVs7YBTgJP3THjRx1=EgG9w@mail.gmail.com","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T05:35:02Z","receivedAt":"2013-01-27T05:35:02Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Jan 26, 2013 at 9:07 PM, David Aguilar <davvid@gmail.com> wrote:\n> On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> John Keeping <john@keeping.me.uk> writes:\n>>\n>>> I'm not sure creating an 'include' directory actually buys us much over\n>>> declaring that 'vimdiff' is the real script and the others just source\n>>> it.\n>>\n>> Is 'include' really used for such a purpose?  It only houses defaults.sh\n>> as far as I can see.\n>>\n>> As that defaults.sh file is used only to define trivially empty\n>> shell functions, I wonder if it is better to get rid of it, and\n>> define these functions in line in git-mergetool--lib.sh.  Such a\n>> change would like the attached on top of the entire series.\n>\n> I think that's much better.\n\nWould you like me to put this together into a proper patch?\n\nYou can also squash it in (along with a removal of the\nlast line of the commit message) if you prefer.\n\n\n>>  Makefile                       |  6 +-----\n>>  git-mergetool--lib.sh          | 24 ++++++++++++++++++++++--\n>>  mergetools/include/defaults.sh | 22 ----------------------\n>>  3 files changed, 23 insertions(+), 29 deletions(-)\n>>\n>> diff --git a/Makefile b/Makefile\n>> index 26f217f..f69979e 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -2724,11 +2724,7 @@ install: all\n>>         $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'\n>>         $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install\n>>         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>> -       $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \\\n>> -               '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>> -       $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n>> -       $(INSTALL) -m 644 mergetools/include/* \\\n>> -               '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'\n>> +       $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'\n>>  ifndef NO_GETTEXT\n>>         $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'\n>>         (cd po/build/locale && $(TAR) cf - .) | \\\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index 7ea7510..1d0fb12 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -57,8 +57,28 @@ setup_tool () {\n>>                 return 2\n>>         fi\n>>\n>> -       # Load the default functions\n>> -       . \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n>> +       # Fallback definitions, to be overriden by tools.\n>> +       can_merge () {\n>> +               return 0\n>> +       }\n>> +\n>> +       can_diff () {\n>> +               return 0\n>> +       }\n>> +\n>> +       diff_cmd () {\n>> +               status=1\n>> +               return $status\n>> +       }\n>> +\n>> +       merge_cmd () {\n>> +               status=1\n>> +               return $status\n>> +       }\n>> +\n>> +       translate_merge_tool_path () {\n>> +               echo \"$1\"\n>> +       }\n>>\n>>         # Load the redefined functions\n>>         . \"$MERGE_TOOLS_DIR/$tool\"\n>> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh\n>> deleted file mode 100644\n>> index 21e63ec..0000000\n>> --- a/mergetools/include/defaults.sh\n>> +++ /dev/null\n>> @@ -1,22 +0,0 @@\n>> -# Redefined by builtin tools\n>> -can_merge () {\n>> -       return 0\n>> -}\n>> -\n>> -can_diff () {\n>> -       return 0\n>> -}\n>> -\n>> -diff_cmd () {\n>> -       status=1\n>> -       return $status\n>> -}\n>> -\n>> -merge_cmd () {\n>> -       status=1\n>> -       return $status\n>> -}\n>> -\n>> -translate_merge_tool_path () {\n>> -       echo \"$1\"\n>> -}\n\n\n\n-- \nDavid\n"},{"id":"207936","messageId":"7vpq0rxis6.fsf@alter.siamese.dyndns.org","threadId":"32731","inReplyTo":"CAJDDKr692zbg+PiFWx1y81yn=s2e=C0pFhsup4z0uTRNOTMPwg@mail.gmail.com","subject":"Re: [PATCH] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T06:05:29Z","receivedAt":"2013-01-27T06:05:29Z","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>> I think that's much better.\n>\n> Would you like me to put this together into a proper patch?\n>\n> You can also squash it in (along with a removal of the\n> last line of the commit message) if you prefer.\n\nI was lazy and didn't want to think about rewriting your log\nmessage, so I just queued it with this log message on top.\n\n    mergetools: remove mergetools/include/defaults.sh\n    \n    This and its containing directory are used only to define trivial\n    fallback definition of five shell functions in a single script.\n    Defining them in-line at the location that wants to have them makes\n    the result much easier to read.\n    \n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nBut as you said, removing the last line of your log message and\nsquashing the change into it would be more preferrable.  Let me do\nthat later.\n\nThanks.\n"}]}