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

[John Keeping <john@keeping.me.uk>]

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