[PATCH 0/1] quote: quote space

[PATCH 0/1] quote: quote space

[Han Young]

We're using 'git format-patch' and 'git am' workflow to sync changes between two repositories. This works great but I've found an edge case in apply.c

If one commit creates a file whose path has a directory segment ending with space will cause the generated patch unappliable. Here is a script to reproduce the edge case:

  mkdir tmp && cd tmp
  git init
  git commit --allow-empty -m empty
  mkdir 'foo '
  touch 'foo /bar'
  git add -A
  git commit -m foo
  git format-patch HEAD~1
  git reset --hard HEAD~1
  git am 0001-foo.patch

Git complains 'error: git diff header lacks filename information when removing 1 leading pathname component (line 9)'. Turns out `git_header_name()` uses the 'wrong' space as separator, and `skip_tree_prefix()` thinks the pathname as an absolute path. In theory, we could quote the pathname for this edge case. But that would require many changes to quote.c, simply quote all pathnames with space also fix the issue. 

Han Young (1):
  quote: quote space

 quote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.44.0

[PATCH 1/1] quote: quote space

[Han Young]

`git_header_name()` in apply.c uses space as separator between the preimage and postimage pathname, filename with space in them normally won't cause apply to fail because `git_header_name()` isn't using simple split. However, if the pathname has a directory whose name ending with space will lead to `skip_tree_prefix()` mistake the path as an absolute path, and git am fails with

	error: git diff header lacks filename information when removing 1 leading pathname component

The simplest fix to this edge case is to quote every path with space, even if the space is not at directory name end.
---
 quote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/quote.c b/quote.c
