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

From: David Aguilar <davvid@gmail.com>
Date: Apr 28, 2019, 23:56 UTC
Message-ID: <20190428235624.GA5304@gmail.com>
In-Reply-To: <c799e871e2a6a6a7fcca45aad71f5a0c406ba3d7.1556185345.git.liu.denton@gmail.com>

On Thu, Apr 25, 2019 at 02:54:41AM -0700, Denton Liu wrote:

Show 24 quoted lines

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

Show 14 quoted lines

>  	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