Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output

From: John Keeping <john@keeping.me.uk>
Date: Jan 25, 2013, 19:54 UTC
Message-ID: <20130125195446.GA7498@serenity.lan>
In-Reply-To: <1359107034-14606-8-git-send-email-davvid@gmail.com>

On Fri, Jan 25, 2013 at 01:43:54AM -0800, David Aguilar wrote:

> Check the can_diff and can_merge functions before deciding whether to
> add the tool to the available/unavailable lists.  This makes --tool-help context-
> sensitive so that "git mergetool --tool-help" displays merge tools only
> and "git difftool --tool-help" displays diff tools only.

This log message is misleading - the existing code in list_merge_tool_candidates already filters the tools like this, so the change is more:

    mergetool--lib: don't use a hardcoded list for "--tool-help"

    Instead of using a list of tools in list_merge_tool_candidates, list
    the available scriptlets and query each of those to know whether it
    applies to diff mode and/or merge mode.

    guess_merge_tool still relies on list_merge_tool_candidates so we
    can't remove that function now.

The patch seems to do the right thing, although I have a couple of minor nits...

Show 26 quoted lines

> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db8218a..c547c59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -168,17 +168,33 @@ list_merge_tool_candidates () {
>  }
>  
>  show_tool_help () {
> -	list_merge_tool_candidates
>  	unavailable= available= LF='
>  '
> -	for i in $tools
> +
> +	scriptlets="$(git --exec-path)"/mergetools
> +	for i in "$scriptlets"/*
>  	do
> -		merge_tool_path=$(translate_merge_tool_path "$i")
> +		. "$scriptlets"/defaults
> +		. "$i"
> +
> +		tool="$(basename "$i")"

Quotes are unnecessary here.

Show 10 quoted lines

> +		if test "$tool" = "defaults"
> +		then
> +			continue
> +		elif merge_mode && ! can_merge
> +		then
> +			continue
> +		elif diff_mode && ! can_diff
> +		then
> +			continue
> +		fi

Would this be better as:

    test "$tool" = "defaults" && continue

    can_merge || ! merge_mode || continue
    can_diff || ! diff_mode || continue

or is that a bit too concise?

I'd prefer to see two separate if statements either way since the "test $tool = defaults" case is different from the "does it apply to the current mode?" case. The "$tool = defaults" case could even move to the top of the loop.

Show 10 quoted lines

> +		merge_tool_path=$(translate_merge_tool_path "$tool")
>  		if type "$merge_tool_path" >/dev/null 2>&1
>  		then
> -			available="$available$i$LF"
> +			available="$available$tool$LF"
>  		else
> -			unavailable="$unavailable$i$LF"
> +			unavailable="$unavailable$tool$LF"
>  		fi
>  	done