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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2013, 23:32 UTC
Message-ID: <7v8v7ecicm.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1359321886-80523-4-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 48 quoted lines

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

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

	printf "%s" "$available"

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

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

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

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

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