[PATCH v2 0/4] Auto-generate mergetool lists

[PATCH v2 0/4] Auto-generate mergetool lists

[David Aguilar]

This is round two of this series.
I think this touched on everything brought up in the code review.
4/4 could use a review as I'm not completely familiar with the
makefile dependencies, though it seems to work correctly.

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         |  22 +++++++-
 Documentation/diff-config.txt  |  13 ++---
 Documentation/merge-config.txt |  12 ++---
 git-mergetool--lib.sh          | 116 ++++++++++++++++++++++-------------------
 5 files changed, 96 insertions(+), 68 deletions(-)

-- 
1.8.0.13.g3ff16bb

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

[David Aguilar]

Update variable assignments to always use $(command "$arg")
in their RHS instead of "$(command "$arg")" as the latter
is harder to read.  Make get_merge_tool_cmd() simpler by
avoiding "echo" and $(command) substitutions completely.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
I reworded the commit message to be more clear.

 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.g3ff16bb

[PATCH v2 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>
---
This now uses a cat << here-doc.

 git-mergetool--lib.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 9a5aae9..db3eb58 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -240,8 +240,13 @@ show_tool_help () {
 
 guess_merge_tool () {
 	list_merge_tool_candidates
-	echo >&2 "merge tool candidates: $tools"
+	cat >&2 <<-EOF
 
+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
+EOF
 	# Loop over each candidate and stop when a valid merge tool is found.
 	for i in $tools
 	do
-- 
1.8.0.13.g3ff16bb

[PATCH v2 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 a separate show_tool_names() function.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
filter_tools renamed to show_tool_names() and simplfied
to use ls -1.  show_tool_names() now has a preamble as discussed.

 git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 29 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index db3eb58..fe068f6 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -2,6 +2,35 @@
 # 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
+}
+
+show_tool_names () {
+	condition=${1:-true} 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" "$toolname")
+		then
+			if test -n "$preamble"
+			then
+				echo "$preamble"
+				preamble=
+			fi
+			printf "%s%s\n" "$per_line_prefix" "$tool"
+		fi
+	done
+}
+
 diff_mode() {
 	test "$TOOL_MODE" = diff
 }
@@ -199,35 +228,21 @@ 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
+	tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
+	available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
+		"$tool_opt may be set to one of the following:")
+	unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
+		"The following tools are valid, but not currently available:")
 	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/^/	/'
+		echo "$available"
 	else
 		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
 	fi
 	if test -n "$unavailable"
 	then
 		echo
-		echo 'The following tools are valid, but not currently available:'
-		echo "$unavailable" | sort | sed -e 's/^/	/'
+		echo "$unavailable"
 	fi
 	if test -n "$unavailable$available"
 	then
@@ -248,17 +263,12 @@ See 'git ${TOOL_MODE}tool --tool-help' or 'git help config' for more details.
 $tools
 EOF
 	# 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.g3ff16bb

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

[David Aguilar]

Use the show_tool_names() 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>
---
Adjusted to use show_tool_names() and reworked the makefile dependencies.
I could use another set of eyes on the Makefile..

 Documentation/.gitignore       |  1 +
 Documentation/Makefile         | 22 ++++++++++++++++++++--
 Documentation/diff-config.txt  | 13 +++++++------
 Documentation/merge-config.txt | 12 ++++++------
 git-mergetool--lib.sh          |  3 ++-
 5 files changed, 36 insertions(+), 15 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..834ec25 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -202,7 +202,11 @@ install-html: html
 #
 # Determine "include::" file references in asciidoc files.
 #
-doc.dep : $(wildcard *.txt) build-docdep.perl
+docdep_prereqs = \
+	mergetools-list.made $(mergetools_txt) \
+	cmd-list.made $(cmds_txt)
+
+doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl
 	$(QUIET_GEN)$(RM) $@+ $@ && \
 	$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
 	mv $@+ $@
@@ -226,13 +230,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 && \
+		show_tool_names can_diff "* "' > mergetools-diff.txt && \
+	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
+		. ../git-mergetool--lib.sh && \
+		show_tool_names 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 fe068f6..f665bee 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 # git-mergetool--lib is a library for common merge tool functions
-MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
+
+: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}
 
 mode_ok () {
 	diff_mode && can_diff ||
-- 
1.8.0.13.g3ff16bb

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Use the show_tool_names() 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>
> ---
> Adjusted to use show_tool_names() and reworked the makefile dependencies.
> I could use another set of eyes on the Makefile..

Looks good from a quick scan and comparison with the previous
round.  Thanks for a quick reroll.


>  Documentation/.gitignore       |  1 +
>  Documentation/Makefile         | 22 ++++++++++++++++++++--
>  Documentation/diff-config.txt  | 13 +++++++------
>  Documentation/merge-config.txt | 12 ++++++------
>  git-mergetool--lib.sh          |  3 ++-
>  5 files changed, 36 insertions(+), 15 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..834ec25 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -202,7 +202,11 @@ install-html: html
>  #
>  # Determine "include::" file references in asciidoc files.
>  #
> -doc.dep : $(wildcard *.txt) build-docdep.perl
> +docdep_prereqs = \
> +	mergetools-list.made $(mergetools_txt) \
> +	cmd-list.made $(cmds_txt)
> +
> +doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl
>  	$(QUIET_GEN)$(RM) $@+ $@ && \
>  	$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
>  	mv $@+ $@
> @@ -226,13 +230,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 && \
> +		show_tool_names can_diff "* "' > mergetools-diff.txt && \
> +	$(SHELL_PATH) -c 'MERGE_TOOLS_DIR=../mergetools && \
> +		. ../git-mergetool--lib.sh && \
> +		show_tool_names 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 fe068f6..f665bee 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -1,6 +1,7 @@
>  #!/bin/sh
>  # git-mergetool--lib is a library for common merge tool functions
> -MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
> +
> +: ${MERGE_TOOLS_DIR=$(git --exec-path)/mergetools}
>  
>  mode_ok () {
>  	diff_mode && can_diff ||

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

[John Keeping]

On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
> Refactor show_tool_help() so that the tool-finding logic is broken out
> into a separate show_tool_names() function.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> filter_tools renamed to show_tool_names() and simplfied
> to use ls -1.  show_tool_names() now has a preamble as discussed.
> 
>  git-mergetool--lib.sh | 68 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db3eb58..fe068f6 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,35 @@
>  # 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
> +}
> +

Can we move show_tool_names() to be above show_tool_help()?  It's a
very minor nit but I prefer having related functionality grouped
together.

> +show_tool_names () {
> +	condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}

Would this be better with one value on each line?  Also perhaps
per_line_prefix -> line_prefix.

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

This needs to be:

    printf "$per_line_prefix%s\n" "$tool"

since $per_line_prefix is usually '\t\t' which isn't expanded if we
format it with %s - an alternative would be to change the value passed
in to '$TAB$TAB' with literal tabs.

> +		fi
> +	done
> +}
> +
>  diff_mode() {
>  	test "$TOOL_MODE" = diff
>  }

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

