{"thread":{"id":"58123","subject":"[PATCH] vimdiff: make layout engine more robust against user vim settings","startedAt":"2022-07-08T18:10:38Z","lastAt":"2022-07-08T22:37:58Z","messageCount":4,"participants":["Fernando Ramos","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458655","messageId":"20220708181024.45839-1-greenfoo@u92.eu","threadId":"58123","inReplyTo":null,"subject":"[PATCH] vimdiff: make layout engine more robust against user vim settings","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-08T18:10:24Z","receivedAt":"2022-07-08T18:10:38Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"'vim' has two configuration options ('splitbelow' and 'splitright') that\nchange the way the 'split' command behaves. When they are set, the\ncommands that the layout engine generates no longer work as expected.\n\nIn order to fix this we can append special keyword 'letfabove' to each\n'split' and 'vertical split' subcommand found inside the command string\ngenerated by the layout engine.\n\nThis works because whatever comes after 'leftabove' will temporally\nignore settings 'splitbelow' and 'splitright'.\n\nReported-by: Matthew Klein <mklein994@gmail.com>\nSigned-off-by: Fernando Ramos <greenfoo@u92.eu>\n---\n mergetools/vimdiff | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 461a89b6f9..b045b10fd7 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -228,14 +228,14 @@ gen_cmd_aux () {\n \n \telif ! test -z \"$index_horizontal_split\"\n \tthen\n-\t\tbefore=\"split\"\n+\t\tbefore=\"leftabove split\"\n \t\tafter=\"wincmd j\"\n \t\tindex=$index_horizontal_split\n \t\tterminate=\"true\"\n \n \telif ! test -z \"$index_vertical_split\"\n \tthen\n-\t\tbefore=\"vertical split\"\n+\t\tbefore=\"leftabove vertical split\"\n \t\tafter=\"wincmd l\"\n \t\tindex=$index_vertical_split\n \t\tterminate=\"true\"\n@@ -310,7 +310,7 @@ gen_cmd () {\n \t#\n \t#     gen_cmd \"@LOCAL , REMOTE\"\n \t#     |\n-\t#     `-> FINAL_CMD    == \"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\t#     `-> FINAL_CMD    == \"-c \\\"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n \t#         FINAL_TARGET == \"LOCAL\"\n \n \tLAYOUT=$1\n@@ -555,22 +555,22 @@ run_unit_tests () {\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 \n-\tEXPECTED_CMD_01=\"-c \\\"echo | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_02=\"-c \\\"echo | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_03=\"-c \\\"echo | vertical split | 1b | wincmd l | vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_01=\"-c \\\"echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_02=\"-c \\\"echo | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_03=\"-c \\\"echo | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 4b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n \tEXPECTED_CMD_04=\"-c \\\"echo | 4b | bufdo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_05=\"-c \\\"echo | split | 1b | wincmd j | split | 4b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_06=\"-c \\\"echo | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_07=\"-c \\\"echo | vertical split | 4b | wincmd l | split | 1b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_08=\"-c \\\"echo | split | vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_09=\"-c \\\"echo | split | 4b | wincmd j | vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_10=\"-c \\\"echo | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_11=\"-c \\\"echo | -tabnew | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_12=\"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_13=\"-c \\\"echo | vertical split | split | vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | vertical split | split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_14=\"-c \\\"echo | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_15=\"-c \\\"echo | -tabnew | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n-\tEXPECTED_CMD_16=\"-c \\\"echo | -tabnew | split | vertical split | 1b | wincmd l | vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | vertical split | 2b | wincmd l | 3b | tabnext | vertical split | split | 1b | wincmd j | split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_05=\"-c \\\"echo | leftabove split | 1b | wincmd j | leftabove split | 4b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_06=\"-c \\\"echo | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_07=\"-c \\\"echo | leftabove vertical split | 4b | wincmd l | leftabove split | 1b | wincmd j | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_08=\"-c \\\"echo | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_09=\"-c \\\"echo | leftabove split | 4b | wincmd j | leftabove vertical split | 1b | wincmd l | 3b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_10=\"-c \\\"echo | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_11=\"-c \\\"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_12=\"-c \\\"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_13=\"-c \\\"echo | leftabove vertical split | leftabove split | leftabove vertical split | 1b | wincmd l | 3b | wincmd j | 2b | wincmd l | leftabove vertical split | leftabove split | 1b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_14=\"-c \\\"echo | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | 2b | wincmd l | 1b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_15=\"-c \\\"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n+\tEXPECTED_CMD_16=\"-c \\\"echo | -tabnew | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | 2b | wincmd l | 3b | wincmd j | 4b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 1b | tabnext | -tabnew | leftabove vertical split | 2b | wincmd l | 3b | tabnext | leftabove vertical split | leftabove split | 1b | wincmd j | leftabove split | 2b | wincmd j | 3b | wincmd l | 4b | tabdo windo diffthis\\\" -c \\\"tabfirst\\\"\"\n \n \tEXPECTED_TARGET_01=\"MERGED\"\n \tEXPECTED_TARGET_02=\"LOCAL\"\n-- \n2.37.0\n\n"},{"id":"458656","messageId":"Ysh0GWhYiyAT21Nn@zacax395.localdomain","threadId":"58123","inReplyTo":"20220708181024.45839-1-greenfoo@u92.eu","subject":"Re: [PATCH] vimdiff: make layout engine more robust against user vim settings","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-08T18:14:49Z","receivedAt":"2022-07-08T18:14:58Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"This bug was originally reported by Matthew. I have verified that the patch\nabove fixes the issue but if anyone else can also test it that would be great.\n\n\n> Reported-by: Matthew Klein <mklein994@gmail.com>\n> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>\n\nMatthew, I have added you to the \"Reported-by\" field. If you prefer to remain\nanonymous, please let me know and I'll create a new patch without your name.\n\nThanks!\n"},{"id":"458660","messageId":"xmqqsfnbuzrl.fsf@gitster.g","threadId":"58123","inReplyTo":"20220708181024.45839-1-greenfoo@u92.eu","subject":"Re: [PATCH] vimdiff: make layout engine more robust against user vim settings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-08T20:15:42Z","receivedAt":"2022-07-08T20:15:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Ramos <greenfoo@u92.eu> writes:\n\n> 'vim' has two configuration options ('splitbelow' and 'splitright') that\n> change the way the 'split' command behaves. When they are set, the\n> commands that the layout engine generates no longer work as expected.\n\nInteresting.  Does that mean that the end-user setting that was\nproblematic with the new layout engine would have also broken the\nlayout before your series?\n\n> In order to fix this we can append special keyword 'letfabove' to each\n\nPresumably \"leftabove\" was meant here.\n\n> 'split' and 'vertical split' subcommand found inside the command string\n> generated by the layout engine.\n>\n> This works because whatever comes after 'leftabove' will temporally\n> ignore settings 'splitbelow' and 'splitright'.\n>\n> Reported-by: Matthew Klein <mklein994@gmail.com>\n> Signed-off-by: Fernando Ramos <greenfoo@u92.eu>\n\nWill queue.  Thanks.\n\n"},{"id":"458671","messageId":"YsixuwhSwTCapW5L@zacax395.localdomain","threadId":"58123","inReplyTo":"xmqqsfnbuzrl.fsf@gitster.g","subject":"Re: [PATCH] vimdiff: make layout engine more robust against user vim settings","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-08T22:37:47Z","receivedAt":"2022-07-08T22:37:58Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 22/07/08 01:15PM, Junio C Hamano wrote:\n>\n> Interesting.  Does that mean that the end-user setting that was\n> problematic with the new layout engine would have also broken the\n> layout before your series?\n\nSurprisingly enough, no. Turns out the commands used before the new layout\nengine was introduced did *not* use 'split' nor 'vertical split' (which are the\ncommands affected by those two global settings).\n\nInstead, it relayed on the fact that 'vim -d' always opens splits vertically and\nthen readjusted window positions with 'wincmd [HJKL]'.\n\nThe old way was limited in the amount of things that could be achieved and\nthat's why it was changed... but I completely missed those two global settings\ncapable of changing the behaviour of the new commands.\n\n\n> > In order to fix this we can append special keyword 'letfabove' to each\n> \n> Presumably \"leftabove\" was meant here.\n\nYou are right. Sorry.\n\n\n> Will queue.  Thanks.\n\nThank you!\n"}]}