Re: [PATCH v4 4/6] mergetool: fallback to tool when guitool unavailable

[David Aguilar <davvid@gmail.com>]

On Thu, Apr 25, 2019 at 02:54:41AM -0700, Denton Liu wrote:
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 68ff26a0f7..c4b16c5e59 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,20 +350,34 @@ guess_merge_tool () {
>  }
>  
>  get_configured_merge_tool () {
> -	# If first argument is true, find the guitool instead
> -	if test "$1" = true
> +	is_gui="$1"
> +	sections="merge"
> +	keys="tool"
> +
> +	if diff_mode
>  	then
> -		gui_prefix=gui
> +		sections="diff $sections"
>  	fi
>  
> -	# Diff mode first tries diff.(gui)tool and falls back to merge.(gui)tool.
> -	# Merge mode only checks merge.(gui)tool
> -	if diff_mode
> +	if "$is_gui" = true

This line looks suspect.  How about,

	if test "$is_gui" = true

instead?  This expression could also be lifted out to an "is_gui"
helper function.


>  	then
> -		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
> -	else
> -		merge_tool=$(git config merge.${gui_prefix}tool)
> +		keys="guitool $keys"
>  	fi
> +
> +	merge_tool=$(
> +		IFS=' '
> +		for key in $keys
> +		do
> +			for section in $sections
> +			do
> +				if selected=$(git config $section.$key)

Would it be simpler to split this conditional into two lines?

	selected=$(git config ...)
	if test -n "$selected"
	then
		...
	fi

Yes, it stops looking at the exit code, but it instead focuses on the
result, which is slightly more bulletproof against a funky user
configuration.


Regarding the two loops above, what would it look like if we
unrolled the logic and just spelled out the keys up front that it's a
little easier to follow?

I agree it is nicer from an implementation sense to use loops,
but we really shouldn't be planning to extend to more permutations in
the future beyond the diff/merge duality, so being explicit and spelling
out each config lookup permutation is simpler to understand since we
only have 4 states.  We should be discouraged from adding any more ;-)

Something like,

	keys=
	if merge_mode
	then
		if gui_mode  # probably worth adding this function
		then
			keys="merge.guitool merge.tool"
		else
			keys="merge.tool"
		fi
	else
		if gui_mode
		then
			keys="diff.guitool merge.guitool diff.tool merge.tool"
		else
			keys="diff.tool merge.tool"
		fi
	fi

.. and then just have a single loop over $keys.
-- 
David