index 3c05194496..ecbbaed061 100644
--- a/quote.c
+++ b/quote.c
@@ -222,7 +222,7 @@ static signed char const cq_lookup[256] = {
 	/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',
 	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
 	/* 0x10 */ X16(1),
-	/* 0x20 */  -1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
+	/* 0x20 */  1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
 	/* 0x28 */ X16(-1), X16(-1), X16(-1),
 	/* 0x58 */  -1,  -1,  -1,  -1,'\\',  -1,  -1,  -1,
 	/* 0x60 */ X16(-1), X8(-1),
-- 
2.44.0

Re: [PATCH 1/1] quote: quote space

[Kristoffer Haugsbakk]

On Tue, Mar 19, 2024, at 10:52, Han Young wrote:
> `git_header_name()` in apply.c uses space as separator between the
> preimage and postimage pathname, filename with space in them normally
> won't cause apply to fail because `git_header_name()` isn't using
> simple split. However, if the pathname has a directory whose name
> ending with space will lead to `skip_tree_prefix()` mistake the path as
> an absolute path, and git am fails with
>
> 	error: git diff header lacks filename information when removing 1
> leading pathname component
>
> The simplest fix to this edge case is to quote every path with space,
> even if the space is not at directory name end.

Missing signoff? See SubmittingPatches section “sign-off”.

Also the commit message should be flowed to 72 columns. See
`.editorconfig`. (My client has flowed this reply automatically but 
that’s not what the original email looks like.)

> ---
>  quote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/quote.c b/quote.c
> index 3c05194496..ecbbaed061 100644
> --- a/quote.c
> +++ b/quote.c
> @@ -222,7 +222,7 @@ static signed char const cq_lookup[256] = {
>  	/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',
>  	/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,
>  	/* 0x10 */ X16(1),
> -	/* 0x20 */  -1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
> +	/* 0x20 */  1,  -1, '"',  -1,  -1,  -1,  -1,  -1,
>  	/* 0x28 */ X16(-1), X16(-1), X16(-1),
>  	/* 0x58 */  -1,  -1,  -1,  -1,'\\',  -1,  -1,  -1,
>  	/* 0x60 */ X16(-1), X8(-1),
> --
> 2.44.0

-- 
Kristoffer Haugsbakk

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

Han Young <hanyang.tony@bytedance.com> writes:

> We're using 'git format-patch' and 'git am' workflow to sync changes between two repositories. This works great but I've found an edge case in apply.c
>
> If one commit creates a file whose path has a directory segment ending with space will cause the generated patch unappliable. Here is a script to reproduce the edge case:
>
>   mkdir tmp && cd tmp
>   git init
>   git commit --allow-empty -m empty
>   mkdir 'foo '
>   touch 'foo /bar'
>   git add -A
>   git commit -m foo
>   git format-patch HEAD~1
>   git reset --hard HEAD~1
>   git am 0001-foo.patch

That is an interesting corner case.  You should make this into a set
of new tests somewhere in t/; I suspect this only will "break" for
creation and deletion but not modification in-place or renaming (and
that should also be in the tests).

But before going into this too deeply.

I have this feeling that we have seen corner cases like this before
and it always turned out that the right solution was to fix the
parser on the "apply" side, not on the generation side.  The tools
in the wild _will_ show a patch with a header like:

    diff --git a/foo /bar b/foo /bar
    new file mode 100644
    index 0000000..e25f181
    --- /dev/null
    +++ b/foo /bar	

even after we noticed this problem and started working on a fix, so
making sure future "git apply" can grok such output should be a lot
more fruitful direction to go into, and when it happens, we do not
have to touch the generation side at all.  Who knows what external
tools break when we suddenly start quoting a path with a space
anywhere in it, which we never did?

Thanks.

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

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

> That is an interesting corner case.  You should make this into a set
> of new tests somewhere in t/; I suspect this only will "break" for
> creation and deletion but not modification in-place or renaming (and
> that should also be in the tests).

It turns out that this is even more unintereseting than I hoped; it
happens ONLY when there is no contents shown at all in the patch,
and the patch is about creation or deletion of a path.  Mode change
without touching any contents may also trigger the same breakage.

Here is a fix, which seems not to break any existing tests.

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] apply: parse names out of "diff --git" more carefully

"git apply" uses the pathname parsed out of the "diff --git" header
to decide which path is being patched, but this is used only when
there is no other names available in the patch.  When there is any
content change (like we can see in this patch, that modifies the
contents of "apply.c") or rename (which comes with "rename from" and
"rename to" extended diff headers), the names are available without
having to parse this header.

When we do need to parse this header, a special care needs to be
taken, as the name of a directory or a file can have a SP in it so
it is not like "find a space, and take everything before the space
and that is the preimage filename, everything after the space is the
postimage filename".  We have a loop that stops at every SP on the
"diff --git a/dir/file b/dir/foo" line and see if that SP is the
right place that separates such a pair of names.

Unfortunately, this loop can terminate prematurely when a crafted
directory name ended with a SP.  The next pathname component after
that SP (i.e. the beginning of the possible postimage filename) will
be a slash, and instead of rejecting that position as the valid
separation point between pre- and post-image filenames and keep
looping, we stopped processing right there.

The fix is simple.  Instead of stopping and giving up, keep going on
when we see such a condition.

Reported-by: Han Young <hanyang.tony@bytedance.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 apply.c                |  9 ++++++++-
 t/t4126-apply-empty.sh | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git c/apply.c w/apply.c
index 432837a674..e311013bc4 100644
--- c/apply.c
+++ w/apply.c
@@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,
 				return NULL; /* no postimage name */
 			second = skip_tree_prefix(p_value, name + len + 1,
 						  line_len - (len + 1));
+			/*
+			 * If we are at the SP at the end of a directory,
+			 * skip_tree_prefix() may return NULL as that makes
+			 * it appears as if we have an absolute path.
+			 * Keep going to find another SP.
+			 */
 			if (!second)
-				return NULL;
+				continue;
+
 			/*
 			 * Does len bytes starting at "name" and "second"
 			 * (that are separated by one HT or SP we just
diff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh
index ece9fae207..eaf0c5304a 100755
--- c/t/t4126-apply-empty.sh
+++ w/t/t4126-apply-empty.sh
@@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '
 	git diff --exit-code
 '
 
+test_expect_success 'apply with no-contents and a funny pathname' '
+	mkdir "funny " &&
+	>"funny /empty" &&
+	git add "funny /empty" &&
+	git diff HEAD "funny /" >sample.patch &&
+	git diff -R HEAD "funny /" >elpmas.patch &&
+	git reset --hard &&
+	rm -fr "funny " &&
+
+	git apply --stat --check --apply sample.patch &&
+	test_must_be_empty "funny /empty" &&
+
+	git apply --stat --check --apply elpmas.patch &&
+	test_path_is_missing "funny /empty" &&
+
+	git apply -R --stat --check --apply elpmas.patch &&
+	test_must_be_empty "funny /empty" &&
+
+	git apply -R --stat --check --apply sample.patch &&
+	test_path_is_missing "funny /empty"
+'
+
 test_done

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

This patch hasn't seen any review, which is understandable because
it was buried in another patch'es discussion thread.

I'll give it a read-over once again, as self-reviews are better than
no reviews at all ;-) and then would mark it for 'next' if I didn't
find anything.

> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] apply: parse names out of "diff --git" more carefully
>
> "git apply" uses the pathname parsed out of the "diff --git" header
> to decide which path is being patched, but this is used only when
> there is no other names available in the patch.  When there is any
> content change (like we can see in this patch, that modifies the
> contents of "apply.c") or rename (which comes with "rename from" and
> "rename to" extended diff headers), the names are available without
> having to parse this header.
>
> When we do need to parse this header, a special care needs to be
> taken, as the name of a directory or a file can have a SP in it so
> it is not like "find a space, and take everything before the space
> and that is the preimage filename, everything after the space is the
> postimage filename".  We have a loop that stops at every SP on the
> "diff --git a/dir/file b/dir/foo" line and see if that SP is the
> right place that separates such a pair of names.
>
> Unfortunately, this loop can terminate prematurely when a crafted
> directory name ended with a SP.  The next pathname component after
> that SP (i.e. the beginning of the possible postimage filename) will
> be a slash, and instead of rejecting that position as the valid
> separation point between pre- and post-image filenames and keep
> looping, we stopped processing right there.
>
> The fix is simple.  Instead of stopping and giving up, keep going on
> when we see such a condition.
>
> Reported-by: Han Young <hanyang.tony@bytedance.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  apply.c                |  9 ++++++++-
>  t/t4126-apply-empty.sh | 22 ++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git c/apply.c w/apply.c
> index 432837a674..e311013bc4 100644
> --- c/apply.c
> +++ w/apply.c
> @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,
>  				return NULL; /* no postimage name */
>  			second = skip_tree_prefix(p_value, name + len + 1,
>  						  line_len - (len + 1));
> +			/*
> +			 * If we are at the SP at the end of a directory,
> +			 * skip_tree_prefix() may return NULL as that makes
> +			 * it appears as if we have an absolute path.
> +			 * Keep going to find another SP.
> +			 */
>  			if (!second)
> -				return NULL;
> +				continue;
> +
>  			/*
>  			 * Does len bytes starting at "name" and "second"
>  			 * (that are separated by one HT or SP we just
> diff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh
> index ece9fae207..eaf0c5304a 100755
> --- c/t/t4126-apply-empty.sh
> +++ w/t/t4126-apply-empty.sh
> @@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '
>  	git diff --exit-code
>  '
>  
> +test_expect_success 'apply with no-contents and a funny pathname' '
> +	mkdir "funny " &&
> +	>"funny /empty" &&
> +	git add "funny /empty" &&
> +	git diff HEAD "funny /" >sample.patch &&
> +	git diff -R HEAD "funny /" >elpmas.patch &&
> +	git reset --hard &&
> +	rm -fr "funny " &&
> +
> +	git apply --stat --check --apply sample.patch &&
> +	test_must_be_empty "funny /empty" &&
> +
> +	git apply --stat --check --apply elpmas.patch &&
> +	test_path_is_missing "funny /empty" &&
> +
> +	git apply -R --stat --check --apply elpmas.patch &&
> +	test_must_be_empty "funny /empty" &&
> +
> +	git apply -R --stat --check --apply sample.patch &&
> +	test_path_is_missing "funny /empty"
> +'
> +
>  test_done

Re: [PATCH 0/1] quote: quote space

[Jeff King]

On Tue, Mar 19, 2024 at 03:56:44PM -0700, Junio C Hamano wrote:

> Unfortunately, this loop can terminate prematurely when a crafted
> directory name ended with a SP.  The next pathname component after
> that SP (i.e. the beginning of the possible postimage filename) will
> be a slash, and instead of rejecting that position as the valid
> separation point between pre- and post-image filenames and keep
> looping, we stopped processing right there.
> 
> The fix is simple.  Instead of stopping and giving up, keep going on
> when we see such a condition.

That makes sense, but leaves me with only one question...

> @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,
>  				return NULL; /* no postimage name */
>  			second = skip_tree_prefix(p_value, name + len + 1,
>  						  line_len - (len + 1));
> +			/*
> +			 * If we are at the SP at the end of a directory,
> +			 * skip_tree_prefix() may return NULL as that makes
> +			 * it appears as if we have an absolute path.
> +			 * Keep going to find another SP.
> +			 */
>  			if (!second)
> -				return NULL;
> +				continue;
> +

If we saw a NULL from skip_tree_prefix() because it really was an
absolute path, is continuing the right thing? Or put another way: will
we continue to correctly reject such an absolute path, and not
accidentally find a pair of names?

I think it may be OK because true absolute paths imply that the first
entry would start with "/", and we would already have bailed earlier in
the function. So:

  diff --git /foo /bar

will already be rejected at the start of "/foo". And in broken input
like:

  diff --git a/foo /bar

we must assume that the start of "/bar" is a possible name, which is
what your patch is fixing. And in broken mixed input like that, we would
fail to find a valid split point, and correctly return NULL.

I guess these happen in practice with "/dev/null" as the left-hand side.
But there we'd never need the names from this line, since we'd have a
separate "deleted file mode ..." header line.

-Peff

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think it may be OK because true absolute paths imply that the first
> entry would start with "/", and we would already have bailed earlier in
> the function. So:
>
>   diff --git /foo /bar
>
> will already be rejected at the start of "/foo". And in broken input
> like:
>
>   diff --git a/foo /bar
>
> we must assume that the start of "/bar" is a possible name, which is
> what your patch is fixing. And in broken mixed input like that, we would
> fail to find a valid split point, and correctly return NULL.

Correct.

> I guess these happen in practice with "/dev/null" as the left-hand side.
> But there we'd never need the names from this line, since we'd have a
> separate "deleted file mode ..." header line.

Correct.  Besides, /dev/null appears on the ---/+++ line but not on
the "diff --git" line that is being parsed here.

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

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

> diff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh
> index ece9fae207..eaf0c5304a 100755
> --- c/t/t4126-apply-empty.sh
> +++ w/t/t4126-apply-empty.sh
> @@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '
>  	git diff --exit-code
>  '
>  
> +test_expect_success 'apply with no-contents and a funny pathname' '
> +	mkdir "funny " &&
> +	>"funny /empty" &&
> +	git add "funny /empty" &&
> +	git diff HEAD "funny /" >sample.patch &&
> +	git diff -R HEAD "funny /" >elpmas.patch &&
> +	git reset --hard &&
> +	rm -fr "funny " &&
> +
> +	git apply --stat --check --apply sample.patch &&
> +	test_must_be_empty "funny /empty" &&
> +
> +	git apply --stat --check --apply elpmas.patch &&
> +	test_path_is_missing "funny /empty" &&
> +
> +	git apply -R --stat --check --apply elpmas.patch &&
> +	test_must_be_empty "funny /empty" &&
> +
> +	git apply -R --stat --check --apply sample.patch &&
> +	test_path_is_missing "funny /empty"
> +'
> +
>  test_done

This seems to fail only on Windows, and I have run out of my today's
allotment of time for this topic.

The earlier part that creates the directory with a trailing SP,
redirects to a file in such a directory to create an empty file, and
adds that path to the index, all succeed and follow the &&-chain,
but the step that runs "git diff" with "funny /" (i.e. the name of
the directory a trailing slash) as the pathspec produces an empty
patch, and "git apply" would of course choke on an empty file as an
input.

With the following band-aid, we can skip the test and the output
from "sh t4126-*.sh -i -v -x" might give us a clue that explains how
such a failure happens.  Unfortunately GitHub CI's win test does not
give us insight into a test that did not fail, so I did not get
anything useful from the "ls -l" down there (I already knew that
sample patches are empty files).

---- >8 ----
Date: Wed, 27 Mar 2024 14:41:26 -0700
Subject: [PATCH] t4126: make sure a directory with SP at the end is usable

If the platform is unable to properly create these sample
patches about a file that lives in a directory whose name
ends with a SP, there is no point testing how "git apply"
behaves there.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4126-apply-empty.sh | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index eaf0c5304a..d2ac7a486f 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -66,14 +66,26 @@ test_expect_success 'apply --index create' '
 	git diff --exit-code
 '
 
-test_expect_success 'apply 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 diff HEAD -- "funny /" >sample.patch &&
+	git diff -R HEAD -- "funny /" >elpmas.patch &&
 	git reset --hard &&
-	rm -fr "funny " &&
+
+	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
+'
+
+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '
 
 	git apply --stat --check --apply sample.patch &&
 	test_must_be_empty "funny /empty" &&
-- 
2.44.0-368-gc75fd8d815

Re: [PATCH 0/1] quote: quote space

[Jeff King]

On Wed, Mar 27, 2024 at 03:11:08PM -0700, Junio C Hamano wrote:

> This seems to fail only on Windows, and I have run out of my today's
> allotment of time for this topic.
> 
> The earlier part that creates the directory with a trailing SP,
> redirects to a file in such a directory to create an empty file, and
> adds that path to the index, all succeed and follow the &&-chain,
> but the step that runs "git diff" with "funny /" (i.e. the name of
> the directory a trailing slash) as the pathspec produces an empty
> patch, and "git apply" would of course choke on an empty file as an
> input.
> 
> With the following band-aid, we can skip the test and the output
> from "sh t4126-*.sh -i -v -x" might give us a clue that explains how
> such a failure happens.  Unfortunately GitHub CI's win test does not
> give us insight into a test that did not fail, so I did not get
> anything useful from the "ls -l" down there (I already knew that
> sample patches are empty files).

We package up the failed test output and trash directories for each run.
You can find the one for this case here:

  https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605

It is sometimes misleading because we don't run with "-i", so subsequent
tests may stomp on things. But in this case the failing test is the last
one. Unfortunately, I don't think it shows us much, because the state we
tried to diff is removed by the test itself (both the funny dir and the
index after we tried to add it).

So I don't know if we failed to even create "funny /" in the first
place, if adding it to the index failed, or if the diff somehow failed.

On the plus side, while trying to find the failing CI job, I ran across
and diagnosed two other unrelated failures in "seen". ;)

