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

John Keeping [off-list ref] writes:

On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:

quoted

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

quoted

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