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

[Junio C Hamano <gitster@pobox.com>]

John Keeping <john@keeping.me.uk> writes:

> On Fri, Jan 25, 2013 at 12:16:42PM -0800, Junio C Hamano wrote:
>> 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.

Ugh.