Re: [PATCH v2] mergetools: vimdiff: use correct tool's name when reading mergetool config
From: Junio C Hamano <hidden>
Date: 2024-02-15 18:42:53
Kipras Melnikovas [off-list ref] 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 ...
quoted hunk
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?
quoted hunk
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.
+ + # 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.
*vimdiff) if test -z "$layout" then base-commit: 4fc51f00ef18d2c0174ab2fd39d0ee473fd144bd