{"thread":{"id":"63194","subject":"[PATCH 0/2] Fix mergetool.vimdiff.layout when \"@\" is used on REMOTE","startedAt":"2025-03-25T22:24:12Z","lastAt":"2025-03-29T20:46:48Z","messageCount":6,"participants":["Fernando Ramos","Fernando","D. Ben Knoble","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"515041","messageId":"20250325222311.400748-1-greenfoo@u92.eu","threadId":"63194","inReplyTo":null,"subject":"[PATCH 0/2] Fix mergetool.vimdiff.layout when \"@\" is used on REMOTE","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2025-03-25T22:23:09Z","receivedAt":"2025-03-25T22:24:12Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"The \"mergetool.vimdiff.layout\" config option accepts a \"@\" marker on one of the\npossible targets (\"LOCAL\", \"BASE\", \"REMOTE\" or \"MERGED\") to specify which window\n(or tab or buffer) will be used to overwrite the file which conflicts we are\ntrying to solve.\n\nThe problem is that it never really worked when used with \"MERGED\" (for all the\nothers it worked fine).\n\nIn this patch series we are fixing that and adding some unit tests to make sure\nwe never break this again in the future.\n\nFernando Ramos (2):\n  mergetools: vimdiff: fix layout where REMOTE is the target\n  mergetools: vimdiff: add tests for layout with REMOTE as the target\n\n mergetools/vimdiff | 14 +++++++++++++-\n 1 file changed, 13 insertions(+), 1 deletion(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.49.0\n\n"},{"id":"515042","messageId":"20250325222311.400748-2-greenfoo@u92.eu","threadId":"63194","inReplyTo":"20250325222311.400748-1-greenfoo@u92.eu","subject":"[PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2025-03-25T22:23:10Z","receivedAt":"2025-03-25T22:24:14Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"\"mergetool.vimdiff.layout\" is used to define the vim layout (ie. how\nwindows, tabs and buffers are physically organized) when resolving\nconflicts.\n\nFor example, if we set it to this:\n\n    \"(LOCAL,BASE,REMOTE)/MERGED\"\n\n...vim will open and show this layout:\n\n    ------------------------------------------\n    |             |           |              |\n    |   LOCAL     |   BASE    |   REMOTE     |\n    |             |           |              |\n    ------------------------------------------\n    |                                        |\n    |                MERGED                  |\n    |                                        |\n    ------------------------------------------\n\nBy default, whatever ends up been written to the \"MERGED\" window will\nbecome the file which conflict we are resolving.\n\nHowever, it is possible to use the \"@\" symbol to specify a different\none.  For example, if we use this slightly different version of the\npreviously used string:\n\n    \"(LOCAL,BASE,@REMOTE)/MERGED\"\n\n...then the user should proceed to edit the contents of the top right\nwindow (instead of the bottom window) as *that* is what will become the\nconflicts free file once vim is closed.\n\nBefore this commit, the \"@\" marker worked for all targets *except* for\n\"REMOTE\". In other words, these worked as expected:\n\n    \"(@LOCAL,BASE,REMOTE)/MERGED\"\n    \"(LOCAL,@BASE,REMOTE)/MERGED\"\n    \"(LOCAL,BASE,REMOTE)/@MERGED\"\n\n...but this didn't:\n\n    \"(LOCAL,BASE,@REMOTE)/MERGED\"\n\nThis commit fixes that.\n\nReported-by: kawarimidoll <kawarimidoll+git@gmail.com>\nSuggested-by: D. Ben Knoble <ben.knoble@gmail.com>\nSigned-off-by: Fernando Ramos <greenfoo@u92.eu>\n---\n mergetools/vimdiff | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex ffc9be86c8..0e3785d230 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -305,6 +305,9 @@ gen_cmd () {\n \telif echo \"$LAYOUT\" | grep @BASE >/dev/null\n \tthen\n \t\tFINAL_TARGET=\"BASE\"\n+\telif echo \"$LAYOUT\" | grep @REMOTE >/dev/null\n+\tthen\n+\t\tFINAL_TARGET=\"REMOTE\"\n \telse\n \t\tFINAL_TARGET=\"MERGED\"\n \tfi\n-- \n2.49.0\n\n"},{"id":"515043","messageId":"20250325222311.400748-3-greenfoo@u92.eu","threadId":"63194","inReplyTo":"20250325222311.400748-1-greenfoo@u92.eu","subject":"[PATCH 2/2] mergetools: vimdiff: add tests for layout with REMOTE as the target","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2025-03-25T22:23:11Z","receivedAt":"2025-03-25T22:24:16Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"Add some tests to make sure that now \"REMOTE\" can be used as a target\n(ie. can be used together with the \"@\" marker) inside\n\"mergetool.vimdiff.layout\"\n\nSigned-off-by: Fernando Ramos <greenfoo@u92.eu>\n---\n mergetools/vimdiff | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 0e3785d230..78710858e8 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -532,7 +532,7 @@ run_unit_tests () {\n \t# Function to make sure that we don't break anything when modifying this\n \t# script.\n \n-\tNUMBER_OF_TEST_CASES=16\n+\tNUMBER_OF_TEST_CASES=19\n \n \tTEST_CASE_01=\"(LOCAL,BASE,REMOTE)/MERGED\"   # default behaviour\n \tTEST_CASE_02=\"@LOCAL,REMOTE\"                # when using vimdiff1\n@@ -550,6 +550,9 @@ run_unit_tests () {\n \tTEST_CASE_14=\"BASE,REMOTE+BASE,LOCAL\"\n \tTEST_CASE_15=\"  ((  (LOCAL , BASE , REMOTE) / MERGED))   +(BASE)   , LOCAL+ BASE , REMOTE+ (((LOCAL / BASE / REMOTE)) ,    MERGED   )  \"\n \tTEST_CASE_16=\"LOCAL,BASE,REMOTE / MERGED + BASE,LOCAL + BASE,REMOTE + (LOCAL / BASE / REMOTE),MERGED\"\n+\tTEST_CASE_17=\"(LOCAL,@BASE,REMOTE)/MERGED\"\n+\tTEST_CASE_18=\"LOCAL,@REMOTE\"\n+\tTEST_CASE_19=\"@REMOTE\"\n \n \tEXPECTED_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\\\"\"\n \tEXPECTED_CMD_02=\"-c \\\"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'tabdo windo diffthis' | tabfirst\\\"\"\n@@ -567,6 +570,9 @@ run_unit_tests () {\n \tEXPECTED_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\\\"\"\n \tEXPECTED_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\\\"\"\n \tEXPECTED_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\\\"\"\n+\tEXPECTED_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\\\"\"\n+\tEXPECTED_CMD_18=\"-c \\\"set hidden diffopt-=hiddenoff | echo | leftabove vertical split | 1b | wincmd l | 3b | execute 'tabdo windo diffthis' | tabfirst\\\"\"\n+\tEXPECTED_CMD_19=\"-c \\\"set hidden diffopt-=hiddenoff | echo | silent execute 'bufdo diffthis' | 3b | execute 'tabdo windo diffthis' | tabfirst\\\"\"\n \n \tEXPECTED_TARGET_01=\"MERGED\"\n \tEXPECTED_TARGET_02=\"LOCAL\"\n@@ -584,6 +590,9 @@ run_unit_tests () {\n \tEXPECTED_TARGET_14=\"MERGED\"\n \tEXPECTED_TARGET_15=\"MERGED\"\n \tEXPECTED_TARGET_16=\"MERGED\"\n+\tEXPECTED_TARGET_17=\"BASE\"\n+\tEXPECTED_TARGET_18=\"REMOTE\"\n+\tEXPECTED_TARGET_19=\"REMOTE\"\n \n \tat_least_one_ko=\"false\"\n \n-- \n2.49.0\n\n"},{"id":"515089","messageId":"7a4d6f02-50a5-4b1b-9d19-9598e66b6f34@app.fastmail.com","threadId":"63194","inReplyTo":"20250325222311.400748-1-greenfoo@u92.eu","subject":"Re: [PATCH 0/2] Fix mergetool.vimdiff.layout when \"@\" is used on REMOTE","fromName":"Fernando","fromEmail":"greenfoo@u92.eu","sentAt":"2025-03-26T10:10:24Z","receivedAt":"2025-03-26T10:10:46Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"\n> The problem is that it never really worked when used with \"MERGED\" (for all the\n> others it worked fine).\n\nSorry, I meant \"REMOTE\" instead of \"MERGED\".\n\nThis is a typo in the cover letter.\nThe rest of the patch series is OK. \n"},{"id":"515256","messageId":"CALnO6CC9M3nBoA-D7rLW_68VkKm9eZ_K7CZn1Z-BiPJWxgNYHQ@mail.gmail.com","threadId":"63194","inReplyTo":"20250325222311.400748-2-greenfoo@u92.eu","subject":"Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-03-29T00:23:37Z","receivedAt":"2025-03-29T00:23:51Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Tue, Mar 25, 2025 at 6:24 PM Fernando Ramos <greenfoo@u92.eu> wrote:\n>\n> \"mergetool.vimdiff.layout\" is used to define the vim layout (ie. how\n> windows, tabs and buffers are physically organized) when resolving\n> conflicts.\n>\n> For example, if we set it to this:\n>\n>     \"(LOCAL,BASE,REMOTE)/MERGED\"\n>\n> ...vim will open and show this layout:\n>\n>     ------------------------------------------\n>     |             |           |              |\n>     |   LOCAL     |   BASE    |   REMOTE     |\n>     |             |           |              |\n>     ------------------------------------------\n>     |                                        |\n>     |                MERGED                  |\n>     |                                        |\n>     ------------------------------------------\n>\n> By default, whatever ends up been written to the \"MERGED\" window will\n> become the file which conflict we are resolving.\n>\n> However, it is possible to use the \"@\" symbol to specify a different\n> one.  For example, if we use this slightly different version of the\n> previously used string:\n>\n>     \"(LOCAL,BASE,@REMOTE)/MERGED\"\n>\n> ...then the user should proceed to edit the contents of the top right\n> window (instead of the bottom window) as *that* is what will become the\n> conflicts free file once vim is closed.\n>\n> Before this commit, the \"@\" marker worked for all targets *except* for\n> \"REMOTE\". In other words, these worked as expected:\n>\n>     \"(@LOCAL,BASE,REMOTE)/MERGED\"\n>     \"(LOCAL,@BASE,REMOTE)/MERGED\"\n>     \"(LOCAL,BASE,REMOTE)/@MERGED\"\n>\n> ...but this didn't:\n>\n>     \"(LOCAL,BASE,@REMOTE)/MERGED\"\n>\n> This commit fixes that.\n>\n> Reported-by: kawarimidoll <kawarimidoll+git@gmail.com>\n> Suggested-by: D. Ben Knoble <ben.knoble@gmail.com>\n> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>\n> ---\n>  mergetools/vimdiff | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> index ffc9be86c8..0e3785d230 100644\n> --- a/mergetools/vimdiff\n> +++ b/mergetools/vimdiff\n> @@ -305,6 +305,9 @@ gen_cmd () {\n>         elif echo \"$LAYOUT\" | grep @BASE >/dev/null\n>         then\n>                 FINAL_TARGET=\"BASE\"\n> +       elif echo \"$LAYOUT\" | grep @REMOTE >/dev/null\n> +       then\n> +               FINAL_TARGET=\"REMOTE\"\n>         else\n>                 FINAL_TARGET=\"MERGED\"\n>         fi\n> --\n> 2.49.0\n>\n\nThis looks pretty obviously correct to me, thanks!\n"},{"id":"515285","messageId":"xmqqa593d0p6.fsf@gitster.g","threadId":"63194","inReplyTo":"CALnO6CC9M3nBoA-D7rLW_68VkKm9eZ_K7CZn1Z-BiPJWxgNYHQ@mail.gmail.com","subject":"Re: [PATCH 1/2] mergetools: vimdiff: fix layout where REMOTE is the target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-29T20:46:45Z","receivedAt":"2025-03-29T20:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" <ben.knoble+github@gmail.com> writes:\n\n>> ...\n>>                 FINAL_TARGET=\"BASE\"\n>> +       elif echo \"$LAYOUT\" | grep @REMOTE >/dev/null\n>> +       then\n>> +               FINAL_TARGET=\"REMOTE\"\n>>         else\n>>                 FINAL_TARGET=\"MERGED\"\n>>         fi\n>> --\n>> 2.49.0\n>>\n>\n> This looks pretty obviously correct to me, thanks!\n\nYeah, thanks, all.  Will queue.\n"}]}