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

From: John Keeping <john@keeping.me.uk>
Date: Jan 28, 2013, 19:37 UTC
Message-ID: <20130128193700.GB7498@serenity.lan>
In-Reply-To: <1359334346-5879-4-git-send-email-davvid@gmail.com>

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

Show 29 quoted lines

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

Show 13 quoted lines

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

Show 7 quoted lines

> +		fi
> +	done
> +}
> +
>  diff_mode() {
>  	test "$TOOL_MODE" = diff
>  }