>> +			printf "%s%s\n" "$per_line_prefix" "$tool"
>
> This needs to be:
>
>     printf "$per_line_prefix%s\n" "$tool"
>
> since $per_line_prefix is usually '\t\t' which isn't expanded if we
> format it with %s - an alternative would be to change the value passed
> in to '$TAB$TAB' with literal tabs.

I would prefer the latter, actually.  I do understand the
convenience of being able to write backslash-t, but I do not think
it outweighs the potential risk of mistakingly passing a string with
per-cent in it.

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

[John Keeping]

On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,35 @@
>  # 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
> +}
> +
> +show_tool_names () {
> +	condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
> +
> +	( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |

Is the '*' necessary here?  I would expect ls to list the current
directory if given no arguments, but perhaps some platforms behave
differently?

> +	while read toolname
> +	do
> +		if setup_tool "$toolname" 2>/dev/null &&
> +			(eval "$condition" "$toolname")
> +		then
> +			if test -n "$preamble"
> +			then
> +				echo "$preamble"
> +				preamble=
> +			fi
> +			printf "%s%s\n" "$per_line_prefix" "$tool"
> +		fi
> +	done
> +}
> +
>  diff_mode() {
>  	test "$TOOL_MODE" = diff
>  }
> @@ -199,35 +228,21 @@ 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
> +	tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
> +	available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
> +		"$tool_opt may be set to one of the following:")
> +	unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
> +		"The following tools are valid, but not currently available:")
>  	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/^/	/'
> +		echo "$available"
>  	else
>  		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>  	fi
>  	if test -n "$unavailable"
>  	then
>  		echo
> -		echo 'The following tools are valid, but not currently available:'
> -		echo "$unavailable" | sort | sed -e 's/^/	/'
> +		echo "$unavailable"
>  	fi
>  	if test -n "$unavailable$available"
>  	then

