Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 24, 2018, 05:10 UTC
Message-ID: <xmqqy3aond55.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181024022500.GA29011@archbookpro.localdomain>

Denton Liu <liu.denton@gmail.com> writes:

> Subject: Re: [PATCH 1/2] mergetool: Accept -g/--[no-]gui as arguments

Other people may point it out, but s/Accept/accept/.

Show 16 quoted lines

> In line with how difftool accepts a -g/--[no-]gui option, make mergetool
> accept the same option in order to use the `merge.guitool` variable to
> find the default mergetool instead of `merge.tool`.
> ...
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 9a8b97a2a..e317ae003 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -350,17 +350,23 @@ guess_merge_tool () {
>  }
>  
>  get_configured_merge_tool () {
> -	# Diff mode first tries diff.tool and falls back to merge.tool.
> -	# Merge mode only checks merge.tool
> +	# If first argument is true, find the guitool instead
> +	if [ "$1" = true ]

Don't use [] in our shell script (see Documentation/CodingGuidelines); say

	if "$1" = true

instead.

Show 14 quoted lines

> +	then
> +		gui_prefix=gui
> +	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
>  	then
> -		merge_tool=$(git config diff.tool || git config merge.tool)
> +		merge_tool=$(git config diff.${gui_prefix}tool || git config merge.${gui_prefix}tool)
>  	else
> -		merge_tool=$(git config merge.tool)
> +		merge_tool=$(git config merge.${gui_prefix}tool)
>  	fi

In mode_ok shell function, we seem to be prepared to deal with a case where neither diff_mode nor merge_mode is true. Should this codepath also be prepared to?

This is not a comment to point out an issue with this patch. It is a genuine question on the code after (or before for that matter) this patch is applied.

Thanks.