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

On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:

John Keeping [off-list ref] writes:

quoted

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

Meaning "setup_tool ought to know which mode we are in and should
fail if we are in merge mode and it does not support merging"?  That
line of reasoning makes tons of sense to me, compared to this script
implementing that logic for these scriptlets.

Yes, that's part of what setup_tool does.  It actually calls "exit" if
the "mode? && can_mode" test fails, which is why we need to call it in
the subshell.

I think this would get even better if we add a preparatory patch like
this, so we can just call setup_tool and then set merge_tool_path:

-- >8 --

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 888ae3e..4644cbf 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh

@@ -67,11 +67,11 @@ setup_tool () {
 	if merge_mode && ! can_merge
 	then
 		echo "error: '$tool' can not be used to resolve merges" >&2
-		exit 1
+		return 1
 	elif diff_mode && ! can_diff
 	then
 		echo "error: '$tool' can only be used to resolve merges" >&2
-		exit 1
+		return 1
 	fi
 	return 0
 }

@@ -100,7 +100,7 @@ run_merge_tool () {
 	status=0
 
 	# Bring tool-specific functions into scope
-	setup_tool "$1"
+	setup_tool "$1" || return
 
 	if merge_mode
 	then

-- 8< --
 

How/when does translate_merge_tool_path fail?

It doesn't - the "|| continue" is to catch errors from setup_tool.


John