-Peff

Re: [PATCH 0/1] quote: quote space

[Jeff King]

On Thu, Mar 28, 2024 at 06:32:54AM -0400, Jeff King wrote:

> We package up the failed test output and trash directories for each run.
> You can find the one for this case here:
> 
>   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605
> 
> It is sometimes misleading because we don't run with "-i", so subsequent
> tests may stomp on things. But in this case the failing test is the last
> one. Unfortunately, I don't think it shows us much, because the state we
> tried to diff is removed by the test itself (both the funny dir and the
> index after we tried to add it).
> 
> So I don't know if we failed to even create "funny /" in the first
> place, if adding it to the index failed, or if the diff somehow failed.

I ran it again using https://github.com/mxschmitt/action-tmate to get an
interactive shell.

It looks like making the directory works fine:

  # mkdir "funny "
  # ls -ld f*
  drwxr-xr-x 1 runneradmin None 0 Mar 28 11:01 'funny '

Likewise making the file:

  # >"funny /empty"
  # ls -l f*/*
  -rw-r--r-- 1 runneradmin None 0 Mar 28 11:02 'funny /empty'

Adding it _seems_ to work, but nothing is put into the index:

  # git add funny\ /empty
  # git ls-files -s
  100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       empty

At first I thought we had somehow created an entry with the wrong
filename, but that is just the existing "empty" entry from the earlier
tests. If you "rm .git/index" and try again, you get an empty index.

