Re: [PATCH] t4126: fix "funny directory name" test on Windows (again)

On Thu, Mar 28, 2024 at 10:37:25PM -0700, Junio C Hamano wrote:

I think this is a reasonable path forward. Looking at the patch itself...

It is a little funny that we set our prereq based only on the emptiness of the patches. That is certainly what happens on Windows, but it is a weird thing to expect. It implies that "mkdir" and "git add" returned success, but the latter without actually adding the index entry. That's what happens now, but given that such paths are forbidden by the filesystem, I'm not sure it's a good thing to rely on.

If we think that Windows is the only problematic platform, should we just use !MINGW as the prereq?

If we think there may be other platforms and would rather test the actual behavior, then we should probably be more careful. If "mkdir" fails above, we'd fail the test, rather than just not set the prereq. To solve that you can put the whole &&-chain into an if, but probably using a lazy prereq block might be more readable. I guess it is a little inefficient, though, because we have to actually add/diff to see if things are working.

IMHO just using !MINGW would be simple, efficient, and effective (especially with "--cached", where we know that the problem is not the filesystem but our own is_valid_win32_path()).

One other possible variant: we could skip the add/diff altogether and just include the patch as a test vector. After all, people on Windows could be sent such a patch without regard to their filesystem. That doesn't solve the whole issue, though, as "git apply" would fail (even with --cached) because of the verify_path() call.

But you could still check the error output to confirm that we parsed the patch correctly. That lets every platform check the main bug fix, and more capable platforms test the whole process.

Like:

-- >8 --
apply_funny_path () {
	expect=$1; shift
	path=$1; shift
	if git apply --cached --stat --check --apply "$@" 2>err
	then
		if test "$expect" = "missing"
		then
			test_must_fail git rev-parse --verify "$path"
		else
			git rev-parse --verify ":$path" >actual &&
			echo "$expect" >expect &&
			test_cmp expect actual
		fi
	else
		# some platforms (like Windows) do not allow path entries with
		# trailing spaces, even just in the index. But we should
		# at least be able to verify that we parsed the patch
		# correctly.
		if test "$expect" = "missing"
		then
			echo "error: $path: does not exist in index"
		else
			echo "error: invalid path '$path'"
		fi >expect &&
		test_cmp expect err
	fi
}

test_expect_success 'apply with no-contents and a funny pathname' '
	empty=$(git rev-parse --verify :empty) &&
	cat >sample.patch <<-EOF &&
	diff --git a/funny /empty b/funny /empty
	new file mode 100644
	index 0000000..$empty
	EOF
	cat >elpmas.patch <<-EOF &&
	diff --git b/funny /empty a/funny /empty
	deleted file mode 100644
	index e69de29..$empty
	EOF

	apply_funny_path $empty "funny /empty" sample.patch &&
	apply_funny_path missing "funny /empty" elpmas.patch &&
	apply_funny_path $empty "funny /empty" -R elpmas.patch &&
	apply_funny_path missing "funny /empty" -R sample.patch
'
-- 8< --

I didn't test it on Windows, but I did check that it does the right thing with a manual: