{"thread":{"id":"32747","subject":"[PATCH 0/4] Documentation: Auto-generate merge tool lists","startedAt":"2013-01-27T21:24:42Z","lastAt":"2013-01-27T23:46:01Z","messageCount":18,"participants":["David Aguilar","Johannes Sixt","Junio C Hamano","John Keeping"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"207986","messageId":"1359321886-80523-1-git-send-email-davvid@gmail.com","threadId":"32747","inReplyTo":null,"subject":"[PATCH 0/4] Documentation: Auto-generate merge tool lists","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T21:24:42Z","receivedAt":"2013-01-27T21:24:42Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Refactor the mergetool-lib so that we can reuse it in\nDocumentation/Makefile.  The end result is that the\ndiff.tool and merge.tool documentation now includes\nan auto-generated list of all available tools.\n\nThis applies on top of jk/mergetool in pu.\n\nDavid Aguilar (4):\n  mergetool--lib: Simplify command expressions\n  mergetool--lib: Improve the help text in guess_merge_tool()\n  mergetool--lib: Add functions for finding available tools\n  doc: Generate a list of valid merge tools\n\n Documentation/.gitignore       |   1 +\n Documentation/Makefile         |  16 +++++-\n Documentation/diff-config.txt  |  13 ++---\n Documentation/merge-config.txt |  12 ++---\n git-mergetool--lib.sh          | 108 ++++++++++++++++++++++-------------------\n 5 files changed, 87 insertions(+), 63 deletions(-)\n\n-- \n1.8.0.13.gf25ae33\n"},{"id":"207987","messageId":"1359321886-80523-2-git-send-email-davvid@gmail.com","threadId":"32747","inReplyTo":"1359321886-80523-1-git-send-email-davvid@gmail.com","subject":"[PATCH 1/4] mergetool--lib: Simplify command expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T21:24:43Z","receivedAt":"2013-01-27T21:24:43Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use $(command \"$arg\") instead of \"$(command \"$arg\")\" as the latter is\nharder to read.  Make the expression in get_merge_tool_cmd() even\nsimpler by avoiding \"echo\" completely.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh | 40 ++++++++++++++++------------------------\n 1 file changed, 16 insertions(+), 24 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 1d0fb12..9a5aae9 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -32,17 +32,10 @@ check_unchanged () {\n \tfi\n }\n \n-valid_tool_config () {\n-\tif test -n \"$(get_merge_tool_cmd \"$1\")\"\n-\tthen\n-\t\treturn 0\n-\telse\n-\t\treturn 1\n-\tfi\n-}\n-\n valid_tool () {\n-\tsetup_tool \"$1\" || valid_tool_config \"$1\"\n+\tsetup_tool \"$1\" && return 0\n+\tcmd=$(get_merge_tool_cmd \"$1\")\n+\ttest -n \"$cmd\"\n }\n \n setup_tool () {\n@@ -96,14 +89,13 @@ setup_tool () {\n }\n \n get_merge_tool_cmd () {\n-\t# Prints the custom command for a merge tool\n \tmerge_tool=\"$1\"\n \tif diff_mode\n \tthen\n-\t\techo \"$(git config difftool.$merge_tool.cmd ||\n-\t\t\tgit config mergetool.$merge_tool.cmd)\"\n+\t\tgit config \"difftool.$merge_tool.cmd\" ||\n+\t\tgit config \"mergetool.$merge_tool.cmd\"\n \telse\n-\t\techo \"$(git config mergetool.$merge_tool.cmd)\"\n+\t\tgit config \"mergetool.$merge_tool.cmd\"\n \tfi\n }\n \n@@ -114,7 +106,7 @@ run_merge_tool () {\n \tGIT_PREFIX=${GIT_PREFIX:-.}\n \texport GIT_PREFIX\n \n-\tmerge_tool_path=\"$(get_merge_tool_path \"$1\")\" || exit\n+\tmerge_tool_path=$(get_merge_tool_path \"$1\") || exit\n \tbase_present=\"$2\"\n \tstatus=0\n \n@@ -145,7 +137,7 @@ 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+\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@@ -158,11 +150,11 @@ run_diff_cmd () {\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+\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\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@@ -253,7 +245,7 @@ guess_merge_tool () {\n \t# Loop over each candidate and stop when a valid merge tool is found.\n \tfor i in $tools\n \tdo\n-\t\tmerge_tool_path=\"$(translate_merge_tool_path \"$i\")\"\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n \t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n \t\tthen\n \t\t\techo \"$i\"\n@@ -300,9 +292,9 @@ get_merge_tool_path () {\n \tfi\n \tif test -z \"$merge_tool_path\"\n \tthen\n-\t\tmerge_tool_path=\"$(translate_merge_tool_path \"$merge_tool\")\"\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$merge_tool\")\n \tfi\n-\tif test -z \"$(get_merge_tool_cmd \"$merge_tool\")\" &&\n+\tif test -z $(get_merge_tool_cmd \"$merge_tool\") &&\n \t\t! type \"$merge_tool_path\" >/dev/null 2>&1\n \tthen\n \t\techo >&2 \"The $TOOL_MODE tool $merge_tool is not available as\"\\\n@@ -314,11 +306,11 @@ get_merge_tool_path () {\n \n get_merge_tool () {\n \t# Check if a merge tool has been configured\n-\tmerge_tool=\"$(get_configured_merge_tool)\"\n+\tmerge_tool=$(get_configured_merge_tool)\n \t# Try to guess an appropriate merge tool if no tool has been set.\n \tif test -z \"$merge_tool\"\n \tthen\n-\t\tmerge_tool=\"$(guess_merge_tool)\" || exit\n+\t\tmerge_tool=$(guess_merge_tool) || exit\n \tfi\n \techo \"$merge_tool\"\n }\n-- \n1.8.0.13.gf25ae33\n"},{"id":"207988","messageId":"1359321886-80523-3-git-send-email-davvid@gmail.com","threadId":"32747","inReplyTo":"1359321886-80523-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/4] mergetool--lib: Improve the help text in guess_merge_tool()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T21:24:44Z","receivedAt":"2013-01-27T21:24:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This code path is only activated when the user does not have a valid\nconfigured tool.  Add a message to guide new users towards configuring a\ndefault tool.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9a5aae9..cf52423 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -240,7 +240,14 @@ show_tool_help () {\n \n guess_merge_tool () {\n \tlist_merge_tool_candidates\n-\techo >&2 \"merge tool candidates: $tools\"\n+\tmsg=\"\\\n+\n+This message is displayed because '$TOOL_MODE.tool' is not configured.\n+See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.\n+'git ${TOOL_MODE}tool' will now attempt to use one of the following tools:\n+$tools\n+\"\n+\tprintf \"$msg\" >&2\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\n \tfor i in $tools\n-- \n1.8.0.13.gf25ae33\n"},{"id":"207990","messageId":"1359321886-80523-4-git-send-email-davvid@gmail.com","threadId":"32747","inReplyTo":"1359321886-80523-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T21:24:45Z","receivedAt":"2013-01-27T21:24:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Refactor show_tool_help() so that the tool-finding logic is broken out\ninto separate functions.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 34 insertions(+), 26 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex cf52423..894b849 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -2,6 +2,33 @@\n # git-mergetool--lib is a library for common merge tool functions\n MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n \n+mode_ok () {\n+\tdiff_mode && can_diff ||\n+\tmerge_mode && can_merge\n+}\n+\n+is_available () {\n+\tmerge_tool_path=$(translate_merge_tool_path \"$1\") &&\n+\ttype \"$merge_tool_path\" >/dev/null 2>&1\n+}\n+\n+filter_tools () {\n+\tfilter=\"$1\"\n+\tprefix=\"$2\"\n+\t(\n+\t\tcd \"$MERGE_TOOLS_DIR\" &&\n+\t\tfor i in *\n+\t\tdo\n+\t\t\techo \"$i\"\n+\t\tdone\n+\t) | sort | while read tool\n+\tdo\n+\t\tsetup_tool \"$tool\" 2>/dev/null &&\n+\t\t(eval \"$filter\" \"$tool\") &&\n+\t\tprintf \"$prefix$tool\\n\"\n+\tdone\n+}\n+\n diff_mode() {\n \ttest \"$TOOL_MODE\" = diff\n }\n@@ -199,27 +226,13 @@ list_merge_tool_candidates () {\n }\n \n show_tool_help () {\n-\tunavailable= available= LF='\n-'\n-\tfor i in \"$MERGE_TOOLS_DIR\"/*\n-\tdo\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\n-\t\tthen\n-\t\t\tavailable=\"$available$tool$LF\"\n-\t\telse\n-\t\t\tunavailable=\"$unavailable$tool$LF\"\n-\t\tfi\n-\tdone\n-\n \tcmd_name=${TOOL_MODE}tool\n+\tavailable=$(filter_tools 'mode_ok && is_available' '\\t\\t')\n+\tunavailable=$(filter_tools 'mode_ok && ! is_available' '\\t\\t')\n \tif test -n \"$available\"\n \tthen\n \t\techo \"'git $cmd_name --tool=<tool>' may be set to one of the following:\"\n-\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\t\tprintf \"$available\"\n \telse\n \t\techo \"No suitable tool for 'git $cmd_name --tool=<tool>' found.\"\n \tfi\n@@ -227,7 +240,7 @@ show_tool_help () {\n \tthen\n \t\techo\n \t\techo 'The following tools are valid, but not currently available:'\n-\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n+\t\tprintf \"$unavailable\"\n \tfi\n \tif test -n \"$unavailable$available\"\n \tthen\n@@ -250,17 +263,12 @@ $tools\n \tprintf \"$msg\" >&2\n \n \t# Loop over each candidate and stop when a valid merge tool is found.\n-\tfor i in $tools\n+\tfor tool in $tools\n \tdo\n-\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n-\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n-\t\tthen\n-\t\t\techo \"$i\"\n-\t\t\treturn 0\n-\t\tfi\n+\t\tis_available \"$tool\" && echo \"$tool\" && return 0\n \tdone\n \n-\techo >&2 \"No known merge resolution program available.\"\n+\techo >&2 \"No known ${TOOL_MODE} tool is available.\"\n \treturn 1\n }\n \n-- \n1.8.0.13.gf25ae33\n"},{"id":"207989","messageId":"1359321886-80523-5-git-send-email-davvid@gmail.com","threadId":"32747","inReplyTo":"1359321886-80523-1-git-send-email-davvid@gmail.com","subject":"[PATCH 4/4] doc: Generate a list of valid merge tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T21:24:46Z","receivedAt":"2013-01-27T21:24:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use the new filter_tools() function to build lists of all\nthe built-in tools supported by difftool and mergetool.\nThis frees us from needing to update the documentation\nwhenever a new tool is added.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n Documentation/.gitignore       |  1 +\n Documentation/Makefile         | 16 +++++++++++++++-\n Documentation/diff-config.txt  | 13 +++++++------\n Documentation/merge-config.txt | 12 ++++++------\n git-mergetool--lib.sh          |  1 +\n 5 files changed, 30 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/.gitignore b/Documentation/.gitignore\nindex d62aebd..2c8b2d6 100644\n--- a/Documentation/.gitignore\n+++ b/Documentation/.gitignore\n@@ -9,4 +9,5 @@ gitman.info\n howto-index.txt\n doc.dep\n cmds-*.txt\n+mergetools-*.txt\n manpage-base-url.xsl\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 267dfe1..f595d26 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -226,13 +226,27 @@ cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)\n \t$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(QUIET_STDERR) && \\\n \tdate >$@\n \n+mergetools_txt = mergetools-diff.txt mergetools-merge.txt\n+\n+$(mergetools_txt): mergetools-list.made\n+\n+mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)\n+\t$(QUIET_GEN)$(RM) $@ && \\\n+\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n+\t\t. ../git-mergetool--lib.sh && \\\n+\t\tfilter_tools can_diff \"* \"' > mergetools-diff.txt && \\\n+\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n+\t\t. ../git-mergetool--lib.sh && \\\n+\t\tfilter_tools can_merge \"* \"' > mergetools-merge.txt && \\\n+\tdate > $@\n+\n clean:\n \t$(RM) *.xml *.xml+ *.html *.html+ *.1 *.5 *.7\n \t$(RM) *.texi *.texi+ *.texi++ git.info gitman.info\n \t$(RM) *.pdf\n \t$(RM) howto-index.txt howto/*.html doc.dep\n \t$(RM) technical/api-*.html technical/api-index.txt\n-\t$(RM) $(cmds_txt) *.made\n+\t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n \t$(RM) manpage-base-url.xsl\n \n $(MAN_HTML): %.html : %.txt\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 67a90a8..7c968d1 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -132,9 +132,10 @@ diff.<driver>.cachetextconv::\n \tconversion outputs.  See linkgit:gitattributes[5] for details.\n \n diff.tool::\n-\tThe diff tool to be used by linkgit:git-difftool[1].  This\n-\toption overrides `merge.tool`, and has the same valid built-in\n-\tvalues as `merge.tool` minus \"tortoisemerge\" and plus\n-\t\"kompare\".  Any other value is treated as a custom diff tool,\n-\tand there must be a corresponding `difftool.<tool>.cmd`\n-\toption.\n+\tControls which diff tool is used by linkgit:git-difftool[1].\n+\tThis variable overrides the value configured in `merge.tool`.\n+\tThe list below shows the valid built-in values.\n+\tAny other value is treated as a custom diff tool and requires\n+\tthat a corresponding difftool.<tool>.cmd variable is defined.\n+\n+include::mergetools-diff.txt[]\ndiff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\nindex 861bd6f..5f40e71 100644\n--- a/Documentation/merge-config.txt\n+++ b/Documentation/merge-config.txt\n@@ -52,12 +52,12 @@ merge.stat::\n \tat the end of the merge.  True by default.\n \n merge.tool::\n-\tControls which merge resolution program is used by\n-\tlinkgit:git-mergetool[1].  Valid built-in values are: \"araxis\",\n-\t\"bc3\", \"diffuse\", \"ecmerge\", \"emerge\", \"gvimdiff\", \"kdiff3\", \"meld\",\n-\t\"opendiff\", \"p4merge\", \"tkdiff\", \"tortoisemerge\", \"vimdiff\"\n-\tand \"xxdiff\".  Any other value is treated is custom merge tool\n-\tand there must be a corresponding mergetool.<tool>.cmd option.\n+\tControls which merge tool is used by linkgit:git-mergetool[1].\n+\tThe list below shows the valid built-in values.\n+\tAny other value is treated as a custom merge tool and requires\n+\tthat a corresponding mergetool.<tool>.cmd variable is defined.\n+\n+include::mergetools-merge.txt[]\n \n merge.verbosity::\n \tControls the amount of output shown by the recursive merge\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 894b849..59e1650 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -1,5 +1,6 @@\n #!/bin/sh\n # git-mergetool--lib is a library for common merge tool functions\n+test -z \"$MERGE_TOOLS_DIR\" &&\n MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n \n mode_ok () {\n-- \n1.8.0.13.gf25ae33\n"},{"id":"207991","messageId":"5105A56E.4010002@kdbg.org","threadId":"32747","inReplyTo":"1359321886-80523-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/4] mergetool--lib: Simplify command expressions","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-01-27T22:08:46Z","receivedAt":"2013-01-27T22:08:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.01.2013 22:24, schrieb David Aguilar:\n> Use $(command \"$arg\") instead of \"$(command \"$arg\")\" as the latter is\n> harder to read.\n\nIf at all, you should restrict yourself to simplify only variable\nassignments. Because this case:\n\n> -\tif test -z \"$(get_merge_tool_cmd \"$merge_tool\")\" &&\n> +\tif test -z $(get_merge_tool_cmd \"$merge_tool\") &&\n\ncannot work as intended: If the output of $() is empty, then without the\nouter quotes this becomes\n\n  test -z\n\nwithout an operand for -z, which is a syntax error (of the test command).\n\n-- Hannes\n"},{"id":"207992","messageId":"CAJDDKr61wS1ruPYDW4+exWBWveyS0Dp7Gmu5gWRh_99-frc_7Q@mail.gmail.com","threadId":"32747","inReplyTo":"5105A56E.4010002@kdbg.org","subject":"Re: [PATCH 1/4] mergetool--lib: Simplify command expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T22:18:15Z","receivedAt":"2013-01-27T22:18:15Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jan 27, 2013 at 2:08 PM, Johannes Sixt <j6t@kdbg.org> wrote:\n> Am 27.01.2013 22:24, schrieb David Aguilar:\n>> Use $(command \"$arg\") instead of \"$(command \"$arg\")\" as the latter is\n>> harder to read.\n>\n> If at all, you should restrict yourself to simplify only variable\n> assignments. Because this case:\n>\n>> -     if test -z \"$(get_merge_tool_cmd \"$merge_tool\")\" &&\n>> +     if test -z $(get_merge_tool_cmd \"$merge_tool\") &&\n>\n> cannot work as intended: If the output of $() is empty, then without the\n> outer quotes this becomes\n>\n>   test -z\n>\n> without an operand for -z, which is a syntax error (of the test command).\n\nDefinitely.  I learned this the hard way when the tests broke on me while\nworking it ;-)  My patch rewrites things to always use var=$(command)\nexpressions with separate test \"$var\" evaluating them.\nThanks for the tip,\n-- \nDavid\n"},{"id":"207993","messageId":"7v1ud6uv1p.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/4] mergetool--lib: Simplify command expressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T22:21:06Z","receivedAt":"2013-01-27T22:21:06Z","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> Use $(command \"$arg\") instead of \"$(command \"$arg\")\" as the latter is\n> harder to read.\n\nDid you miss my comment that this is about RHS of an assignment?\n"},{"id":"207994","messageId":"5105A9AC.1040808@kdbg.org","threadId":"32747","inReplyTo":"1359321886-80523-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-01-27T22:26:52Z","receivedAt":"2013-01-27T22:26:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.01.2013 22:24, schrieb David Aguilar:\n> Refactor show_tool_help() so that the tool-finding logic is broken out\n> into separate functions.\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------\n>  1 file changed, 34 insertions(+), 26 deletions(-)\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index cf52423..894b849 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -2,6 +2,33 @@\n>  # git-mergetool--lib is a library for common merge tool functions\n>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n>  \n> +mode_ok () {\n> +\tdiff_mode && can_diff ||\n> +\tmerge_mode && can_merge\n> +}\n\n&& and || have the same precedence: if diff_mode and can_diff both are\n\"true\", then the result of the function is that of can_merge. I don't\nthink that is what is intended.\n\n> +filter_tools () {\n> +\tfilter=\"$1\"\n> +\tprefix=\"$2\"\n> +\t(\n> +\t\tcd \"$MERGE_TOOLS_DIR\" &&\n> +\t\tfor i in *\n> +\t\tdo\n> +\t\t\techo \"$i\"\n> +\t\tdone\n\n\t\tcd \"$MERGE_TOOLS_DIR\" &&\n\t\tprintf \"%s\\n\" *\n\nBut what's wrong with \"ls -1\"? It would save the explicit sort.\n\n> +\t) | sort | while read tool\n> +\tdo\n> +\t\tsetup_tool \"$tool\" 2>/dev/null &&\n> +\t\t(eval \"$filter\" \"$tool\") &&\n> +\t\tprintf \"$prefix$tool\\n\"\n> +\tdone\n> +}\n\n-- Hannes\n"},{"id":"207995","messageId":"7vvcaitfzq.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T22:31:37Z","receivedAt":"2013-01-27T22:31:37Z","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> Refactor show_tool_help() so that the tool-finding logic is broken out\n> into separate functions.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------\n>  1 file changed, 34 insertions(+), 26 deletions(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index cf52423..894b849 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -2,6 +2,33 @@\n>  # git-mergetool--lib is a library for common merge tool functions\n>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n>  \n> +mode_ok () {\n> +\tdiff_mode && can_diff ||\n> +\tmerge_mode && can_merge\n> +}\n\nI think you got the operator precedence mixed up.\n\nWhat happens when diff_mode=true, can_diff=true, merge_mode=true and\ncan_merge=false?\n\n\tif true && true || true && false\n\tthen\n\t\techo OK\n\telse\n\t\techo NO\n\tfi\n\tif (true && true) || (true && false)\n\tthen\n\t\techo OK\n\telse\n\t\techo NO\n\tfi\n\n> +is_available () {\n> +\tmerge_tool_path=$(translate_merge_tool_path \"$1\") &&\n> +\ttype \"$merge_tool_path\" >/dev/null 2>&1\n> +}\n> +\n> +filter_tools () {\n> +\tfilter=\"$1\"\n> +\tprefix=\"$2\"\n> +\t(\n> +\t\tcd \"$MERGE_TOOLS_DIR\" &&\n> +\t\tfor i in *\n> +\t\tdo\n> +\t\t\techo \"$i\"\n> +\t\tdone\n> +\t) | sort | while read tool\n> +\tdo\n\nPlease start a new line before keywords that define the syntactic\nstructure, like \"while\", i.e.\n\n\t... piped | commands |\n\twhile read tool\n\tdo\n\t\t...\n"},{"id":"207996","messageId":"7vr4l6tfy6.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"CAJDDKr61wS1ruPYDW4+exWBWveyS0Dp7Gmu5gWRh_99-frc_7Q@mail.gmail.com","subject":"Re: [PATCH 1/4] mergetool--lib: Simplify command expressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T22:32:33Z","receivedAt":"2013-01-27T22:32:33Z","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> Definitely.  I learned this the hard way when the tests broke on me while\n> working it ;-)  My patch rewrites things to always use var=$(command)\n> expressions with separate test \"$var\" evaluating them.\n\nOK; that wasn't clear from the log message.\n"},{"id":"207997","messageId":"7vmwvutfqv.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-5-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 4/4] doc: Generate a list of valid merge tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T22:36:56Z","receivedAt":"2013-01-27T22:36:56Z","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> +mergetools_txt = mergetools-diff.txt mergetools-merge.txt\n> +\n> +$(mergetools_txt): mergetools-list.made\n> +\n> +mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)\n> +\t$(QUIET_GEN)$(RM) $@ && \\\n> +\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n> +\t\t. ../git-mergetool--lib.sh && \\\n> +\t\tfilter_tools can_diff \"* \"' > mergetools-diff.txt && \\\n> +\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n> +\t\t. ../git-mergetool--lib.sh && \\\n> +\t\tfilter_tools can_merge \"* \"' > mergetools-merge.txt && \\\n> +\tdate > $@\n\nNicely done.\n\nBy omitting is_available check and only checking can_*, we would get\nthe same result on all platforms and I'd see tortoise appear in the\noutput even I do not do Windows.\n\nThanks.\n"},{"id":"207999","messageId":"20130127225423.GW7498@serenity.lan","threadId":"32747","inReplyTo":"1359321886-80523-5-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 4/4] doc: Generate a list of valid merge tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-27T22:54:23Z","receivedAt":"2013-01-27T22:54:23Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jan 27, 2013 at 01:24:46PM -0800, David Aguilar wrote:\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -1,5 +1,6 @@\n>  #!/bin/sh\n>  # git-mergetool--lib is a library for common merge tool functions\n> +test -z \"$MERGE_TOOLS_DIR\" &&\n>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools\n\nThe preferred pattern in Git seems to be this:\n\n    : ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}\n\n\nJohn\n"},{"id":"208001","messageId":"20130127230404.GX7498@serenity.lan","threadId":"32747","inReplyTo":"1359321886-80523-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-27T23:04:04Z","receivedAt":"2013-01-27T23:04:04Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jan 27, 2013 at 01:24:45PM -0800, David Aguilar wrote:\n> +filter_tools () {\n> +\tfilter=\"$1\"\n> +\tprefix=\"$2\"\n> +\t(\n> +\t\tcd \"$MERGE_TOOLS_DIR\" &&\n> +\t\tfor i in *\n> +\t\tdo\n> +\t\t\techo \"$i\"\n> +\t\tdone\n> +\t) | sort | while read tool\n> +\tdo\n> +\t\tsetup_tool \"$tool\" 2>/dev/null &&\n> +\t\t(eval \"$filter\" \"$tool\") &&\n> +\t\tprintf \"$prefix$tool\\n\"\n> +\tdone\n> +}\n\nCan we change this so that it does this:\n\n    filter_tools () {\n    \tfilter=\"$1\"\n    \t(\n    \t\tcd \"$MERGE_TOOLS_DIR\" &&\n    \t\tfor i in *\n    \t\tdo\n    \t\t\techo \"$i\"\n    \t\tdone\n    \t) |\n        while read tool\n    \tdo\n    \t\tsetup_tool \"$tool\" 2>/dev/null &&\n    \t\t(eval \"$filter\" \"$tool\") &&\n    \t\techo \"$tool\"\n    \tdone\n    }\n\nand keep the sorting and prefix in show_tool_help?  This will make it\neasier to integrate the user-configured tools from git-config.\n\n\nJohn\n"},{"id":"208002","messageId":"7vehh6te3j.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-3-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 2/4] mergetool--lib: Improve the help text in guess_merge_tool()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T23:12:32Z","receivedAt":"2013-01-27T23:12:32Z","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> This code path is only activated when the user does not have a valid\n> configured tool.  Add a message to guide new users towards configuring a\n> default tool.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n>  git-mergetool--lib.sh | 9 ++++++++-\n>  1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 9a5aae9..cf52423 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -240,7 +240,14 @@ show_tool_help () {\n>  \n>  guess_merge_tool () {\n>  \tlist_merge_tool_candidates\n> -\techo >&2 \"merge tool candidates: $tools\"\n> +\tmsg=\"\\\n> +\n> +This message is displayed because '$TOOL_MODE.tool' is not configured.\n> +See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.\n> +'git ${TOOL_MODE}tool' will now attempt to use one of the following tools:\n> +$tools\n> +\"\n> +\tprintf \"$msg\" >&2\n\nThis is not wrong per-se, but wouldn't it be much easier to read to\nuse an indented HERE-text like this?\n\n\tcat >&2 <<-EOF\n\t... a long message with $var substitution ...\n        ... comes here ...\n        EOF\n"},{"id":"208005","messageId":"7v8v7ecicm.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T23:32:41Z","receivedAt":"2013-01-27T23:32:41Z","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> +filter_tools () {\n> +\tfilter=\"$1\"\n> +\tprefix=\"$2\"\n> +\t(\n> +\t\tcd \"$MERGE_TOOLS_DIR\" &&\n> +\t\tfor i in *\n> +\t\tdo\n> +\t\t\techo \"$i\"\n> +\t\tdone\n> +\t) | sort | while read tool\n> +\tdo\n> +\t\tsetup_tool \"$tool\" 2>/dev/null &&\n> +\t\t(eval \"$filter\" \"$tool\") &&\n> +\t\tprintf \"$prefix$tool\\n\"\n> +\tdone\n> +}\n> +\n>  diff_mode() {\n>  \ttest \"$TOOL_MODE\" = diff\n>  }\n> @@ -199,27 +226,13 @@ list_merge_tool_candidates () {\n>  }\n>  \n>  show_tool_help () {\n> -\tunavailable= available= LF='\n> -'\n> -\tfor i in \"$MERGE_TOOLS_DIR\"/*\n> -\tdo\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\n> -\t\tthen\n> -\t\t\tavailable=\"$available$tool$LF\"\n> -\t\telse\n> -\t\t\tunavailable=\"$unavailable$tool$LF\"\n> -\t\tfi\n> -\tdone\n> -\n>  \tcmd_name=${TOOL_MODE}tool\n> +\tavailable=$(filter_tools 'mode_ok && is_available' '\\t\\t')\n> +\tunavailable=$(filter_tools 'mode_ok && ! is_available' '\\t\\t')\n>  \tif test -n \"$available\"\n>  \tthen\n>  \t\techo \"'git $cmd_name --tool=<tool>' may be set to one of the following:\"\n> -\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n> +\t\tprintf \"$available\"\n\nThis may happen to be safe as available will not have anything with\na per-cent % in it, but is not a good discipline in general.\n\n\tprintf \"%s\" \"$available\"\n\nIf you are giving filter_tools an optional \"prefix-per-line\", I do\nnot think it is too much of a stretch to introduce another optional\n\"perfix for the whole thing\" and let this call site say something\nlike this:\n\n\tcmd_name=${TOOL_MODE}tool\n\tshow_tool_names 'mode_ok && is_available' '\\t\\t' \\\n\t\t\"'git $cmd_name --tool=<tool>' may be set to one of these:\"\n\tshow_tool_names 'mode_ok && !is_available' '\\t\\t' \\\n\t\t\"These are valid but not available:\"\n\nwithout any of the above logic (and the same for unav).  It may look like this:\n\nshow_tool_names () {\n\tcondition=${1?condition} per_line_prefix=${2:-} preamble=${3:-}\n\n        ( cd \"$MERGE_TOOLS_DIR && ls -1 * ) |\n\twhile read toolname\n\tdo\n\t\tif setup_tool \"$toolname\" 2>/dev/null && (eval \"$condition\")\n\t\tthen\n\t\t\tif test -n \"$preamble\"\n\t\t\tthen\n\t\t\t\techo \"$preamble\"\n\t\t\t\tpreamble=\n\t\t\tfi\n                        printf \"%s%s\\n\" \"$prefix\" \"$toolname\"\n\t\tfi\n\tdone\n}\n"},{"id":"208008","messageId":"CAJDDKr6eTN6FE6mF_6boXBzw5TqmA6f5+7V0S6nQSkhQKZAmCg@mail.gmail.com","threadId":"32747","inReplyTo":"7v8v7ecicm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-27T23:45:08Z","receivedAt":"2013-01-27T23:45:08Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jan 27, 2013 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Aguilar <davvid@gmail.com> writes:\n>\n>> +filter_tools () {\n>> +     filter=\"$1\"\n>> +     prefix=\"$2\"\n>> +     (\n>> +             cd \"$MERGE_TOOLS_DIR\" &&\n>> +             for i in *\n>> +             do\n>> +                     echo \"$i\"\n>> +             done\n>> +     ) | sort | while read tool\n>> +     do\n>> +             setup_tool \"$tool\" 2>/dev/null &&\n>> +             (eval \"$filter\" \"$tool\") &&\n>> +             printf \"$prefix$tool\\n\"\n>> +     done\n>> +}\n>> +\n>>  diff_mode() {\n>>       test \"$TOOL_MODE\" = diff\n>>  }\n>> @@ -199,27 +226,13 @@ list_merge_tool_candidates () {\n>>  }\n>>\n>>  show_tool_help () {\n>> -     unavailable= available= LF='\n>> -'\n>> -     for i in \"$MERGE_TOOLS_DIR\"/*\n>> -     do\n>> -             tool=$(basename \"$i\")\n>> -             setup_tool \"$tool\" 2>/dev/null || continue\n>> -\n>> -             merge_tool_path=$(translate_merge_tool_path \"$tool\")\n>> -             if type \"$merge_tool_path\" >/dev/null 2>&1\n>> -             then\n>> -                     available=\"$available$tool$LF\"\n>> -             else\n>> -                     unavailable=\"$unavailable$tool$LF\"\n>> -             fi\n>> -     done\n>> -\n>>       cmd_name=${TOOL_MODE}tool\n>> +     available=$(filter_tools 'mode_ok && is_available' '\\t\\t')\n>> +     unavailable=$(filter_tools 'mode_ok && ! is_available' '\\t\\t')\n>>       if test -n \"$available\"\n>>       then\n>>               echo \"'git $cmd_name --tool=<tool>' may be set to one of the following:\"\n>> -             echo \"$available\" | sort | sed -e 's/^/ /'\n>> +             printf \"$available\"\n>\n> This may happen to be safe as available will not have anything with\n> a per-cent % in it, but is not a good discipline in general.\n>\n>         printf \"%s\" \"$available\"\n>\n> If you are giving filter_tools an optional \"prefix-per-line\", I do\n> not think it is too much of a stretch to introduce another optional\n> \"perfix for the whole thing\" and let this call site say something\n> like this:\n>\n>         cmd_name=${TOOL_MODE}tool\n>         show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n>                 \"'git $cmd_name --tool=<tool>' may be set to one of these:\"\n>         show_tool_names 'mode_ok && !is_available' '\\t\\t' \\\n>                 \"These are valid but not available:\"\n>\n> without any of the above logic (and the same for unav).  It may look like this:\n>\n> show_tool_names () {\n>         condition=${1?condition} per_line_prefix=${2:-} preamble=${3:-}\n>\n>         ( cd \"$MERGE_TOOLS_DIR && ls -1 * ) |\n>         while read toolname\n>         do\n>                 if setup_tool \"$toolname\" 2>/dev/null && (eval \"$condition\")\n>                 then\n>                         if test -n \"$preamble\"\n>                         then\n>                                 echo \"$preamble\"\n>                                 preamble=\n>                         fi\n>                         printf \"%s%s\\n\" \"$prefix\" \"$toolname\"\n>                 fi\n>         done\n> }\n\nThanks guys.  I'll re-roll this soon.\n-- \nDavid\n"},{"id":"208009","messageId":"7v4ni2chqe.fsf@alter.siamese.dyndns.org","threadId":"32747","inReplyTo":"1359321886-80523-5-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 4/4] doc: Generate a list of valid merge tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T23:46:01Z","receivedAt":"2013-01-27T23:46:01Z","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> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 267dfe1..f595d26 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -226,13 +226,27 @@ cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)\n>  \t$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(QUIET_STDERR) && \\\n>  \tdate >$@\n>  \n> +mergetools_txt = mergetools-diff.txt mergetools-merge.txt\n> +\n> +$(mergetools_txt): mergetools-list.made\n\nThe product of files that include these two and doc.dep target have\nto depend on these two files being up to date.  doc.dep depends on\n$(wildcard *.txt) so you may be lucky and $(mergetools_txt) have\nbeen built once in which case they would depend on it, or unlukcy\nand end up doc.dep that does not know about them, no?\n"}]}