threads / patch / 63194

patch, 2 partsFix mergetool.vimdiff.layout when "@" is used on REMOTE

Subject: [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE

## tl;dr

6 messages between Mar 25, 2025 and Mar 29, 2025. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Fernando Ramos· Mar 25, 2025, 22:23 UTC · lore

The "mergetool.vimdiff.layout" config option accepts a "@" marker on one of the possible targets ("LOCAL", "BASE", "REMOTE" or "MERGED") to specify which window (or tab or buffer) will be used to overwrite the file which conflicts we are trying to solve.

The problem is that it never really worked when used with "MERGED" (for all the others it worked fine).

In this patch series we are fixing that and adding some unit tests to make sure we never break this again in the future.

Fernando Ramos (2):
  mergetools: vimdiff: fix layout where REMOTE is the target
  mergetools: vimdiff: add tests for layout with REMOTE as the target

 mergetools/vimdiff | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e

-- 
2.49.0

Fernando Ramos· Mar 25, 2025, 22:23 UTC · re: Fernando Ramos · lore

[PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target

"mergetool.vimdiff.layout" is used to define the vim layout (ie. how windows, tabs and buffers are physically organized) when resolving conflicts.

For example, if we set it to this:

    "(LOCAL,BASE,REMOTE)/MERGED"

...vim will open and show this layout:

    ------------------------------------------
    |             |           |              |
    |   LOCAL     |   BASE    |   REMOTE     |
    |             |           |              |
    ------------------------------------------
    |                                        |
    |                MERGED                  |
    |                                        |
    ------------------------------------------

By default, whatever ends up been written to the "MERGED" window will become the file which conflict we are resolving.

However, it is possible to use the "@" symbol to specify a different one. For example, if we use this slightly different version of the previously used string:

    "(LOCAL,BASE,@REMOTE)/MERGED"

...then the user should proceed to edit the contents of the top right window (instead of the bottom window) as *that* is what will become the conflicts free file once vim is closed.

Before this commit, the "@" marker worked for all targets *except* for "REMOTE". In other words, these worked as expected:

    "(@LOCAL,BASE,REMOTE)/MERGED"
    "(LOCAL,@BASE,REMOTE)/MERGED"
    "(LOCAL,BASE,REMOTE)/@MERGED"

...but this didn't:

    "(LOCAL,BASE,@REMOTE)/MERGED"

This commit fixes that.

Reported-by: kawarimidoll <kawarimidoll+git@gmail.com>
Suggested-by: D. Ben Knoble <ben.knoble@gmail.com>
Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 3 +++
 1 file changed, 3 insertions(+)

Show changes to mergetools/vimdiff +3 −0

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index ffc9be86c8..0e3785d230 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -305,6 +305,9 @@ gen_cmd () {
 	elif echo "$LAYOUT" | grep @BASE >/dev/null
 	then
 		FINAL_TARGET="BASE"
+	elif echo "$LAYOUT" | grep @REMOTE >/dev/null
+	then
+		FINAL_TARGET="REMOTE"
 	else
 		FINAL_TARGET="MERGED"
 	fi

-- 
2.49.0

D. Ben Knoble· Mar 29, 2025, 00:23 UTC · re: Fernando Ramos · lore

Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target

On Tue, Mar 25, 2025 at 6:24 PM Fernando Ramos <greenfoo@u92.eu> wrote:

Show 71 quoted lines

>
> "mergetool.vimdiff.layout" is used to define the vim layout (ie. how
> windows, tabs and buffers are physically organized) when resolving
> conflicts.
>
> For example, if we set it to this:
>
>     "(LOCAL,BASE,REMOTE)/MERGED"
>
> ...vim will open and show this layout:
>
>     ------------------------------------------
>     |             |           |              |
>     |   LOCAL     |   BASE    |   REMOTE     |
>     |             |           |              |
>     ------------------------------------------
>     |                                        |
>     |                MERGED                  |
>     |                                        |
>     ------------------------------------------
>
> By default, whatever ends up been written to the "MERGED" window will
> become the file which conflict we are resolving.
>
> However, it is possible to use the "@" symbol to specify a different
> one.  For example, if we use this slightly different version of the
> previously used string:
>
>     "(LOCAL,BASE,@REMOTE)/MERGED"
>
> ...then the user should proceed to edit the contents of the top right
> window (instead of the bottom window) as *that* is what will become the
> conflicts free file once vim is closed.
>
> Before this commit, the "@" marker worked for all targets *except* for
> "REMOTE". In other words, these worked as expected:
>
>     "(@LOCAL,BASE,REMOTE)/MERGED"
>     "(LOCAL,@BASE,REMOTE)/MERGED"
>     "(LOCAL,BASE,REMOTE)/@MERGED"
>
> ...but this didn't:
>
>     "(LOCAL,BASE,@REMOTE)/MERGED"
>
> This commit fixes that.
>
> Reported-by: kawarimidoll <kawarimidoll+git@gmail.com>
> Suggested-by: D. Ben Knoble <ben.knoble@gmail.com>
> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
> ---
>  mergetools/vimdiff | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mergetools/vimdiff b/mergetools/vimdiff
> index ffc9be86c8..0e3785d230 100644
> --- a/mergetools/vimdiff
> +++ b/mergetools/vimdiff
> @@ -305,6 +305,9 @@ gen_cmd () {
>         elif echo "$LAYOUT" | grep @BASE >/dev/null
>         then
>                 FINAL_TARGET="BASE"
> +       elif echo "$LAYOUT" | grep @REMOTE >/dev/null
> +       then
> +               FINAL_TARGET="REMOTE"
>         else
>                 FINAL_TARGET="MERGED"
>         fi
> --
> 2.49.0
>

This looks pretty obviously correct to me, thanks!

Junio C Hamano· Mar 29, 2025, 20:46 UTC · re: D. Ben Knoble · lore

Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

Show 13 quoted lines

>> ...
>>                 FINAL_TARGET="BASE"
>> +       elif echo "$LAYOUT" | grep @REMOTE >/dev/null
>> +       then
>> +               FINAL_TARGET="REMOTE"
>>         else
>>                 FINAL_TARGET="MERGED"
>>         fi
>> --
>> 2.49.0
>>
>
> This looks pretty obviously correct to me, thanks!

Yeah, thanks, all.  Will queue.

Fernando Ramos· Mar 25, 2025, 22:23 UTC · re: Fernando Ramos · lore

[PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as the target

Add some tests to make sure that now "REMOTE" can be used as a target (ie. can be used together with the "@" marker) inside "mergetool.vimdiff.layout"

Signed-off-by: Fernando Ramos <greenfoo@u92.eu>
---
 mergetools/vimdiff | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Show changes to mergetools/vimdiff +10 −1

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 0e3785d230..78710858e8 100644
--- a/mergetools/vimdiff
+++ b/mergetools/vimdiff
@@ -532,7 +532,7 @@ run_unit_tests () {
 	# Function to make sure that we don't break anything when modifying this
 	# script.
 
-	NUMBER_OF_TEST_CASES=16
+	NUMBER_OF_TEST_CASES=19
 
 	TEST_CASE_01="(LOCAL,BASE,REMOTE)/MERGED"   # default behaviour
 	TEST_CASE_02="@LOCAL,REMOTE"                # when using vimdiff1
@@ -550,6 +550,9 @@ run_unit_tests () {
 	TEST_CASE_14="BASE,REMOTE+BASE,LOCAL"
 	TEST_CASE_15="  ((  (LOCAL , BASE , REMOTE) / MERGED))   +(BASE)   , LOCAL+ BASE , REMOTE+ (((LOCAL / BASE / REMOTE)) ,    MERGED   )  "
 	TEST_CASE_16="LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE + (LOCAL / BASE / REMOTE),MERGED"
+	TEST_CASE_17="(LOCAL,@BASE,REMOTE)/MERGED"
+	TEST_CASE_18="LOCAL,@REMOTE"
+	TEST_CASE_19="@REMOTE"
 
 	EXPECTED_CMD_01="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'tabdo windo diffthis' | tabfirst\""
 	EXPECTED_CMD_02="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'tabdo windo diffthis' | tabfirst\""
@@ -567,6 +570,9 @@ run_unit_tests () {
 	EXPECTED_CMD_14="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 2b | wincmd l | 3b | tabnew | leftabove vertical split | 2b | wincmd l | 1b | execute 'tabdo windo diffthis' | tabfirst\""
 	EXPECTED_CMD_15="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnew | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'tabdo windo diffthis' | tabfirst\""
 	EXPECTED_CMD_16="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnew | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | execute 'tabdo windo diffthis' | tabfirst\""
+	EXPECTED_CMD_17="-c \"set hidden diffopt-=hiddenoff | echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | execute 'tabdo windo diffthis' | tabfirst\""
+	EXPECTED_CMD_18="-c \"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'tabdo windo diffthis' | tabfirst\""
+	EXPECTED_CMD_19="-c \"set hidden diffopt-=hiddenoff | echo | silent execute 'bufdo diffthis' | 3b | execute 'tabdo windo diffthis' | tabfirst\""
 
 	EXPECTED_TARGET_01="MERGED"
 	EXPECTED_TARGET_02="LOCAL"
@@ -584,6 +590,9 @@ run_unit_tests () {
 	EXPECTED_TARGET_14="MERGED"
 	EXPECTED_TARGET_15="MERGED"
 	EXPECTED_TARGET_16="MERGED"
+	EXPECTED_TARGET_17="BASE"
+	EXPECTED_TARGET_18="REMOTE"
+	EXPECTED_TARGET_19="REMOTE"
 
 	at_least_one_ko="false"

-- 
2.49.0

Fernando· Mar 26, 2025, 10:10 UTC · re: Fernando Ramos · lore

Re: [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE

> The problem is that it never really worked when used with "MERGED" (for all the
> others it worked fine).

Sorry, I meant "REMOTE" instead of "MERGED".

This is a typo in the cover letter. The rest of the patch series is OK.