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:
> 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
>  }