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

STALE3739d

12 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:55

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

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:55

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 <redacted>
---
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 3/4] mergetool--lib: Add functions for finding available tools

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:55

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 <redacted>
---
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 2/4] mergetool--lib: Improve the help text in guess_merge_tool()

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:55

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 <redacted>
---
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 4/4] doc: Generate a list of valid merge tools

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:55

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 <redacted>
---
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 0/4] Auto-generate mergetool lists

From: Philip Oakley <hidden>
Date: 2016-06-15 22:55:56

From: "David Aguilar" <redacted>
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

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:56

On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley [off-list ref] wrote:
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 12:52 AM
quoted
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 3/4] mergetool--lib: Add functions for finding available tools

From: John Keeping <hidden>
Date: 2016-06-15 22:55:56

On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
quoted hunk
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 <redacted>
---
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 0/4] Auto-generate mergetool lists

From: Philip Oakley <hidden>
Date: 2016-06-15 22:55:56

From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 9:16 AM
On Mon, Jan 28, 2013 at 12:20 AM, Philip Oakley [off-list ref] 
wrote:
quoted
From: "David Aguilar" <redacted>
Sent: Monday, January 28, 2013 12:52 AM
quoted
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 

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

From: John Keeping <hidden>
Date: 2016-06-15 22:55:57

On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
quoted hunk
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 <redacted>
---
@@ -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 3/4] mergetool--lib: Add functions for finding available tools

From: John Keeping <hidden>
Date: 2016-06-15 22:55:57

On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
quoted hunk
--- 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?
quoted hunk
+	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 1/4] mergetool--lib: Simplify command expressions

From: David Aguilar <hidden>
Date: 2016-06-15 22:55:57

On Tue, Jan 29, 2013 at 11:22 AM, John Keeping [off-list ref] wrote:
On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
quoted
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 <redacted>
---
@@ -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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help