Re: [PATCH 7/7] mergetool--lib: Improve show_tool_help() output
- From
John Keeping <john@keeping.me.uk>
- Date
- Jan 25, 2013, 20:46 UTC
- Message-ID
- <20130125204619.GC7498@serenity.lan>
- In-Reply-To
- <7vpq0t2f2t.fsf@alter.siamese.dyndns.org>
On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> John Keeping <john@keeping.me.uk> writes: > > > 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