You haven't taken full advantage of the simplification Junio suggested
in response to v1 here.  We can change the "unavailable" block to be:

    show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
        "${LF}The following tools are valid, but not currently available:"

If you also add a "not_found_msg" parameter to show_tool_names then the
"available" case is also simplified:

    show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
        "$tool_opt may be set to one of the following:" \
        "No suitable tool for 'git $cmd_name --tool=<tool>' found."

with this at the end of show_tool_names:

    test -n "$preamble" && test -n "$not_found_msg" && \
        echo "$not_found_msg"


John

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -2,6 +2,35 @@
>>  # 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
>> +}
>> +
>> +show_tool_names () {
>> +	condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
>> +
>> +	( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
>
> Is the '*' necessary here?

No, it was just from a bad habit (I have ls aliased to ls -A or ls
-a in my interactive environment, which trained my fingers to this).

I also think you can lose -1, although it does not hurt.
>> +	tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
>> +	available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
>> +		"$tool_opt may be set to one of the following:")
>> +	unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
>> +		"The following tools are valid, but not currently available:")
>>  	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/^/	/'
>> +		echo "$available"
>>  	else
>>  		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>>  	fi
>>  	if test -n "$unavailable"
>>  	then
>>  		echo
>> -		echo 'The following tools are valid, but not currently available:'
>> -		echo "$unavailable" | sort | sed -e 's/^/	/'
>> +		echo "$unavailable"
>>  	fi
>>  	if test -n "$unavailable$available"
>>  	then
>
> You haven't taken full advantage of the simplification Junio suggested
> in response to v1 here.  We can change the "unavailable" block to be:
>
>     show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
>         "${LF}The following tools are valid, but not currently available:"

Actually I was hoping that we can enhance show_tool_names so that we
can do without the $available and $unavailable variables at all.

> If you also add a "not_found_msg" parameter to show_tool_names then the
> "available" case is also simplified:
>
>     show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
>         "$tool_opt may be set to one of the following:" \
>         "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>
> with this at the end of show_tool_names:
>
>     test -n "$preamble" && test -n "$not_found_msg" && \
>         echo "$not_found_msg"

Yes, something along that line.

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

[David Aguilar]

On Tue, Jan 29, 2013 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -2,6 +2,35 @@
>>>  # 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
>>> +}
>>> +
>>> +show_tool_names () {
>>> +    condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
>>> +
>>> +    ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
>>
>> Is the '*' necessary here?
>
> No, it was just from a bad habit (I have ls aliased to ls -A or ls
> -a in my interactive environment, which trained my fingers to this).
>
> I also think you can lose -1, although it does not hurt.
>>> +    tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
>>> +    available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
>>> +            "$tool_opt may be set to one of the following:")
>>> +    unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
>>> +            "The following tools are valid, but not currently available:")
>>>      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/^/ /'
>>> +            echo "$available"
>>>      else
>>>              echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>>>      fi
>>>      if test -n "$unavailable"
>>>      then
>>>              echo
>>> -            echo 'The following tools are valid, but not currently available:'
>>> -            echo "$unavailable" | sort | sed -e 's/^/       /'
>>> +            echo "$unavailable"
>>>      fi
>>>      if test -n "$unavailable$available"
>>>      then
>>
>> You haven't taken full advantage of the simplification Junio suggested
>> in response to v1 here.  We can change the "unavailable" block to be:
>>
>>     show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
>>         "${LF}The following tools are valid, but not currently available:"
>
> Actually I was hoping that we can enhance show_tool_names so that we
> can do without the $available and $unavailable variables at all.
>
>> If you also add a "not_found_msg" parameter to show_tool_names then the
>> "available" case is also simplified:
>>
>>     show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
>>         "$tool_opt may be set to one of the following:" \
>>         "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>>
>> with this at the end of show_tool_names:
>>
>>     test -n "$preamble" && test -n "$not_found_msg" && \
>>         echo "$not_found_msg"
>
> Yes, something along that line.

I don't want to stomp on your feet and poke at this file too much if
you're planning on building on top of it (I already did a few times
;-).  My git time is a bit limited for the next few days so I don't
want to hold you up.  I can help shepherd through small fixups that
come up until the weekend rolls around and I have more time, but I
also don't want to hold you back until then.

I will have some time tonight.  If you guys would prefer an
incremental patch I can send one that changes the "ls" expression and
the way the unavailable block is structured.  Otherwise, I can send
replacements to handle the "test -z" thing, $TAB$TAB, and the
simplification of the unavailable block.

