Re: [PATCH v2] mergetools: vimdiff: use correct tool's name when reading mergetool config

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2024, 18:42 UTC
Message-ID: <xmqq8r3lr2l2.fsf@gitster.g>
In-Reply-To: <20240215142002.36870-1-kipras@kipras.org>

Kipras Melnikovas <kipras@kipras.org> writes:

> Though, for backwards-compatibility, I've kept the mergetool.vimdiff
> fallback, so that people who unknowingly relied on it, won't have their
> setup broken now.

It is a good consideration, and should be documented ...

Show 17 quoted lines

> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 294f61efd1..8e3d321a57 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -45,10 +45,11 @@ mergetool.meld.useAutoMerge::
>  	value of `false` avoids using `--auto-merge` altogether, and is the
>  	default value.
>  
> -mergetool.vimdiff.layout::
> -	The vimdiff backend uses this variable to control how its split
> -	windows appear. Applies even if you are using Neovim (`nvim`) or
> -	gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
> +mergetool.{g,n,}vimdiff.layout::
> +	The vimdiff backend uses this variable to control how its split windows
> +	appear. Use `mergetool.vimdiff` for regular Vim, `mergetool.nvimdiff` for
> +	Neovim and `mergetool.gvimdiff` for gVim to configure the merge tool. See
> +	BACKEND SPECIFIC HINTS section

... perhaps before "See BACKEND SPECIFIC HINTS section."  E.g.

	When a variant of vimdiff (vim, Neovim, or gVim) is used as
	a mergetool backend, they use this variable to control how
	the split windows appear.
	The variable `mergetool.<variant>.layout` (where <variant>
	is one of `vimdiff`, `nvimdiff`, or `gvimdiff`, depending on
	what you are using) is consulted first, and if it is missing,
	`mergetool.vimdiff.layout` is used as a fallback.  See
	BACKEND SPECIFIC HINTS section.

or something?

Show 13 quoted lines

> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index 06937acbf5..0e3058868a 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -371,9 +371,17 @@ diff_cmd_help () {
>  
>  
>  merge_cmd () {
> -	layout=$(git config mergetool.vimdiff.layout)
> +	TOOL=$1
>  
> -	case "$1" in
> +	layout=$(git config mergetool.$TOOL.layout)

The callers of merge_cmd are careful to do

	merge_cmd "$1"

so it would be a good hygiene to also quote $TOOL here, i.e.

	layout=$(git config "mergetool.$TOOL.layout")

It might not matter if the caller of run_merge_cmd (which calls merge_cmd) eventually chooses from a known set of strings hardcoded in mergetools--lib.sh, but it is much easier to show that you are doing the right thing without relying on such a detail of what happens far in the code to quote what you get from the caller appropriately.

Show 8 quoted lines

> +
> +	# backwards-compatibility:
> +	if test -z "$layout"
> +	then
> +		layout=$(git config mergetool.vimdiff.layout)
> +	fi
> +
> +	case "$TOOL" in

This one is quoted properly (and TOOL=$1 at the beginning does not require quoting). The "git config" call above is the only one that needs to be fixed.

Thanks.

Show 5 quoted lines

>  	*vimdiff)
>  		if test -z "$layout"
>  		then
>
> base-commit: 4fc51f00ef18d2c0174ab2fd39d0ee473fd144bd