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

git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE
@ 2025-03-25 22:23 Fernando Ramos
  2025-03-25 22:23 ` [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Fernando Ramos
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Fernando Ramos @ 2025-03-25 22:23 UTC (permalink / raw)
  To: git; +Cc: D . Ben Knoble, Fernando Ramos, Junio C Hamano, kawarimidoll

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target
  2025-03-25 22:23 [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando Ramos
@ 2025-03-25 22:23 ` Fernando Ramos
  2025-03-29  0:23   ` D. Ben Knoble
  2025-03-25 22:23 ` [PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as " Fernando Ramos
  2025-03-26 10:10 ` [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando
  2 siblings, 1 reply; 6+ messages in thread
From: Fernando Ramos @ 2025-03-25 22:23 UTC (permalink / raw)
  To: git; +Cc: D . Ben Knoble, Fernando Ramos, Junio C Hamano, kawarimidoll

"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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target
  2025-03-25 22:23 ` [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Fernando Ramos
@ 2025-03-29  0:23   ` D. Ben Knoble
  2025-03-29 20:46     ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: D. Ben Knoble @ 2025-03-29  0:23 UTC (permalink / raw)
  To: Fernando Ramos; +Cc: git, Junio C Hamano, kawarimidoll

On Tue, Mar 25, 2025 at 6:24 PM Fernando Ramos <greenfoo@u92.eu> 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 <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!

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target
  2025-03-29  0:23   ` D. Ben Knoble
@ 2025-03-29 20:46     ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2025-03-29 20:46 UTC (permalink / raw)
  To: D. Ben Knoble; +Cc: Fernando Ramos, git, kawarimidoll

"D. Ben Knoble" <ben.knoble+github@gmail.com> 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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as the target
  2025-03-25 22:23 [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando Ramos
  2025-03-25 22:23 ` [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Fernando Ramos
@ 2025-03-25 22:23 ` Fernando Ramos
  2025-03-26 10:10 ` [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando
  2 siblings, 0 replies; 6+ messages in thread
From: Fernando Ramos @ 2025-03-25 22:23 UTC (permalink / raw)
  To: git; +Cc: D . Ben Knoble, Fernando Ramos, Junio C Hamano, kawarimidoll

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(-)

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE
  2025-03-25 22:23 [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando Ramos
  2025-03-25 22:23 ` [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Fernando Ramos
  2025-03-25 22:23 ` [PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as " Fernando Ramos
@ 2025-03-26 10:10 ` Fernando
  2 siblings, 0 replies; 6+ messages in thread
From: Fernando @ 2025-03-26 10:10 UTC (permalink / raw)
  To: git; +Cc: D . Ben Knoble, Junio C Hamano, kawarimidoll


> 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. 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-03-29 20:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-25 22:23 [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando Ramos
2025-03-25 22:23 ` [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target Fernando Ramos
2025-03-29  0:23   ` D. Ben Knoble
2025-03-29 20:46     ` Junio C Hamano
2025-03-25 22:23 ` [PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as " Fernando Ramos
2025-03-26 10:10 ` [PATCH 0/2] Fix mergetool.vimdiff.layout when "@" is used on REMOTE Fernando

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).