{"thread":{"id":"32737","subject":"[PATCH v3 2/2] mergetools: Simplify how we handle \"vim\" and \"defaults\"","startedAt":"2013-01-27T00:46:12Z","lastAt":"2013-01-27T04:25:38Z","messageCount":4,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"207901","messageId":"1359247573-75825-1-git-send-email-davvid@gmail.com","threadId":"32737","inReplyTo":null,"subject":"[PATCH v3 2/2] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T00:46:12Z","receivedAt":"2013-01-27T00:46:12Z","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---\nv2 used \"include\" instead of \"mergetools/include\"\nin the Makefile.  Please ignore it.\n\n Makefile                                     |  6 +++-\n git-mergetool--lib.sh                        | 41 ++++++++--------------------\n mergetools/gvimdiff                          |  1 +\n mergetools/gvimdiff2                         |  1 +\n mergetools/{defaults => include/defaults.sh} |  0\n mergetools/{vim => vimdiff}                  |  0\n mergetools/vimdiff2                          |  1 +\n 7 files changed, 19 insertions(+), 31 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 => vimdiff} (100%)\n create mode 100644 mergetools/vimdiff2\n\ndiff --git a/Makefile b/Makefile\nindex f69979e..26f217f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2724,7 +2724,11 @@ 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+\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 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 f1bb372..7ea7510 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,19 +46,9 @@ 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\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@@ -65,8 +57,11 @@ setup_tool () {\n \t\treturn 2\n \tfi\n \n+\t# Load the default functions\n+\t. \"$MERGE_TOOLS_DIR/include/defaults.sh\"\n+\n \t# Load the redefined functions\n-\t. \"$mergetools/$tool\"\n+\t. \"$MERGE_TOOLS_DIR/$tool\"\n \n \tif merge_mode && ! can_merge\n \tthen\n@@ -194,24 +189,10 @@ 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\tthen\n-\t\t\tcontinue\n-\t\telif merge_mode && ! can_merge\n-\t\tthen\n-\t\t\tcontinue\n-\t\telif diff_mode && ! can_diff\n-\t\tthen\n-\t\t\tcontinue\n-\t\tfi\n+\t\ttool=$(basename \"$i\")\n+\t\tsetup_tool \"$tool\" 2>/dev/null || continue\n \n \t\tmerge_tool_path=$(translate_merge_tool_path \"$tool\")\n \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\ndiff --git a/mergetools/gvimdiff b/mergetools/gvimdiff\nnew file mode 100644\nindex 0000000..04a5bb0\n--- /dev/null\n+++ b/mergetools/gvimdiff\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/vimdiff\"\ndiff --git a/mergetools/gvimdiff2 b/mergetools/gvimdiff2\nnew file mode 100644\nindex 0000000..04a5bb0\n--- /dev/null\n+++ b/mergetools/gvimdiff2\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/vimdiff\"\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/vimdiff\nsimilarity index 100%\nrename from mergetools/vim\nrename to mergetools/vimdiff\ndiff --git a/mergetools/vimdiff2 b/mergetools/vimdiff2\nnew file mode 100644\nindex 0000000..04a5bb0\n--- /dev/null\n+++ b/mergetools/vimdiff2\n@@ -0,0 +1 @@\n+. \"$MERGE_TOOLS_DIR/vimdiff\"\n-- \n1.8.0.8.gd6b90fb.dirty\n"},{"id":"207916","messageId":"7vobgbz58a.fsf@alter.siamese.dyndns.org","threadId":"32737","inReplyTo":"1359247573-75825-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3 2/2] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T03:15:17Z","receivedAt":"2013-01-27T03:15:17Z","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> @@ -44,19 +46,9 @@ 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\nThis part was an eyesore every time I looked at mergetools scripts.\nGood riddance.\n\nIs there still other special case like this, or was this the last\none?\n\nThanks, both of you, for working on this.\n"},{"id":"207927","messageId":"CAJDDKr7E1NkMV0_G+6oY9O3iCS9OCEzR1HYssKpwh77swDUcig@mail.gmail.com","threadId":"32737","inReplyTo":"7vobgbz58a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/2] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T03:56:45Z","receivedAt":"2013-01-27T03:56:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Aguilar <davvid@gmail.com> writes:\n>\n>> @@ -44,19 +46,9 @@ 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>\n> This part was an eyesore every time I looked at mergetools scripts.\n> Good riddance.\n>\n> Is there still other special case like this, or was this the last\n> one?\n>\n> Thanks, both of you, for working on this.\n\nI believe that was the last special case. :-)  It should be easier\nto auto-generate a list of tools for use in the documentation now.\nThat'll be the the next topic I look into.\n\nI have a question. John mentioned that we can replace the use of\n\"$(..)\" with $(..) in shell.\n\nI have a trivial style patches to replace \"$(..)\" with $(..)\nsitting uncommitted in my tree for mergetool--lib.\n\nGrepping the rest of the tree shows that there are many\noccurrences of the \"$(..)\" idiom in the shell code.\n\nIs this a general rule that should be in CodingStyle,\nor is it better left as-is?  Are there cases where DQ should\nbe used around $(..)?  My understanding is \"no\", but I don't\nknow whether that is correct.\n\nDoing the documentation stuff is more important IMO so I probably\nshouldn't let myself get too distracted by it, though I am curious.\n-- \nDavid\n"},{"id":"207928","messageId":"7vd2wrz1z1.fsf@alter.siamese.dyndns.org","threadId":"32737","inReplyTo":"CAJDDKr7E1NkMV0_G+6oY9O3iCS9OCEzR1HYssKpwh77swDUcig@mail.gmail.com","subject":"Re: [PATCH v3 2/2] mergetools: Simplify how we handle \"vim\" and \"defaults\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T04:25:38Z","receivedAt":"2013-01-27T04:25:38Z","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 have a question. John mentioned that we can replace the use of\n> \"$(..)\" with $(..) in shell.\n\nI think it isn't so much about $(...) but more about quoting the RHS\nof assignment.\n\nConsider these:\n\n\tvar=\"$another_var\"\n\tvar=\"$(command)\"\n\tvar=\"$one_var$two_var\"\n\tvar=\"$another_var$(command)\"\n\nall of which _can_ be done without dq around the RHS, because the\nresult of variable substitution and command substitution will not be\nsubject to further word splitting.\n\nI however find it easier to read with dq around the latter two, i.e.\nsubstitution and then concatenation of the result of substitution.\nThe extra dq makes the intent of the author clearer, especially\nwhile reviewing a script written by other people whose understanding\nof the syntax I am not sure about ;-).  Between var=$another and\nvar=\"$another\", the latter is slightly preferrable for the same\nreason.\n\nOne questionable case is:\n\n\tvar=$(command \"with quoted parameter\")\n\nIt makes it a bit harder to scan if there is an extra set of dq\naround RHS, i.e.\n\n\tvar=\"$(command \"with quoted parameter\")\"\n\nThat case is easier to read without dq around it, or you could cheat\nby doing this:\n\n\tvar=\"$(command 'with quoted parameter')\"\n\nIn any case, as long as the result is correct, I do not have very\nstrong preference either way.\n"}]}