Later patches (that are working towards the new feature) can
generalize show_tool_names() further and eliminate the need for the
available/unavailable variables altogether.  John, I would imagine
that you'd want to pick that up since you're driving towards having
--tool-help honor custom tools.

What do you think?
-- 
David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I don't want to stomp on your feet and poke at this file too much if
> you're planning on building on top of it (I already did a few times
> ;-).  My git time is a bit limited for the next few days so I don't
> want to hold you up.  I can help shepherd through small fixups that
> come up until the weekend rolls around and I have more time, but I
> also don't want to hold you back until then.

I can work with John to get this part into a shape to support his
extended use sometime toward the end of this week, by which time
hopefully you have some time to comment on the result.  John, how
does that sound?

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

[John Keeping]

On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:
> David Aguilar <davvid@gmail.com> writes:
> 
> > I don't want to stomp on your feet and poke at this file too much if
> > you're planning on building on top of it (I already did a few times
> > ;-).  My git time is a bit limited for the next few days so I don't
> > want to hold you up.  I can help shepherd through small fixups that
> > come up until the weekend rolls around and I have more time, but I
> > also don't want to hold you back until then.
> 
> I can work with John to get this part into a shape to support his
> extended use sometime toward the end of this week, by which time
> hopefully you have some time to comment on the result.  John, how
> does that sound?

My email crossed with yours - that sounds good to me.  If
da/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I
should be able to have a look at it then - if not I'm happy to hold off
longer.


John

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Tue, Jan 29, 2013 at 02:55:26PM -0800, Junio C Hamano wrote:
> ...
>> I can work with John to get this part into a shape to support his
>> extended use sometime toward the end of this week, by which time
>> hopefully you have some time to comment on the result.  John, how
>> does that sound?
>
> My email crossed with yours - that sounds good to me.  If
> da/mergetool-docs is in a reasonable state by tomorrow evening (GMT) I
> should be able to have a look at it then - if not I'm happy to hold off
> longer.

Heh, I actually was hoping that you will send in a replacement for
David's patch ;-)

Here is what I will squash into the one we have been discussing.  In
a few hours, I expect I'll be able to push this out in the 'pu'
branch.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 29 Jan 2013 18:57:55 -0800
Subject: [PATCH] [SQUASH] mergetools: tweak show_tool_names and its users

Use show_tool_names as a function to produce output, not as a
function to compute a string.  Indicate if any output was given
with its return status, so that the caller can say "if it didn't
give any output, I'll say this instead" easily.