Running it under a debugger, it looks like treat_leading_path() realizes
it needs to look at "funny /", which it then feeds to is_directory().
That calls stat(), which returns -1. Digging there it looks like we feed
the expected name to GetFileAttributesExW(), but it returns an error
(123?) which we don't match in the switch statement, and we declare it
ENOENT.

So I suspect this isn't a bug in Git so much as we are running afoul of
OS limitations. And that is corroborated by these:

  https://superuser.com/questions/1733673/how-to-determine-if-a-file-with-a-trailing-space-exists

  https://stackoverflow.com/questions/48439697/trailing-whitespace-in-filename

There's some Win32 API magic you can do by prepending "\\?\", but I
couldn't get it to do anything useful.  Curiously, asking Git to
traverse itself yields another failure mode:

  # git add "funny "
  error: open("funny /empty"): No such file or directory
  error: unable to index file 'funny /empty'
  fatal: adding files failed

I'm way over my head here in terms of Windows quirks, so I'll stop
digging and assume it's not worth trying to make this work.

The patch you showed earlier functions as a workaround. But I think we
could also skip the filesystem entirely, since what we care about is
parsing the patch itself. Something like this:

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index eaf0c5304a..003b117362 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -67,25 +67,23 @@ test_expect_success 'apply --index create' '
 '
 
 test_expect_success 'apply with no-contents and a funny pathname' '
-	mkdir "funny " &&
-	>"funny /empty" &&
-	git add "funny /empty" &&
-	git diff HEAD "funny /" >sample.patch &&
-	git diff -R HEAD "funny /" >elpmas.patch &&
-	git reset --hard &&
-	rm -fr "funny " &&
+	blob=$(git rev-parse HEAD:empty) &&
+	git update-index --add --cacheinfo 100644,$blob,"funny /empty" &&
+	git diff --cached HEAD -- "funny /" >sample.patch &&
+	git diff --cached -R HEAD -- "funny /" >elpmas.patch &&
+	git reset &&
 
-	git apply --stat --check --apply sample.patch &&
-	test_must_be_empty "funny /empty" &&
+	git apply --cached --stat --check --apply sample.patch &&
+	git rev-parse --verify ":funny /empty" &&
 
-	git apply --stat --check --apply elpmas.patch &&
-	test_path_is_missing "funny /empty" &&
+	git apply --cached --stat --check --apply elpmas.patch &&
+	test_must_fail git rev-parse --verify ":funny /empty" &&
 
-	git apply -R --stat --check --apply elpmas.patch &&
-	test_must_be_empty "funny /empty" &&
+	git apply --cached -R --stat --check --apply elpmas.patch &&
+	git rev-parse --verify ":funny /empty" &&
 
-	git apply -R --stat --check --apply sample.patch &&
-	test_path_is_missing "funny /empty"
+	git apply --cached -R --stat --check --apply sample.patch &&
+	test_must_fail git rev-parse --verify ":funny /empty"
 '
 
 test_done

-Peff

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> With the following band-aid, we can skip the test and the output
>> from "sh t4126-*.sh -i -v -x" might give us a clue that explains how
>> such a failure happens.  Unfortunately GitHub CI's win test does not
>> give us insight into a test that did not fail, so I did not get
>> anything useful from the "ls -l" down there (I already knew that
>> sample patches are empty files).
>
> We package up the failed test output and trash directories for each run.
> You can find the one for this case here:
>
>   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605

What I meant was that with the band-aid that (1) sets prerequisite
so that Windows would not fail and (2) has some diagnostic in the
code that sets prerequisite, because the overall test does not fail,
we do not package up that diagnostic output.

> On the plus side, while trying to find the failing CI job, I ran across
> and diagnosed two other unrelated failures in "seen". ;)

Thanks.

Re: [PATCH 0/1] quote: quote space

[Jeff King]

