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

[Jeff King <peff@peff.net>]

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

> Even though "git update-index --cacheinfo" ought to be filesystem
> agnostic, somehow
> 
>     $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty"
> 
> fails only there.  That unfortunately makes the approach of the
> previous step unworkable.
> 
> Resurrect the earlier approach to protect the test with a
> prerequisite to make sure we do not needlessly fail the CI.

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

> -test_expect_success 'parsing a patch with no-contents and a funny pathname' '
> +test_expect_success 'setup patches in dir ending in SP' '
> +	test_when_finished "rm -fr \"funny \"" &&
> +	mkdir "funny " &&
> +	>"funny /empty" &&
> +	git add "funny /empty" &&
> +	git diff HEAD -- "funny /" >sample.patch &&
> +	git diff -R HEAD -- "funny /" >elpmas.patch &&
>  	git reset --hard &&
> -	empty_blob=$(test_oid empty_blob) &&
> -	echo "$empty_blob" >expect &&
>  
> -	git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" &&
> -	git diff --cached HEAD -- "funny /" >sample.patch &&
> -	git diff --cached -R HEAD -- "funny /" >elpmas.patch &&
> -	git reset &&
> +	if  grep "a/funny /empty b/funny /empty" sample.patch &&
> +	    grep "b/funny /empty a/funny /empty" elpmas.patch
> +	then
> +		test_set_prereq DIR_ENDS_WITH_SP
> +	else
> +		# Win test???
> +		ls -l
> +	fi
> +'

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:

diff --git a/read-cache.c b/read-cache.c
index d1aef437aa..b75aeb0be8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -976,6 +976,8 @@ static enum verify_path_result verify_path_internal(const char *path,
 	if (has_dos_drive_prefix(path))
 		return PATH_INVALID;
 
+	if (strchr(path, ' '))
+		return PATH_INVALID;
 	if (!is_valid_path(path))
 		return PATH_INVALID;
 

-Peff