patchmergetool(vimdiff): allow paths to contain spaces again

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
    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.

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

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

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:

Yes. 100% Ack'ed.

Thanks Johannes for the bug report and the patch!

Fernando Ramos <greenfoo@u92.eu> writes:

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:

I think s/1/true/ or something is in order, perhaps?

On 22/07/13 02:08PM, Junio C Hamano wrote:

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:

Fernando Ramos <greenfoo@u92.eu> writes:

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?

Junio C Hamano <gitster@pobox.com> writes:

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(-)

On 22/07/13 03:06PM, Junio C Hamano wrote:

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.

On 22/07/13 02:54PM, Junio C Hamano wrote:

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.

Fernando Ramos <greenfoo@u92.eu> writes:

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.

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
    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(-)

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.

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.

Thanks.