# [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE 6 messages from 2025-03-25 to 2025-03-29. Participants: Fernando Ramos, Fernando, D. Ben Knoble, Junio C Hamano. Thread: https://gitlist.dev/t/63194 ## Fernando Ramos, 2025-03-25 22:23 Subject: [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Message-ID: <20250325222311.400748-1-greenfoo@u92.eu> URL: https://gitlist.dev/e/20250325222311.400748-1-greenfoo%40u92.eu ``` 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, 2025-03-25 22:23 Subject: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Message-ID: <20250325222311.400748-2-greenfoo@u92.eu> URL: https://gitlist.dev/e/20250325222311.400748-2-greenfoo%40u92.eu In-Reply-To: <20250325222311.400748-1-greenfoo@u92.eu> ``` "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 Suggested-by: D. Ben Knoble Signed-off-by: Fernando Ramos --- 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 ``` ## Fernando Ramos, 2025-03-25 22:23 Subject: [PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as the target Message-ID: <20250325222311.400748-3-greenfoo@u92.eu> URL: https://gitlist.dev/e/20250325222311.400748-3-greenfoo%40u92.eu In-Reply-To: <20250325222311.400748-1-greenfoo@u92.eu> ``` 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 --- mergetools/vimdiff | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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, 2025-03-26 10:10 Subject: Re: [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Message-ID: <7a4d6f02-50a5-4b1b-9d19-9598e66b6f34@app.fastmail.com> URL: https://gitlist.dev/e/7a4d6f02-50a5-4b1b-9d19-9598e66b6f34%40app.fastmail.com In-Reply-To: <20250325222311.400748-1-greenfoo@u92.eu> ``` > 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. ``` ## D. Ben Knoble, 2025-03-29 00:23 Subject: Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Message-ID: URL: https://gitlist.dev/e/CALnO6CC9M3nBoA-D7rLW_68VkKm9eZ_K7CZn1Z-BiPJWxgNYHQ%40mail.gmail.com In-Reply-To: <20250325222311.400748-2-greenfoo@u92.eu> ``` On Tue, Mar 25, 2025 at 6:24 PM Fernando Ramos wrote: > > "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 > Suggested-by: D. Ben Knoble > Signed-off-by: Fernando Ramos > --- > 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, 2025-03-29 20:46 Subject: Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Message-ID: URL: https://gitlist.dev/e/xmqqa593d0p6.fsf%40gitster.g In-Reply-To: ``` "D. Ben Knoble" writes: >> ... >> 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. ```