On Thu, Mar 28, 2024 at 09:19:08AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> With the following band-aid, we can skip the test and the output
> >> from "sh t4126-*.sh -i -v -x" might give us a clue that explains how
> >> such a failure happens.  Unfortunately GitHub CI's win test does not
> >> give us insight into a test that did not fail, so I did not get
> >> anything useful from the "ls -l" down there (I already knew that
> >> sample patches are empty files).
> >
> > We package up the failed test output and trash directories for each run.
> > You can find the one for this case here:
> >
> >   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605
> 
> What I meant was that with the band-aid that (1) sets prerequisite
> so that Windows would not fail and (2) has some diagnostic in the
> code that sets prerequisite, because the overall test does not fail,
> we do not package up that diagnostic output.

Right, I meant that we could look at the run without the band-aid (which
is what the link points to). But I guess maybe you realized already that
it would not be helpful because of the "reset --hard" that the test
does.

-Peff

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 28, 2024 at 09:19:08AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> With the following band-aid, we can skip the test and the output
>> >> from "sh t4126-*.sh -i -v -x" might give us a clue that explains how
>> >> such a failure happens.  Unfortunately GitHub CI's win test does not
>> >> give us insight into a test that did not fail, so I did not get
>> >> anything useful from the "ls -l" down there (I already knew that
>> >> sample patches are empty files).
>> >
>> > We package up the failed test output and trash directories for each run.
>> > You can find the one for this case here:
>> >
>> >   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605
>> 
>> What I meant was that with the band-aid that (1) sets prerequisite
>> so that Windows would not fail and (2) has some diagnostic in the
>> code that sets prerequisite, because the overall test does not fail,
>> we do not package up that diagnostic output.
>
> Right, I meant that we could look at the run without the band-aid (which
> is what the link points to). But I guess maybe you realized already that
> it would not be helpful because of the "reset --hard" that the test
> does.

Actually, looking at the trash directory of the failed test was how
"I already knew that sample patches are empty files", and my hope
was that with the band-aid patch I could gather more information ;-)

Re: [PATCH 0/1] quote: quote space

[Eric Sunshine]

On Thu, Mar 28, 2024 at 7:40 AM Jeff King <peff@peff.net> wrote:
> It looks like making the directory works fine:
>
>   # mkdir "funny "
>   # ls -ld f*
>   drwxr-xr-x 1 runneradmin None 0 Mar 28 11:01 'funny '
>
> So I suspect this isn't a bug in Git so much as we are running afoul of
> OS limitations. And that is corroborated by these:
>
>   https://superuser.com/questions/1733673/how-to-determine-if-a-file-with-a-trailing-space-exists
>   https://stackoverflow.com/questions/48439697/trailing-whitespace-in-filename
>
> There's some Win32 API magic you can do by prepending "\\?\", but I
> couldn't get it to do anything useful.  Curiously, asking Git to
> traverse itself yields another failure mode:
>
>   # git add "funny "
>   error: open("funny /empty"): No such file or directory
>   error: unable to index file 'funny /empty'
>   fatal: adding files failed

This reminded me very much of [1] which exhibited the same failure
mode and was due to the same limitation(s) of the OS.

[1]: https://lore.kernel.org/git/20211209051115.52629-3-sunshine@sunshineco.com/

Re: [PATCH 0/1] quote: quote space

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> This reminded me very much of [1] which exhibited the same failure
> mode and was due to the same limitation(s) of the OS.
>
> [1]: https://lore.kernel.org/git/20211209051115.52629-3-sunshine@sunshineco.com/

Ahhhh.  That one gives the official excuse to apply the band-aid.
You quoted from their documentation

    Do not end a file or directory name with a space or a period.
    Although the underlying file system may support such names, the
    Windows shell and user interface does not.

As this test _is_, unlike the cited patch that was not about a
directory with a funny name, about parsing a patch and applying it
to a path with a directory with a funny name, I am tempted to keep
the test with the filesystem, instead of replacing it with the one
using the "--cached" that Peff suggested.  I am _also_ tempted to
add that "--cached" thing (instead of replacing), though.

Thanks

[PATCH v2] t4126: make sure a directory with SP at the end is usable

[Junio C Hamano]

As afb31ad9 (t1010: fix unnoticed failure on Windows, 2021-12-11)
said:

    On Microsoft Windows, a directory name should never end with a period.
    Quoting from Microsoft documentation[1]:

	Do not end a file or directory name with a space or a period.
	Although the underlying file system may support such names, the
	Windows shell and user interface does not.

    [1]: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

and the condition addressed by this change is exactly that.  If the
platform is unable to properly create these sample patches about a
file that lives in a directory whose name ends with a SP, there is
no point testing how "git apply" behaves there on the filesystem.

Even though the ultimate purpose of "git apply" is to apply a patch
and to update the filesystem entities, this particular test is
mainly about parsing a patch on a funny pathname correctly, and even
on a system that is incapable of checking out the resulting state
correctly on its filesystem, at least the parsing can and should work
fine.  Rewrite the test to work inside the index without touching the
filesystem.

Helped-by: Jeff King <peff@peff.net>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

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

    > As this test _is_, unlike the cited patch that was not about a
    > directory with a funny name, about parsing a patch and applying it
    > to a path with a directory with a funny name, I am tempted to keep
    > the test with the filesystem, instead of replacing it with the one
    > using the "--cached" that Peff suggested.  I am _also_ tempted to
    > add that "--cached" thing (instead of replacing), though.

    So, I changed my mind and just took Peff's "--cached" approach
    with no filesystem-based test.  format-patch --range-diff just
    didn't understand that the single patch corresponds to the only
    one patch in the older "series", and I had to force it to match
    them with --creation-factor=999 in a separate invocation.  The
    patch text has changed too much so it is useless, but the log
    message change may be easier to see in the range-diff.

