All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kipras Melnikovas <kipras@kipras.org>
To: git@vger.kernel.org
Cc: greenfoo@u92.eu, Kipras Melnikovas <kipras@kipras.org>
Subject: [PATCH v2] mergetools: vimdiff: use correct tool's name when reading mergetool config
Date: Thu, 15 Feb 2024 16:20:02 +0200	[thread overview]
Message-ID: <20240215142002.36870-1-kipras@kipras.org> (raw)
In-Reply-To: <20240215083917.98218-2-kipras@kipras.org>

The /mergetools/vimdiff script, which handles both vimdiff, nvimdiff
and gvimdiff mergetools (the latter 2 simply source the vimdiff script), has a
function merge_cmd() which read the layout variable from git config, and it
would always read the value of mergetool.**vimdiff**.layout, instead of the
mergetool being currently used (vimdiff or nvimdiff or gvimdiff).

It looks like in 7b5cf8be18 (vimdiff: add tool documentation, 2022-03-30),
we explained the current behavior in Documentation/config/mergetool.txt:

---
mergetool.vimdiff.layout::
	The vimdiff backend uses this variable to control how its split
	windows look like. Applies even if you are using Neovim (`nvim`) or
	gVim (`gvim`) as the merge tool. See BACKEND SPECIFIC HINTS section
---

which makes sense why it's explained this way - the vimdiff backend is used by
gvim and nvim. But the mergetool's configuration should be separate for each tool,
and indeed that's confirmed in same commit at Documentation/mergetools/vimdiff.txt:

---
Variants

Instead of `--tool=vimdiff`, you can also use one of these other variants:
  * `--tool=gvimdiff`, to open gVim instead of Vim.
  * `--tool=nvimdiff`, to open Neovim instead of Vim.

When using these variants, in order to specify a custom layout you will have to
set configuration variables `mergetool.gvimdiff.layout` and
`mergetool.nvimdiff.layout` instead of `mergetool.vimdiff.layout`
---

So it looks like we just forgot to update the 1 part of the vimdiff script
that read the config variable. Cheers.

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.

Signed-off-by: Kipras Melnikovas <kipras@kipras.org>
---
Range-diff against v1:
1:  197e42deef ! 1:  070280d95d mergetools: vimdiff: use correct tool's name when reading mergetool config
    @@ Metadata
      ## Commit message ##
         So it looks like we just forgot to update the 1 part of the vimdiff script
         that read the config variable. Cheers.
     
    +    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.
    +
         Signed-off-by: Kipras Melnikovas <kipras@kipras.org>
     
    @@ mergetools/vimdiff: diff_cmd_help () {
     -	case "$1" in
     +	layout=$(git config mergetool.$TOOL.layout)
     +
    ++	# backwards-compatibility:
    ++	if test -z "$layout"
    ++	then
    ++		layout=$(git config mergetool.vimdiff.layout)
    ++	fi
    ++
     +	case "$TOOL" in
      	*vimdiff)
      		if test -z "$layout"

 Documentation/config/mergetool.txt |  9 +++++----
 mergetools/vimdiff                 | 12 ++++++++++--
 2 files changed, 15 insertions(+), 6 deletions(-)

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
 ifndef::git-mergetool[]
 	in linkgit:git-mergetool[1].
 endif::[]
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)
+
+	# backwards-compatibility:
+	if test -z "$layout"
+	then
+		layout=$(git config mergetool.vimdiff.layout)
+	fi
+
+	case "$TOOL" in
 	*vimdiff)
 		if test -z "$layout"
 		then

base-commit: 4fc51f00ef18d2c0174ab2fd39d0ee473fd144bd
-- 
2.43.1


  reply	other threads:[~2024-02-15 14:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-15  8:39 [PATCH] mergetools: vimdiff: use correct tool's name when reading mergetool config Kipras Melnikovas
2024-02-15 14:20 ` Kipras Melnikovas [this message]
2024-02-15 18:42   ` [PATCH v2] " Junio C Hamano
2024-02-15 20:43   ` Fernando Ramos
2024-02-17  7:43   ` [PATCH v3] " Kipras Melnikovas
2024-02-17 16:27     ` [PATCH v4] " Kipras Melnikovas
2024-02-20  2:52       ` Junio C Hamano
2024-02-21  5:20         ` Kipras Melnikovas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240215142002.36870-1-kipras@kipras.org \
    --to=kipras@kipras.org \
    --cc=git@vger.kernel.org \
    --cc=greenfoo@u92.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.