Vimdiff Mergetool Regressed Between 2.36.1.windows.1 and 2.37.0.windows.1 #3945
Comments
|
This sounds like 0041797 is the culprit |
|
At a glance that looks like it still quotes all the filename arguments to vim correctly. |
I fear that is not quite correct. Note the eval "$merge_tool_path" \
-f "$FINAL_CMD" "$LOCAL" "$BASE" "$REMOTE" "$MERGED"At a first glance, it all looks quoted, right? But that's quoting it for the |
Good catch. |
|
@bambams to find out whether our suspicion is correct, would you mind editing (as administrator) and then running your |
|
create-merge-failure.cmd is a cmd command script that creates a demo repo to reproduce the issue in. It tracks a single file with a space in the file's name and attempts to merge conflicting branches for that file. This is the output I was able to capture from The shell prompt was overwritten in the scrollback so I'm not certain that's everything. Redirecting the output seemed to anger the command (perhaps because part of the output becomes Vim's UI?). I tried escaping the quotes in the two eval commands of the vimdiff script, but it does not work either. |
|
@dscho I could be wrong, but wouldn't this require quoting doubling up the quoting (once for the execution, and once for the |
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes git-for-windows#3945 Signed-off-by: Johannes Schindelin <[email protected]>
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes git-for-windows#3945 Signed-off-by: Johannes Schindelin <[email protected]>
@bambams thank you for that reproducer, that helped be investigate and fix it within a reasonable time frame: gitgitgadget#1287
@landstander668 At first, I thought that was the case, but I could not make it work. But then I looked around how |
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes git-for-windows#3945 Signed-off-by: Johannes Schindelin <[email protected]>
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes git-for-windows#3945 Signed-off-by: Johannes Schindelin <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes git-for-windows#3945 Signed-off-by: Johannes Schindelin <[email protected]>
A [regression in the `vimdiff` mode of `git mergetool`](git-for-windows/git#3945) has been [fixed](git-for-windows/git#3960). Signed-off-by: Johannes Schindelin <[email protected]>
In 0041797 (vimdiff: new implementation with layout support, 2022-03-30), we introduced a completely new implementation of the `vimdiff` backend for `git mergetool`. In this implementation, we no longer call `vim` directly but we accumulate in the variable `FINAL_CMD` an arbitrary number of commands for `vim` to execute, which necessitates the use of `eval` to split the commands properly into multiple command-line arguments. That same `eval` command also needs to pass the paths to `vim`, and while it looks as if they are quoted correctly, that quoting only reaches the `eval` instruction and is lost after that, therefore paths that contain whitespace characters (or other characters that are interpreted by the POSIX shell) are handled incorrectly. This is a simple reproducer: git init -b main bam-merge-fail cd bam-merge-fail echo a>"a file.txt" git add "a file.txt" git commit -m "added 'a file.txt'" echo b>"a file.txt" git add "a file.txt" git commit -m "diverged b 'a file.txt'" git checkout -b c HEAD~ echo c>"a file.txt" git add "a file.txt" git commit -m "diverged c 'a file.txt'" git checkout main git merge c git mergetool --tool=vimdiff With Git v2.37.0/v2.37.1, this will open 7 buffers, not four, and not display the correct contents at all. 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. This fixes #3945 Signed-off-by: Johannes Schindelin <[email protected]>
Setup
defaults?
to the issue you're seeing?
None that come to mind.
Details
cmd.exe
Minimal, Complete, and Verifiable example
this will help us understand the issue.
vim opens in diff mode with 4 buffers open in the default arrangement with the correct files open in each buffer.
vim opens in diff mode with 7 buffers open. Paths with spaces in them are split into multiple buffers instead of opening the singular file per path, and so all of the buffers are empty because they refer to the wrong files.
URL to that repository to help us with testing?
N/A
I downgraded to git version 2.36.1.windows.1 as described below and the issue went away. I also tried 2.37.0.rc0.windows.1, 2.37.0.rc1.windows.1, and 2.37.0.rc2.windows.1, but none of them worked either.
The text was updated successfully, but these errors were encountered: