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

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

On Fri, Jan 25, 2013 at 07:54:46PM +0000, John Keeping wrote:
> 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...
> 
> > 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.
> 
> > +		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.
> 
> > +		merge_tool_path=$(translate_merge_tool_path "$tool")

Actually, can we just change all of the above part of the loop to:

	test "$tool" = defaults && continue

	merge_tool_path=$(
		setup_tool "$tool" >/dev/null 2>&1 &&
		translate_merge_tool_path "$tool"
	) || continue

> >  		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
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html