[PATCH 0/4] Documentation: Auto-generate merge tool lists

[PATCH 0/4] Documentation: Auto-generate merge tool lists

[David Aguilar]

Refactor the mergetool-lib so that we can reuse it in
Documentation/Makefile.  The end result is that the
diff.tool and merge.tool documentation now includes
an auto-generated list of all available tools.

This applies on top of jk/mergetool in pu.

David Aguilar (4):
  mergetool--lib: Simplify command expressions
  mergetool--lib: Improve the help text in guess_merge_tool()
  mergetool--lib: Add functions for finding available tools
  doc: Generate a list of valid merge tools

 Documentation/.gitignore       |   1 +
 Documentation/Makefile         |  16 +++++-
 Documentation/diff-config.txt  |  13 ++---
 Documentation/merge-config.txt |  12 ++---
 git-mergetool--lib.sh          | 108 ++++++++++++++++++++++-------------------
 5 files changed, 87 insertions(+), 63 deletions(-)

-- 
1.8.0.13.gf25ae33

[PATCH 1/4] mergetool--lib: Simplify command expressions

[David Aguilar]

Use $(command "$arg") instead of "$(command "$arg")" as the latter is
harder to read.  Make the expression in get_merge_tool_cmd() even
simpler by avoiding "echo" completely.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1d0fb12..9a5aae9 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -32,17 +32,10 @@ check_unchanged () {
 	fi
 }
 
-valid_tool_config () {
-	if test -n "$(get_merge_tool_cmd "$1")"
-	then
-		return 0
-	else
-		return 1
-	fi
-}
-
 valid_tool () {
-	setup_tool "$1" || valid_tool_config "$1"
+	setup_tool "$1" && return 0
+	cmd=$(get_merge_tool_cmd "$1")
+	test -n "$cmd"
 }
 
 setup_tool () {
@@ -96,14 +89,13 @@ setup_tool () {
 }
 
 get_merge_tool_cmd () {
-	# Prints the custom command for a merge tool
 	merge_tool="$1"
 	if diff_mode
 	then
-		echo "$(git config difftool.$merge_tool.cmd ||
-			git config mergetool.$merge_tool.cmd)"
+		git config "difftool.$merge_tool.cmd" ||
+		git config "mergetool.$merge_tool.cmd"
 	else
-		echo "$(git config mergetool.$merge_tool.cmd)"
+		git config "mergetool.$merge_tool.cmd"
 	fi
 }
 