To be squashed into the previous; no need to keep this log message.
---
 git-mergetool--lib.sh | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 135da96..79cbdc7 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -22,7 +22,7 @@ is_available () {
 show_tool_names () {
 	condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
 
-	( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
+	( cd "$MERGE_TOOLS_DIR" && ls ) |
 	while read toolname
 	do
 		if setup_tool "$toolname" 2>/dev/null &&
@@ -36,6 +36,7 @@ show_tool_names () {
 			printf "%s%s\n" "$per_line_prefix" "$tool"
 		fi
 	done
+	test -n "$preamble"
 }
 
 diff_mode() {
@@ -236,27 +237,30 @@ list_merge_tool_candidates () {
 
 show_tool_help () {
 	tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
-	available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
-		"$tool_opt may be set to one of the following:")
-	unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
-		"The following tools are valid, but not currently available:")
-	if test -n "$available"
+
+	tab='	' av_shown= unav_shown=
+
+	if show_tool_names 'mode_ok && is_available' "$tab$tab" \
+		"$tool_opt may be set to one of the following:"
 	then
-		echo "$available"
+		av_shown=yes
 	else
 		echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
+		av_shown=no
 	fi
-	if test -n "$unavailable"
+
+	if show_tool_names 'mode_ok && ! is_available' "$tab$tab" \
+		"The following tools are valid, but not currently available:"
 	then
-		echo
-		echo "$unavailable"
+		unav_shown=yes
 	fi
-	if test -n "$unavailable$available"
-	then
+
+	case ",$av_shown,$unav_shown," in
+	*,yes,*)
 		echo
 		echo "Some of the tools listed above only work in a windowed"
 		echo "environment. If run in a terminal-only session, they will fail."
-	fi
+	esac
 	exit 0
 }
 
-- 
1.8.1.2.555.gedafe79

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Heh, I actually was hoping that you will send in a replacement for
> David's patch ;-)
>
> Here is what I will squash into the one we have been discussing.  In
> a few hours, I expect I'll be able to push this out in the 'pu'
> branch.

I ended up doing this a bit differently; will push out the result
after merging the other topics to 'pu'.

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

[John Keeping]

On Tue, Jan 29, 2013 at 02:27:21PM -0800, David Aguilar wrote:
> I don't want to stomp on your feet and poke at this file too much if
> you're planning on building on top of it (I already did a few times
> ;-).  My git time is a bit limited for the next few days so I don't
> want to hold you up.  I can help shepherd through small fixups that
> come up until the weekend rolls around and I have more time, but I
> also don't want to hold you back until then.
> 
> I will have some time tonight.  If you guys would prefer an
> incremental patch I can send one that changes the "ls" expression and
> the way the unavailable block is structured.  Otherwise, I can send
> replacements to handle the "test -z" thing, $TAB$TAB, and the
> simplification of the unavailable block.
> 
> Later patches (that are working towards the new feature) can
> generalize show_tool_names() further and eliminate the need for the
> available/unavailable variables altogether.  John, I would imagine
> that you'd want to pick that up since you're driving towards having
> --tool-help honor custom tools.
> 
> What do you think?

I was planning to hold off until this series is in a reasonable state -
there's no rush as far as I'm concerned, but if Junio's happy to leave
these patches with just the small fixups I'm happy to build on that with
a patch that removes the available and unavailable variables before
adding the tools from git-config.


John

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

[John Keeping]

On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
> Update variable assignments to always use $(command "$arg")
> in their RHS instead of "$(command "$arg")" as the latter
> is harder to read.  Make get_merge_tool_cmd() simpler by
> avoiding "echo" and $(command) substitutions completely.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> @@ -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") &&

This change should be reverted to avoid calling "test -z" without any
other arguments, as Johannes pointed out in v1.

The rest of this patch looks good to me.

>  		! type "$merge_tool_path" >/dev/null 2>&1
>  	then
>  		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\

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

[David Aguilar]

On Tue, Jan 29, 2013 at 11:22 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
>> Update variable assignments to always use $(command "$arg")
>> in their RHS instead of "$(command "$arg")" as the latter
>> is harder to read.  Make get_merge_tool_cmd() simpler by
>> avoiding "echo" and $(command) substitutions completely.
>>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> @@ -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") &&
>
> This change should be reverted to avoid calling "test -z" without any
> other arguments, as Johannes pointed out in v1.
>
> The rest of this patch looks good to me.

You're right.  My eyes have probably been staring at it too long and I
missed this (even though I thought I had checked).

Junio, how would you like these patches?
Incrementals on top of da/mergetool-docs?

I won't be able to get to them until later tonight (PST) at the
earliest, though.
-- 
David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On Tue, Jan 29, 2013 at 11:22 AM, John Keeping <john@keeping.me.uk> wrote:
>> On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
>>> Update variable assignments to always use $(command "$arg")
>>> in their RHS instead of "$(command "$arg")" as the latter
>>> is harder to read.  Make get_merge_tool_cmd() simpler by
>>> avoiding "echo" and $(command) substitutions completely.
>>>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> ---
>>> @@ -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") &&
>>
>> This change should be reverted to avoid calling "test -z" without any
>> other arguments, as Johannes pointed out in v1.
>>
>> The rest of this patch looks good to me.
>
> You're right.  My eyes have probably been staring at it too long and I
> missed this (even though I thought I had checked).

By now you (and people who were following this thread) are beginning
to see why I said "I'd feel safer with extra dq" ;-)

I'll amend locally and push the result out.

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

[David Aguilar]

Update variable assignments to always use $(command "$arg")
in their RHS instead of "$(command "$arg")" as the latter
is harder to read.  Make get_merge_tool_cmd() simpler by
avoiding "echo" and $(command) substitutions completely.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
This is a replacement patch for what's currently in pu to
fix the empty "test -z" expression.

 git-mergetool--lib.sh | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1d0fb12..1ff6d38 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,11 @@ 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")" &&
+
+	merge_tool_cmd=$(get_merge_tool_cmd "$merge_tool")
+	if test -z "$merge_tool_cmd" &&
 		! type "$merge_tool_path" >/dev/null 2>&1
 	then
 		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\
@@ -314,11 +308,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.9.g3370a50

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Update variable assignments to always use $(command "$arg")
> in their RHS instead of "$(command "$arg")" as the latter
> is harder to read.  Make get_merge_tool_cmd() simpler by
> avoiding "echo" and $(command) substitutions completely.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This is a replacement patch for what's currently in pu to
> fix the empty "test -z" expression.

Thanks.

I think I already pushed out what I locally amended; will double
check.

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Junio C Hamano]

