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")
> 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