1:  7e84d0f64f ! 1:  a107f21ea2 t4126: make sure a directory with SP at the end is usable
    @@ Commit message
         and the condition addressed by this change is exactly that.  If the
         platform is unable to properly create these sample patches about a
         file that lives in a directory whose name ends with a SP, there is
    -    no point testing how "git apply" behaves there.
    +    no point testing how "git apply" behaves there on the filesystem.
     
    -    Protect the test that involves the filesystem access with a
    -    prerequisite, and perform the same test only within the index
    -    everywhere.
    +    Even though the ultimate purpose of "git apply" is to apply a patch
    +    and to update the filesystem entities, this particular test is
    +    mainly about parsing a patch on a funny pathname correctly, and even
    +    on a system that is incapable of checking out the resulting state
    +    correctly on its filesystem, at least the parsing can and should work
    +    fine.  Rewrite the test to work inside the index without touching the
    +    filesystem.
     
         Helped-by: Jeff King <peff@peff.net>
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
    @@ t/t4126-apply-empty.sh: test_expect_success 'apply --index create' '
      '
      
     -test_expect_success 'apply 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" &&
    +-	mkdir "funny " &&
    +-	>"funny /empty" &&
    +-	git add "funny /empty" &&
     -	git diff HEAD "funny /" >sample.patch &&
     -	git diff -R HEAD "funny /" >elpmas.patch &&
    -+	git diff HEAD -- "funny /" >sample.patch &&
    -+	git diff -R HEAD -- "funny /" >elpmas.patch &&
    ++test_expect_success 'parsing a patch with no-contents and a funny pathname' '
      	git reset --hard &&
     -	rm -fr "funny " &&
    -+
    -+	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
    -+'
    -+
    -+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '
    -+	test_when_finished "rm -fr \"funny \"" &&
    - 
    - 	git apply --stat --check --apply sample.patch &&
    - 	test_must_be_empty "funny /empty" &&
    -@@ t/t4126-apply-empty.sh: test_expect_success 'apply with no-contents and a funny pathname' '
    - 	test_path_is_missing "funny /empty"
    - '
    - 
    -+test_expect_success 'parsing a patch with no-contents and a funny pathname' '
    -+	git reset --hard &&
    -+
     +	empty_blob=$(test_oid empty_blob) &&
    -+	echo $empty_blob >expect &&
    -+
    ++	echo "$empty_blob" >expect &&
    + 
    +-	git apply --stat --check --apply sample.patch &&
    +-	test_must_be_empty "funny /empty" &&
     +	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 &&
    -+
    + 
    +-	git apply --stat --check --apply elpmas.patch &&
    +-	test_path_is_missing "funny /empty" &&
     +	git apply --cached --stat --check --apply sample.patch &&
     +	git rev-parse --verify ":funny /empty" >actual &&
     +	test_cmp expect actual &&
    -+
    + 
    +-	git apply -R --stat --check --apply elpmas.patch &&
    +-	test_must_be_empty "funny /empty" &&
     +	git apply --cached --stat --check --apply elpmas.patch &&
     +	test_must_fail git rev-parse --verify ":funny /empty" &&
    -+
    + 
    +-	git apply -R --stat --check --apply sample.patch &&
    +-	test_path_is_missing "funny /empty"
     +	git apply -R --cached --stat --check --apply elpmas.patch &&
     +	git rev-parse --verify ":funny /empty" >actual &&
     +	test_cmp expect actual &&
     +
     +	git apply -R --cached --stat --check --apply sample.patch &&
     +	test_must_fail git rev-parse --verify ":funny /empty"
    -+'
    -+
    + '
    + 
      test_done

 t/t4126-apply-empty.sh | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index eaf0c5304a..2462cdf904 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -66,26 +66,29 @@ test_expect_success 'apply --index create' '
 	git diff --exit-code
 '
 
-test_expect_success 'apply with no-contents and a funny pathname' '
-	mkdir "funny " &&
-	>"funny /empty" &&
-	git add "funny /empty" &&
-	git diff HEAD "funny /" >sample.patch &&
-	git diff -R HEAD "funny /" >elpmas.patch &&
+test_expect_success 'parsing a patch with no-contents and a funny pathname' '
 	git reset --hard &&
-	rm -fr "funny " &&
+	empty_blob=$(test_oid empty_blob) &&
+	echo "$empty_blob" >expect &&
 
-	git apply --stat --check --apply sample.patch &&
-	test_must_be_empty "funny /empty" &&
+	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 &&
 
-	git apply --stat --check --apply elpmas.patch &&
-	test_path_is_missing "funny /empty" &&
+	git apply --cached --stat --check --apply sample.patch &&
+	git rev-parse --verify ":funny /empty" >actual &&
+	test_cmp expect actual &&
 
-	git apply -R --stat --check --apply elpmas.patch &&
-	test_must_be_empty "funny /empty" &&
+	git apply --cached --stat --check --apply elpmas.patch &&
+	test_must_fail git rev-parse --verify ":funny /empty" &&
 
-	git apply -R --stat --check --apply sample.patch &&
-	test_path_is_missing "funny /empty"
+	git apply -R --cached --stat --check --apply elpmas.patch &&
+	git rev-parse --verify ":funny /empty" >actual &&
+	test_cmp expect actual &&
+
+	git apply -R --cached --stat --check --apply sample.patch &&
+	test_must_fail git rev-parse --verify ":funny /empty"
 '
 
 test_done
-- 
2.44.0-368-gc75fd8d815

Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable

[Junio C Hamano]

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

> +test_expect_success 'parsing a patch with no-contents and a funny pathname' '
>  	git reset --hard &&
> +	empty_blob=$(test_oid empty_blob) &&
> +	echo "$empty_blob" >expect &&
>  
> +	git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" &&

It seems that on Windows, this step fails with "funny /empty" as
"invalid path".

https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244

So I'll have to redo this step; unfortunately I think it is already
in 'next', so an additional patch needs to resurrect that prerequisite
trick.

Sorry for breaking CI for 'next'.

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

[Junio C Hamano]

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t4126-apply-empty.sh | 43 +++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index 2462cdf904..d2ac7a486f 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -66,29 +66,38 @@ test_expect_success 'apply --index create' '
 	git diff --exit-code
 '
 
-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
+'
+
+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '
 