I think our works crossed, while I was tweaking the previous series
to push out as part of 'pu' you were already rerolling.  Could you
compare this series with what I pushed out and see if anything you
missed?  I think I fixed the (a && b || c && d) issue in the version
I pushed out, but it is still there in this series.

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[David Aguilar]

On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think our works crossed, while I was tweaking the previous series
> to push out as part of 'pu' you were already rerolling.  Could you
> compare this series with what I pushed out and see if anything you
> missed?  I think I fixed the (a && b || c && d) issue in the version
> I pushed out, but it is still there in this series.

Ah, I see.

I can add the addition of preamble for use by show_tool_help()
as a follow up along with using a here-doc when printing.

The other diff is the Makefile dependencies.

I currently have this diff against da/mergetool-docs:
diff --git a/Documentation/Makefile b/Documentation/Makefile
index f595d26..834ec25 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -202,7 +202,11 @@ install-html: html
 #
 # Determine "include::" file references in asciidoc files.
 #
-doc.dep : $(wildcard *.txt) build-docdep.perl
+docdep_prereqs = \
+       mergetools-list.made $(mergetools_txt) \
+       cmd-list.made $(cmds_txt)
+
+doc.dep : $(docdep_prereqs) $(wildcard *.txt) build-docdep.perl
        $(QUIET_GEN)$(RM) $@+ $@ && \
        $(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
        mv $@+ $@

I'll send three follow-up patches.  - here-doc, preamble stuff,
and Makefile deps, based on what's currently in pu.
-- 
David

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think our works crossed, while I was tweaking the previous series
>> to push out as part of 'pu' you were already rerolling.  Could you
>> compare this series with what I pushed out and see if anything you
>> missed?  I think I fixed the (a && b || c && d) issue in the version
>> I pushed out, but it is still there in this series.
>
> Ah, I see.
>
> I can add the addition of preamble for use by show_tool_help()
> as a follow up along with using a here-doc when printing.

I think the progression of the series is just fine as-is with the
new series you posted (I didn't amend the old one with all the
suggestions I made in the review, just only with the more important
ones that would affect correctness, so please consider that the
changes you have in this new round that I didn't have in 'pu' are
good ones to keep.

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[David Aguilar]

On Sun, Jan 27, 2013 at 6:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> On Sun, Jan 27, 2013 at 6:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> I think our works crossed, while I was tweaking the previous series
>>> to push out as part of 'pu' you were already rerolling.  Could you
>>> compare this series with what I pushed out and see if anything you
>>> missed?  I think I fixed the (a && b || c && d) issue in the version
>>> I pushed out, but it is still there in this series.
>>
>> Ah, I see.
>>
>> I can add the addition of preamble for use by show_tool_help()
>> as a follow up along with using a here-doc when printing.
>
> I think the progression of the series is just fine as-is with the
> new series you posted (I didn't amend the old one with all the
> suggestions I made in the review, just only with the more important
> ones that would affect correctness, so please consider that the
> changes you have in this new round that I didn't have in 'pu' are
> good ones to keep.

Okay, cool, so no need to reroll, ya?

The one thing missing in my latest series was the fixed mode_ok()
which you corrected in cfb611b34089a0b5794f4ec453289a4764d94050.

Let me know if there's anything else I should send out or splice together.


John, I didn't completely address your question about keeping
the sort and prefix in show_tool_help() but I can stop poking at
it now in case you want to start looking at what it would take
to get custom tools listed in the --tool-help output.
-- 
David

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Okay, cool, so no need to reroll, ya?

It was more like "please don't switch to incremental yet"; I tweaked
the mode_ok in your v2 and pushed out the result on 'pu' again.

There may later be comments from others that make us realize some
patches need to be rerolled, but nothing from me for now.

Thanks.

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[John Keeping]

On Sun, Jan 27, 2013 at 06:41:04PM -0800, David Aguilar wrote:
> John, I didn't completely address your question about keeping
> the sort and prefix in show_tool_help() but I can stop poking at
> it now in case you want to start looking at what it would take
> to get custom tools listed in the --tool-help output.

I've had a quick look and it's quite straightforward to build on top of
this to get an output format like this:

    'git mergetool --tool-<tool>' may be set to one of the following:
                    araxis
                    gvimdiff
                    gvimdiff2
                    vimdiff
                    vimdiff2

            user-defined:
                    mytool

    The following tools are valid, but not currently available:
                    bc3
                    codecompare
                    deltawalker
                    diffuse
                    ecmerge
                    emerge
                    kdiff3
                    meld
                    opendiff
                    p4merge
                    tkdiff
                    tortoisemerge
                    xxdiff

            user-defined:
                    mybrokentool

    Some of the tools listed above only work in a windowed
    environment. If run in a terminal-only session, they will fail.


I don't think the suffix form would be too hard either - it just
requires moving an explicit sort into the top-level shot_tool_help
function.

I'm going to hold off doing any more on this until da/mergetool-docs has
graduated to next since I think it will be easier to just build on that
rather than trying to put all the necessary pieces into place now.


John

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:


> I've had a quick look and it's quite straightforward to build on top of
> this to get an output format like this:
>
>     'git mergetool --tool-<tool>' may be set to one of the following:
>                     araxis
>...
>                     vimdiff2
>
>             user-defined:
>                     mytool
>
>     The following tools are valid, but not currently available:
>                     bc3
>...
>                     xxdiff
>
>             user-defined:
>                     mybrokentool
>
>     Some of the tools listed above only work in a windowed
>     environment. If run in a terminal-only session, they will fail.
>
> I don't think the suffix form would be too hard either - it just
> requires moving an explicit sort into the top-level shot_tool_help
> function.

I tend to think that one-tool-per-line format like the above looks
nicer.

What are the situations where a valid user-defined tools is
unavailable, by the way?

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[John Keeping]

On Mon, Jan 28, 2013 at 01:50:19PM -0800, Junio C Hamano wrote:
> What are the situations where a valid user-defined tools is
> unavailable, by the way?

The same as a built-in tool: the command isn't available.

Currently I'm extracting the command word using:

    cmd=$(eval -- "set -- $(git config mergetool.$tool.cmd); echo \"$1\"")

(it's slightly more complicated due to handling difftool.$tool.cmd as
well, but that's essentially it).  Then it just uses the same "type
$cmd" test as for built-in tools.

I don't know if there's a better way to extract the first word, but
that's the best I've come up with so far.


John

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Joachim Schmitz]

John Keeping wrote:
> On Mon, Jan 28, 2013 at 01:50:19PM -0800, Junio C Hamano wrote:
>> What are the situations where a valid user-defined tools is
>> unavailable, by the way?
>
> The same as a built-in tool: the command isn't available.
>
> Currently I'm extracting the command word using:
>
>    cmd=$(eval -- "set -- $(git config mergetool.$tool.cmd); echo
> \"$1\"")

Shouldnt this work?
cmd=$((git config "mergetool.$tool.cmd" || git config "difftool.$tool.cmd") 
| awk '{print $1}')


> (it's slightly more complicated due to handling difftool.$tool.cmd as
> well, but that's essentially it).  Then it just uses the same "type
> $cmd" test as for built-in tools.
>
> I don't know if there's a better way to extract the first word, but
> that's the best I've come up with so far.
>
>
> John 

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[John Keeping]

On Tue, Jan 29, 2013 at 12:56:58PM +0100, Joachim Schmitz wrote:
> John Keeping wrote:
> > Currently I'm extracting the command word using:
> >
> >    cmd=$(eval -- "set -- $(git config mergetool.$tool.cmd); echo
> > \"$1\"")
> 
> Shouldnt this work?
> cmd=$((git config "mergetool.$tool.cmd" || git config "difftool.$tool.cmd") 
> | awk '{print $1}')

That doesn't handle paths with spaces in, whereas the eval in a subshell
does:

    $ cmd='"my command" $BASE $LOCAL $REMOTE'
    $ echo "$cmd" | awk '{print $1}'
    "my
    $ ( eval -- "set -- $cmd; echo \"\$1\"" )
    my command


John

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Tue, Jan 29, 2013 at 12:56:58PM +0100, Joachim Schmitz wrote:
>> John Keeping wrote:
>> > Currently I'm extracting the command word using:
>> >
>> >    cmd=$(eval -- "set -- $(git config mergetool.$tool.cmd); echo
>> > \"$1\"")
>> 
>> Shouldnt this work?
>> cmd=$((git config "mergetool.$tool.cmd" || git config "difftool.$tool.cmd") 
>> | awk '{print $1}')
>
> That doesn't handle paths with spaces in, whereas the eval in a subshell
> does:
>
>     $ cmd='"my command" $BASE $LOCAL $REMOTE'
>     $ echo "$cmd" | awk '{print $1}'
>     "my
>     $ ( eval -- "set -- $cmd; echo \"\$1\"" )
>     my command

I'd rather not to see you do any of the above.

With any backend that is non-trivial, it would not be unusual for
the *tool.cmd to look like:

     [mergetool]
     	mytool = sh -c '
        	... some massaging to prepare the command line
                ... to run the real tool backend comes here, and
     		... then ...
                my_real_tool $arg1 $arg2 ...
	'

and you will end up detecting the presence of the shell, which is
not very useful.

I think it is perfectly fine to say "you configured it, so it must
exist; it may fail when we try to run it but it is your problem".
It is simpler to explain and requires one less eval.

        
                

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[John Keeping]

On Tue, Jan 29, 2013 at 08:15:15AM -0800, Junio C Hamano wrote:
> With any backend that is non-trivial, it would not be unusual for
> the *tool.cmd to look like:
> 
>      [mergetool]
>      	mytool = sh -c '
>         	... some massaging to prepare the command line
>                 ... to run the real tool backend comes here, and
>      		... then ...
>                 my_real_tool $arg1 $arg2 ...
> 	'
> 
> and you will end up detecting the presence of the shell, which is
> not very useful.
> 
> I think it is perfectly fine to say "you configured it, so it must
> exist; it may fail when we try to run it but it is your problem".
> It is simpler to explain and requires one less eval.

I think you're right.  The even worse case from this point of view is if
you configure it as:

    [mergetool]
        mytool = 'f() {
            ... code to actually run the tool here ...
        }; f $BASE $REMOTE $LOCAL $MERGED'

which results in a false "unavailable" rather than just a useless check.


John

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Philip Oakley]

From: "David Aguilar" <davvid@gmail.com>
Sent: Monday, January 28, 2013 12:52 AM
> This is round two of this series.
> I think this touched on everything brought up in the code review.
> 4/4 could use a review as I'm not completely familiar with the
> makefile dependencies, though it seems to work correctly.

Does this 4/4 have any effect on the Msysgit / Git for Windows 
documentation which simply refers [IIRC] to HTML documenation made by 
Junio?

That is, how easy is it to create a 'default' set of docs, rather than 
personalised documenation. Or have I misunderstood how it is working?

>
> 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         |  22 +++++++-
> Documentation/diff-config.txt  |  13 ++---
> Documentation/merge-config.txt |  12 ++---
> git-mergetool--lib.sh          | 116 
> ++++++++++++++++++++++-------------------
> 5 files changed, 96 insertions(+), 68 deletions(-)
>
> -- 
> 1.8.0.13.g3ff16bb
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> -----
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2013.0.2890 / Virus Database: 2639/6061 - Release Date: 
> 01/27/13
> 

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[David Aguilar]

On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley <philipoakley@iee.org> wrote:
> From: "David Aguilar" <davvid@gmail.com>
> Sent: Monday, January 28, 2013 12:52 AM
>
>> This is round two of this series.
>> I think this touched on everything brought up in the code review.
>> 4/4 could use a review as I'm not completely familiar with the
>> makefile dependencies, though it seems to work correctly.
>
>
> Does this 4/4 have any effect on the Msysgit / Git for Windows documentation
> which simply refers [IIRC] to HTML documenation made by Junio?
>
> That is, how easy is it to create a 'default' set of docs, rather than
> personalised documenation. Or have I misunderstood how it is working?

It doesn't have any effect on Msysgit. The resulting documentation
lists all available tools, on all platforms.
-- 
David

Re: [PATCH v2 0/4] Auto-generate mergetool lists

[Philip Oakley]

From: "David Aguilar" <davvid@gmail.com>
Sent: Monday, January 28, 2013 9:16 AM
> On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley <philipoakley@iee.org> 
> wrote:
>> From: "David Aguilar" <davvid@gmail.com>
>> Sent: Monday, January 28, 2013 12:52 AM
>>
>>> This is round two of this series.
>>> I think this touched on everything brought up in the code review.
>>> 4/4 could use a review as I'm not completely familiar with the
>>> makefile dependencies, though it seems to work correctly.
>>
>>
>> Does this 4/4 have any effect on the Msysgit / Git for Windows 
>> documentation
>> which simply refers [IIRC] to HTML documenation made by Junio?
>>
>> That is, how easy is it to create a 'default' set of docs, rather 
>> than
>> personalised documenation. Or have I misunderstood how it is working?
>
> It doesn't have any effect on Msysgit. The resulting documentation
> lists all available tools, on all platforms.
>
That's useful to know. I must have misunderstood one of the earlier 
messages suggesting it would also list all the users other (non typical) 
installed mergetools and hence add them into the documentation.

Philip