{"thread":{"id":"32748","subject":"[PATCH v2 0/4] Auto-generate mergetool lists","startedAt":"2013-01-28T00:52:22Z","lastAt":"2013-01-30T07:04:08Z","messageCount":36,"participants":["David Aguilar","Junio C Hamano","Philip Oakley","John Keeping","Joachim Schmitz"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"208011","messageId":"1359334346-5879-1-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":null,"subject":"[PATCH v2 0/4] Auto-generate mergetool lists","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T00:52:22Z","receivedAt":"2013-01-28T00:52:22Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This is round two of this series.\nI think this touched on everything brought up in the code review.\n4/4 could use a review as I'm not completely familiar with the\nmakefile dependencies, though it seems to work correctly.\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 | 22 +++++++-\n Documentation/diff-config.txt | 13 ++---\n Documentation/merge-config.txt | 12 ++---\n git-mergetool--lib.sh | 116 ++++++++++++++++++++++-------------------\n 5 files changed, 96 insertions(+), 68 deletions(-)\n\n-- \n1.8.0.13.g3ff16bb\n"},{"id":"208012","messageId":"1359334346-5879-2-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":"1359334346-5879-1-git-send-email-davvid@gmail.com","subject":"[PATCH v2 1/4] mergetool--lib: Simplify command expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T00:52:23Z","receivedAt":"2013-01-28T00:52:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Update variable assignments to always use $(command \"$arg\")\nin their RHS instead of \"$(command \"$arg\")\" as the latter\nis harder to read. Make get_merge_tool_cmd() simpler by\navoiding \"echo\" and $(command) substitutions completely.\n\nSigned-off-by: David Aguilar \n---\nI reworded the commit message to be more clear.\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.g3ff16bb\n"},{"id":"208014","messageId":"1359334346-5879-3-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":"1359334346-5879-2-git-send-email-davvid@gmail.com","subject":"[PATCH v2 2/4] mergetool--lib: Improve the help text in guess_merge_tool()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T00:52:24Z","receivedAt":"2013-01-28T00:52:24Z","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 \n---\nThis now uses a cat << here-doc.\n\n git-mergetool--lib.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 9a5aae9..db3eb58 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -240,8 +240,13 @@ show_tool_help () {\n \n guess_merge_tool () {\n \tlist_merge_tool_candidates\n-\techo >&2 \"merge tool candidates: $tools\"\n+\tcat >&2 <<-EOF\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+EOF\n \t# Loop over each candidate and stop when a valid merge tool is found.\n \tfor i in $tools\n \tdo\n-- \n1.8.0.13.g3ff16bb\n"},{"id":"208013","messageId":"1359334346-5879-4-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":"1359334346-5879-3-git-send-email-davvid@gmail.com","subject":"[PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T00:52:25Z","receivedAt":"2013-01-28T00:52:25Z","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 a separate show_tool_names() function.\n\nSigned-off-by: David Aguilar \n---\nfilter_tools renamed to show_tool_names() and simplfied\nto use ls -1. show_tool_names() now has a preamble as discussed.\n\n git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------\n 1 file changed, 39 insertions(+), 29 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex db3eb58..fe068f6 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -2,6 +2,35 @@\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+show_tool_names () {\n+\tcondition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n+\n+\t( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n+\twhile read toolname\n+\tdo\n+\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n+\t\t\t(eval \"$condition\" \"$toolname\")\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+\t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$tool\"\n+\t\tfi\n+\tdone\n+}\n+\n diff_mode() {\n \ttest \"$TOOL_MODE\" = diff\n }\n@@ -199,35 +228,21 @@ 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+\ttool_opt=\"'git ${TOOL_MODE}tool --tool-'\"\n+\tavailable=$(show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n+\t\t\"$tool_opt may be set to one of the following:\")\n+\tunavailable=$(show_tool_names 'mode_ok && ! is_available' '\\t\\t' \\\n+\t\t\"The following tools are valid, but not currently available:\")\n \tif test -n \"$available\"\n \tthen\n-\t\techo \"'git $cmd_name --tool=' may be set to one of the following:\"\n-\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\t\techo \"$available\"\n \telse\n \t\techo \"No suitable tool for 'git $cmd_name --tool=' found.\"\n \tfi\n \tif test -n \"$unavailable\"\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\techo \"$unavailable\"\n \tfi\n \tif test -n \"$unavailable$available\"\n \tthen\n@@ -248,17 +263,12 @@ See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.\n $tools\n EOF\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.g3ff16bb\n"},{"id":"208015","messageId":"1359334346-5879-5-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":"1359334346-5879-4-git-send-email-davvid@gmail.com","subject":"[PATCH v2 4/4] doc: Generate a list of valid merge tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T00:52:26Z","receivedAt":"2013-01-28T00:52:26Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use the show_tool_names() 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 \n---\nAdjusted to use show_tool_names() and reworked the makefile dependencies.\nI could use another set of eyes on the Makefile..\n\n Documentation/.gitignore | 1 +\n Documentation/Makefile | 22 ++++++++++++++++++++--\n Documentation/diff-config.txt | 13 +++++++------\n Documentation/merge-config.txt | 12 ++++++------\n git-mergetool--lib.sh | 3 ++-\n 5 files changed, 36 insertions(+), 15 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..834ec25 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -202,7 +202,11 @@ install-html: html\n #\n # Determine \"include::\" file references in asciidoc files.\n #\n-doc.dep : $(wildcard *.txt) build-docdep.perl\n+docdep_prereqs = \\\n+\tmergetools-list.made $(mergetools_txt) \\\n+\tcmd-list.made $(cmds_txt)\n+\n+doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl\n \t$(QUIET_GEN)$(RM) $@+ $@ && \\\n \t$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n \tmv $@+ $@\n@@ -226,13 +230,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\tshow_tool_names can_diff \"* \"' > mergetools-diff.txt && \\\n+\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n+\t\t. ../git-mergetool--lib.sh && \\\n+\t\tshow_tool_names 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..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..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..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..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..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 fe068f6..f665bee 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -1,6 +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+: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}\n \n mode_ok () {\n \tdiff_mode && can_diff ||\n-- \n1.8.0.13.g3ff16bb\n"},{"id":"208037","messageId":"7v38xm12kk.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"1359334346-5879-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T02:08:59Z","receivedAt":"2013-01-28T02:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think our works crossed, while I was tweaking the previous series\nto push out as part of 'pu' you were already rerolling. Could you\ncompare this series with what I pushed out and see if anything you\nmissed? I think I fixed the (a && b || c && d) issue in the version\nI pushed out, but it is still there in this series.\n"},{"id":"208038","messageId":"7vvcaiyrys.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"1359334346-5879-5-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 4/4] doc: Generate a list of valid merge tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T02:14:03Z","receivedAt":"2013-01-28T02:14:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Use the show_tool_names() function to build lists of all\n> the built-in tools supported by difftool and mergetool.\n> This frees us from needing to update the documentation\n> whenever a new tool is added.\n>\n> Signed-off-by: David Aguilar \n> ---\n> Adjusted to use show_tool_names() and reworked the makefile dependencies.\n> I could use another set of eyes on the Makefile..\n\nLooks good from a quick scan and comparison with the previous\nround. Thanks for a quick reroll.\n\n\n> Documentation/.gitignore | 1 +\n> Documentation/Makefile | 22 ++++++++++++++++++++--\n> Documentation/diff-config.txt | 13 +++++++------\n> Documentation/merge-config.txt | 12 ++++++------\n> git-mergetool--lib.sh | 3 ++-\n> 5 files changed, 36 insertions(+), 15 deletions(-)\n>\n> diff --git a/Documentation/.gitignore b/Documentation/.gitignore\n> index 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\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 267dfe1..834ec25 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -202,7 +202,11 @@ install-html: html\n> #\n> # Determine \"include::\" file references in asciidoc files.\n> #\n> -doc.dep : $(wildcard *.txt) build-docdep.perl\n> +docdep_prereqs = \\\n> +\tmergetools-list.made $(mergetools_txt) \\\n> +\tcmd-list.made $(cmds_txt)\n> +\n> +doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl\n> \t$(QUIET_GEN)$(RM) $@+ $@ && \\\n> \t$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n> \tmv $@+ $@\n> @@ -226,13 +230,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\tshow_tool_names can_diff \"* \"' > mergetools-diff.txt && \\\n> +\t$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \\\n> +\t\t. ../git-mergetool--lib.sh && \\\n> +\t\tshow_tool_names 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\n> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\n> index 67a90a8..7c968d1 100644\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -132,9 +132,10 @@ diff..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..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..cmd variable is defined.\n> +\n> +include::mergetools-diff.txt[]\n> diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt\n> index 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..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..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\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index fe068f6..f665bee 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -1,6 +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> +: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}\n> \n> mode_ok () {\n> \tdiff_mode && can_diff ||\n"},{"id":"208039","messageId":"CAJDDKr7sQKDNPpaFJi+en479TF=aLXL2pvXODeo6CC3xD1KMGw@mail.gmail.com","threadId":"32748","inReplyTo":"7v38xm12kk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T02:21:00Z","receivedAt":"2013-01-28T02:21:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano wrote:\n> I think our works crossed, while I was tweaking the previous series\n> to push out as part of 'pu' you were already rerolling. Could you\n> compare this series with what I pushed out and see if anything you\n> missed? I think I fixed the (a && b || c && d) issue in the version\n> I pushed out, but it is still there in this series.\n\nAh, I see.\n\nI can add the addition of preamble for use by show_tool_help()\nas a follow up along with using a here-doc when printing.\n\nThe other diff is the Makefile dependencies.\n\nI currently have this diff against da/mergetool-docs:\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex f595d26..834ec25 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -202,7 +202,11 @@ install-html: html\n #\n # Determine \"include::\" file references in asciidoc files.\n #\n-doc.dep : $(wildcard *.txt) build-docdep.perl\n+docdep_prereqs = \\\n+ mergetools-list.made $(mergetools_txt) \\\n+ cmd-list.made $(cmds_txt)\n+\n+doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl\n $(QUIET_GEN)$(RM) $@+ $@ && \\\n $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \\\n mv $@+ $@\n\nI'll send three follow-up patches. - here-doc, preamble stuff,\nand Makefile deps, based on what's currently in pu.\n-- \nDavid\n"},{"id":"208041","messageId":"7vr4l6yrd3.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"CAJDDKr7sQKDNPpaFJi+en479TF=aLXL2pvXODeo6CC3xD1KMGw@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T02:27:04Z","receivedAt":"2013-01-28T02:27:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano wrote:\n>> I think our works crossed, while I was tweaking the previous series\n>> to push out as part of 'pu' you were already rerolling. Could you\n>> compare this series with what I pushed out and see if anything you\n>> missed? I think I fixed the (a && b || c && d) issue in the version\n>> I pushed out, but it is still there in this series.\n>\n> Ah, I see.\n>\n> I can add the addition of preamble for use by show_tool_help()\n> as a follow up along with using a here-doc when printing.\n\nI think the progression of the series is just fine as-is with the\nnew series you posted (I didn't amend the old one with all the\nsuggestions I made in the review, just only with the more important\nones that would affect correctness, so please consider that the\nchanges you have in this new round that I didn't have in 'pu' are\ngood ones to keep.\n"},{"id":"208043","messageId":"CAJDDKr75K3RGgU79nrznbpjQMLQGkDs=W8XEofURNsS1X1bvjg@mail.gmail.com","threadId":"32748","inReplyTo":"7vr4l6yrd3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T02:41:04Z","receivedAt":"2013-01-28T02:41:04Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jan 27, 2013 at 6:27 PM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano wrote:\n>>> I think our works crossed, while I was tweaking the previous series\n>>> to push out as part of 'pu' you were already rerolling. Could you\n>>> compare this series with what I pushed out and see if anything you\n>>> missed? I think I fixed the (a && b || c && d) issue in the version\n>>> I pushed out, but it is still there in this series.\n>>\n>> Ah, I see.\n>>\n>> I can add the addition of preamble for use by show_tool_help()\n>> as a follow up along with using a here-doc when printing.\n>\n> I think the progression of the series is just fine as-is with the\n> new series you posted (I didn't amend the old one with all the\n> suggestions I made in the review, just only with the more important\n> ones that would affect correctness, so please consider that the\n> changes you have in this new round that I didn't have in 'pu' are\n> good ones to keep.\n\nOkay, cool, so no need to reroll, ya?\n\nThe one thing missing in my latest series was the fixed mode_ok()\nwhich you corrected in cfb611b34089a0b5794f4ec453289a4764d94050.\n\nLet me know if there's anything else I should send out or splice together.\n\n\nJohn, I didn't completely address your question about keeping\nthe sort and prefix in show_tool_help() but I can stop poking at\nit now in case you want to start looking at what it would take\nto get custom tools listed in the --tool-help output.\n-- \nDavid\n"},{"id":"208045","messageId":"7vk3qyyq5d.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"CAJDDKr75K3RGgU79nrznbpjQMLQGkDs=W8XEofURNsS1X1bvjg@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T02:53:18Z","receivedAt":"2013-01-28T02:53:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Okay, cool, so no need to reroll, ya?\n\nIt was more like \"please don't switch to incremental yet\"; I tweaked\nthe mode_ok in your v2 and pushed out the result on 'pu' again.\n\nThere may later be comments from others that make us realize some\npatches need to be rerolled, but nothing from me for now.\n\nThanks.\n"},{"id":"208070","messageId":"5F78436DB1994B6DA686EC1BFA96B54E@PhilipOakley","threadId":"32748","inReplyTo":"1359334346-5879-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2013-01-28T08:10:06Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"David Aguilar\" \nSent: Monday, January 28, 2013 12:52 AM\n> This is round two of this series.\n> I think this touched on everything brought up in the code review.\n> 4/4 could use a review as I'm not completely familiar with the\n> makefile dependencies, though it seems to work correctly.\n\nDoes this 4/4 have any effect on the Msysgit / Git for Windows \ndocumentation which simply refers [IIRC] to HTML documenation made by \nJunio?\n\nThat is, how easy is it to create a 'default' set of docs, rather than \npersonalised documenation. Or have I misunderstood how it is working?\n\n>\n> David 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 | 22 +++++++-\n> Documentation/diff-config.txt | 13 ++---\n> Documentation/merge-config.txt | 12 ++---\n> git-mergetool--lib.sh | 116 \n> ++++++++++++++++++++++-------------------\n> 5 files changed, 96 insertions(+), 68 deletions(-)\n>\n> -- \n> 1.8.0.13.g3ff16bb\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>\n>\n> -----\n> No virus found in this message.\n> Checked by AVG - www.avg.com\n> Version: 2013.0.2890 / Virus Database: 2639/6061 - Release Date: \n> 01/27/13\n> \n"},{"id":"208072","messageId":"CAJDDKr4BT_1YnnfJv-YFHOpWhYpuA_5CMRw_hPTiowMr49RLKQ@mail.gmail.com","threadId":"32748","inReplyTo":"5F78436DB1994B6DA686EC1BFA96B54E@PhilipOakley","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-28T09:16:34Z","receivedAt":"2013-01-28T09:16:34Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley wrote:\n> From: \"David Aguilar\" \n> Sent: Monday, January 28, 2013 12:52 AM\n>\n>> This is round two of this series.\n>> I think this touched on everything brought up in the code review.\n>> 4/4 could use a review as I'm not completely familiar with the\n>> makefile dependencies, though it seems to work correctly.\n>\n>\n> Does this 4/4 have any effect on the Msysgit / Git for Windows documentation\n> which simply refers [IIRC] to HTML documenation made by Junio?\n>\n> That is, how easy is it to create a 'default' set of docs, rather than\n> personalised documenation. Or have I misunderstood how it is working?\n\nIt doesn't have any effect on Msysgit. The resulting documentation\nlists all available tools, on all platforms.\n-- \nDavid\n"},{"id":"208109","messageId":"20130128193700.GB7498@serenity.lan","threadId":"32748","inReplyTo":"1359334346-5879-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-28T19:37:00Z","receivedAt":"2013-01-28T19:37:00Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:\n> Refactor show_tool_help() so that the tool-finding logic is broken out\n> into a separate show_tool_names() function.\n> \n> Signed-off-by: David Aguilar \n> ---\n> filter_tools renamed to show_tool_names() and simplfied\n> to use ls -1. show_tool_names() now has a preamble as discussed.\n> \n> git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------\n> 1 file changed, 39 insertions(+), 29 deletions(-)\n> \n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index db3eb58..fe068f6 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -2,6 +2,35 @@\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\nCan we move show_tool_names() to be above show_tool_help()? It's a\nvery minor nit but I prefer having related functionality grouped\ntogether.\n\n> +show_tool_names () {\n> +\tcondition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n\nWould this be better with one value on each line? Also perhaps\nper_line_prefix -> line_prefix.\n\n> +\n> +\t( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n> +\twhile read toolname\n> +\tdo\n> +\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n> +\t\t\t(eval \"$condition\" \"$toolname\")\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> +\t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$tool\"\n\nThis needs to be:\n\n printf \"$per_line_prefix%s\\n\" \"$tool\"\n\nsince $per_line_prefix is usually '\\t\\t' which isn't expanded if we\nformat it with %s - an alternative would be to change the value passed\nin to '$TAB$TAB' with literal tabs.\n\n> +\t\tfi\n> +\tdone\n> +}\n> +\n> diff_mode() {\n> \ttest \"$TOOL_MODE\" = diff\n> }\n"},{"id":"208116","messageId":"7vham1vydf.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"20130128193700.GB7498@serenity.lan","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T20:36:12Z","receivedAt":"2013-01-28T20:36:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n>> +\t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$tool\"\n>\n> This needs to be:\n>\n> printf \"$per_line_prefix%s\\n\" \"$tool\"\n>\n> since $per_line_prefix is usually '\\t\\t' which isn't expanded if we\n> format it with %s - an alternative would be to change the value passed\n> in to '$TAB$TAB' with literal tabs.\n\nI would prefer the latter, actually. I do understand the\nconvenience of being able to write backslash-t, but I do not think\nit outweighs the potential risk of mistakingly passing a string with\nper-cent in it.\n"},{"id":"208124","messageId":"20130128210136.GC7498@serenity.lan","threadId":"32748","inReplyTo":"CAJDDKr75K3RGgU79nrznbpjQMLQGkDs=W8XEofURNsS1X1bvjg@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-28T21:01:36Z","receivedAt":"2013-01-28T21:01:36Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jan 27, 2013 at 06:41:04PM -0800, David Aguilar wrote:\n> John, I didn't completely address your question about keeping\n> the sort and prefix in show_tool_help() but I can stop poking at\n> it now in case you want to start looking at what it would take\n> to get custom tools listed in the --tool-help output.\n\nI've had a quick look and it's quite straightforward to build on top of\nthis to get an output format like this:\n\n 'git mergetool --tool-' may be set to one of the following:\n araxis\n gvimdiff\n gvimdiff2\n vimdiff\n vimdiff2\n\n user-defined:\n mytool\n\n The following tools are valid, but not currently available:\n bc3\n codecompare\n deltawalker\n diffuse\n ecmerge\n emerge\n kdiff3\n meld\n opendiff\n p4merge\n tkdiff\n tortoisemerge\n xxdiff\n\n user-defined:\n mybrokentool\n\n Some of the tools listed above only work in a windowed\n environment. If run in a terminal-only session, they will fail.\n\n\nI don't think the suffix form would be too hard either - it just\nrequires moving an explicit sort into the top-level shot_tool_help\nfunction.\n\nI'm going to hold off doing any more on this until da/mergetool-docs has\ngraduated to next since I think it will be easier to just build on that\nrather than trying to put all the necessary pieces into place now.\n\n\nJohn\n"},{"id":"208125","messageId":"2CAD0EB4A81B49E6AAF06BB29A4C8E05@PhilipOakley","threadId":"32748","inReplyTo":"CAJDDKr4BT_1YnnfJv-YFHOpWhYpuA_5CMRw_hPTiowMr49RLKQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2013-01-28T21:01:36Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"David Aguilar\" \nSent: Monday, January 28, 2013 9:16 AM\n> On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley \n> wrote:\n>> From: \"David Aguilar\" \n>> Sent: Monday, January 28, 2013 12:52 AM\n>>\n>>> This is round two of this series.\n>>> I think this touched on everything brought up in the code review.\n>>> 4/4 could use a review as I'm not completely familiar with the\n>>> makefile dependencies, though it seems to work correctly.\n>>\n>>\n>> Does this 4/4 have any effect on the Msysgit / Git for Windows \n>> documentation\n>> which simply refers [IIRC] to HTML documenation made by Junio?\n>>\n>> That is, how easy is it to create a 'default' set of docs, rather \n>> than\n>> personalised documenation. Or have I misunderstood how it is working?\n>\n> It doesn't have any effect on Msysgit. The resulting documentation\n> lists all available tools, on all platforms.\n>\nThat's useful to know. I must have misunderstood one of the earlier \nmessages suggesting it would also list all the users other (non typical) \ninstalled mergetools and hence add them into the documentation.\n\nPhilip \n"},{"id":"208127","messageId":"7vk3qxugdg.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"20130128210136.GC7498@serenity.lan","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T21:50:19Z","receivedAt":"2013-01-28T21:50:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n\n> I've had a quick look and it's quite straightforward to build on top of\n> this to get an output format like this:\n>\n> 'git mergetool --tool-' may be set to one of the following:\n> araxis\n>...\n> vimdiff2\n>\n> user-defined:\n> mytool\n>\n> The following tools are valid, but not currently available:\n> bc3\n>...\n> xxdiff\n>\n> user-defined:\n> mybrokentool\n>\n> Some of the tools listed above only work in a windowed\n> environment. If run in a terminal-only session, they will fail.\n>\n> I don't think the suffix form would be too hard either - it just\n> requires moving an explicit sort into the top-level shot_tool_help\n> function.\n\nI tend to think that one-tool-per-line format like the above looks\nnicer.\n\nWhat are the situations where a valid user-defined tools is\nunavailable, by the way?\n"},{"id":"208131","messageId":"20130128222147.GD7498@serenity.lan","threadId":"32748","inReplyTo":"7vk3qxugdg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-28T22:21:47Z","receivedAt":"2013-01-28T22:21:47Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Jan 28, 2013 at 01:50:19PM -0800, Junio C Hamano wrote:\n> What are the situations where a valid user-defined tools is\n> unavailable, by the way?\n\nThe same as a built-in tool: the command isn't available.\n\nCurrently I'm extracting the command word using:\n\n cmd=$(eval -- \"set -- $(git config mergetool.$tool.cmd); echo \\\"$1\\\"\")\n\n(it's slightly more complicated due to handling difftool.$tool.cmd as\nwell, but that's essentially it). Then it just uses the same \"type\n$cmd\" test as for built-in tools.\n\nI don't know if there's a better way to extract the first word, but\nthat's the best I've come up with so far.\n\n\nJohn\n"},{"id":"208179","messageId":"ke8de9$lk5$1@ger.gmane.org","threadId":"32748","inReplyTo":"20130128222147.GD7498@serenity.lan","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-01-29T11:56:58Z","receivedAt":"2013-01-29T11:56:58Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"John Keeping wrote:\n> On Mon, Jan 28, 2013 at 01:50:19PM -0800, Junio C Hamano wrote:\n>> What are the situations where a valid user-defined tools is\n>> unavailable, by the way?\n>\n> The same as a built-in tool: the command isn't available.\n>\n> Currently I'm extracting the command word using:\n>\n> cmd=$(eval -- \"set -- $(git config mergetool.$tool.cmd); echo\n> \\\"$1\\\"\")\n\nShouldnt this work?\ncmd=$((git config \"mergetool.$tool.cmd\" || git config \"difftool.$tool.cmd\") \n| awk '{print $1}')\n\n\n> (it's slightly more complicated due to handling difftool.$tool.cmd as\n> well, but that's essentially it). Then it just uses the same \"type\n> $cmd\" test as for built-in tools.\n>\n> I don't know if there's a better way to extract the first word, but\n> that's the best I've come up with so far.\n>\n>\n> John \n"},{"id":"208180","messageId":"20130129120923.GE7498@serenity.lan","threadId":"32748","inReplyTo":"ke8de9$lk5$1@ger.gmane.org","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T12:09:23Z","receivedAt":"2013-01-29T12:09:23Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Jan 29, 2013 at 12:56:58PM +0100, Joachim Schmitz wrote:\n> John Keeping wrote:\n> > Currently I'm extracting the command word using:\n> >\n> > cmd=$(eval -- \"set -- $(git config mergetool.$tool.cmd); echo\n> > \\\"$1\\\"\")\n> \n> Shouldnt this work?\n> cmd=$((git config \"mergetool.$tool.cmd\" || git config \"difftool.$tool.cmd\") \n> | awk '{print $1}')\n\nThat doesn't handle paths with spaces in, whereas the eval in a subshell\ndoes:\n\n $ cmd='\"my command\" $BASE $LOCAL $REMOTE'\n $ echo \"$cmd\" | awk '{print $1}'\n \"my\n $ ( eval -- \"set -- $cmd; echo \\\"\\$1\\\"\" )\n my command\n\n\nJohn\n"},{"id":"208192","messageId":"7vsj5krmng.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"20130129120923.GE7498@serenity.lan","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-29T16:15:15Z","receivedAt":"2013-01-29T16:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Tue, Jan 29, 2013 at 12:56:58PM +0100, Joachim Schmitz wrote:\n>> John Keeping wrote:\n>> > Currently I'm extracting the command word using:\n>> >\n>> > cmd=$(eval -- \"set -- $(git config mergetool.$tool.cmd); echo\n>> > \\\"$1\\\"\")\n>> \n>> Shouldnt this work?\n>> cmd=$((git config \"mergetool.$tool.cmd\" || git config \"difftool.$tool.cmd\") \n>> | awk '{print $1}')\n>\n> That doesn't handle paths with spaces in, whereas the eval in a subshell\n> does:\n>\n> $ cmd='\"my command\" $BASE $LOCAL $REMOTE'\n> $ echo \"$cmd\" | awk '{print $1}'\n> \"my\n> $ ( eval -- \"set -- $cmd; echo \\\"\\$1\\\"\" )\n> my command\n\nI'd rather not to see you do any of the above.\n\nWith any backend that is non-trivial, it would not be unusual for\nthe *tool.cmd to look like:\n\n [mergetool]\n \tmytool = sh -c '\n \t... some massaging to prepare the command line\n ... to run the real tool backend comes here, and\n \t\t... then ...\n my_real_tool $arg1 $arg2 ...\n\t'\n\nand you will end up detecting the presence of the shell, which is\nnot very useful.\n\nI think it is perfectly fine to say \"you configured it, so it must\nexist; it may fail when we try to run it but it is your problem\".\nIt is simpler to explain and requires one less eval.\n\n \n \n"},{"id":"208197","messageId":"20130129164619.GA1342@serenity.lan","threadId":"32748","inReplyTo":"7vsj5krmng.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 0/4] Auto-generate mergetool lists","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T16:46:20Z","receivedAt":"2013-01-29T16:46:20Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Jan 29, 2013 at 08:15:15AM -0800, Junio C Hamano wrote:\n> With any backend that is non-trivial, it would not be unusual for\n> the *tool.cmd to look like:\n> \n> [mergetool]\n> \tmytool = sh -c '\n> \t... some massaging to prepare the command line\n> ... to run the real tool backend comes here, and\n> \t\t... then ...\n> my_real_tool $arg1 $arg2 ...\n> \t'\n> \n> and you will end up detecting the presence of the shell, which is\n> not very useful.\n> \n> I think it is perfectly fine to say \"you configured it, so it must\n> exist; it may fail when we try to run it but it is your problem\".\n> It is simpler to explain and requires one less eval.\n\nI think you're right. The even worse case from this point of view is if\nyou configure it as:\n\n [mergetool]\n mytool = 'f() {\n ... code to actually run the tool here ...\n }; f $BASE $REMOTE $LOCAL $MERGED'\n\nwhich results in a false \"unavailable\" rather than just a useless check.\n\n\nJohn\n"},{"id":"208209","messageId":"20130129192204.GC1342@serenity.lan","threadId":"32748","inReplyTo":"1359334346-5879-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T19:22:04Z","receivedAt":"2013-01-29T19:22: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 04:52:23PM -0800, David Aguilar wrote:\n> Update variable assignments to always use $(command \"$arg\")\n> in their RHS instead of \"$(command \"$arg\")\" as the latter\n> is harder to read. Make get_merge_tool_cmd() simpler by\n> avoiding \"echo\" and $(command) substitutions completely.\n> \n> Signed-off-by: David Aguilar \n> ---\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\nThis change should be reverted to avoid calling \"test -z\" without any\nother arguments, as Johannes pointed out in v1.\n\nThe rest of this patch looks good to me.\n\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"},{"id":"208213","messageId":"20130129194846.GD1342@serenity.lan","threadId":"32748","inReplyTo":"1359334346-5879-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T19:48:47Z","receivedAt":"2013-01-29T19:48:47Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -2,6 +2,35 @@\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> +show_tool_names () {\n> +\tcondition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n> +\n> +\t( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n\nIs the '*' necessary here? I would expect ls to list the current\ndirectory if given no arguments, but perhaps some platforms behave\ndifferently?\n\n> +\twhile read toolname\n> +\tdo\n> +\t\tif setup_tool \"$toolname\" 2>/dev/null &&\n> +\t\t\t(eval \"$condition\" \"$toolname\")\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> +\t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$tool\"\n> +\t\tfi\n> +\tdone\n> +}\n> +\n> diff_mode() {\n> \ttest \"$TOOL_MODE\" = diff\n> }\n> @@ -199,35 +228,21 @@ 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> +\ttool_opt=\"'git ${TOOL_MODE}tool --tool-'\"\n> +\tavailable=$(show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n> +\t\t\"$tool_opt may be set to one of the following:\")\n> +\tunavailable=$(show_tool_names 'mode_ok && ! is_available' '\\t\\t' \\\n> +\t\t\"The following tools are valid, but not currently available:\")\n> \tif test -n \"$available\"\n> \tthen\n> -\t\techo \"'git $cmd_name --tool=' may be set to one of the following:\"\n> -\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n> +\t\techo \"$available\"\n> \telse\n> \t\techo \"No suitable tool for 'git $cmd_name --tool=' found.\"\n> \tfi\n> \tif test -n \"$unavailable\"\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\techo \"$unavailable\"\n> \tfi\n> \tif test -n \"$unavailable$available\"\n> \tthen\n\nYou haven't taken full advantage of the simplification Junio suggested\nin response to v1 here. We can change the \"unavailable\" block to be:\n\n show_tool_names 'mode_ok && ! is_available' \"$TAB$TAB\" \\\n \"${LF}The following tools are valid, but not currently available:\"\n\nIf you also add a \"not_found_msg\" parameter to show_tool_names then the\n\"available\" case is also simplified:\n\n show_tool_names 'mode_ok && is_available' \"$TAB$TAB\" \\\n \"$tool_opt may be set to one of the following:\" \\\n \"No suitable tool for 'git $cmd_name --tool=' found.\"\n\nwith this at the end of show_tool_names:\n\n test -n \"$preamble\" && test -n \"$not_found_msg\" && \\\n echo \"$not_found_msg\"\n\n\nJohn\n"},{"id":"208218","messageId":"7vr4l3oi1z.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"20130129194846.GD1342@serenity.lan","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-29T20:22:48Z","receivedAt":"2013-01-29T20:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -2,6 +2,35 @@\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>> +show_tool_names () {\n>> +\tcondition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n>> +\n>> +\t( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n>\n> Is the '*' necessary here?\n\nNo, it was just from a bad habit (I have ls aliased to ls -A or ls\n-a in my interactive environment, which trained my fingers to this).\n\nI also think you can lose -1, although it does not hurt.\n>> +\ttool_opt=\"'git ${TOOL_MODE}tool --tool-'\"\n>> +\tavailable=$(show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n>> +\t\t\"$tool_opt may be set to one of the following:\")\n>> +\tunavailable=$(show_tool_names 'mode_ok && ! is_available' '\\t\\t' \\\n>> +\t\t\"The following tools are valid, but not currently available:\")\n>> \tif test -n \"$available\"\n>> \tthen\n>> -\t\techo \"'git $cmd_name --tool=' may be set to one of the following:\"\n>> -\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n>> +\t\techo \"$available\"\n>> \telse\n>> \t\techo \"No suitable tool for 'git $cmd_name --tool=' found.\"\n>> \tfi\n>> \tif test -n \"$unavailable\"\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\techo \"$unavailable\"\n>> \tfi\n>> \tif test -n \"$unavailable$available\"\n>> \tthen\n>\n> You haven't taken full advantage of the simplification Junio suggested\n> in response to v1 here. We can change the \"unavailable\" block to be:\n>\n> show_tool_names 'mode_ok && ! is_available' \"$TAB$TAB\" \\\n> \"${LF}The following tools are valid, but not currently available:\"\n\nActually I was hoping that we can enhance show_tool_names so that we\ncan do without the $available and $unavailable variables at all.\n\n> If you also add a \"not_found_msg\" parameter to show_tool_names then the\n> \"available\" case is also simplified:\n>\n> show_tool_names 'mode_ok && is_available' \"$TAB$TAB\" \\\n> \"$tool_opt may be set to one of the following:\" \\\n> \"No suitable tool for 'git $cmd_name --tool=' found.\"\n>\n> with this at the end of show_tool_names:\n>\n> test -n \"$preamble\" && test -n \"$not_found_msg\" && \\\n> echo \"$not_found_msg\"\n\nYes, something along that line.\n"},{"id":"208223","messageId":"CAJDDKr4CFyQrAec3jCxyDCx0+4BMXmQAciG1YcnMYS=PAeW-Mw@mail.gmail.com","threadId":"32748","inReplyTo":"20130129192204.GC1342@serenity.lan","subject":"Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-29T22:09:21Z","receivedAt":"2013-01-29T22:09:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jan 29, 2013 at 11:22 AM, John Keeping wrote:\n> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:\n>> Update variable assignments to always use $(command \"$arg\")\n>> in their RHS instead of \"$(command \"$arg\")\" as the latter\n>> is harder to read. Make get_merge_tool_cmd() simpler by\n>> avoiding \"echo\" and $(command) substitutions completely.\n>>\n>> Signed-off-by: David Aguilar \n>> ---\n>> @@ -300,9 +292,9 @@ get_merge_tool_path () {\n>> fi\n>> if test -z \"$merge_tool_path\"\n>> then\n>> - merge_tool_path=\"$(translate_merge_tool_path \"$merge_tool\")\"\n>> + merge_tool_path=$(translate_merge_tool_path \"$merge_tool\")\n>> fi\n>> - if test -z \"$(get_merge_tool_cmd \"$merge_tool\")\" &&\n>> + if test -z $(get_merge_tool_cmd \"$merge_tool\") &&\n>\n> This change should be reverted to avoid calling \"test -z\" without any\n> other arguments, as Johannes pointed out in v1.\n>\n> The rest of this patch looks good to me.\n\nYou're right. My eyes have probably been staring at it too long and I\nmissed this (even though I thought I had checked).\n\nJunio, how would you like these patches?\nIncrementals on top of da/mergetool-docs?\n\nI won't be able to get to them until later tonight (PST) at the\nearliest, though.\n-- \nDavid\n"},{"id":"208225","messageId":"CAJDDKr4e=pg=YJ4CfUk7guUCcikBtXTveVX9j6CV5NtGvPB=9Q@mail.gmail.com","threadId":"32748","inReplyTo":"7vr4l3oi1z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-29T22:27:21Z","receivedAt":"2013-01-29T22:27:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jan 29, 2013 at 12:22 PM, Junio C Hamano wrote:\n> John Keeping writes:\n>\n>> On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:\n>>> --- a/git-mergetool--lib.sh\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -2,6 +2,35 @@\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>>> + diff_mode && can_diff ||\n>>> + merge_mode && can_merge\n>>> +}\n>>> +\n>>> +is_available () {\n>>> + merge_tool_path=$(translate_merge_tool_path \"$1\") &&\n>>> + type \"$merge_tool_path\" >/dev/null 2>&1\n>>> +}\n>>> +\n>>> +show_tool_names () {\n>>> + condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n>>> +\n>>> + ( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n>>\n>> Is the '*' necessary here?\n>\n> No, it was just from a bad habit (I have ls aliased to ls -A or ls\n> -a in my interactive environment, which trained my fingers to this).\n>\n> I also think you can lose -1, although it does not hurt.\n>>> + tool_opt=\"'git ${TOOL_MODE}tool --tool-'\"\n>>> + available=$(show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n>>> + \"$tool_opt may be set to one of the following:\")\n>>> + unavailable=$(show_tool_names 'mode_ok && ! is_available' '\\t\\t' \\\n>>> + \"The following tools are valid, but not currently available:\")\n>>> if test -n \"$available\"\n>>> then\n>>> - echo \"'git $cmd_name --tool=' may be set to one of the following:\"\n>>> - echo \"$available\" | sort | sed -e 's/^/ /'\n>>> + echo \"$available\"\n>>> else\n>>> echo \"No suitable tool for 'git $cmd_name --tool=' found.\"\n>>> fi\n>>> if test -n \"$unavailable\"\n>>> then\n>>> echo\n>>> - echo 'The following tools are valid, but not currently available:'\n>>> - echo \"$unavailable\" | sort | sed -e 's/^/ /'\n>>> + echo \"$unavailable\"\n>>> fi\n>>> if test -n \"$unavailable$available\"\n>>> then\n>>\n>> You haven't taken full advantage of the simplification Junio suggested\n>> in response to v1 here. We can change the \"unavailable\" block to be:\n>>\n>> show_tool_names 'mode_ok && ! is_available' \"$TAB$TAB\" \\\n>> \"${LF}The following tools are valid, but not currently available:\"\n>\n> Actually I was hoping that we can enhance show_tool_names so that we\n> can do without the $available and $unavailable variables at all.\n>\n>> If you also add a \"not_found_msg\" parameter to show_tool_names then the\n>> \"available\" case is also simplified:\n>>\n>> show_tool_names 'mode_ok && is_available' \"$TAB$TAB\" \\\n>> \"$tool_opt may be set to one of the following:\" \\\n>> \"No suitable tool for 'git $cmd_name --tool=' found.\"\n>>\n>> with this at the end of show_tool_names:\n>>\n>> test -n \"$preamble\" && test -n \"$not_found_msg\" && \\\n>> echo \"$not_found_msg\"\n>\n> Yes, something along that line.\n\nI don't want to stomp on your feet and poke at this file too much if\nyou're planning on building on top of it (I already did a few times\n;-). My git time is a bit limited for the next few days so I don't\nwant to hold you up. I can help shepherd through small fixups that\ncome up until the weekend rolls around and I have more time, but I\nalso don't want to hold you back until then.\n\nI will have some time tonight. If you guys would prefer an\nincremental patch I can send one that changes the \"ls\" expression and\nthe way the unavailable block is structured. Otherwise, I can send\nreplacements to handle the \"test -z\" thing, $TAB$TAB, and the\nsimplification of the unavailable block.\n\nLater patches (that are working towards the new feature) can\ngeneralize show_tool_names() further and eliminate the need for the\navailable/unavailable variables altogether. John, I would imagine\nthat you'd want to pick that up since you're driving towards having\n--tool-help honor custom tools.\n\nWhat do you think?\n-- \nDavid\n"},{"id":"208226","messageId":"7vip6foc9m.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"CAJDDKr4CFyQrAec3jCxyDCx0+4BMXmQAciG1YcnMYS=PAeW-Mw@mail.gmail.com","subject":"Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-29T22:27:49Z","receivedAt":"2013-01-29T22:27:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> On Tue, Jan 29, 2013 at 11:22 AM, John Keeping wrote:\n>> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:\n>>> Update variable assignments to always use $(command \"$arg\")\n>>> in their RHS instead of \"$(command \"$arg\")\" as the latter\n>>> is harder to read. Make get_merge_tool_cmd() simpler by\n>>> avoiding \"echo\" and $(command) substitutions completely.\n>>>\n>>> Signed-off-by: David Aguilar \n>>> ---\n>>> @@ -300,9 +292,9 @@ get_merge_tool_path () {\n>>> fi\n>>> if test -z \"$merge_tool_path\"\n>>> then\n>>> - merge_tool_path=\"$(translate_merge_tool_path \"$merge_tool\")\"\n>>> + merge_tool_path=$(translate_merge_tool_path \"$merge_tool\")\n>>> fi\n>>> - if test -z \"$(get_merge_tool_cmd \"$merge_tool\")\" &&\n>>> + if test -z $(get_merge_tool_cmd \"$merge_tool\") &&\n>>\n>> This change should be reverted to avoid calling \"test -z\" without any\n>> other arguments, as Johannes pointed out in v1.\n>>\n>> The rest of this patch looks good to me.\n>\n> You're right. My eyes have probably been staring at it too long and I\n> missed this (even though I thought I had checked).\n\nBy now you (and people who were following this thread) are beginning\nto see why I said \"I'd feel safer with extra dq\" ;-)\n\nI'll amend locally and push the result out.\n"},{"id":"208228","messageId":"7va9rroazl.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"CAJDDKr4e=pg=YJ4CfUk7guUCcikBtXTveVX9j6CV5NtGvPB=9Q@mail.gmail.com","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-29T22:55:26Z","receivedAt":"2013-01-29T22:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> I don't want to stomp on your feet and poke at this file too much if\n> you're planning on building on top of it (I already did a few times\n> ;-). My git time is a bit limited for the next few days so I don't\n> want to hold you up. I can help shepherd through small fixups that\n> come up until the weekend rolls around and I have more time, but I\n> also don't want to hold you back until then.\n\nI can work with John to get this part into a shape to support his\nextended use sometime toward the end of this week, by which time\nhopefully you have some time to comment on the result. John, how\ndoes that sound?\n"},{"id":"208229","messageId":"20130129230238.GF1342@serenity.lan","threadId":"32748","inReplyTo":"CAJDDKr4e=pg=YJ4CfUk7guUCcikBtXTveVX9j6CV5NtGvPB=9Q@mail.gmail.com","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T23:02:38Z","receivedAt":"2013-01-29T23:02:38Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Jan 29, 2013 at 02:27:21PM -0800, David Aguilar wrote:\n> I don't want to stomp on your feet and poke at this file too much if\n> you're planning on building on top of it (I already did a few times\n> ;-). My git time is a bit limited for the next few days so I don't\n> want to hold you up. I can help shepherd through small fixups that\n> come up until the weekend rolls around and I have more time, but I\n> also don't want to hold you back until then.\n> \n> I will have some time tonight. If you guys would prefer an\n> incremental patch I can send one that changes the \"ls\" expression and\n> the way the unavailable block is structured. Otherwise, I can send\n> replacements to handle the \"test -z\" thing, $TAB$TAB, and the\n> simplification of the unavailable block.\n> \n> Later patches (that are working towards the new feature) can\n> generalize show_tool_names() further and eliminate the need for the\n> available/unavailable variables altogether. John, I would imagine\n> that you'd want to pick that up since you're driving towards having\n> --tool-help honor custom tools.\n> \n> What do you think?\n\nI was planning to hold off until this series is in a reasonable state -\nthere's no rush as far as I'm concerned, but if Junio's happy to leave\nthese patches with just the small fixups I'm happy to build on that with\na patch that removes the available and unavailable variables before\nadding the tools from git-config.\n\n\nJohn\n"},{"id":"208230","messageId":"20130129230607.GG1342@serenity.lan","threadId":"32748","inReplyTo":"7va9rroazl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-29T23:06:08Z","receivedAt":"2013-01-29T23:06:08Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:\n> David Aguilar writes:\n> \n> > I don't want to stomp on your feet and poke at this file too much if\n> > you're planning on building on top of it (I already did a few times\n> > ;-). My git time is a bit limited for the next few days so I don't\n> > want to hold you up. I can help shepherd through small fixups that\n> > come up until the weekend rolls around and I have more time, but I\n> > also don't want to hold you back until then.\n> \n> I can work with John to get this part into a shape to support his\n> extended use sometime toward the end of this week, by which time\n> hopefully you have some time to comment on the result. John, how\n> does that sound?\n\nMy email crossed with yours - that sounds good to me. If\nda/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I\nshould be able to have a look at it then - if not I'm happy to hold off\nlonger.\n\n\nJohn\n"},{"id":"208237","messageId":"7vwquvmkon.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"20130129230607.GG1342@serenity.lan","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-30T03:08:56Z","receivedAt":"2013-01-30T03:08:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:\n> ...\n>> I can work with John to get this part into a shape to support his\n>> extended use sometime toward the end of this week, by which time\n>> hopefully you have some time to comment on the result. John, how\n>> does that sound?\n>\n> My email crossed with yours - that sounds good to me. If\n> da/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I\n> should be able to have a look at it then - if not I'm happy to hold off\n> longer.\n\nHeh, I actually was hoping that you will send in a replacement for\nDavid's patch ;-)\n\nHere is what I will squash into the one we have been discussing. In\na few hours, I expect I'll be able to push this out in the 'pu'\nbranch.\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 29 Jan 2013 18:57:55 -0800\nSubject: [PATCH] [SQUASH] mergetools: tweak show_tool_names and its users\n\nUse show_tool_names as a function to produce output, not as a\nfunction to compute a string. Indicate if any output was given\nwith its return status, so that the caller can say \"if it didn't\ngive any output, I'll say this instead\" easily.\n\nTo be squashed into the previous; no need to keep this log message.\n---\n git-mergetool--lib.sh | 30 +++++++++++++++++-------------\n 1 file changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 135da96..79cbdc7 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -22,7 +22,7 @@ is_available () {\n show_tool_names () {\n \tcondition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}\n \n-\t( cd \"$MERGE_TOOLS_DIR\" && ls -1 * ) |\n+\t( cd \"$MERGE_TOOLS_DIR\" && ls ) |\n \twhile read toolname\n \tdo\n \t\tif setup_tool \"$toolname\" 2>/dev/null &&\n@@ -36,6 +36,7 @@ show_tool_names () {\n \t\t\tprintf \"%s%s\\n\" \"$per_line_prefix\" \"$tool\"\n \t\tfi\n \tdone\n+\ttest -n \"$preamble\"\n }\n \n diff_mode() {\n@@ -236,27 +237,30 @@ list_merge_tool_candidates () {\n \n show_tool_help () {\n \ttool_opt=\"'git ${TOOL_MODE}tool --tool-'\"\n-\tavailable=$(show_tool_names 'mode_ok && is_available' '\\t\\t' \\\n-\t\t\"$tool_opt may be set to one of the following:\")\n-\tunavailable=$(show_tool_names 'mode_ok && ! is_available' '\\t\\t' \\\n-\t\t\"The following tools are valid, but not currently available:\")\n-\tif test -n \"$available\"\n+\n+\ttab='\t' av_shown= unav_shown=\n+\n+\tif show_tool_names 'mode_ok && is_available' \"$tab$tab\" \\\n+\t\t\"$tool_opt may be set to one of the following:\"\n \tthen\n-\t\techo \"$available\"\n+\t\tav_shown=yes\n \telse\n \t\techo \"No suitable tool for 'git $cmd_name --tool=' found.\"\n+\t\tav_shown=no\n \tfi\n-\tif test -n \"$unavailable\"\n+\n+\tif show_tool_names 'mode_ok && ! is_available' \"$tab$tab\" \\\n+\t\t\"The following tools are valid, but not currently available:\"\n \tthen\n-\t\techo\n-\t\techo \"$unavailable\"\n+\t\tunav_shown=yes\n \tfi\n-\tif test -n \"$unavailable$available\"\n-\tthen\n+\n+\tcase \",$av_shown,$unav_shown,\" in\n+\t*,yes,*)\n \t\techo\n \t\techo \"Some of the tools listed above only work in a windowed\"\n \t\techo \"environment. If run in a terminal-only session, they will fail.\"\n-\tfi\n+\tesac\n \texit 0\n }\n \n-- \n1.8.1.2.555.gedafe79\n"},{"id":"208239","messageId":"7vsj5jmjie.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"7vwquvmkon.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-30T03:34:17Z","receivedAt":"2013-01-30T03:34:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Heh, I actually was hoping that you will send in a replacement for\n> David's patch ;-)\n>\n> Here is what I will squash into the one we have been discussing. In\n> a few hours, I expect I'll be able to push this out in the 'pu'\n> branch.\n\nI ended up doing this a bit differently; will push out the result\nafter merging the other topics to 'pu'.\n"},{"id":"208242","messageId":"1359526854-25132-1-git-send-email-davvid@gmail.com","threadId":"32748","inReplyTo":"7vip6foc9m.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3 1/4] mergetool--lib: simplify command expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-30T06:20:54Z","receivedAt":"2013-01-30T06:20:54Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Update variable assignments to always use $(command \"$arg\")\nin their RHS instead of \"$(command \"$arg\")\" as the latter\nis harder to read. Make get_merge_tool_cmd() simpler by\navoiding \"echo\" and $(command) substitutions completely.\n\nSigned-off-by: David Aguilar \n---\nThis is a replacement patch for what's currently in pu to\nfix the empty \"test -z\" expression.\n\n git-mergetool--lib.sh | 42 ++++++++++++++++++------------------------\n 1 file changed, 18 insertions(+), 24 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 1d0fb12..1ff6d38 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,11 @@ 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+\n+\tmerge_tool_cmd=$(get_merge_tool_cmd \"$merge_tool\")\n+\tif test -z \"$merge_tool_cmd\" &&\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 +308,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.9.g3370a50\n"},{"id":"208245","messageId":"7vobg7m9sn.fsf@alter.siamese.dyndns.org","threadId":"32748","inReplyTo":"1359526854-25132-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v3 1/4] mergetool--lib: simplify command expressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-30T07:04:08Z","receivedAt":"2013-01-30T07:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Update variable assignments to always use $(command \"$arg\")\n> in their RHS instead of \"$(command \"$arg\")\" as the latter\n> is harder to read. Make get_merge_tool_cmd() simpler by\n> avoiding \"echo\" and $(command) substitutions completely.\n>\n> Signed-off-by: David Aguilar \n> ---\n> This is a replacement patch for what's currently in pu to\n> fix the empty \"test -z\" expression.\n\nThanks.\n\nI think I already pushed out what I locally amended; will double\ncheck.\n"}]}