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

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:

quoted

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

quoted

Signed-off-by: David Aguilar <redacted>
---
 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.

quoted

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

quoted

+		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

quoted

 		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