Skip to content

Commit

Permalink
mergetool(vimdiff): allow paths to contain spaces again
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
dscho committed Jul 13, 2022
1 parent 980145f commit dde9c7c
Showing 1 changed file with 35 additions and 4 deletions.
39 changes: 35 additions & 4 deletions mergetools/vimdiff
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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="$?"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit dde9c7c

Please sign in to comment.