{"thread":{"id":"61150","subject":"[PATCH 0/1] quote: quote space","startedAt":"2024-03-19T09:52:23Z","lastAt":"2024-04-27T17:21:02Z","messageCount":26,"participants":["Han Young","Kristoffer Haugsbakk","Junio C Hamano","Jeff King","Eric Sunshine","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"490920","messageId":"20240319095212.42332-1-hanyang.tony@bytedance.com","threadId":"61150","inReplyTo":null,"subject":"[PATCH 0/1] quote: quote space","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-03-19T09:52:11Z","receivedAt":"2024-03-19T09:52:23Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"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\n\nIf 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:\n\n  mkdir tmp && cd tmp\n  git init\n  git commit --allow-empty -m empty\n  mkdir 'foo '\n  touch 'foo /bar'\n  git add -A\n  git commit -m foo\n  git format-patch HEAD~1\n  git reset --hard HEAD~1\n  git am 0001-foo.patch\n\nGit 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. \n\nHan Young (1):\n  quote: quote space\n\n quote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n-- \n2.44.0\n\n"},{"id":"490921","messageId":"20240319095212.42332-2-hanyang.tony@bytedance.com","threadId":"61150","inReplyTo":"20240319095212.42332-1-hanyang.tony@bytedance.com","subject":"[PATCH 1/1] quote: quote space","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-03-19T09:52:12Z","receivedAt":"2024-03-19T09:52:28Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"`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\n\n\terror: git diff header lacks filename information when removing 1 leading pathname component\n\nThe simplest fix to this edge case is to quote every path with space, even if the space is not at directory name end.\n---\n quote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/quote.c b/quote.c\nindex 3c05194496..ecbbaed061 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -222,7 +222,7 @@ static signed char const cq_lookup[256] = {\n \t/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',\n \t/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,\n \t/* 0x10 */ X16(1),\n-\t/* 0x20 */  -1,  -1, '\"',  -1,  -1,  -1,  -1,  -1,\n+\t/* 0x20 */  1,  -1, '\"',  -1,  -1,  -1,  -1,  -1,\n \t/* 0x28 */ X16(-1), X16(-1), X16(-1),\n \t/* 0x58 */  -1,  -1,  -1,  -1,'\\\\',  -1,  -1,  -1,\n \t/* 0x60 */ X16(-1), X8(-1),\n-- \n2.44.0\n\n"},{"id":"490922","messageId":"aad45109-0a3b-45bd-b9d0-5d289d5e6b9d@app.fastmail.com","threadId":"61150","inReplyTo":"20240319095212.42332-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH 1/1] quote: quote space","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-03-19T09:59:25Z","receivedAt":"2024-03-19T09:59:57Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Mar 19, 2024, at 10:52, Han Young wrote:\n> `git_header_name()` in apply.c uses space as separator between the\n> preimage and postimage pathname, filename with space in them normally\n> won't cause apply to fail because `git_header_name()` isn't using\n> simple split. However, if the pathname has a directory whose name\n> ending with space will lead to `skip_tree_prefix()` mistake the path as\n> an absolute path, and git am fails with\n>\n> \terror: git diff header lacks filename information when removing 1\n> leading pathname component\n>\n> The simplest fix to this edge case is to quote every path with space,\n> even if the space is not at directory name end.\n\nMissing signoff? See SubmittingPatches section “sign-off”.\n\nAlso the commit message should be flowed to 72 columns. See\n`.editorconfig`. (My client has flowed this reply automatically but \nthat’s not what the original email looks like.)\n\n> ---\n>  quote.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/quote.c b/quote.c\n> index 3c05194496..ecbbaed061 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -222,7 +222,7 @@ static signed char const cq_lookup[256] = {\n>  \t/* 0x00 */   1,   1,   1,   1,   1,   1,   1, 'a',\n>  \t/* 0x08 */ 'b', 't', 'n', 'v', 'f', 'r',   1,   1,\n>  \t/* 0x10 */ X16(1),\n> -\t/* 0x20 */  -1,  -1, '\"',  -1,  -1,  -1,  -1,  -1,\n> +\t/* 0x20 */  1,  -1, '\"',  -1,  -1,  -1,  -1,  -1,\n>  \t/* 0x28 */ X16(-1), X16(-1), X16(-1),\n>  \t/* 0x58 */  -1,  -1,  -1,  -1,'\\\\',  -1,  -1,  -1,\n>  \t/* 0x60 */ X16(-1), X8(-1),\n> --\n> 2.44.0\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"490933","messageId":"xmqqttl2qml9.fsf@gitster.g","threadId":"61150","inReplyTo":"20240319095212.42332-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T15:15:46Z","receivedAt":"2024-03-19T15:15:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young <hanyang.tony@bytedance.com> writes:\n\n> 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\n>\n> 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:\n>\n>   mkdir tmp && cd tmp\n>   git init\n>   git commit --allow-empty -m empty\n>   mkdir 'foo '\n>   touch 'foo /bar'\n>   git add -A\n>   git commit -m foo\n>   git format-patch HEAD~1\n>   git reset --hard HEAD~1\n>   git am 0001-foo.patch\n\nThat is an interesting corner case.  You should make this into a set\nof new tests somewhere in t/; I suspect this only will \"break\" for\ncreation and deletion but not modification in-place or renaming (and\nthat should also be in the tests).\n\nBut before going into this too deeply.\n\nI have this feeling that we have seen corner cases like this before\nand it always turned out that the right solution was to fix the\nparser on the \"apply\" side, not on the generation side.  The tools\nin the wild _will_ show a patch with a header like:\n\n    diff --git a/foo /bar b/foo /bar\n    new file mode 100644\n    index 0000000..e25f181\n    --- /dev/null\n    +++ b/foo /bar\t\n\neven after we noticed this problem and started working on a fix, so\nmaking sure future \"git apply\" can grok such output should be a lot\nmore fruitful direction to go into, and when it happens, we do not\nhave to touch the generation side at all.  Who knows what external\ntools break when we suddenly start quoting a path with a space\nanywhere in it, which we never did?\n\nThanks.\n"},{"id":"490976","messageId":"xmqqfrwlltjn.fsf@gitster.g","threadId":"61150","inReplyTo":"xmqqttl2qml9.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-19T22:56:44Z","receivedAt":"2024-03-19T22:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> That is an interesting corner case.  You should make this into a set\n> of new tests somewhere in t/; I suspect this only will \"break\" for\n> creation and deletion but not modification in-place or renaming (and\n> that should also be in the tests).\n\nIt turns out that this is even more unintereseting than I hoped; it\nhappens ONLY when there is no contents shown at all in the patch,\nand the patch is about creation or deletion of a path.  Mode change\nwithout touching any contents may also trigger the same breakage.\n\nHere is a fix, which seems not to break any existing tests.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] apply: parse names out of \"diff --git\" more carefully\n\n\"git apply\" uses the pathname parsed out of the \"diff --git\" header\nto decide which path is being patched, but this is used only when\nthere is no other names available in the patch.  When there is any\ncontent change (like we can see in this patch, that modifies the\ncontents of \"apply.c\") or rename (which comes with \"rename from\" and\n\"rename to\" extended diff headers), the names are available without\nhaving to parse this header.\n\nWhen we do need to parse this header, a special care needs to be\ntaken, as the name of a directory or a file can have a SP in it so\nit is not like \"find a space, and take everything before the space\nand that is the preimage filename, everything after the space is the\npostimage filename\".  We have a loop that stops at every SP on the\n\"diff --git a/dir/file b/dir/foo\" line and see if that SP is the\nright place that separates such a pair of names.\n\nUnfortunately, this loop can terminate prematurely when a crafted\ndirectory name ended with a SP.  The next pathname component after\nthat SP (i.e. the beginning of the possible postimage filename) will\nbe a slash, and instead of rejecting that position as the valid\nseparation point between pre- and post-image filenames and keep\nlooping, we stopped processing right there.\n\nThe fix is simple.  Instead of stopping and giving up, keep going on\nwhen we see such a condition.\n\nReported-by: Han Young <hanyang.tony@bytedance.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n apply.c                |  9 ++++++++-\n t/t4126-apply-empty.sh | 22 ++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git c/apply.c w/apply.c\nindex 432837a674..e311013bc4 100644\n--- c/apply.c\n+++ w/apply.c\n@@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,\n \t\t\t\treturn NULL; /* no postimage name */\n \t\t\tsecond = skip_tree_prefix(p_value, name + len + 1,\n \t\t\t\t\t\t  line_len - (len + 1));\n+\t\t\t/*\n+\t\t\t * If we are at the SP at the end of a directory,\n+\t\t\t * skip_tree_prefix() may return NULL as that makes\n+\t\t\t * it appears as if we have an absolute path.\n+\t\t\t * Keep going to find another SP.\n+\t\t\t */\n \t\t\tif (!second)\n-\t\t\t\treturn NULL;\n+\t\t\t\tcontinue;\n+\n \t\t\t/*\n \t\t\t * Does len bytes starting at \"name\" and \"second\"\n \t\t\t * (that are separated by one HT or SP we just\ndiff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh\nindex ece9fae207..eaf0c5304a 100755\n--- c/t/t4126-apply-empty.sh\n+++ w/t/t4126-apply-empty.sh\n@@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '\n \tgit diff --exit-code\n '\n \n+test_expect_success 'apply with no-contents and a funny pathname' '\n+\tmkdir \"funny \" &&\n+\t>\"funny /empty\" &&\n+\tgit add \"funny /empty\" &&\n+\tgit diff HEAD \"funny /\" >sample.patch &&\n+\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n+\tgit reset --hard &&\n+\trm -fr \"funny \" &&\n+\n+\tgit apply --stat --check --apply sample.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n+\n+\tgit apply --stat --check --apply elpmas.patch &&\n+\ttest_path_is_missing \"funny /empty\" &&\n+\n+\tgit apply -R --stat --check --apply elpmas.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n+\n+\tgit apply -R --stat --check --apply sample.patch &&\n+\ttest_path_is_missing \"funny /empty\"\n+'\n+\n test_done\n"},{"id":"491610","messageId":"xmqqa5mk8ycm.fsf@gitster.g","threadId":"61150","inReplyTo":"xmqqfrwlltjn.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-26T21:41:45Z","receivedAt":"2024-03-26T21:41:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This patch hasn't seen any review, which is understandable because\nit was buried in another patch'es discussion thread.\n\nI'll give it a read-over once again, as self-reviews are better than\nno reviews at all ;-) and then would mark it for 'next' if I didn't\nfind anything.\n\n> ----- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] apply: parse names out of \"diff --git\" more carefully\n>\n> \"git apply\" uses the pathname parsed out of the \"diff --git\" header\n> to decide which path is being patched, but this is used only when\n> there is no other names available in the patch.  When there is any\n> content change (like we can see in this patch, that modifies the\n> contents of \"apply.c\") or rename (which comes with \"rename from\" and\n> \"rename to\" extended diff headers), the names are available without\n> having to parse this header.\n>\n> When we do need to parse this header, a special care needs to be\n> taken, as the name of a directory or a file can have a SP in it so\n> it is not like \"find a space, and take everything before the space\n> and that is the preimage filename, everything after the space is the\n> postimage filename\".  We have a loop that stops at every SP on the\n> \"diff --git a/dir/file b/dir/foo\" line and see if that SP is the\n> right place that separates such a pair of names.\n>\n> Unfortunately, this loop can terminate prematurely when a crafted\n> directory name ended with a SP.  The next pathname component after\n> that SP (i.e. the beginning of the possible postimage filename) will\n> be a slash, and instead of rejecting that position as the valid\n> separation point between pre- and post-image filenames and keep\n> looping, we stopped processing right there.\n>\n> The fix is simple.  Instead of stopping and giving up, keep going on\n> when we see such a condition.\n>\n> Reported-by: Han Young <hanyang.tony@bytedance.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  apply.c                |  9 ++++++++-\n>  t/t4126-apply-empty.sh | 22 ++++++++++++++++++++++\n>  2 files changed, 30 insertions(+), 1 deletion(-)\n>\n> diff --git c/apply.c w/apply.c\n> index 432837a674..e311013bc4 100644\n> --- c/apply.c\n> +++ w/apply.c\n> @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,\n>  \t\t\t\treturn NULL; /* no postimage name */\n>  \t\t\tsecond = skip_tree_prefix(p_value, name + len + 1,\n>  \t\t\t\t\t\t  line_len - (len + 1));\n> +\t\t\t/*\n> +\t\t\t * If we are at the SP at the end of a directory,\n> +\t\t\t * skip_tree_prefix() may return NULL as that makes\n> +\t\t\t * it appears as if we have an absolute path.\n> +\t\t\t * Keep going to find another SP.\n> +\t\t\t */\n>  \t\t\tif (!second)\n> -\t\t\t\treturn NULL;\n> +\t\t\t\tcontinue;\n> +\n>  \t\t\t/*\n>  \t\t\t * Does len bytes starting at \"name\" and \"second\"\n>  \t\t\t * (that are separated by one HT or SP we just\n> diff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh\n> index ece9fae207..eaf0c5304a 100755\n> --- c/t/t4126-apply-empty.sh\n> +++ w/t/t4126-apply-empty.sh\n> @@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '\n>  \tgit diff --exit-code\n>  '\n>  \n> +test_expect_success 'apply with no-contents and a funny pathname' '\n> +\tmkdir \"funny \" &&\n> +\t>\"funny /empty\" &&\n> +\tgit add \"funny /empty\" &&\n> +\tgit diff HEAD \"funny /\" >sample.patch &&\n> +\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n> +\tgit reset --hard &&\n> +\trm -fr \"funny \" &&\n> +\n> +\tgit apply --stat --check --apply sample.patch &&\n> +\ttest_must_be_empty \"funny /empty\" &&\n> +\n> +\tgit apply --stat --check --apply elpmas.patch &&\n> +\ttest_path_is_missing \"funny /empty\" &&\n> +\n> +\tgit apply -R --stat --check --apply elpmas.patch &&\n> +\ttest_must_be_empty \"funny /empty\" &&\n> +\n> +\tgit apply -R --stat --check --apply sample.patch &&\n> +\ttest_path_is_missing \"funny /empty\"\n> +'\n> +\n>  test_done\n"},{"id":"491675","messageId":"20240327091742.GA847537@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqqfrwlltjn.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-27T09:17:42Z","receivedAt":"2024-03-27T09:17:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 19, 2024 at 03:56:44PM -0700, Junio C Hamano wrote:\n\n> Unfortunately, this loop can terminate prematurely when a crafted\n> directory name ended with a SP.  The next pathname component after\n> that SP (i.e. the beginning of the possible postimage filename) will\n> be a slash, and instead of rejecting that position as the valid\n> separation point between pre- and post-image filenames and keep\n> looping, we stopped processing right there.\n> \n> The fix is simple.  Instead of stopping and giving up, keep going on\n> when we see such a condition.\n\nThat makes sense, but leaves me with only one question...\n\n> @@ -1292,8 +1292,15 @@ static char *git_header_name(int p_value,\n>  \t\t\t\treturn NULL; /* no postimage name */\n>  \t\t\tsecond = skip_tree_prefix(p_value, name + len + 1,\n>  \t\t\t\t\t\t  line_len - (len + 1));\n> +\t\t\t/*\n> +\t\t\t * If we are at the SP at the end of a directory,\n> +\t\t\t * skip_tree_prefix() may return NULL as that makes\n> +\t\t\t * it appears as if we have an absolute path.\n> +\t\t\t * Keep going to find another SP.\n> +\t\t\t */\n>  \t\t\tif (!second)\n> -\t\t\t\treturn NULL;\n> +\t\t\t\tcontinue;\n> +\n\nIf we saw a NULL from skip_tree_prefix() because it really was an\nabsolute path, is continuing the right thing? Or put another way: will\nwe continue to correctly reject such an absolute path, and not\naccidentally find a pair of names?\n\nI think it may be OK because true absolute paths imply that the first\nentry would start with \"/\", and we would already have bailed earlier in\nthe function. So:\n\n  diff --git /foo /bar\n\nwill already be rejected at the start of \"/foo\". And in broken input\nlike:\n\n  diff --git a/foo /bar\n\nwe must assume that the start of \"/bar\" is a possible name, which is\nwhat your patch is fixing. And in broken mixed input like that, we would\nfail to find a valid split point, and correctly return NULL.\n\nI guess these happen in practice with \"/dev/null\" as the left-hand side.\nBut there we'd never need the names from this line, since we'd have a\nseparate \"deleted file mode ...\" header line.\n\n-Peff\n"},{"id":"491698","messageId":"xmqqttkr4t6h.fsf@gitster.g","threadId":"61150","inReplyTo":"20240327091742.GA847537@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-27T14:59:18Z","receivedAt":"2024-03-27T14:59:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think it may be OK because true absolute paths imply that the first\n> entry would start with \"/\", and we would already have bailed earlier in\n> the function. So:\n>\n>   diff --git /foo /bar\n>\n> will already be rejected at the start of \"/foo\". And in broken input\n> like:\n>\n>   diff --git a/foo /bar\n>\n> we must assume that the start of \"/bar\" is a possible name, which is\n> what your patch is fixing. And in broken mixed input like that, we would\n> fail to find a valid split point, and correctly return NULL.\n\nCorrect.\n\n> I guess these happen in practice with \"/dev/null\" as the left-hand side.\n> But there we'd never need the names from this line, since we'd have a\n> separate \"deleted file mode ...\" header line.\n\nCorrect.  Besides, /dev/null appears on the ---/+++ line but not on\nthe \"diff --git\" line that is being parsed here.\n\n"},{"id":"491713","messageId":"xmqqsf0bz5oj.fsf@gitster.g","threadId":"61150","inReplyTo":"xmqqfrwlltjn.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-27T22:11:08Z","receivedAt":"2024-03-27T22:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> diff --git c/t/t4126-apply-empty.sh w/t/t4126-apply-empty.sh\n> index ece9fae207..eaf0c5304a 100755\n> --- c/t/t4126-apply-empty.sh\n> +++ w/t/t4126-apply-empty.sh\n> @@ -66,4 +66,26 @@ test_expect_success 'apply --index create' '\n>  \tgit diff --exit-code\n>  '\n>  \n> +test_expect_success 'apply with no-contents and a funny pathname' '\n> +\tmkdir \"funny \" &&\n> +\t>\"funny /empty\" &&\n> +\tgit add \"funny /empty\" &&\n> +\tgit diff HEAD \"funny /\" >sample.patch &&\n> +\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n> +\tgit reset --hard &&\n> +\trm -fr \"funny \" &&\n> +\n> +\tgit apply --stat --check --apply sample.patch &&\n> +\ttest_must_be_empty \"funny /empty\" &&\n> +\n> +\tgit apply --stat --check --apply elpmas.patch &&\n> +\ttest_path_is_missing \"funny /empty\" &&\n> +\n> +\tgit apply -R --stat --check --apply elpmas.patch &&\n> +\ttest_must_be_empty \"funny /empty\" &&\n> +\n> +\tgit apply -R --stat --check --apply sample.patch &&\n> +\ttest_path_is_missing \"funny /empty\"\n> +'\n> +\n>  test_done\n\nThis seems to fail only on Windows, and I have run out of my today's\nallotment of time for this topic.\n\nThe earlier part that creates the directory with a trailing SP,\nredirects to a file in such a directory to create an empty file, and\nadds that path to the index, all succeed and follow the &&-chain,\nbut the step that runs \"git diff\" with \"funny /\" (i.e. the name of\nthe directory a trailing slash) as the pathspec produces an empty\npatch, and \"git apply\" would of course choke on an empty file as an\ninput.\n\nWith the following band-aid, we can skip the test and the output\nfrom \"sh t4126-*.sh -i -v -x\" might give us a clue that explains how\nsuch a failure happens.  Unfortunately GitHub CI's win test does not\ngive us insight into a test that did not fail, so I did not get\nanything useful from the \"ls -l\" down there (I already knew that\nsample patches are empty files).\n\n---- >8 ----\nDate: Wed, 27 Mar 2024 14:41:26 -0700\nSubject: [PATCH] t4126: make sure a directory with SP at the end is usable\n\nIf the platform is unable to properly create these sample\npatches about a file that lives in a directory whose name\nends with a SP, there is no point testing how \"git apply\"\nbehaves there.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4126-apply-empty.sh | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex eaf0c5304a..d2ac7a486f 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -66,14 +66,26 @@ test_expect_success 'apply --index create' '\n \tgit diff --exit-code\n '\n \n-test_expect_success 'apply with no-contents and a funny pathname' '\n+test_expect_success 'setup patches in dir ending in SP' '\n+\ttest_when_finished \"rm -fr \\\"funny \\\"\" &&\n \tmkdir \"funny \" &&\n \t>\"funny /empty\" &&\n \tgit add \"funny /empty\" &&\n-\tgit diff HEAD \"funny /\" >sample.patch &&\n-\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n+\tgit diff HEAD -- \"funny /\" >sample.patch &&\n+\tgit diff -R HEAD -- \"funny /\" >elpmas.patch &&\n \tgit reset --hard &&\n-\trm -fr \"funny \" &&\n+\n+\tif  grep \"a/funny /empty b/funny /empty\" sample.patch &&\n+\t    grep \"b/funny /empty a/funny /empty\" elpmas.patch\n+\tthen\n+\t\ttest_set_prereq DIR_ENDS_WITH_SP\n+\telse\n+\t\t# Win test???\n+\t\tls -l\n+\tfi\n+'\n+\n+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '\n \n \tgit apply --stat --check --apply sample.patch &&\n \ttest_must_be_empty \"funny /empty\" &&\n-- \n2.44.0-368-gc75fd8d815\n\n"},{"id":"491744","messageId":"20240328103254.GA898963@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqqsf0bz5oj.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-28T10:32:54Z","receivedAt":"2024-03-28T10:32:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 27, 2024 at 03:11:08PM -0700, Junio C Hamano wrote:\n\n> This seems to fail only on Windows, and I have run out of my today's\n> allotment of time for this topic.\n> \n> The earlier part that creates the directory with a trailing SP,\n> redirects to a file in such a directory to create an empty file, and\n> adds that path to the index, all succeed and follow the &&-chain,\n> but the step that runs \"git diff\" with \"funny /\" (i.e. the name of\n> the directory a trailing slash) as the pathspec produces an empty\n> patch, and \"git apply\" would of course choke on an empty file as an\n> input.\n> \n> With the following band-aid, we can skip the test and the output\n> from \"sh t4126-*.sh -i -v -x\" might give us a clue that explains how\n> such a failure happens.  Unfortunately GitHub CI's win test does not\n> give us insight into a test that did not fail, so I did not get\n> anything useful from the \"ls -l\" down there (I already knew that\n> sample patches are empty files).\n\nWe package up the failed test output and trash directories for each run.\nYou can find the one for this case here:\n\n  https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605\n\nIt is sometimes misleading because we don't run with \"-i\", so subsequent\ntests may stomp on things. But in this case the failing test is the last\none. Unfortunately, I don't think it shows us much, because the state we\ntried to diff is removed by the test itself (both the funny dir and the\nindex after we tried to add it).\n\nSo I don't know if we failed to even create \"funny /\" in the first\nplace, if adding it to the index failed, or if the diff somehow failed.\n\nOn the plus side, while trying to find the failing CI job, I ran across\nand diagnosed two other unrelated failures in \"seen\". ;)\n\n-Peff\n"},{"id":"491745","messageId":"20240328114038.GA1394725@coredump.intra.peff.net","threadId":"61150","inReplyTo":"20240328103254.GA898963@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-28T11:40:38Z","receivedAt":"2024-03-28T11:40:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2024 at 06:32:54AM -0400, Jeff King wrote:\n\n> We package up the failed test output and trash directories for each run.\n> You can find the one for this case here:\n> \n>   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605\n> \n> It is sometimes misleading because we don't run with \"-i\", so subsequent\n> tests may stomp on things. But in this case the failing test is the last\n> one. Unfortunately, I don't think it shows us much, because the state we\n> tried to diff is removed by the test itself (both the funny dir and the\n> index after we tried to add it).\n> \n> So I don't know if we failed to even create \"funny /\" in the first\n> place, if adding it to the index failed, or if the diff somehow failed.\n\nI ran it again using https://github.com/mxschmitt/action-tmate to get an\ninteractive shell.\n\nIt looks like making the directory works fine:\n\n  # mkdir \"funny \"\n  # ls -ld f*\n  drwxr-xr-x 1 runneradmin None 0 Mar 28 11:01 'funny '\n\nLikewise making the file:\n\n  # >\"funny /empty\"\n  # ls -l f*/*\n  -rw-r--r-- 1 runneradmin None 0 Mar 28 11:02 'funny /empty'\n\nAdding it _seems_ to work, but nothing is put into the index:\n\n  # git add funny\\ /empty\n  # git ls-files -s\n  100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0       empty\n\nAt first I thought we had somehow created an entry with the wrong\nfilename, but that is just the existing \"empty\" entry from the earlier\ntests. If you \"rm .git/index\" and try again, you get an empty index.\n\nRunning it under a debugger, it looks like treat_leading_path() realizes\nit needs to look at \"funny /\", which it then feeds to is_directory().\nThat calls stat(), which returns -1. Digging there it looks like we feed\nthe expected name to GetFileAttributesExW(), but it returns an error\n(123?) which we don't match in the switch statement, and we declare it\nENOENT.\n\nSo I suspect this isn't a bug in Git so much as we are running afoul of\nOS limitations. And that is corroborated by these:\n\n  https://superuser.com/questions/1733673/how-to-determine-if-a-file-with-a-trailing-space-exists\n\n  https://stackoverflow.com/questions/48439697/trailing-whitespace-in-filename\n\nThere's some Win32 API magic you can do by prepending \"\\\\?\\\", but I\ncouldn't get it to do anything useful.  Curiously, asking Git to\ntraverse itself yields another failure mode:\n\n  # git add \"funny \"\n  error: open(\"funny /empty\"): No such file or directory\n  error: unable to index file 'funny /empty'\n  fatal: adding files failed\n\nI'm way over my head here in terms of Windows quirks, so I'll stop\ndigging and assume it's not worth trying to make this work.\n\nThe patch you showed earlier functions as a workaround. But I think we\ncould also skip the filesystem entirely, since what we care about is\nparsing the patch itself. Something like this:\n\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex eaf0c5304a..003b117362 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -67,25 +67,23 @@ test_expect_success 'apply --index create' '\n '\n \n test_expect_success 'apply with no-contents and a funny pathname' '\n-\tmkdir \"funny \" &&\n-\t>\"funny /empty\" &&\n-\tgit add \"funny /empty\" &&\n-\tgit diff HEAD \"funny /\" >sample.patch &&\n-\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n-\tgit reset --hard &&\n-\trm -fr \"funny \" &&\n+\tblob=$(git rev-parse HEAD:empty) &&\n+\tgit update-index --add --cacheinfo 100644,$blob,\"funny /empty\" &&\n+\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n+\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n+\tgit reset &&\n \n-\tgit apply --stat --check --apply sample.patch &&\n-\ttest_must_be_empty \"funny /empty\" &&\n+\tgit apply --cached --stat --check --apply sample.patch &&\n+\tgit rev-parse --verify \":funny /empty\" &&\n \n-\tgit apply --stat --check --apply elpmas.patch &&\n-\ttest_path_is_missing \"funny /empty\" &&\n+\tgit apply --cached --stat --check --apply elpmas.patch &&\n+\ttest_must_fail git rev-parse --verify \":funny /empty\" &&\n \n-\tgit apply -R --stat --check --apply elpmas.patch &&\n-\ttest_must_be_empty \"funny /empty\" &&\n+\tgit apply --cached -R --stat --check --apply elpmas.patch &&\n+\tgit rev-parse --verify \":funny /empty\" &&\n \n-\tgit apply -R --stat --check --apply sample.patch &&\n-\ttest_path_is_missing \"funny /empty\"\n+\tgit apply --cached -R --stat --check --apply sample.patch &&\n+\ttest_must_fail git rev-parse --verify \":funny /empty\"\n '\n \n test_done\n\n-Peff\n"},{"id":"491756","messageId":"xmqq34sawcqr.fsf@gitster.g","threadId":"61150","inReplyTo":"20240328103254.GA898963@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-28T16:19:08Z","receivedAt":"2024-03-28T16:19:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> With the following band-aid, we can skip the test and the output\n>> from \"sh t4126-*.sh -i -v -x\" might give us a clue that explains how\n>> such a failure happens.  Unfortunately GitHub CI's win test does not\n>> give us insight into a test that did not fail, so I did not get\n>> anything useful from the \"ls -l\" down there (I already knew that\n>> sample patches are empty files).\n>\n> We package up the failed test output and trash directories for each run.\n> You can find the one for this case here:\n>\n>   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605\n\nWhat I meant was that with the band-aid that (1) sets prerequisite\nso that Windows would not fail and (2) has some diagnostic in the\ncode that sets prerequisite, because the overall test does not fail,\nwe do not package up that diagnostic output.\n\n> On the plus side, while trying to find the failing CI job, I ran across\n> and diagnosed two other unrelated failures in \"seen\". ;)\n\nThanks.\n"},{"id":"491758","messageId":"20240328163028.GB1403492@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqq34sawcqr.fsf@gitster.g","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-28T16:30:28Z","receivedAt":"2024-03-28T16:30:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2024 at 09:19:08AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> With the following band-aid, we can skip the test and the output\n> >> from \"sh t4126-*.sh -i -v -x\" might give us a clue that explains how\n> >> such a failure happens.  Unfortunately GitHub CI's win test does not\n> >> give us insight into a test that did not fail, so I did not get\n> >> anything useful from the \"ls -l\" down there (I already knew that\n> >> sample patches are empty files).\n> >\n> > We package up the failed test output and trash directories for each run.\n> > You can find the one for this case here:\n> >\n> >   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605\n> \n> What I meant was that with the band-aid that (1) sets prerequisite\n> so that Windows would not fail and (2) has some diagnostic in the\n> code that sets prerequisite, because the overall test does not fail,\n> we do not package up that diagnostic output.\n\nRight, I meant that we could look at the run without the band-aid (which\nis what the link points to). But I guess maybe you realized already that\nit would not be helpful because of the \"reset --hard\" that the test\ndoes.\n\n-Peff\n"},{"id":"491761","messageId":"xmqqjzlmuwkw.fsf@gitster.g","threadId":"61150","inReplyTo":"20240328163028.GB1403492@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-28T16:53:35Z","receivedAt":"2024-03-28T16:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 28, 2024 at 09:19:08AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >> With the following band-aid, we can skip the test and the output\n>> >> from \"sh t4126-*.sh -i -v -x\" might give us a clue that explains how\n>> >> such a failure happens.  Unfortunately GitHub CI's win test does not\n>> >> give us insight into a test that did not fail, so I did not get\n>> >> anything useful from the \"ls -l\" down there (I already knew that\n>> >> sample patches are empty files).\n>> >\n>> > We package up the failed test output and trash directories for each run.\n>> > You can find the one for this case here:\n>> >\n>> >   https://github.com/git/git/actions/runs/8458842054/artifacts/1364695605\n>> \n>> What I meant was that with the band-aid that (1) sets prerequisite\n>> so that Windows would not fail and (2) has some diagnostic in the\n>> code that sets prerequisite, because the overall test does not fail,\n>> we do not package up that diagnostic output.\n>\n> Right, I meant that we could look at the run without the band-aid (which\n> is what the link points to). But I guess maybe you realized already that\n> it would not be helpful because of the \"reset --hard\" that the test\n> does.\n\nActually, looking at the trash directory of the failed test was how\n\"I already knew that sample patches are empty files\", and my hope\nwas that with the band-aid patch I could gather more information ;-)\n"},{"id":"491762","messageId":"CAPig+cQe1rAN2MUFTwo7JoCt3sO2eCk_psnJL9D=Rs=Q9MWO9A@mail.gmail.com","threadId":"61150","inReplyTo":"20240328114038.GA1394725@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-28T17:05:10Z","receivedAt":"2024-03-28T17:05:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 28, 2024 at 7:40 AM Jeff King <peff@peff.net> wrote:\n> It looks like making the directory works fine:\n>\n>   # mkdir \"funny \"\n>   # ls -ld f*\n>   drwxr-xr-x 1 runneradmin None 0 Mar 28 11:01 'funny '\n>\n> So I suspect this isn't a bug in Git so much as we are running afoul of\n> OS limitations. And that is corroborated by these:\n>\n>   https://superuser.com/questions/1733673/how-to-determine-if-a-file-with-a-trailing-space-exists\n>   https://stackoverflow.com/questions/48439697/trailing-whitespace-in-filename\n>\n> There's some Win32 API magic you can do by prepending \"\\\\?\\\", but I\n> couldn't get it to do anything useful.  Curiously, asking Git to\n> traverse itself yields another failure mode:\n>\n>   # git add \"funny \"\n>   error: open(\"funny /empty\"): No such file or directory\n>   error: unable to index file 'funny /empty'\n>   fatal: adding files failed\n\nThis reminded me very much of [1] which exhibited the same failure\nmode and was due to the same limitation(s) of the OS.\n\n[1]: https://lore.kernel.org/git/20211209051115.52629-3-sunshine@sunshineco.com/\n"},{"id":"491767","messageId":"xmqqa5miuutd.fsf@gitster.g","threadId":"61150","inReplyTo":"CAPig+cQe1rAN2MUFTwo7JoCt3sO2eCk_psnJL9D=Rs=Q9MWO9A@mail.gmail.com","subject":"Re: [PATCH 0/1] quote: quote space","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-28T17:31:42Z","receivedAt":"2024-03-28T17:31:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> This reminded me very much of [1] which exhibited the same failure\n> mode and was due to the same limitation(s) of the OS.\n>\n> [1]: https://lore.kernel.org/git/20211209051115.52629-3-sunshine@sunshineco.com/\n\nAhhhh.  That one gives the official excuse to apply the band-aid.\nYou quoted from their documentation\n\n    Do not end a file or directory name with a space or a period.\n    Although the underlying file system may support such names, the\n    Windows shell and user interface does not.\n\nAs this test _is_, unlike the cited patch that was not about a\ndirectory with a funny name, about parsing a patch and applying it\nto a path with a directory with a funny name, I am tempted to keep\nthe test with the filesystem, instead of replacing it with the one\nusing the \"--cached\" that Peff suggested.  I am _also_ tempted to\nadd that \"--cached\" thing (instead of replacing), though.\n\nThanks\n"},{"id":"491784","messageId":"xmqqh6gqt674.fsf_-_@gitster.g","threadId":"61150","inReplyTo":"xmqqa5miuutd.fsf@gitster.g","subject":"[PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-28T21:08:47Z","receivedAt":"2024-03-28T21:08:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As afb31ad9 (t1010: fix unnoticed failure on Windows, 2021-12-11)\nsaid:\n\n    On Microsoft Windows, a directory name should never end with a period.\n    Quoting from Microsoft documentation[1]:\n\n\tDo not end a file or directory name with a space or a period.\n\tAlthough the underlying file system may support such names, the\n\tWindows shell and user interface does not.\n\n    [1]: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file\n\nand the condition addressed by this change is exactly that.  If the\nplatform is unable to properly create these sample patches about a\nfile that lives in a directory whose name ends with a SP, there is\nno point testing how \"git apply\" behaves there on the filesystem.\n\nEven though the ultimate purpose of \"git apply\" is to apply a patch\nand to update the filesystem entities, this particular test is\nmainly about parsing a patch on a funny pathname correctly, and even\non a system that is incapable of checking out the resulting state\ncorrectly on its filesystem, at least the parsing can and should work\nfine.  Rewrite the test to work inside the index without touching the\nfilesystem.\n\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n    Junio C Hamano <gitster@pobox.com> writes:\n\n    > As this test _is_, unlike the cited patch that was not about a\n    > directory with a funny name, about parsing a patch and applying it\n    > to a path with a directory with a funny name, I am tempted to keep\n    > the test with the filesystem, instead of replacing it with the one\n    > using the \"--cached\" that Peff suggested.  I am _also_ tempted to\n    > add that \"--cached\" thing (instead of replacing), though.\n\n    So, I changed my mind and just took Peff's \"--cached\" approach\n    with no filesystem-based test.  format-patch --range-diff just\n    didn't understand that the single patch corresponds to the only\n    one patch in the older \"series\", and I had to force it to match\n    them with --creation-factor=999 in a separate invocation.  The\n    patch text has changed too much so it is useless, but the log\n    message change may be easier to see in the range-diff.\n\n1:  7e84d0f64f ! 1:  a107f21ea2 t4126: make sure a directory with SP at the end is usable\n    @@ Commit message\n         and the condition addressed by this change is exactly that.  If the\n         platform is unable to properly create these sample patches about a\n         file that lives in a directory whose name ends with a SP, there is\n    -    no point testing how \"git apply\" behaves there.\n    +    no point testing how \"git apply\" behaves there on the filesystem.\n     \n    -    Protect the test that involves the filesystem access with a\n    -    prerequisite, and perform the same test only within the index\n    -    everywhere.\n    +    Even though the ultimate purpose of \"git apply\" is to apply a patch\n    +    and to update the filesystem entities, this particular test is\n    +    mainly about parsing a patch on a funny pathname correctly, and even\n    +    on a system that is incapable of checking out the resulting state\n    +    correctly on its filesystem, at least the parsing can and should work\n    +    fine.  Rewrite the test to work inside the index without touching the\n    +    filesystem.\n     \n         Helped-by: Jeff King <peff@peff.net>\n         Helped-by: Eric Sunshine <sunshine@sunshineco.com>\n    @@ t/t4126-apply-empty.sh: test_expect_success 'apply --index create' '\n      '\n      \n     -test_expect_success 'apply with no-contents and a funny pathname' '\n    -+test_expect_success 'setup patches in dir ending in SP' '\n    -+\ttest_when_finished \"rm -fr \\\"funny \\\"\" &&\n    - \tmkdir \"funny \" &&\n    - \t>\"funny /empty\" &&\n    - \tgit add \"funny /empty\" &&\n    +-\tmkdir \"funny \" &&\n    +-\t>\"funny /empty\" &&\n    +-\tgit add \"funny /empty\" &&\n     -\tgit diff HEAD \"funny /\" >sample.patch &&\n     -\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n    -+\tgit diff HEAD -- \"funny /\" >sample.patch &&\n    -+\tgit diff -R HEAD -- \"funny /\" >elpmas.patch &&\n    ++test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n      \tgit reset --hard &&\n     -\trm -fr \"funny \" &&\n    -+\n    -+\tif  grep \"a/funny /empty b/funny /empty\" sample.patch &&\n    -+\t    grep \"b/funny /empty a/funny /empty\" elpmas.patch\n    -+\tthen\n    -+\t\ttest_set_prereq DIR_ENDS_WITH_SP\n    -+\telse\n    -+\t\t# Win test???\n    -+\t\tls -l\n    -+\tfi\n    -+'\n    -+\n    -+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '\n    -+\ttest_when_finished \"rm -fr \\\"funny \\\"\" &&\n    - \n    - \tgit apply --stat --check --apply sample.patch &&\n    - \ttest_must_be_empty \"funny /empty\" &&\n    -@@ t/t4126-apply-empty.sh: test_expect_success 'apply with no-contents and a funny pathname' '\n    - \ttest_path_is_missing \"funny /empty\"\n    - '\n    - \n    -+test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n    -+\tgit reset --hard &&\n    -+\n     +\tempty_blob=$(test_oid empty_blob) &&\n    -+\techo $empty_blob >expect &&\n    -+\n    ++\techo \"$empty_blob\" >expect &&\n    + \n    +-\tgit apply --stat --check --apply sample.patch &&\n    +-\ttest_must_be_empty \"funny /empty\" &&\n     +\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n     +\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n     +\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n     +\tgit reset &&\n    -+\n    + \n    +-\tgit apply --stat --check --apply elpmas.patch &&\n    +-\ttest_path_is_missing \"funny /empty\" &&\n     +\tgit apply --cached --stat --check --apply sample.patch &&\n     +\tgit rev-parse --verify \":funny /empty\" >actual &&\n     +\ttest_cmp expect actual &&\n    -+\n    + \n    +-\tgit apply -R --stat --check --apply elpmas.patch &&\n    +-\ttest_must_be_empty \"funny /empty\" &&\n     +\tgit apply --cached --stat --check --apply elpmas.patch &&\n     +\ttest_must_fail git rev-parse --verify \":funny /empty\" &&\n    -+\n    + \n    +-\tgit apply -R --stat --check --apply sample.patch &&\n    +-\ttest_path_is_missing \"funny /empty\"\n     +\tgit apply -R --cached --stat --check --apply elpmas.patch &&\n     +\tgit rev-parse --verify \":funny /empty\" >actual &&\n     +\ttest_cmp expect actual &&\n     +\n     +\tgit apply -R --cached --stat --check --apply sample.patch &&\n     +\ttest_must_fail git rev-parse --verify \":funny /empty\"\n    -+'\n    -+\n    + '\n    + \n      test_done\n\n t/t4126-apply-empty.sh | 33 ++++++++++++++++++---------------\n 1 file changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex eaf0c5304a..2462cdf904 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -66,26 +66,29 @@ test_expect_success 'apply --index create' '\n \tgit diff --exit-code\n '\n \n-test_expect_success 'apply with no-contents and a funny pathname' '\n-\tmkdir \"funny \" &&\n-\t>\"funny /empty\" &&\n-\tgit add \"funny /empty\" &&\n-\tgit diff HEAD \"funny /\" >sample.patch &&\n-\tgit diff -R HEAD \"funny /\" >elpmas.patch &&\n+test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n \tgit reset --hard &&\n-\trm -fr \"funny \" &&\n+\tempty_blob=$(test_oid empty_blob) &&\n+\techo \"$empty_blob\" >expect &&\n \n-\tgit apply --stat --check --apply sample.patch &&\n-\ttest_must_be_empty \"funny /empty\" &&\n+\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n+\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n+\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n+\tgit reset &&\n \n-\tgit apply --stat --check --apply elpmas.patch &&\n-\ttest_path_is_missing \"funny /empty\" &&\n+\tgit apply --cached --stat --check --apply sample.patch &&\n+\tgit rev-parse --verify \":funny /empty\" >actual &&\n+\ttest_cmp expect actual &&\n \n-\tgit apply -R --stat --check --apply elpmas.patch &&\n-\ttest_must_be_empty \"funny /empty\" &&\n+\tgit apply --cached --stat --check --apply elpmas.patch &&\n+\ttest_must_fail git rev-parse --verify \":funny /empty\" &&\n \n-\tgit apply -R --stat --check --apply sample.patch &&\n-\ttest_path_is_missing \"funny /empty\"\n+\tgit apply -R --cached --stat --check --apply elpmas.patch &&\n+\tgit rev-parse --verify \":funny /empty\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit apply -R --cached --stat --check --apply sample.patch &&\n+\ttest_must_fail git rev-parse --verify \":funny /empty\"\n '\n \n test_done\n-- \n2.44.0-368-gc75fd8d815\n"},{"id":"491798","messageId":"xmqqil15srub.fsf@gitster.g","threadId":"61150","inReplyTo":"xmqqh6gqt674.fsf_-_@gitster.g","subject":"Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T02:18:52Z","receivedAt":"2024-03-29T02:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> +test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n>  \tgit reset --hard &&\n> +\tempty_blob=$(test_oid empty_blob) &&\n> +\techo \"$empty_blob\" >expect &&\n>  \n> +\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n\nIt seems that on Windows, this step fails with \"funny /empty\" as\n\"invalid path\".\n\nhttps://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244\n\nSo I'll have to redo this step; unfortunately I think it is already\nin 'next', so an additional patch needs to resurrect that prerequisite\ntrick.\n\nSorry for breaking CI for 'next'.\n"},{"id":"491813","messageId":"xmqqwmplvbsa.fsf_-_@gitster.g","threadId":"61150","inReplyTo":"xmqqil15srub.fsf@gitster.g","subject":"[PATCH] t4126: fix \"funny directory name\" test on Windows (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T05:37:25Z","receivedAt":"2024-03-29T05:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though \"git update-index --cacheinfo\" ought to be filesystem\nagnostic, somehow\n\n    $ git update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\"\n\nfails only there.  That unfortunately makes the approach of the\nprevious step unworkable.\n\nResurrect the earlier approach to protect the test with a\nprerequisite to make sure we do not needlessly fail the CI.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t4126-apply-empty.sh | 43 +++++++++++++++++++++++++-----------------\n 1 file changed, 26 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex 2462cdf904..d2ac7a486f 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -66,29 +66,38 @@ test_expect_success 'apply --index create' '\n \tgit diff --exit-code\n '\n \n-test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n+test_expect_success 'setup patches in dir ending in SP' '\n+\ttest_when_finished \"rm -fr \\\"funny \\\"\" &&\n+\tmkdir \"funny \" &&\n+\t>\"funny /empty\" &&\n+\tgit add \"funny /empty\" &&\n+\tgit diff HEAD -- \"funny /\" >sample.patch &&\n+\tgit diff -R HEAD -- \"funny /\" >elpmas.patch &&\n \tgit reset --hard &&\n-\tempty_blob=$(test_oid empty_blob) &&\n-\techo \"$empty_blob\" >expect &&\n \n-\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n-\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n-\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n-\tgit reset &&\n+\tif  grep \"a/funny /empty b/funny /empty\" sample.patch &&\n+\t    grep \"b/funny /empty a/funny /empty\" elpmas.patch\n+\tthen\n+\t\ttest_set_prereq DIR_ENDS_WITH_SP\n+\telse\n+\t\t# Win test???\n+\t\tls -l\n+\tfi\n+'\n+\n+test_expect_success DIR_ENDS_WITH_SP 'apply with no-contents and a funny pathname' '\n \n-\tgit apply --cached --stat --check --apply sample.patch &&\n-\tgit rev-parse --verify \":funny /empty\" >actual &&\n-\ttest_cmp expect actual &&\n+\tgit apply --stat --check --apply sample.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n \n-\tgit apply --cached --stat --check --apply elpmas.patch &&\n-\ttest_must_fail git rev-parse --verify \":funny /empty\" &&\n+\tgit apply --stat --check --apply elpmas.patch &&\n+\ttest_path_is_missing \"funny /empty\" &&\n \n-\tgit apply -R --cached --stat --check --apply elpmas.patch &&\n-\tgit rev-parse --verify \":funny /empty\" >actual &&\n-\ttest_cmp expect actual &&\n+\tgit apply -R --stat --check --apply elpmas.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n \n-\tgit apply -R --cached --stat --check --apply sample.patch &&\n-\ttest_must_fail git rev-parse --verify \":funny /empty\"\n+\tgit apply -R --stat --check --apply sample.patch &&\n+\ttest_path_is_missing \"funny /empty\"\n '\n \n test_done\n-- \n2.44.0-413-gd6fd04375f\n\n"},{"id":"491827","messageId":"20240329112730.GA15842@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqqil15srub.fsf@gitster.g","subject":"Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-29T11:27:30Z","receivedAt":"2024-03-29T11:27:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n> >  \tgit reset --hard &&\n> > +\tempty_blob=$(test_oid empty_blob) &&\n> > +\techo \"$empty_blob\" >expect &&\n> >  \n> > +\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n> \n> It seems that on Windows, this step fails with \"funny /empty\" as\n> \"invalid path\".\n> \n> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244\n\nAh, sorry, I didn't actually try my suggestion on Windows. I guess we\nare falling afoul of verify_path(), which calls is_valid_path(). That is\na noop on most platforms, but is_valid_win32_path() has:\n\n                  switch (c) {\n                  case '\\0':\n                  case '/': case '\\\\':\n                          /* cannot end in ` ` or `.`, except for `.` and `..` */\n                          if (preceding_space_or_period &&\n                              (i != periods || periods > 2))\n                                  return 0;\n\nI'm mildly surprised that we did not hit the same problem via \"git add\".\nBut I think it does indeed call verify_path(). It's just that the\nfilesystem confusion prevented us from even seeing the path in the first\nplace, and we never got that far.\n\nIt's interesting that there is no way to override this check via\nupdate-index, etc (like we have \"--literally\" for hash-object when you\nwant to do something stupid). I think it would be sufficient to make\nthings work everywhere for this test case. On the other hand, if you\nhave to resort to \"please add this index entry which is broken on my\nfilesystem\" to run the test, maybe that is a good sign it should just be\nskipped on that platform. ;)\n\n-Peff\n"},{"id":"491829","messageId":"20240329120038.GB15842@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqqwmplvbsa.fsf_-_@gitster.g","subject":"Re: [PATCH] t4126: fix \"funny directory name\" test on Windows (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-29T12:00:38Z","receivedAt":"2024-03-29T12:00:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 28, 2024 at 10:37:25PM -0700, Junio C Hamano wrote:\n\n> Even though \"git update-index --cacheinfo\" ought to be filesystem\n> agnostic, somehow\n> \n>     $ git update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\"\n> \n> fails only there.  That unfortunately makes the approach of the\n> previous step unworkable.\n> \n> Resurrect the earlier approach to protect the test with a\n> prerequisite to make sure we do not needlessly fail the CI.\n\nI think this is a reasonable path forward. Looking at the patch\nitself...\n\n> -test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n> +test_expect_success 'setup patches in dir ending in SP' '\n> +\ttest_when_finished \"rm -fr \\\"funny \\\"\" &&\n> +\tmkdir \"funny \" &&\n> +\t>\"funny /empty\" &&\n> +\tgit add \"funny /empty\" &&\n> +\tgit diff HEAD -- \"funny /\" >sample.patch &&\n> +\tgit diff -R HEAD -- \"funny /\" >elpmas.patch &&\n>  \tgit reset --hard &&\n> -\tempty_blob=$(test_oid empty_blob) &&\n> -\techo \"$empty_blob\" >expect &&\n>  \n> -\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n> -\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n> -\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n> -\tgit reset &&\n> +\tif  grep \"a/funny /empty b/funny /empty\" sample.patch &&\n> +\t    grep \"b/funny /empty a/funny /empty\" elpmas.patch\n> +\tthen\n> +\t\ttest_set_prereq DIR_ENDS_WITH_SP\n> +\telse\n> +\t\t# Win test???\n> +\t\tls -l\n> +\tfi\n> +'\n\nIt is a little funny that we set our prereq based only on the emptiness\nof the patches. That is certainly what happens on Windows, but it is a\nweird thing to expect. It implies that \"mkdir\" and \"git add\" returned\nsuccess, but the latter without actually adding the index entry. That's\nwhat happens now, but given that such paths are forbidden by the\nfilesystem, I'm not sure it's a good thing to rely on.\n\nIf we think that Windows is the only problematic platform, should we\njust use !MINGW as the prereq?\n\nIf we think there may be other platforms and would rather test the\nactual behavior, then we should probably be more careful. If \"mkdir\"\nfails above, we'd fail the test, rather than just not set the prereq.\nTo solve that you can put the whole &&-chain into an if, but probably\nusing a lazy prereq block might be more readable. I guess it is a little\ninefficient, though, because we have to actually add/diff to see if\nthings are working.\n\nIMHO just using !MINGW would be simple, efficient, and effective\n(especially with \"--cached\", where we know that the problem is not the\nfilesystem but our own is_valid_win32_path()).\n\nOne other possible variant: we could skip the add/diff altogether and\njust include the patch as a test vector. After all, people on Windows\ncould be sent such a patch without regard to their filesystem. That\ndoesn't solve the whole issue, though, as \"git apply\" would fail (even\nwith --cached) because of the verify_path() call.\n\nBut you could still check the error output to confirm that we parsed the\npatch correctly. That lets every platform check the main bug fix, and\nmore capable platforms test the whole process.\n\nLike:\n\n-- >8 --\napply_funny_path () {\n\texpect=$1; shift\n\tpath=$1; shift\n\tif git apply --cached --stat --check --apply \"$@\" 2>err\n\tthen\n\t\tif test \"$expect\" = \"missing\"\n\t\tthen\n\t\t\ttest_must_fail git rev-parse --verify \"$path\"\n\t\telse\n\t\t\tgit rev-parse --verify \":$path\" >actual &&\n\t\t\techo \"$expect\" >expect &&\n\t\t\ttest_cmp expect actual\n\t\tfi\n\telse\n\t\t# some platforms (like Windows) do not allow path entries with\n\t\t# trailing spaces, even just in the index. But we should\n\t\t# at least be able to verify that we parsed the patch\n\t\t# correctly.\n\t\tif test \"$expect\" = \"missing\"\n\t\tthen\n\t\t\techo \"error: $path: does not exist in index\"\n\t\telse\n\t\t\techo \"error: invalid path '$path'\"\n\t\tfi >expect &&\n\t\ttest_cmp expect err\n\tfi\n}\n\ntest_expect_success 'apply with no-contents and a funny pathname' '\n\tempty=$(git rev-parse --verify :empty) &&\n\tcat >sample.patch <<-EOF &&\n\tdiff --git a/funny /empty b/funny /empty\n\tnew file mode 100644\n\tindex 0000000..$empty\n\tEOF\n\tcat >elpmas.patch <<-EOF &&\n\tdiff --git b/funny /empty a/funny /empty\n\tdeleted file mode 100644\n\tindex e69de29..$empty\n\tEOF\n\n\tapply_funny_path $empty \"funny /empty\" sample.patch &&\n\tapply_funny_path missing \"funny /empty\" elpmas.patch &&\n\tapply_funny_path $empty \"funny /empty\" -R elpmas.patch &&\n\tapply_funny_path missing \"funny /empty\" -R sample.patch\n'\n-- 8< --\n\nI didn't test it on Windows, but I did check that it does the right\nthing with a manual:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex d1aef437aa..b75aeb0be8 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -976,6 +976,8 @@ static enum verify_path_result verify_path_internal(const char *path,\n \tif (has_dos_drive_prefix(path))\n \t\treturn PATH_INVALID;\n \n+\tif (strchr(path, ' '))\n+\t\treturn PATH_INVALID;\n \tif (!is_valid_path(path))\n \t\treturn PATH_INVALID;\n \n\n-Peff\n"},{"id":"491832","messageId":"xmqqplvd0y6c.fsf@gitster.g","threadId":"61150","inReplyTo":"20240329112730.GA15842@coredump.intra.peff.net","subject":"Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T17:01:47Z","receivedAt":"2024-03-29T17:01:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n>> >  \tgit reset --hard &&\n>> > +\tempty_blob=$(test_oid empty_blob) &&\n>> > +\techo \"$empty_blob\" >expect &&\n>> >  \n>> > +\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n>> \n>> It seems that on Windows, this step fails with \"funny /empty\" as\n>> \"invalid path\".\n>> \n>> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244\n>\n> Ah, sorry, I didn't actually try my suggestion on Windows. I guess we\n> are falling afoul of verify_path(), which calls is_valid_path(). That is\n> a noop on most platforms, but is_valid_win32_path() has:\n>\n>                   switch (c) {\n>                   case '\\0':\n>                   case '/': case '\\\\':\n>                           /* cannot end in ` ` or `.`, except for `.` and `..` */\n>                           if (preceding_space_or_period &&\n>                               (i != periods || periods > 2))\n>                                   return 0;\n\nYes, and no need to say sorry.  I was also surprised, as I thought\nthat the non working tree operations ought to be platform\nindependenty, with this.\n\n> It's interesting that there is no way to override this check via\n> update-index, etc (like we have \"--literally\" for hash-object when you\n> want to do something stupid). I think it would be sufficient to make\n> things work everywhere for this test case. On the other hand, if you\n> have to resort to \"please add this index entry which is broken on my\n> filesystem\" to run the test, maybe that is a good sign it should just be\n> skipped on that platform. ;)\n\nThis is a far-away tangent but we may want to think about \"the core\nof Git made into a library that works only with the objects in the\nobject-store and does not deal with working trees\".  To work with\nthe objects, we would probably need something like the index that is\nused in the original sense of the word (a database you consult with\na pathname as a key and obtain the object name with mode bits and a\nstage number), etc.  Elijah's merge-tree may fit well within the\nscheme.\n\nThere is no place like the above code in such a world.  The\nrestriction must exist somewhere to protect the users that use on a\nlimited system, but should come in a layer far above that \"core\nlibrary\".\n\nAnyway, I think you convinced me in the other response that we\nshould just use an existing prerequisite, perhaps FUNNYNAMES.  The\nidea is to exclude platforms that are known to break with the test\nwithout any hope of fix.  Because they are incapable of taking their\nusers into the problematic state being tested in the first place,\nthis is not making things any worse.\n\n\n"},{"id":"491833","messageId":"xmqq5xx50x8p.fsf_-_@gitster.g","threadId":"61150","inReplyTo":"xmqqwmplvbsa.fsf_-_@gitster.g","subject":"[PATCH v2] t4126: fix \"funny directory name\" test on Windows (again)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T17:21:58Z","receivedAt":"2024-03-29T17:22:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though \"git update-index --cacheinfo\" ought to be filesystem\nagnostic,\n\n    $ git update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\"\n\nfails only on Windows, and this unfortunately makes the approach of\nthe previous step unworkable.\n\nResurrect the earlier approach to give up on running the test on\nknown-bad platforms.  Instead of computing a custom prerequisite,\njust use !MINGW we have used elsewhere.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n Another reason for using MINGW is that the custom prerequisite\n would not have been a good match for lazy_prereq mechanism, which\n wants to isolate itself by creating a temporary directory to run\n the test for prerequisites, which means we are not expected use the\n main index or object store to test for prerequisites, either, which\n in turn means we are pretty much forbidden from using Git while\n computing the prerequisite.  \"a platform fails the prerequisite if\n the steps to create sample patches do not work\" was how the earlier\n step computed the custom prerequisite, which cannot be done without\n creating another repository in the temporary place given, which\n means we cannot reuse the patches created in the real test.\n\n Also, if a platform other than MINGW fails the early part of this\n test, we would want to _know_ about it, even if we may not want to\n fix it.  A custom prerequisite will defeat that.\n\n t/t4126-apply-empty.sh | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nindex 2462cdf904..56210b5609 100755\n--- a/t/t4126-apply-empty.sh\n+++ b/t/t4126-apply-empty.sh\n@@ -66,29 +66,28 @@ test_expect_success 'apply --index create' '\n \tgit diff --exit-code\n '\n \n-test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n-\tgit reset --hard &&\n-\tempty_blob=$(test_oid empty_blob) &&\n-\techo \"$empty_blob\" >expect &&\n+test_expect_success !MINGW 'apply with no-contents and a funny pathname' '\n+\ttest_when_finished \"rm -fr \\\"funny \\\"; git reset --hard\" &&\n+\n+\tmkdir \"funny \" &&\n+\t>\"funny /empty\" &&\n+\tgit add \"funny /empty\" &&\n+\tgit diff HEAD -- \"funny /\" >sample.patch &&\n+\tgit diff -R HEAD -- \"funny /\" >elpmas.patch &&\n \n-\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n-\tgit diff --cached HEAD -- \"funny /\" >sample.patch &&\n-\tgit diff --cached -R HEAD -- \"funny /\" >elpmas.patch &&\n-\tgit reset &&\n+\tgit reset --hard &&\n \n-\tgit apply --cached --stat --check --apply sample.patch &&\n-\tgit rev-parse --verify \":funny /empty\" >actual &&\n-\ttest_cmp expect actual &&\n+\tgit apply --stat --check --apply sample.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n \n-\tgit apply --cached --stat --check --apply elpmas.patch &&\n-\ttest_must_fail git rev-parse --verify \":funny /empty\" &&\n+\tgit apply --stat --check --apply elpmas.patch &&\n+\ttest_path_is_missing \"funny /empty\" &&\n \n-\tgit apply -R --cached --stat --check --apply elpmas.patch &&\n-\tgit rev-parse --verify \":funny /empty\" >actual &&\n-\ttest_cmp expect actual &&\n+\tgit apply -R --stat --check --apply elpmas.patch &&\n+\ttest_must_be_empty \"funny /empty\" &&\n \n-\tgit apply -R --cached --stat --check --apply sample.patch &&\n-\ttest_must_fail git rev-parse --verify \":funny /empty\"\n+\tgit apply -R --stat --check --apply sample.patch &&\n+\ttest_path_is_missing \"funny /empty\"\n '\n \n test_done\n-- \n2.44.0-413-gd6fd04375f\n\n"},{"id":"491840","messageId":"20240329183431.GB31800@coredump.intra.peff.net","threadId":"61150","inReplyTo":"xmqq5xx50x8p.fsf_-_@gitster.g","subject":"Re: [PATCH v2] t4126: fix \"funny directory name\" test on Windows (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-29T18:34:31Z","receivedAt":"2024-03-29T18:34:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2024 at 10:21:58AM -0700, Junio C Hamano wrote:\n\n> Even though \"git update-index --cacheinfo\" ought to be filesystem\n> agnostic,\n> \n>     $ git update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\"\n> \n> fails only on Windows, and this unfortunately makes the approach of\n> the previous step unworkable.\n> \n> Resurrect the earlier approach to give up on running the test on\n> known-bad platforms.  Instead of computing a custom prerequisite,\n> just use !MINGW we have used elsewhere.\n\nThanks, this looks good to me. You mentioned FUNNYNAMES earlier (which I\nforgot even existed). That would probably work in practice, but it is\nkind of overloaded already. I think using MINGW here gets to the point,\nand as you note, if some other platforms fails we'd want to hear about\nit.\n\n-Peff\n"},{"id":"493540","messageId":"386aecc6-d94b-3f76-9a11-f05c68fb6767@gmx.de","threadId":"61150","inReplyTo":"xmqqplvd0y6c.fsf@gitster.g","subject":"Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-04-27T14:47:50Z","receivedAt":"2024-04-27T14:48:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 29 Mar 2024, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n>\n> > On Thu, Mar 28, 2024 at 07:18:52PM -0700, Junio C Hamano wrote:\n> >\n> >> Junio C Hamano <gitster@pobox.com> writes:\n> >>\n> >> > +test_expect_success 'parsing a patch with no-contents and a funny pathname' '\n> >> >  \tgit reset --hard &&\n> >> > +\tempty_blob=$(test_oid empty_blob) &&\n> >> > +\techo \"$empty_blob\" >expect &&\n> >> >\n> >> > +\tgit update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\" &&\n> >>\n> >> It seems that on Windows, this step fails with \"funny /empty\" as\n> >> \"invalid path\".\n> >>\n> >> https://github.com/git/git/actions/runs/8475098601/job/23222724707#step:6:244\n> >\n> > Ah, sorry, I didn't actually try my suggestion on Windows. I guess we\n> > are falling afoul of verify_path(), which calls is_valid_path(). That is\n> > a noop on most platforms, but is_valid_win32_path() has:\n> >\n> >                   switch (c) {\n> >                   case '\\0':\n> >                   case '/': case '\\\\':\n> >                           /* cannot end in ` ` or `.`, except for `.` and `..` */\n> >                           if (preceding_space_or_period &&\n> >                               (i != periods || periods > 2))\n> >                                   return 0;\n>\n> Yes, and no need to say sorry.  I was also surprised, as I thought\n> that the non working tree operations ought to be platform\n> independenty, with this.\n>\n> > It's interesting that there is no way to override this check via\n> > update-index, etc (like we have \"--literally\" for hash-object when you\n> > want to do something stupid). I think it would be sufficient to make\n> > things work everywhere for this test case. On the other hand, if you\n> > have to resort to \"please add this index entry which is broken on my\n> > filesystem\" to run the test, maybe that is a good sign it should just be\n> > skipped on that platform. ;)\n>\n> This is a far-away tangent but we may want to think about \"the core\n> of Git made into a library that works only with the objects in the\n> object-store and does not deal with working trees\".  To work with\n> the objects, we would probably need something like the index that is\n> used in the original sense of the word (a database you consult with\n> a pathname as a key and obtain the object name with mode bits and a\n> stage number), etc.  Elijah's merge-tree may fit well within the\n> scheme.\n>\n> There is no place like the above code in such a world.  The\n> restriction must exist somewhere to protect the users that use on a\n> limited system, but should come in a layer far above that \"core\n> library\".\n\nIndeed, it should have been at another layer, but alas, I could not find a\n_better_ layer back when.\n\nBTW it _is_ possible to override this check. This invocation works:\n\n$ git -c core.protectNTFS=false update-index --add --cacheinfo \"100644,$empty_blob,funny /empty\"\n\nIt has been on my radar for a long time that in particular with sparse\ncheckouts, this check is overzealous.\n\nI would have loved to work on it, and once I find a position where I am\nfunded to work meaningfully on Git for Windows again, I will.\n\n> Anyway, I think you convinced me in the other response that we\n> should just use an existing prerequisite, perhaps FUNNYNAMES.  The\n> idea is to exclude platforms that are known to break with the test\n> without any hope of fix.  Because they are incapable of taking their\n> users into the problematic state being tested in the first place,\n> this is not making things any worse.\n\nThat was indeed the correct thing to do, as far as I am concerned.\n\nThank you for fixing this, and sorry that I was not able to contribute\nmeaningfully to the fix.\n\nCiao,\nJohannes\n"},{"id":"493544","messageId":"xmqqmspe67t0.fsf@gitster.g","threadId":"61150","inReplyTo":"386aecc6-d94b-3f76-9a11-f05c68fb6767@gmx.de","subject":"Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-27T17:20:59Z","receivedAt":"2024-04-27T17:21:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Indeed, it should have been at another layer, but alas, I could not find a\n> _better_ layer back when.\n> ...\n> I would have loved to work on it, and once I find a position where I am\n> funded to work meaningfully on Git for Windows again, I will.\n\nWell, I would think you are working meaningfully on GfW.  Putting\nthat logic somewhere is what GfW person needed to do.  Putting it in\na layer (if there is no existing one, inventing a layer for it and\nproperly rearranging the systme) is what a \"libified Git\" minded\nperson may want to have, but that is far beyond the scope of working\nmeaningfully on GfW.  That is one of the things \"libified Git\"\npeople would need to do.  And if the \"libified Git\" folks do not do\nit themselves but ask for help from GfW folks for their area\nexpertise, that is a perfectly acceptable way for \"libified Git\" to\nbehave, too---after all making \"Git as a whole\" a better system is a\nteam effort.\n\nThanks.\n\n"}]}