| Message ID | 20240215142002.36870-1-kipras@kipras.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] mergetools: vimdiff: use correct tool's name when reading mergetool config | expand |
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 ... > 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? > 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
On 24/02/15 04:20PM, Kipras Melnikovas wrote: > 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). You are 100% right. I completely overlooked this detail in the original implementation. Thanks for the fix!
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