| Message ID | pull.1287.git.1657726969774.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mergetool(vimdiff): allow paths to contain spaces again | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > To fix this, let's not expand the variables containing the path > parameters before passing them to the `eval` command, but let that > command expand the variables instead. Yup, that is exactly the right approach to fix this kind of breakage. Thanks for a clear description of the problem and the solution. Fernando? Ack?
On 22/07/13 09:22AM, Junio C Hamano wrote: > Thanks for a clear description of the problem and the solution. > > Fernando? Ack? Yes. 100% Ack'ed. Thanks Johannes for the bug report and the patch!
Fernando Ramos <greenfoo@u92.eu> writes: > On 22/07/13 09:22AM, Junio C Hamano wrote: >> Thanks for a clear description of the problem and the solution. >> >> Fernando? Ack? > > Yes. 100% Ack'ed. > > Thanks Johannes for the bug report and the patch! Funny that we now seem to fail t7609 mergetool --tool=vimdiff tests, e.g. https://github.com/git/git/actions/runs/2665800752
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > if $base_present > then > ... > + base_present=1 I think s/1/true/ or something is in order, perhaps? > + LOCAL='lo cal' > + BASE='ba se' > + REMOTE="' '" > + MERGED='mer ged' > + merge_tool_path=record_parameters > + > + merge_cmd vimdiff || at_least_one_ko=true > + > + cat >expect <<-\EOF > + -f > + -c > + echo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis > + -c > + tabfirst > + lo cal > + ' ' > + mer ged > + EOF > + > + diff -u expect actual || at_least_one_ko=true > + > if test "$at_least_one_ko" = "true" > then > return 255 > > base-commit: 980145f7470e20826ca22d7343494712eda9c81d
On 22/07/13 02:08PM, Junio C Hamano wrote: > > I think s/1/true/ or something is in order, perhaps? > Yes. I was just looking into that. For one, as you said, "1" should be "true". That also changes the expected output. Then, in addition, the expected output needs to be re-adjusted once again if we plan to apply this patch on top of the other one from two days ago (the one that adds the "leftabove" keyword to split subcommands). After these changes, this is how the original patch from Johannes needs to be updated: diff --git a/mergetools/vimdiff b/mergetools/vimdiff index 56516ae271..3046dcd0dc 100644 --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -623,7 +623,7 @@ run_unit_tests () { done } - base_present=1 + base_present=true LOCAL='lo cal' BASE='ba se' REMOTE="' '" @@ -635,10 +635,11 @@ run_unit_tests () { cat >expect <<-\EOF -f -c - echo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis + 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 lo cal + ba se ' ' mer ged EOF
Fernando Ramos <greenfoo@u92.eu> writes: > On 22/07/13 02:08PM, Junio C Hamano wrote: >> >> I think s/1/true/ or something is in order, perhaps? >> > > Yes. I was just looking into that. > > For one, as you said, "1" should be "true". That also changes the expected > output. OK, because "1" fails to execute, the expected output Dscho had (which is for the case without base) would become invalid when we use "true". Perhaps we should use "false" instead? Or do we need to test both? > Then, in addition, the expected output needs to be re-adjusted once again if we > plan to apply this patch on top of the other one from two days ago (the one that > adds the "leftabove" keyword to split subcommands). > After these changes, this is how the original patch from Johannes needs to be > updated: > > > diff --git a/mergetools/vimdiff b/mergetools/vimdiff > index 56516ae271..3046dcd0dc 100644 > --- a/mergetools/vimdiff > +++ b/mergetools/vimdiff > @@ -623,7 +623,7 @@ run_unit_tests () { > done > } > > - base_present=1 > + base_present=true > LOCAL='lo cal' > BASE='ba se' > REMOTE="' '" > @@ -635,10 +635,11 @@ run_unit_tests () { > cat >expect <<-\EOF > -f > -c > - echo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis > + 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 > lo cal > + ba se > ' ' > mer ged > EOF
Junio C Hamano <gitster@pobox.com> writes: >> For one, as you said, "1" should be "true". That also changes the expected >> output. > > OK, because "1" fails to execute, the expected output Dscho had > (which is for the case without base) would become invalid when we > use "true". > > Perhaps we should use "false" instead? Or do we need to test both? How confident are you with the "leftabove" stuff? Is it solid enough that it is safe to assume that we can queue this fix on top of it? This is how it would look like on top of the "leftabove" topic. mergetools/vimdiff | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git c/mergetools/vimdiff w/mergetools/vimdiff index b045b10fd7..f770b8fe24 100644 --- c/mergetools/vimdiff +++ w/mergetools/vimdiff @@ -414,8 +414,8 @@ merge_cmd () { if $base_present then - eval "$merge_tool_path" \ - -f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED" + eval '"$merge_tool_path"' \ + -f "$FINAL_CMD" '"$LOCAL"' '"$BASE"' '"$REMOTE"' '"$MERGED"' else # If there is no BASE (example: a merge conflict in a new file # with the same name created in both braches which didn't exist @@ -424,8 +424,8 @@ merge_cmd () { FINAL_CMD=$(echo "$FINAL_CMD" | \ sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g') - eval "$merge_tool_path" \ - -f "$FINAL_CMD" "$LOCAL" "$REMOTE" "$MERGED" + eval '"$merge_tool_path"' \ + -f "$FINAL_CMD" '"$LOCAL"' '"$REMOTE"' '"$MERGED"' fi ret="$?" @@ -614,6 +614,37 @@ run_unit_tests () { fi done + # verify that `merge_cmd` handles paths with spaces + record_parameters () { + >actual + for arg + do + echo "$arg" >>actual + done + } + + base_present=false + LOCAL='lo cal' + BASE='ba se' + REMOTE="' '" + MERGED='mer ged' + merge_tool_path=record_parameters + + merge_cmd vimdiff || at_least_one_ko=true + + cat >expect <<-\EOF + -f + -c + echo | leftabove split | leftabove vertical split | 1b | wincmd l | leftabove vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis + -c + tabfirst + lo cal + ' ' + mer ged + EOF + + diff -u expect actual || at_least_one_ko=true + if test "$at_least_one_ko" = "true" then return 255
On 22/07/13 02:54PM, Junio C Hamano wrote: > > OK, because "1" fails to execute, the expected output Dscho had > (which is for the case without base) would become invalid when we > use "true". > > Perhaps we should use "false" instead? Or do we need to test both? > I think testing both is not really needed as the unit test is just making sure that filenames with spaces are processed correctly. Whatever comes before (which changes depending on the value of "base_present") doesn't really matter as long as there is something. If we have to select one ("true" or "false") using "true" seems more practical, as that way the BASE file is also included, and thus an extra argument is tested.
On 22/07/13 03:06PM, Junio C Hamano wrote: > > How confident are you with the "leftabove" stuff? Is it solid > enough that it is safe to assume that we can queue this fix on top > of it? > I would say yes. I have been testing it since without problems. But it is also true that the "leftabove" patch fixes a corner case that can wait until the next release. I will keep testing it in any case and let you know if I find a case where it doesn't work.
Fernando Ramos <greenfoo@u92.eu> writes: > On 22/07/13 02:54PM, Junio C Hamano wrote: >> >> OK, because "1" fails to execute, the expected output Dscho had >> (which is for the case without base) would become invalid when we >> use "true". >> >> Perhaps we should use "false" instead? Or do we need to test both? >> > I think testing both is not really needed as the unit test is just making sure > that filenames with spaces are processed correctly. Whatever comes before > (which changes depending on the value of "base_present") doesn't really matter > as long as there is something. As the quoting glitch are distributed on both sides if $base_present then eval "$merge_tool_path" \ -f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED" else # If there is no BASE (example: a merge conflict in a new file # with the same name created in both braches which didn't exist # before), close all BASE windows using vim's "quit" command FINAL_CMD=$(echo "$FINAL_CMD" | \ sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g') eval "$merge_tool_path" \ -f "$FINAL_CMD" "$LOCAL" "$REMOTE" "$MERGED" fi I suspect you can fix only one, and carefully choose which side to test, and leave the other side broken ;-) In any case, to minimize disruption to Dscho's patch, I replaced the "1" with "false" (which will make his patch pass the new test without other stuff) and tweak its merge into 'seen' to adjust for the "leftabove" topic. Thanks.
diff --git a/mergetools/vimdiff b/mergetools/vimdiff index 461a89b6f98..59e1e2f5ca5 100644 --- a/mergetools/vimdiff +++ b/mergetools/vimdiff @@ -414,8 +414,8 @@ merge_cmd () { if $base_present then - eval "$merge_tool_path" \ - -f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED" + eval '"$merge_tool_path"' \ + -f "$FINAL_CMD" '"$LOCAL"' '"$BASE"' '"$REMOTE"' '"$MERGED"' else # If there is no BASE (example: a merge conflict in a new file # with the same name created in both braches which didn't exist @@ -424,8 +424,8 @@ merge_cmd () { FINAL_CMD=$(echo "$FINAL_CMD" | \ sed -e 's:2b:quit:g' -e 's:3b:2b:g' -e 's:4b:3b:g') - eval "$merge_tool_path" \ - -f "$FINAL_CMD" "$LOCAL" "$REMOTE" "$MERGED" + eval '"$merge_tool_path"' \ + -f "$FINAL_CMD" '"$LOCAL"' '"$REMOTE"' '"$MERGED"' fi ret="$?" @@ -614,6 +614,37 @@ run_unit_tests () { fi done + # verify that `merge_cmd` handles paths with spaces + record_parameters () { + >actual + for arg + do + echo "$arg" >>actual + done + } + + base_present=1 + LOCAL='lo cal' + BASE='ba se' + REMOTE="' '" + MERGED='mer ged' + merge_tool_path=record_parameters + + merge_cmd vimdiff || at_least_one_ko=true + + cat >expect <<-\EOF + -f + -c + echo | split | vertical split | 1b | wincmd l | vertical split | quit | wincmd l | 2b | wincmd j | 3b | tabdo windo diffthis + -c + tabfirst + lo cal + ' ' + mer ged + EOF + + diff -u expect actual || at_least_one_ko=true + if test "$at_least_one_ko" = "true" then return 255