@@ -114,7 +106,7 @@ run_merge_tool () {
 	GIT_PREFIX=${GIT_PREFIX:-.}
 	export GIT_PREFIX
 
-	merge_tool_path="$(get_merge_tool_path "$1")" || exit
+	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 	status=0
 
@@ -145,7 +137,7 @@ run_merge_tool () {
 
 # Run a either a configured or built-in diff tool
 run_diff_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	merge_tool_cmd=$(get_merge_tool_cmd "$1")
 	if test -n "$merge_tool_cmd"
 	then
 		( eval $merge_tool_cmd )
@@ -158,11 +150,11 @@ run_diff_cmd () {
 
 # Run a either a configured or built-in merge tool
 run_merge_cmd () {
-	merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+	merge_tool_cmd=$(get_merge_tool_cmd "$1")
 	if test -n "$merge_tool_cmd"
 	then
-		trust_exit_code="$(git config --bool \
-			mergetool."$1".trustExitCode || echo false)"
+		trust_exit_code=$(git config --bool \
+			"mergetool.$1.trustExitCode" || echo false)
 		if test "$trust_exit_code" = "false"
 		then
 			touch "$BACKUP"
@@ -253,7 +245,7 @@ guess_merge_tool () {
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $tools
 	do
-		merge_tool_path="$(translate_merge_tool_path "$i")"
+		merge_tool_path=$(translate_merge_tool_path "$i")
 		if type "$merge_tool_path" >/dev/null 2>&1
 		then
 			echo "$i"
@@ -300,9 +292,9 @@ get_merge_tool_path () {
 	fi
 	if test -z "$merge_tool_path"
 	then
-		merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
+		merge_tool_path=$(translate_merge_tool_path "$merge_tool")
 	fi
-	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
+	if test -z $(get_merge_tool_cmd "$merge_tool") &&
 		! type "$merge_tool_path" >/dev/null 2>&1
 	then
 		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
@@ -314,11 +306,11 @@ get_merge_tool_path () {
 
 get_merge_tool () {
 	# Check if a merge tool has been configured
-	merge_tool="$(get_configured_merge_tool)"
+	merge_tool=$(get_configured_merge_tool)
 	# Try to guess an appropriate merge tool if no tool has been set.
 	if test -z "$merge_tool"
 	then
-		merge_tool="$(guess_merge_tool)" || exit
+		merge_tool=$(guess_merge_tool) || exit
 	fi
 	echo "$merge_tool"
 }
-- 
1.8.0.13.gf25ae33

[PATCH 2/4] mergetool--lib: Improve the help text in guess_merge_tool()

[David Aguilar]

This code path is only activated when the user does not have a valid
configured tool.  Add a message to guide new users towards configuring a
default tool.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9a5aae9..cf52423 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -240,7 +240,14 @@ show_tool_help () {
 
 guess_merge_tool () {
 	list_merge_tool_candidates
-	echo >&2 "merge tool candidates: $tools"
+	msg="\
+
+This message is displayed because '$TOOL_MODE.tool' is not configured.
+See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.
+'git ${TOOL_MODE}tool' will now attempt to use one of the following tools:
+$tools
+"
+	printf "$msg" >&2
 
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $tools
-- 
1.8.0.13.gf25ae33

[PATCH 3/4] mergetool--lib: Add functions for finding available tools

[David Aguilar]

Refactor show_tool_help() so that the tool-finding logic is broken out
into separate functions.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 26 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index cf52423..894b849 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -2,6 +2,33 @@
 # git-mergetool--lib is a library for common merge tool functions
 MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
 
+mode_ok () {
+	diff_mode && can_diff ||
+	merge_mode && can_merge
+}
+
+is_available () {
+	merge_tool_path=$(translate_merge_tool_path "$1") &&
+	type "$merge_tool_path" >/dev/null 2>&1
+}
+
+filter_tools () {
+	filter="$1"
+	prefix="$2"
+	(
+		cd "$MERGE_TOOLS_DIR" &&
+		for i in *
+		do
+			echo "$i"
+		done
+	) | sort | while read tool
+	do
+		setup_tool "$tool" 2>/dev/null &&
+		(eval "$filter" "$tool") &&
+		printf "$prefix$tool\n"
+	done
+}
+
 diff_mode() {
 	test "$TOOL_MODE" = diff
 }
@@ -199,27 +226,13 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-	unavailable= available= LF='
-'
-	for i in "$MERGE_TOOLS_DIR"/*
-	do
-		tool=$(basename "$i")
-		setup_tool "$tool" 2>/dev/null || continue
-
-		merge_tool_path=$(translate_merge_tool_path "$tool")
-		if type "$merge_tool_path" >/dev/null 2>&1
-		then
-			available="$available$tool$LF"
-		else
-			unavailable="$unavailable$tool$LF"
-		fi
-	done
-
 	cmd_name=${TOOL_MODE}tool
+	available=$(filter_tools 'mode_ok && is_available' '\t\t')
+	unavailable=$(filter_tools 'mode_ok && ! is_available' '\t\t')
 	if test -n "$available"
 	then
 		echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
-		echo "$available" | sort | sed -e 's/^/	/'
+		printf "$available"
 	else
 		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
 	fi
@@ -227,7 +240,7 @@ show_tool_help () {
 	then
 		echo
 		echo 'The following tools are valid, but not currently available:'
-		echo "$unavailable" | sort | sed -e 's/^/	/'
+		printf "$unavailable"
 	fi
 	if test -n "$unavailable$available"
 	then
@@ -250,17 +263,12 @@ $tools
 	printf "$msg" >&2
 
 	# Loop over each candidate and stop when a valid merge tool is found.
-	for i in $tools
+	for tool in $tools
 	do
-		merge_tool_path=$(translate_merge_tool_path "$i")
-		if type "$merge_tool_path" >/dev/null 2>&1
-		then
-			echo "$i"
-			return 0
-		fi
+		is_available "$tool" && echo "$tool" && return 0
 	done
 
-	echo >&2 "No known merge resolution program available."
+	echo >&2 "No known ${TOOL_MODE} tool is available."
 	return 1
 }
 
-- 
1.8.0.13.gf25ae33

[PATCH 4/4] doc: Generate a list of valid merge tools

[David Aguilar]

Use the new filter_tools() function to build lists of all
the built-in tools supported by difftool and mergetool.
This frees us from needing to update the documentation
whenever a new tool is added.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/.gitignore       |  1 +
 Documentation/Makefile         | 16 +++++++++++++++-
 Documentation/diff-config.txt  | 13 +++++++------
 Documentation/merge-config.txt | 12 ++++++------
 git-mergetool--lib.sh          |  1 +
 5 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/Documentation/.gitignore b/Documentation/.gitignore
index d62aebd..2c8b2d6 100644
--- a/Documentation/.gitignore
+++ b/Documentation/.gitignore
@@ -9,4 +9,5 @@ gitman.info
 howto-index.txt
 doc.dep
 cmds-*.txt
+mergetools-*.txt
 manpage-base-url.xsl
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 267dfe1..f595d26 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -226,13 +226,27 @@ cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)
 	$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(QUIET_STDERR) && \
 	date >$@
 
+mergetools_txt = mergetools-diff.txt mergetools-merge.txt
+
+$(mergetools_txt): mergetools-list.made
+
+mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
+	$(QUIET_GEN)$(RM) $@ && \
+	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
+		. ../git-mergetool--lib.sh && \
+		filter_tools can_diff "* "' > mergetools-diff.txt && \
+	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
+		. ../git-mergetool--lib.sh && \
+		filter_tools can_merge "* "' > mergetools-merge.txt && \
+	date > $@
+
 clean:
 	$(RM) *.xml *.xml+ *.html *.html+ *.1 *.5 *.7
 	$(RM) *.texi *.texi+ *.texi++ git.info gitman.info
 	$(RM) *.pdf
 	$(RM) howto-index.txt howto/*.html doc.dep
 	$(RM) technical/api-*.html technical/api-index.txt
-	$(RM) $(cmds_txt) *.made
+	$(RM) $(cmds_txt) $(mergetools_txt) *.made
 	$(RM) manpage-base-url.xsl
 
 $(MAN_HTML): %.html : %.txt
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 67a90a8..7c968d1 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -132,9 +132,10 @@ diff.<driver>.cachetextconv::
 	conversion outputs.  See linkgit:gitattributes[5] for details.
 
 diff.tool::
-	The diff tool to be used by linkgit:git-difftool[1].  This
-	option overrides `merge.tool`, and has the same valid built-in
-	values as `merge.tool` minus "tortoisemerge" and plus
-	"kompare".  Any other value is treated as a custom diff tool,
-	and there must be a corresponding `difftool.<tool>.cmd`
-	option.
+	Controls which diff tool is used by linkgit:git-difftool[1].
+	This variable overrides the value configured in `merge.tool`.
+	The list below shows the valid built-in values.
+	Any other value is treated as a custom diff tool and requires
+	that a corresponding difftool.<tool>.cmd variable is defined.
+
+include::mergetools-diff.txt[]
diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 861bd6f..5f40e71 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -52,12 +52,12 @@ merge.stat::
 	at the end of the merge.  True by default.
 
 merge.tool::
-	Controls which merge resolution program is used by
-	linkgit:git-mergetool[1].  Valid built-in values are: "araxis",
-	"bc3", "diffuse", "ecmerge", "emerge", "gvimdiff", "kdiff3", "meld",
-	"opendiff", "p4merge", "tkdiff", "tortoisemerge", "vimdiff"
-	and "xxdiff".  Any other value is treated is custom merge tool
-	and there must be a corresponding mergetool.<tool>.cmd option.
+	Controls which merge tool is used by linkgit:git-mergetool[1].
+	The list below shows the valid built-in values.
+	Any other value is treated as a custom merge tool and requires
+	that a corresponding mergetool.<tool>.cmd variable is defined.
+
+include::mergetools-merge.txt[]
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 894b849..59e1650 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,5 +1,6 @@
 #!/bin/sh
 # git-mergetool--lib is a library for common merge tool functions
+test -z "$MERGE_TOOLS_DIR" &&
 MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
 
 mode_ok () {
-- 
1.8.0.13.gf25ae33

Re: [PATCH 1/4] mergetool--lib: Simplify command expressions

[Johannes Sixt]

Am 27.01.2013 22:24, schrieb David Aguilar:
> Use $(command "$arg") instead of "$(command "$arg")" as the latter is
> harder to read.

If at all, you should restrict yourself to simplify only variable
assignments. Because this case:

> -	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
> +	if test -z $(get_merge_tool_cmd "$merge_tool") &&

cannot work as intended: If the output of $() is empty, then without the
outer quotes this becomes

  test -z

without an operand for -z, which is a syntax error (of the test command).

-- Hannes

Re: [PATCH 1/4] mergetool--lib: Simplify command expressions

[David Aguilar]

On Sun, Jan 27, 2013 at 2:08 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 27.01.2013 22:24, schrieb David Aguilar:
>> Use $(command "$arg") instead of "$(command "$arg")" as the latter is
>> harder to read.
>
> If at all, you should restrict yourself to simplify only variable
> assignments. Because this case:
>
>> -     if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
>> +     if test -z $(get_merge_tool_cmd "$merge_tool") &&
>
> cannot work as intended: If the output of $() is empty, then without the
> outer quotes this becomes
>
>   test -z
>
> without an operand for -z, which is a syntax error (of the test command).

Definitely.  I learned this the hard way when the tests broke on me while
working it ;-)  My patch rewrites things to always use var=$(command)
expressions with separate test "$var" evaluating them.
Thanks for the tip,
-- 
David

Re: [PATCH 1/4] mergetool--lib: Simplify command expressions

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Use $(command "$arg") instead of "$(command "$arg")" as the latter is
> harder to read.

Did you miss my comment that this is about RHS of an assignment?

Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools

[Johannes Sixt]

Am 27.01.2013 22:24, schrieb David Aguilar:
> Refactor show_tool_help() so that the tool-finding logic is broken out
> into separate functions.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index cf52423..894b849 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,33 @@
>  # git-mergetool--lib is a library for common merge tool functions
>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>  
> +mode_ok () {
> +	diff_mode && can_diff ||
> +	merge_mode && can_merge
> +}

&& and || have the same precedence: if diff_mode and can_diff both are
"true", then the result of the function is that of can_merge. I don't
think that is what is intended.

> +filter_tools () {
> +	filter="$1"
> +	prefix="$2"
> +	(
> +		cd "$MERGE_TOOLS_DIR" &&
> +		for i in *
> +		do
> +			echo "$i"
> +		done

		cd "$MERGE_TOOLS_DIR" &&
		printf "%s\n" *

But what's wrong with "ls -1"? It would save the explicit sort.

> +	) | sort | while read tool
> +	do
> +		setup_tool "$tool" 2>/dev/null &&
> +		(eval "$filter" "$tool") &&
> +		printf "$prefix$tool\n"
> +	done
> +}

-- Hannes

Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Refactor show_tool_help() so that the tool-finding logic is broken out
> into separate functions.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index cf52423..894b849 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,33 @@
>  # git-mergetool--lib is a library for common merge tool functions
>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>  
> +mode_ok () {
> +	diff_mode && can_diff ||
> +	merge_mode && can_merge
> +}

I think you got the operator precedence mixed up.

What happens when diff_mode=true, can_diff=true, merge_mode=true and
can_merge=false?

	if true && true || true && false
	then
		echo OK
	else
		echo NO
	fi
	if (true && true) || (true && false)
	then
		echo OK
	else
		echo NO
	fi

> +is_available () {
> +	merge_tool_path=$(translate_merge_tool_path "$1") &&
> +	type "$merge_tool_path" >/dev/null 2>&1
> +}
> +
> +filter_tools () {
> +	filter="$1"
> +	prefix="$2"
> +	(
> +		cd "$MERGE_TOOLS_DIR" &&
> +		for i in *
> +		do
> +			echo "$i"
> +		done
> +	) | sort | while read tool
> +	do

Please start a new line before keywords that define the syntactic
structure, like "while", i.e.

	... piped | commands |
	while read tool
	do
		...

Re: [PATCH 1/4] mergetool--lib: Simplify command expressions

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Definitely.  I learned this the hard way when the tests broke on me while
> working it ;-)  My patch rewrites things to always use var=$(command)
> expressions with separate test "$var" evaluating them.

OK; that wasn't clear from the log message.

Re: [PATCH 4/4] doc: Generate a list of valid merge tools

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> +mergetools_txt = mergetools-diff.txt mergetools-merge.txt
> +
> +$(mergetools_txt): mergetools-list.made
> +
> +mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
> +	$(QUIET_GEN)$(RM) $@ && \
> +	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
> +		. ../git-mergetool--lib.sh && \
> +		filter_tools can_diff "* "' > mergetools-diff.txt && \
> +	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
> +		. ../git-mergetool--lib.sh && \
> +		filter_tools can_merge "* "' > mergetools-merge.txt && \
> +	date > $@

Nicely done.

By omitting is_available check and only checking can_*, we would get
the same result on all platforms and I'd see tortoise appear in the
output even I do not do Windows.

Thanks.

Re: [PATCH 4/4] doc: Generate a list of valid merge tools

[John Keeping]

On Sun, Jan 27, 2013 at 01:24:46PM -0800, David Aguilar wrote:
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -1,5 +1,6 @@
>  #!/bin/sh
>  # git-mergetool--lib is a library for common merge tool functions
> +test -z "$MERGE_TOOLS_DIR" &&
>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools

The preferred pattern in Git seems to be this:

    : ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}


John

Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools

[John Keeping]

On Sun, Jan 27, 2013 at 01:24:45PM -0800, David Aguilar wrote:
> +filter_tools () {
> +	filter="$1"
> +	prefix="$2"
> +	(
> +		cd "$MERGE_TOOLS_DIR" &&
> +		for i in *
> +		do
> +			echo "$i"
> +		done
> +	) | sort | while read tool
> +	do
> +		setup_tool "$tool" 2>/dev/null &&
> +		(eval "$filter" "$tool") &&
> +		printf "$prefix$tool\n"
> +	done
> +}

Can we change this so that it does this:

    filter_tools () {
    	filter="$1"
    	(
    		cd "$MERGE_TOOLS_DIR" &&
    		for i in *
    		do
    			echo "$i"
    		done
    	) |
        while read tool
    	do
    		setup_tool "$tool" 2>/dev/null &&
    		(eval "$filter" "$tool") &&
    		echo "$tool"
    	done
    }

and keep the sorting and prefix in show_tool_help?  This will make it
easier to integrate the user-configured tools from git-config.


John

Re: [PATCH 2/4] mergetool--lib: Improve the help text in guess_merge_tool()

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> This code path is only activated when the user does not have a valid
> configured tool.  Add a message to guide new users towards configuring a
> default tool.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 9a5aae9..cf52423 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -240,7 +240,14 @@ show_tool_help () {
>  
>  guess_merge_tool () {
>  	list_merge_tool_candidates
> -	echo >&2 "merge tool candidates: $tools"
> +	msg="\
> +
> +This message is displayed because '$TOOL_MODE.tool' is not configured.
> +See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.
> +'git ${TOOL_MODE}tool' will now attempt to use one of the following tools:
> +$tools
> +"
> +	printf "$msg" >&2

This is not wrong per-se, but wouldn't it be much easier to read to
use an indented HERE-text like this?

	cat >&2 <<-EOF
	... a long message with $var substitution ...
        ... comes here ...
        EOF

Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> +filter_tools () {
> +	filter="$1"
> +	prefix="$2"
> +	(
> +		cd "$MERGE_TOOLS_DIR" &&
> +		for i in *
> +		do
> +			echo "$i"
> +		done
> +	) | sort | while read tool
> +	do
> +		setup_tool "$tool" 2>/dev/null &&
> +		(eval "$filter" "$tool") &&
> +		printf "$prefix$tool\n"
> +	done
> +}
> +
>  diff_mode() {
>  	test "$TOOL_MODE" = diff
>  }
> @@ -199,27 +226,13 @@ list_merge_tool_candidates () {
>  }
>  
>  show_tool_help () {
> -	unavailable= available= LF='
> -'
> -	for i in "$MERGE_TOOLS_DIR"/*
> -	do
> -		tool=$(basename "$i")
> -		setup_tool "$tool" 2>/dev/null || continue
> -
> -		merge_tool_path=$(translate_merge_tool_path "$tool")
> -		if type "$merge_tool_path" >/dev/null 2>&1
> -		then
> -			available="$available$tool$LF"
> -		else
> -			unavailable="$unavailable$tool$LF"
> -		fi
> -	done
> -
>  	cmd_name=${TOOL_MODE}tool
> +	available=$(filter_tools 'mode_ok && is_available' '\t\t')
> +	unavailable=$(filter_tools 'mode_ok && ! is_available' '\t\t')
>  	if test -n "$available"
>  	then
>  		echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
> -		echo "$available" | sort | sed -e 's/^/	/'
> +		printf "$available"

This may happen to be safe as available will not have anything with
a per-cent % in it, but is not a good discipline in general.

	printf "%s" "$available"

If you are giving filter_tools an optional "prefix-per-line", I do
not think it is too much of a stretch to introduce another optional
"perfix for the whole thing" and let this call site say something
like this:

	cmd_name=${TOOL_MODE}tool
	show_tool_names 'mode_ok && is_available' '\t\t' \
		"'git $cmd_name --tool=<tool>' may be set to one of these:"
	show_tool_names 'mode_ok && !is_available' '\t\t' \
		"These are valid but not available:"

without any of the above logic (and the same for unav).  It may look like this:

show_tool_names () {
	condition=${1?condition} per_line_prefix=${2:-} preamble=${3:-}

        ( cd "$MERGE_TOOLS_DIR && ls -1 * ) |
	while read toolname
	do
		if setup_tool "$toolname" 2>/dev/null && (eval "$condition")
		then
			if test -n "$preamble"
			then
				echo "$preamble"
				preamble=
			fi
                        printf "%s%s\n" "$prefix" "$toolname"
		fi
	done
}

Re: [PATCH 3/4] mergetool--lib: Add functions for finding available tools

[David Aguilar]

On Sun, Jan 27, 2013 at 3:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> +filter_tools () {
>> +     filter="$1"
>> +     prefix="$2"
>> +     (
>> +             cd "$MERGE_TOOLS_DIR" &&
>> +             for i in *
>> +             do
>> +                     echo "$i"
>> +             done
>> +     ) | sort | while read tool
>> +     do
>> +             setup_tool "$tool" 2>/dev/null &&
>> +             (eval "$filter" "$tool") &&
>> +             printf "$prefix$tool\n"
>> +     done
>> +}
>> +
>>  diff_mode() {
>>       test "$TOOL_MODE" = diff
>>  }
>> @@ -199,27 +226,13 @@ list_merge_tool_candidates () {
>>  }
>>
>>  show_tool_help () {
>> -     unavailable= available= LF='
>> -'
>> -     for i in "$MERGE_TOOLS_DIR"/*
>> -     do
>> -             tool=$(basename "$i")
>> -             setup_tool "$tool" 2>/dev/null || continue
>> -
>> -             merge_tool_path=$(translate_merge_tool_path "$tool")
>> -             if type "$merge_tool_path" >/dev/null 2>&1
>> -             then
>> -                     available="$available$tool$LF"
>> -             else
>> -                     unavailable="$unavailable$tool$LF"
>> -             fi
>> -     done
>> -
>>       cmd_name=${TOOL_MODE}tool
>> +     available=$(filter_tools 'mode_ok && is_available' '\t\t')
>> +     unavailable=$(filter_tools 'mode_ok && ! is_available' '\t\t')
>>       if test -n "$available"
>>       then
>>               echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
>> -             echo "$available" | sort | sed -e 's/^/ /'
>> +             printf "$available"
>
> This may happen to be safe as available will not have anything with
> a per-cent % in it, but is not a good discipline in general.
>
>         printf "%s" "$available"
>
> If you are giving filter_tools an optional "prefix-per-line", I do
> not think it is too much of a stretch to introduce another optional
> "perfix for the whole thing" and let this call site say something
> like this:
>
>         cmd_name=${TOOL_MODE}tool
>         show_tool_names 'mode_ok && is_available' '\t\t' \
>                 "'git $cmd_name --tool=<tool>' may be set to one of these:"
>         show_tool_names 'mode_ok && !is_available' '\t\t' \
>                 "These are valid but not available:"
>
> without any of the above logic (and the same for unav).  It may look like this:
>
> show_tool_names () {
>         condition=${1?condition} per_line_prefix=${2:-} preamble=${3:-}
>
>         ( cd "$MERGE_TOOLS_DIR && ls -1 * ) |
>         while read toolname
>         do
>                 if setup_tool "$toolname" 2>/dev/null && (eval "$condition")
>                 then
>                         if test -n "$preamble"
>                         then
>                                 echo "$preamble"
>                                 preamble=
>                         fi
>                         printf "%s%s\n" "$prefix" "$toolname"
>                 fi
>         done
> }

Thanks guys.  I'll re-roll this soon.
-- 
David

Re: [PATCH 4/4] doc: Generate a list of valid merge tools

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 267dfe1..f595d26 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -226,13 +226,27 @@ cmd-list.made: cmd-list.perl ../command-list.txt $(MAN1_TXT)
>  	$(PERL_PATH) ./cmd-list.perl ../command-list.txt $(QUIET_STDERR) && \
>  	date >$@
>  
> +mergetools_txt = mergetools-diff.txt mergetools-merge.txt
> +
> +$(mergetools_txt): mergetools-list.made

The product of files that include these two and doc.dep target have
to depend on these two files being up to date.  doc.dep depends on
$(wildcard *.txt) so you may be lucky and $(mergetools_txt) have
been built once in which case they would depend on it, or unlukcy
and end up doc.dep that does not know about them, no?