-	git apply --cached --stat --check --apply sample.patch &&
-	git rev-parse --verify ":funny /empty" >actual &&
-	test_cmp expect actual &&
+	git apply --stat --check --apply sample.patch &&
+	test_must_be_empty "funny /empty" &&
 
-	git apply --cached --stat --check --apply elpmas.patch &&
-	test_must_fail git rev-parse --verify ":funny /empty" &&
+	git apply --stat --check --apply elpmas.patch &&
+	test_path_is_missing "funny /empty" &&
 
-	git apply -R --cached --stat --check --apply elpmas.patch &&
-	git rev-parse --verify ":funny /empty" >actual &&
-	test_cmp expect actual &&
+	git apply -R --stat --check --apply elpmas.patch &&
+	test_must_be_empty "funny /empty" &&
 
-	git apply -R --cached --stat --check --apply sample.patch &&
-	test_must_fail git rev-parse --verify ":funny /empty"
+	git apply -R --stat --check --apply sample.patch &&
+	test_path_is_missing "funny /empty"
 '
 
 test_done
-- 
2.44.0-413-gd6fd04375f

Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable

[Jeff King]

On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '
> >  	git reset --hard &&
> > +	empty_blob=$(test_oid empty_blob) &&
> > +	echo "$empty_blob" >expect &&
> >  
> > +	git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" &&
> 
> It seems that on Windows, this step fails with "funny /empty" as
> "invalid path".
> 
> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244

Ah, sorry, I didn't actually try my suggestion on Windows. I guess we
are falling afoul of verify_path(), which calls is_valid_path(). That is
a noop on most platforms, but is_valid_win32_path() has:

                  switch (c) {
                  case '\0':
                  case '/': case '\\':
                          /* cannot end in ` ` or `.`, except for `.` and `..` */
                          if (preceding_space_or_period &&
                              (i != periods || periods > 2))
                                  return 0;

I'm mildly surprised that we did not hit the same problem via "git add".
But I think it does indeed call verify_path(). It's just that the
filesystem confusion prevented us from even seeing the path in the first
place, and we never got that far.

It's interesting that there is no way to override this check via
update-index, etc (like we have "--literally" for hash-object when you
want to do something stupid). I think it would be sufficient to make
things work everywhere for this test case. On the other hand, if you
have to resort to "please add this index entry which is broken on my
filesystem" to run the test, maybe that is a good sign it should just be
skipped on that platform. ;)

-Peff

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

[Jeff King]

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

Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>> 
>> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '
>> >  	git reset --hard &&
>> > +	empty_blob=$(test_oid empty_blob) &&
>> > +	echo "$empty_blob" >expect &&
>> >  
>> > +	git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" &&
>> 
>> It seems that on Windows, this step fails with "funny /empty" as
>> "invalid path".
>> 
>> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244
>
> Ah, sorry, I didn't actually try my suggestion on Windows. I guess we
> are falling afoul of verify_path(), which calls is_valid_path(). That is
> a noop on most platforms, but is_valid_win32_path() has:
>
>                   switch (c) {
>                   case '\0':
>                   case '/': case '\\':
>                           /* cannot end in ` ` or `.`, except for `.` and `..` */
>                           if (preceding_space_or_period &&
>                               (i != periods || periods > 2))
>                                   return 0;

Yes, and no need to say sorry.  I was also surprised, as I thought
that the non working tree operations ought to be platform
independenty, with this.

> It's interesting that there is no way to override this check via
> update-index, etc (like we have "--literally" for hash-object when you
> want to do something stupid). I think it would be sufficient to make
> things work everywhere for this test case. On the other hand, if you
> have to resort to "please add this index entry which is broken on my
> filesystem" to run the test, maybe that is a good sign it should just be
> skipped on that platform. ;)

This is a far-away tangent but we may want to think about "the core
of Git made into a library that works only with the objects in the
object-store and does not deal with working trees".  To work with
the objects, we would probably need something like the index that is
used in the original sense of the word (a database you consult with
a pathname as a key and obtain the object name with mode bits and a
stage number), etc.  Elijah's merge-tree may fit well within the
scheme.

There is no place like the above code in such a world.  The
restriction must exist somewhere to protect the users that use on a
limited system, but should come in a layer far above that "core
library".

Anyway, I think you convinced me in the other response that we
should just use an existing prerequisite, perhaps FUNNYNAMES.  The
idea is to exclude platforms that are known to break with the test
without any hope of fix.  Because they are incapable of taking their
users into the problematic state being tested in the first place,
this is not making things any worse.

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

[Junio C Hamano]

Even though "git update-index --cacheinfo" ought to be filesystem
agnostic,

    $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty"

fails only on Windows, and this unfortunately makes the approach of
the previous step unworkable.

Resurrect the earlier approach to give up on running the test on
known-bad platforms.  Instead of computing a custom prerequisite,
just use !MINGW we have used elsewhere.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Another reason for using MINGW is that the custom prerequisite
 would not have been a good match for lazy_prereq mechanism, which
 wants to isolate itself by creating a temporary directory to run
 the test for prerequisites, which means we are not expected use the
 main index or object store to test for prerequisites, either, which
 in turn means we are pretty much forbidden from using Git while
 computing the prerequisite.  "a platform fails the prerequisite if
 the steps to create sample patches do not work" was how the earlier
 step computed the custom prerequisite, which cannot be done without
 creating another repository in the temporary place given, which
 means we cannot reuse the patches created in the real test.

 Also, if a platform other than MINGW fails the early part of this
 test, we would want to _know_ about it, even if we may not want to
 fix it.  A custom prerequisite will defeat that.

 t/t4126-apply-empty.sh | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index 2462cdf904..56210b5609 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -66,29 +66,28 @@ test_expect_success 'apply --index create' '
 	git diff --exit-code
 '
 
-test_expect_success 'parsing a patch with no-contents and a funny pathname' '
-	git reset --hard &&
-	empty_blob=$(test_oid empty_blob) &&
-	echo "$empty_blob" >expect &&
+test_expect_success !MINGW 'apply with no-contents and a funny pathname' '
+	test_when_finished "rm -fr \"funny \"; git reset --hard" &&
+
+	mkdir "funny " &&
+	>"funny /empty" &&
+	git add "funny /empty" &&
+	git diff HEAD -- "funny /" >sample.patch &&
+	git diff -R HEAD -- "funny /" >elpmas.patch &&
 
