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