[v2] mergetool(vimdiff): allow paths to contain spaces again

Commit Message

Johannes Schindelin July 14, 2022, 2:31 p.m. UTC

From: Johannes Schindelin <[e-mail]>

In 0041797449d (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 https://github.com/git-for-windows/git/issues/3945

Signed-off-by: Johannes Schindelin <[e-mail]>
---
    mergetool(vimdiff): allow paths to contain spaces again
    
    In https://github.com/git-for-windows/git/issues/3945, it was reported
    that as of v2.37.0, git mergetool --tool=vimdiff fails to handle paths
    containing spaces. Let's fix that.
    
    Changes since v1:
    
     * Using base_present=false instead of the misleading base_present=1
       (thanks Junio & Fernando!)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1287%2Fdscho%2Ffix-vimdiff-with-spaces-in-paths-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1287/dscho/fix-vimdiff-with-spaces-in-paths-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1287

Range-diff vs v1:

 1:  dde9c7cd2ea ! 1:  7de381b6913 mergetool(vimdiff): allow paths to contain spaces again
     @@ mergetools/vimdiff: run_unit_tests () {
      +		done
      +	}
      +
     -+	base_present=1
     ++	base_present=false
      +	LOCAL='lo cal'
      +	BASE='ba se'
      +	REMOTE="' '"


 mergetools/vimdiff | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)


base-commit: 980145f7470e20826ca22d7343494712eda9c81d

Comments

Junio C Hamano July 14, 2022, 4:27 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

>  	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

If there were another syntactic fix we need in the future, we may by
mistake fix one but not the other, and the test we add in this patch
checks only one side but not the other.  In a follow-up we may want
to unify the two eval invocations to make the testing of this part
more robust.

But as a minimum and obvious fix, stopping at the above is
appropriate for this patch.

> + ...
> +	diff -u expect actual || at_least_one_ko=true

I wonder if we still should care about platforms that need to set
GIT_TEST_CMP_USE_COPIED_CONTEXT while running our tests.  If we use
"diff -u" hardcoded like this here, we are declaring that they are
now unwelcome to run our tests.

It might be just the matter of using "cmp -s" here (or run "diff"
without any option).  Do we care about emitting the difference in
the output?  I doubt it.

>  	if test "$at_least_one_ko" = "true"
>  	then
>  		return 255

Thanks.

Patch

diff --git a/mergetools/vimdiff b/mergetools/vimdiff
index 461a89b6f98..f7e7f270285 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=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 | 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