-	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 &&
+	git reset --hard &&
 
-	git apply --cached --stat --check --apply sample.patch &&
-	git rev-parse --verify ":funny /empty" >actual &&
-	test_cmp expect actual &&
+	git apply --stat --check --apply sample.patch &&
+	test_must_be_empty "funny /empty" &&
 
-	git apply --cached --stat --check --apply elpmas.patch &&
-	test_must_fail git rev-parse --verify ":funny /empty" &&
+	git apply --stat --check --apply elpmas.patch &&
+	test_path_is_missing "funny /empty" &&
 
-	git apply -R --cached --stat --check --apply elpmas.patch &&
-	git rev-parse --verify ":funny /empty" >actual &&
-	test_cmp expect actual &&
+	git apply -R --stat --check --apply elpmas.patch &&
+	test_must_be_empty "funny /empty" &&
 
-	git apply -R --cached --stat --check --apply sample.patch &&
-	test_must_fail git rev-parse --verify ":funny /empty"
+	git apply -R --stat --check --apply sample.patch &&
+	test_path_is_missing "funny /empty"
 '
 
 test_done
-- 
2.44.0-413-gd6fd04375f

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

[Jeff King]

On Fri, Mar 29, 2024 at 10:21:58AM -0700, Junio C Hamano wrote:

> Even though "git update-index --cacheinfo" ought to be filesystem
> agnostic,
> 
>     $ git update-index --add --cacheinfo "100644,$empty_blob,funny /empty"
> 
> fails only on Windows, and this unfortunately makes the approach of
> the previous step unworkable.
> 
> Resurrect the earlier approach to give up on running the test on
> known-bad platforms.  Instead of computing a custom prerequisite,
> just use !MINGW we have used elsewhere.

Thanks, this looks good to me. You mentioned FUNNYNAMES earlier (which I
forgot even existed). That would probably work in practice, but it is
kind of overloaded already. I think using MINGW here gets to the point,
and as you note, if some other platforms fails we'd want to hear about
it.

-Peff

Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable

[Johannes Schindelin]

Hi Junio,

On Fri, 29 Mar 2024, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
> > On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:
> >
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '
> >> >  	git reset --hard &&
> >> > +	empty_blob=$(test_oid empty_blob) &&
> >> > +	echo "$empty_blob" >expect &&
> >> >
> >> > +	git update-index --add --cacheinfo "100644,$empty_blob,funny /empty" &&
> >>
> >> It seems that on Windows, this step fails with "funny /empty" as
> >> "invalid path".
> >>
> >> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244
> >
> > Ah, sorry, I didn't actually try my suggestion on Windows. I guess we
> > are falling afoul of verify_path(), which calls is_valid_path(). That is
> > a noop on most platforms, but is_valid_win32_path() has:
> >
> >                   switch (c) {
> >                   case '\0':
> >                   case '/': case '\\':
> >                           /* cannot end in ` ` or `.`, except for `.` and `..` */
> >                           if (preceding_space_or_period &&
> >                               (i != periods || periods > 2))
> >                                   return 0;
>
> Yes, and no need to say sorry.  I was also surprised, as I thought
> that the non working tree operations ought to be platform
> independenty, with this.
>
> > It's interesting that there is no way to override this check via
> > update-index, etc (like we have "--literally" for hash-object when you
> > want to do something stupid). I think it would be sufficient to make
> > things work everywhere for this test case. On the other hand, if you
> > have to resort to "please add this index entry which is broken on my
> > filesystem" to run the test, maybe that is a good sign it should just be
> > skipped on that platform. ;)
>
> This is a far-away tangent but we may want to think about "the core
> of Git made into a library that works only with the objects in the
> object-store and does not deal with working trees".  To work with
> the objects, we would probably need something like the index that is
> used in the original sense of the word (a database you consult with
> a pathname as a key and obtain the object name with mode bits and a
> stage number), etc.  Elijah's merge-tree may fit well within the
> scheme.
>
> There is no place like the above code in such a world.  The
> restriction must exist somewhere to protect the users that use on a
> limited system, but should come in a layer far above that "core
> library".

Indeed, it should have been at another layer, but alas, I could not find a
_better_ layer back when.

BTW it _is_ possible to override this check. This invocation works:

$ git -c core.protectNTFS=false update-index --add --cacheinfo "100644,$empty_blob,funny /empty"

It has been on my radar for a long time that in particular with sparse
checkouts, this check is overzealous.

I would have loved to work on it, and once I find a position where I am
funded to work meaningfully on Git for Windows again, I will.

> Anyway, I think you convinced me in the other response that we
> should just use an existing prerequisite, perhaps FUNNYNAMES.  The
> idea is to exclude platforms that are known to break with the test
> without any hope of fix.  Because they are incapable of taking their
> users into the problematic state being tested in the first place,
> this is not making things any worse.

That was indeed the correct thing to do, as far as I am concerned.

Thank you for fixing this, and sorry that I was not able to contribute
meaningfully to the fix.

Ciao,
Johannes

Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Indeed, it should have been at another layer, but alas, I could not find a
> _better_ layer back when.
> ...
> I would have loved to work on it, and once I find a position where I am
> funded to work meaningfully on Git for Windows again, I will.

Well, I would think you are working meaningfully on GfW.  Putting
that logic somewhere is what GfW person needed to do.  Putting it in
a layer (if there is no existing one, inventing a layer for it and
properly rearranging the systme) is what a "libified Git" minded
person may want to have, but that is far beyond the scope of working
meaningfully on GfW.  That is one of the things "libified Git"
people would need to do.  And if the "libified Git" folks do not do
it themselves but ask for help from GfW folks for their area
expertise, that is a perfectly acceptable way for "libified Git" to
behave, too---after all making "Git as a whole" a better system is a
team effort.

Thanks.