{"thread":{"id":"58153","subject":"[PATCH] mergetool(vimdiff): allow paths to contain spaces again","startedAt":"2022-07-13T15:42:55Z","lastAt":"2022-07-14T16:27:38Z","messageCount":13,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Fernando Ramos"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"458987","messageId":"pull.1287.git.1657726969774.gitgitgadget@gmail.com","threadId":"58153","inReplyTo":null,"subject":"[PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-13T15:42:49Z","receivedAt":"2022-07-13T15:42:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 0041797449d (vimdiff: new implementation with layout support,\n2022-03-30), we introduced a completely new implementation of the\n`vimdiff` backend for `git mergetool`.\n\nIn this implementation, we no longer call `vim` directly but we\naccumulate in the variable `FINAL_CMD` an arbitrary number of commands\nfor `vim` to execute, which necessitates the use of `eval` to split the\ncommands properly into multiple command-line arguments.\n\nThat same `eval` command also needs to pass the paths to `vim`, and\nwhile it looks as if they are quoted correctly, that quoting only\nreaches the `eval` instruction and is lost after that, therefore paths\nthat contain whitespace characters (or other characters that are\ninterpreted by the POSIX shell) are handled incorrectly.\n\nThis is a simple reproducer:\n\n\tgit init -b main bam-merge-fail\n\tcd bam-merge-fail\n\techo a>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"added 'a file.txt'\"\n\techo b>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"diverged b 'a file.txt'\"\n\tgit checkout -b c HEAD~\n\techo c>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"diverged c 'a file.txt'\"\n\tgit checkout main\n\tgit merge c\n\tgit mergetool --tool=vimdiff\n\nWith Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not\ndisplay the correct contents at all.\n\nTo fix this, let's not expand the variables containing the path\nparameters before passing them to the `eval` command, but let that\ncommand expand the variables instead.\n\nThis fixes https://github.com/git-for-windows/git/issues/3945\n\nSigned-off-by: Johannes Schindelin \n---\n mergetool(vimdiff): allow paths to contain spaces again\n \n In https://github.com/git-for-windows/git/issues/3945, it was reported\n that as of v2.37.0, git mergetool --tool=vimdiff fails to handle paths\n containing spaces. Let's fix that.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1287%2Fdscho%2Ffix-vimdiff-with-spaces-in-paths-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1287/dscho/fix-vimdiff-with-spaces-in-paths-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1287\n\n mergetools/vimdiff | 39 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 461a89b6f98..59e1e2f5ca5 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -414,8 +414,8 @@ merge_cmd () {\n \n \tif $base_present\n \tthen\n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$BASE\"' '\"$REMOTE\"' '\"$MERGED\"'\n \telse\n \t\t# If there is no BASE (example: a merge conflict in a new file\n \t\t# with the same name created in both braches which didn't exist\n@@ -424,8 +424,8 @@ merge_cmd () {\n \t\tFINAL_CMD=$(echo \"$FINAL_CMD\" | \\\n \t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n \n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$REMOTE\"' '\"$MERGED\"'\n \tfi\n \n \tret=\"$?\"\n@@ -614,6 +614,37 @@ run_unit_tests () {\n \t\tfi\n \tdone\n \n+\t# verify that `merge_cmd` handles paths with spaces\n+\trecord_parameters () {\n+\t\t>actual\n+\t\tfor arg\n+\t\tdo\n+\t\t\techo \"$arg\" >>actual\n+\t\tdone\n+\t}\n+\n+\tbase_present=1\n+\tLOCAL='lo cal'\n+\tBASE='ba se'\n+\tREMOTE=\"' '\"\n+\tMERGED='mer ged'\n+\tmerge_tool_path=record_parameters\n+\n+\tmerge_cmd vimdiff || at_least_one_ko=true\n+\n+\tcat >expect <<-\\EOF\n+\t-f\n+\t-c\n+\techo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n+\t-c\n+\ttabfirst\n+\tlo cal\n+\t' '\n+\tmer ged\n+\tEOF\n+\n+\tdiff -u expect actual || at_least_one_ko=true\n+\n \tif test \"$at_least_one_ko\" = \"true\"\n \tthen\n \t\treturn 255\n\nbase-commit: 980145f7470e20826ca22d7343494712eda9c81d\n-- \ngitgitgadget\n"},{"id":"458990","messageId":"xmqqfsj5dlt7.fsf@gitster.g","threadId":"58153","inReplyTo":"pull.1287.git.1657726969774.gitgitgadget@gmail.com","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T16:22:44Z","receivedAt":"2022-07-13T16:22:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> To fix this, let's not expand the variables containing the path\n> parameters before passing them to the `eval` command, but let that\n> command expand the variables instead.\n\nYup, that is exactly the right approach to fix this kind of\nbreakage.\n\nThanks for a clear description of the problem and the solution.\n\nFernando? Ack?\n"},{"id":"459013","messageId":"Ys8wkC1f2qc+7IX8@zacax395.localdomain","threadId":"58153","inReplyTo":"xmqqfsj5dlt7.fsf@gitster.g","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-13T20:52:32Z","receivedAt":"2022-07-13T20:52:46Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 22/07/13 09:22AM, Junio C Hamano wrote:\n> Thanks for a clear description of the problem and the solution.\n> \n> Fernando? Ack?\n\nYes. 100% Ack'ed.\n\nThanks Johannes for the bug report and the patch!\n"},{"id":"459014","messageId":"xmqqbktsbuo7.fsf@gitster.g","threadId":"58153","inReplyTo":"Ys8wkC1f2qc+7IX8@zacax395.localdomain","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T20:54:16Z","receivedAt":"2022-07-13T20:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Ramos writes:\n\n> On 22/07/13 09:22AM, Junio C Hamano wrote:\n>> Thanks for a clear description of the problem and the solution.\n>> \n>> Fernando? Ack?\n>\n> Yes. 100% Ack'ed.\n>\n> Thanks Johannes for the bug report and the patch!\n\nFunny that we now seem to fail t7609 mergetool --tool=vimdiff\ntests, e.g.\n\n https://github.com/git/git/actions/runs/2665800752\n\n"},{"id":"459016","messageId":"xmqq7d4gbu10.fsf@gitster.g","threadId":"58153","inReplyTo":"pull.1287.git.1657726969774.gitgitgadget@gmail.com","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T21:08:11Z","receivedAt":"2022-07-13T21:08:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> \tif $base_present\n> \tthen\n> ...\n> +\tbase_present=1\n\nI think s/1/true/ or something is in order, perhaps?\n\n> +\tLOCAL='lo cal'\n> +\tBASE='ba se'\n> +\tREMOTE=\"' '\"\n> +\tMERGED='mer ged'\n> +\tmerge_tool_path=record_parameters\n> +\n> +\tmerge_cmd vimdiff || at_least_one_ko=true\n> +\n> +\tcat >expect <<-\\EOF\n> +\t-f\n> +\t-c\n> +\techo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n> +\t-c\n> +\ttabfirst\n> +\tlo cal\n> +\t' '\n> +\tmer ged\n> +\tEOF\n> +\n> +\tdiff -u expect actual || at_least_one_ko=true\n> +\n> \tif test \"$at_least_one_ko\" = \"true\"\n> \tthen\n> \t\treturn 255\n>\n> base-commit: 980145f7470e20826ca22d7343494712eda9c81d\n"},{"id":"459018","messageId":"Ys86MnwhembiXRxj@zacax395.localdomain","threadId":"58153","inReplyTo":"xmqq7d4gbu10.fsf@gitster.g","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-13T21:33:38Z","receivedAt":"2022-07-13T21:33:51Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 22/07/13 02:08PM, Junio C Hamano wrote:\n> \n> I think s/1/true/ or something is in order, perhaps?\n> \n\nYes. I was just looking into that.\n\nFor one, as you said, \"1\" should be \"true\". That also changes the expected\noutput.\n\nThen, in addition, the expected output needs to be re-adjusted once again if we\nplan to apply this patch on top of the other one from two days ago (the one that\nadds the \"leftabove\" keyword to split subcommands).\n\nAfter these changes, this is how the original patch from Johannes needs to be\nupdated:\n\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 56516ae271..3046dcd0dc 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -623,7 +623,7 @@ run_unit_tests () {\n \t\tdone\n \t}\n \n-\tbase_present=1\n+\tbase_present=true\n \tLOCAL='lo cal'\n \tBASE='ba se'\n \tREMOTE=\"' '\"\n@@ -635,10 +635,11 @@ run_unit_tests () {\n \tcat >expect <<-\\EOF\n \t-f\n \t-c\n-\techo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n+\techo | -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\n \t-c\n \ttabfirst\n \tlo cal\n+\tba se\n \t' '\n \tmer ged\n \tEOF\n-- \n2.37.0\n\n"},{"id":"459019","messageId":"xmqqr12oadbk.fsf@gitster.g","threadId":"58153","inReplyTo":"Ys86MnwhembiXRxj@zacax395.localdomain","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T21:54:23Z","receivedAt":"2022-07-13T21:54:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Ramos writes:\n\n> On 22/07/13 02:08PM, Junio C Hamano wrote:\n>> \n>> I think s/1/true/ or something is in order, perhaps?\n>> \n>\n> Yes. I was just looking into that.\n>\n> For one, as you said, \"1\" should be \"true\". That also changes the expected\n> output.\n\nOK, because \"1\" fails to execute, the expected output Dscho had\n(which is for the case without base) would become invalid when we\nuse \"true\".\n\nPerhaps we should use \"false\" instead? Or do we need to test both?\n\n> Then, in addition, the expected output needs to be re-adjusted once again if we\n> plan to apply this patch on top of the other one from two days ago (the one that\n> adds the \"leftabove\" keyword to split subcommands).\n\n> After these changes, this is how the original patch from Johannes needs to be\n> updated:\n>\n>\n> diff --git a/mergetools/vimdiff b/mergetools/vimdiff\n> index 56516ae271..3046dcd0dc 100644\n> --- a/mergetools/vimdiff\n> +++ b/mergetools/vimdiff\n> @@ -623,7 +623,7 @@ run_unit_tests () {\n> \t\tdone\n> \t}\n> \n> -\tbase_present=1\n> +\tbase_present=true\n> \tLOCAL='lo cal'\n> \tBASE='ba se'\n> \tREMOTE=\"' '\"\n> @@ -635,10 +635,11 @@ run_unit_tests () {\n> \tcat >expect <<-\\EOF\n> \t-f\n> \t-c\n> -\techo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n> +\techo | -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\n> \t-c\n> \ttabfirst\n> \tlo cal\n> +\tba se\n> \t' '\n> \tmer ged\n> \tEOF\n"},{"id":"459020","messageId":"xmqqmtdcacr7.fsf@gitster.g","threadId":"58153","inReplyTo":"xmqqr12oadbk.fsf@gitster.g","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T22:06:36Z","receivedAt":"2022-07-13T22:06:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> For one, as you said, \"1\" should be \"true\". That also changes the expected\n>> output.\n>\n> OK, because \"1\" fails to execute, the expected output Dscho had\n> (which is for the case without base) would become invalid when we\n> use \"true\".\n>\n> Perhaps we should use \"false\" instead? Or do we need to test both?\n\nHow confident are you with the \"leftabove\" stuff? Is it solid\nenough that it is safe to assume that we can queue this fix on top\nof it?\n\nThis is how it would look like on top of the \"leftabove\" topic.\n\n mergetools/vimdiff | 39 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git c/mergetools/vimdiff w/mergetools/vimdiff\nindex b045b10fd7..f770b8fe24 100644\n--- c/mergetools/vimdiff\n+++ w/mergetools/vimdiff\n@@ -414,8 +414,8 @@ merge_cmd () {\n \n \tif $base_present\n \tthen\n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$BASE\"' '\"$REMOTE\"' '\"$MERGED\"'\n \telse\n \t\t# If there is no BASE (example: a merge conflict in a new file\n \t\t# with the same name created in both braches which didn't exist\n@@ -424,8 +424,8 @@ merge_cmd () {\n \t\tFINAL_CMD=$(echo \"$FINAL_CMD\" | \\\n \t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n \n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$REMOTE\"' '\"$MERGED\"'\n \tfi\n \n \tret=\"$?\"\n@@ -614,6 +614,37 @@ run_unit_tests () {\n \t\tfi\n \tdone\n \n+\t# verify that `merge_cmd` handles paths with spaces\n+\trecord_parameters () {\n+\t\t>actual\n+\t\tfor arg\n+\t\tdo\n+\t\t\techo \"$arg\" >>actual\n+\t\tdone\n+\t}\n+\n+\tbase_present=false\n+\tLOCAL='lo cal'\n+\tBASE='ba se'\n+\tREMOTE=\"' '\"\n+\tMERGED='mer ged'\n+\tmerge_tool_path=record_parameters\n+\n+\tmerge_cmd vimdiff || at_least_one_ko=true\n+\n+\tcat >expect <<-\\EOF\n+\t-f\n+\t-c\n+\techo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n+\t-c\n+\ttabfirst\n+\tlo cal\n+\t' '\n+\tmer ged\n+\tEOF\n+\n+\tdiff -u expect actual || at_least_one_ko=true\n+\n \tif test \"$at_least_one_ko\" = \"true\"\n \tthen\n \t\treturn 255\n"},{"id":"459021","messageId":"Ys9Ftu3z6zedCsoX@zacax395.localdomain","threadId":"58153","inReplyTo":"xmqqr12oadbk.fsf@gitster.g","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-13T22:22:46Z","receivedAt":"2022-07-13T22:22:58Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 22/07/13 02:54PM, Junio C Hamano wrote:\n> \n> OK, because \"1\" fails to execute, the expected output Dscho had\n> (which is for the case without base) would become invalid when we\n> use \"true\".\n> \n> Perhaps we should use \"false\" instead? Or do we need to test both?\n> \nI think testing both is not really needed as the unit test is just making sure\nthat filenames with spaces are processed correctly. Whatever comes before\n(which changes depending on the value of \"base_present\") doesn't really matter\nas long as there is something.\n\nIf we have to select one (\"true\" or \"false\") using \"true\" seems more practical,\nas that way the BASE file is also included, and thus an extra argument is\ntested.\n\n"},{"id":"459023","messageId":"Ys9HJFbvLh51GYuM@zacax395.localdomain","threadId":"58153","inReplyTo":"xmqqmtdcacr7.fsf@gitster.g","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Fernando Ramos","fromEmail":"greenfoo@u92.eu","sentAt":"2022-07-13T22:28:52Z","receivedAt":"2022-07-13T22:29:00Z","isPatch":true,"sender":{"key":"greenfoo@u92.eu","avatar":"https://avatars.githubusercontent.com/u/42691086?v=4"},"body":"On 22/07/13 03:06PM, Junio C Hamano wrote:\n>\n> How confident are you with the \"leftabove\" stuff? Is it solid\n> enough that it is safe to assume that we can queue this fix on top\n> of it?\n> \nI would say yes. I have been testing it since without problems.\n\nBut it is also true that the \"leftabove\" patch fixes a corner case that can wait\nuntil the next release.\n\nI will keep testing it in any case and let you know if I find a case where it\ndoesn't work.\n\n\n"},{"id":"459024","messageId":"xmqqa69cabhq.fsf@gitster.g","threadId":"58153","inReplyTo":"Ys9Ftu3z6zedCsoX@zacax395.localdomain","subject":"Re: [PATCH] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T22:33:53Z","receivedAt":"2022-07-13T22:34:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fernando Ramos writes:\n\n> On 22/07/13 02:54PM, Junio C Hamano wrote:\n>> \n>> OK, because \"1\" fails to execute, the expected output Dscho had\n>> (which is for the case without base) would become invalid when we\n>> use \"true\".\n>> \n>> Perhaps we should use \"false\" instead? Or do we need to test both?\n>> \n> I think testing both is not really needed as the unit test is just making sure\n> that filenames with spaces are processed correctly. Whatever comes before\n> (which changes depending on the value of \"base_present\") doesn't really matter\n> as long as there is something.\n\nAs the quoting glitch are distributed on both sides\n\n\tif $base_present\n\tthen\n\t\teval \"$merge_tool_path\" \\\n\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n\telse\n\t\t# If there is no BASE (example: a merge conflict in a new file\n\t\t# with the same name created in both braches which didn't exist\n\t\t# before), close all BASE windows using vim's \"quit\" command\n\n\t\tFINAL_CMD=$(echo \"$FINAL_CMD\" | \\\n\t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n\n\t\teval \"$merge_tool_path\" \\\n\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n\tfi\n\nI suspect you can fix only one, and carefully choose which side to\ntest, and leave the other side broken ;-)\n\nIn any case, to minimize disruption to Dscho's patch, I replaced the\n\"1\" with \"false\" (which will make his patch pass the new test\nwithout other stuff) and tweak its merge into 'seen' to adjust for\nthe \"leftabove\" topic.\n\nThanks.\n\n"},{"id":"459043","messageId":"pull.1287.v2.git.1657809063728.gitgitgadget@gmail.com","threadId":"58153","inReplyTo":"pull.1287.git.1657726969774.gitgitgadget@gmail.com","subject":"[PATCH v2] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-07-14T14:31:03Z","receivedAt":"2022-07-14T14:31:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 0041797449d (vimdiff: new implementation with layout support,\n2022-03-30), we introduced a completely new implementation of the\n`vimdiff` backend for `git mergetool`.\n\nIn this implementation, we no longer call `vim` directly but we\naccumulate in the variable `FINAL_CMD` an arbitrary number of commands\nfor `vim` to execute, which necessitates the use of `eval` to split the\ncommands properly into multiple command-line arguments.\n\nThat same `eval` command also needs to pass the paths to `vim`, and\nwhile it looks as if they are quoted correctly, that quoting only\nreaches the `eval` instruction and is lost after that, therefore paths\nthat contain whitespace characters (or other characters that are\ninterpreted by the POSIX shell) are handled incorrectly.\n\nThis is a simple reproducer:\n\n\tgit init -b main bam-merge-fail\n\tcd bam-merge-fail\n\techo a>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"added 'a file.txt'\"\n\techo b>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"diverged b 'a file.txt'\"\n\tgit checkout -b c HEAD~\n\techo c>\"a file.txt\"\n\tgit add \"a file.txt\"\n\tgit commit -m \"diverged c 'a file.txt'\"\n\tgit checkout main\n\tgit merge c\n\tgit mergetool --tool=vimdiff\n\nWith Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not\ndisplay the correct contents at all.\n\nTo fix this, let's not expand the variables containing the path\nparameters before passing them to the `eval` command, but let that\ncommand expand the variables instead.\n\nThis fixes https://github.com/git-for-windows/git/issues/3945\n\nSigned-off-by: Johannes Schindelin \n---\n mergetool(vimdiff): allow paths to contain spaces again\n \n In https://github.com/git-for-windows/git/issues/3945, it was reported\n that as of v2.37.0, git mergetool --tool=vimdiff fails to handle paths\n containing spaces. Let's fix that.\n \n Changes since v1:\n \n * Using base_present=false instead of the misleading base_present=1\n (thanks Junio & Fernando!)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1287%2Fdscho%2Ffix-vimdiff-with-spaces-in-paths-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1287/dscho/fix-vimdiff-with-spaces-in-paths-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1287\n\nRange-diff vs v1:\n\n 1: dde9c7cd2ea ! 1: 7de381b6913 mergetool(vimdiff): allow paths to contain spaces again\n @@ mergetools/vimdiff: run_unit_tests () {\n +\t\tdone\n +\t}\n +\n -+\tbase_present=1\n ++\tbase_present=false\n +\tLOCAL='lo cal'\n +\tBASE='ba se'\n +\tREMOTE=\"' '\"\n\n\n mergetools/vimdiff | 39 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/mergetools/vimdiff b/mergetools/vimdiff\nindex 461a89b6f98..f7e7f270285 100644\n--- a/mergetools/vimdiff\n+++ b/mergetools/vimdiff\n@@ -414,8 +414,8 @@ merge_cmd () {\n \n \tif $base_present\n \tthen\n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$BASE\"' '\"$REMOTE\"' '\"$MERGED\"'\n \telse\n \t\t# If there is no BASE (example: a merge conflict in a new file\n \t\t# with the same name created in both braches which didn't exist\n@@ -424,8 +424,8 @@ merge_cmd () {\n \t\tFINAL_CMD=$(echo \"$FINAL_CMD\" | \\\n \t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n \n-\t\teval \"$merge_tool_path\" \\\n-\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n+\t\teval '\"$merge_tool_path\"' \\\n+\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$REMOTE\"' '\"$MERGED\"'\n \tfi\n \n \tret=\"$?\"\n@@ -614,6 +614,37 @@ run_unit_tests () {\n \t\tfi\n \tdone\n \n+\t# verify that `merge_cmd` handles paths with spaces\n+\trecord_parameters () {\n+\t\t>actual\n+\t\tfor arg\n+\t\tdo\n+\t\t\techo \"$arg\" >>actual\n+\t\tdone\n+\t}\n+\n+\tbase_present=false\n+\tLOCAL='lo cal'\n+\tBASE='ba se'\n+\tREMOTE=\"' '\"\n+\tMERGED='mer ged'\n+\tmerge_tool_path=record_parameters\n+\n+\tmerge_cmd vimdiff || at_least_one_ko=true\n+\n+\tcat >expect <<-\\EOF\n+\t-f\n+\t-c\n+\techo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis\n+\t-c\n+\ttabfirst\n+\tlo cal\n+\t' '\n+\tmer ged\n+\tEOF\n+\n+\tdiff -u expect actual || at_least_one_ko=true\n+\n \tif test \"$at_least_one_ko\" = \"true\"\n \tthen\n \t\treturn 255\n\nbase-commit: 980145f7470e20826ca22d7343494712eda9c81d\n-- \ngitgitgadget\n"},{"id":"459050","messageId":"xmqqy1wv7j7u.fsf@gitster.g","threadId":"58153","inReplyTo":"pull.1287.v2.git.1657809063728.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] mergetool(vimdiff): allow paths to contain spaces again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-14T16:27:33Z","receivedAt":"2022-07-14T16:27:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> \tif $base_present\n> \tthen\n> -\t\teval \"$merge_tool_path\" \\\n> -\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$BASE\" \"$REMOTE\" \"$MERGED\"\n> +\t\teval '\"$merge_tool_path\"' \\\n> +\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$BASE\"' '\"$REMOTE\"' '\"$MERGED\"'\n> \telse\n> \t\t# If there is no BASE (example: a merge conflict in a new file\n> \t\t# with the same name created in both braches which didn't exist\n> @@ -424,8 +424,8 @@ merge_cmd () {\n> \t\tFINAL_CMD=$(echo \"$FINAL_CMD\" | \\\n> \t\t\tsed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g')\n> \n> -\t\teval \"$merge_tool_path\" \\\n> -\t\t\t-f \"$FINAL_CMD\" \"$LOCAL\" \"$REMOTE\" \"$MERGED\"\n> +\t\teval '\"$merge_tool_path\"' \\\n> +\t\t\t-f \"$FINAL_CMD\" '\"$LOCAL\"' '\"$REMOTE\"' '\"$MERGED\"'\n> \tfi\n\nIf there were another syntactic fix we need in the future, we may by\nmistake fix one but not the other, and the test we add in this patch\nchecks only one side but not the other. In a follow-up we may want\nto unify the two eval invocations to make the testing of this part\nmore robust.\n\nBut as a minimum and obvious fix, stopping at the above is\nappropriate for this patch.\n\n> + ...\n> +\tdiff -u expect actual || at_least_one_ko=true\n\nI wonder if we still should care about platforms that need to set\nGIT_TEST_CMP_USE_COPIED_CONTEXT while running our tests. If we use\n\"diff -u\" hardcoded like this here, we are declaring that they are\nnow unwelcome to run our tests.\n\nIt might be just the matter of using \"cmp -s\" here (or run \"diff\"\nwithout any option). Do we care about emitting the difference in\nthe output? I doubt it.\n\n> \tif test \"$at_least_one_ko\" = \"true\"\n> \tthen\n> \t\treturn 255\n\nThanks.\n"}]}