* [PATCH 0/1] quote: quote space @ 2024-03-19 9:52 Han Young 2024-03-19 9:52 ` [PATCH 1/1] " Han Young 2024-03-19 15:15 ` [PATCH 0/1] " Junio C Hamano 0 siblings, 2 replies; 26+ messages in thread From: Han Young @ 2024-03-19 9:52 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/1] quote: quote space 2024-03-19 9:52 [PATCH 0/1] quote: quote space Han Young @ 2024-03-19 9:52 ` Han Young 2024-03-19 9:59 ` Kristoffer Haugsbakk 2024-03-19 15:15 ` [PATCH 0/1] " Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Han Young @ 2024-03-19 9:52 UTC (permalink / raw) To: git; +Cc: 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/1] quote: quote space 2024-03-19 9:52 ` [PATCH 1/1] " Han Young @ 2024-03-19 9:59 ` Kristoffer Haugsbakk 0 siblings, 0 replies; 26+ messages in thread From: Kristoffer Haugsbakk @ 2024-03-19 9:59 UTC (permalink / raw) To: Han Young; +Cc: git 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-19 9:52 [PATCH 0/1] quote: quote space Han Young 2024-03-19 9:52 ` [PATCH 1/1] " Han Young @ 2024-03-19 15:15 ` Junio C Hamano 2024-03-19 22:56 ` Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-19 15:15 UTC (permalink / raw) To: Han Young; +Cc: git 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-19 15:15 ` [PATCH 0/1] " Junio C Hamano @ 2024-03-19 22:56 ` Junio C Hamano 2024-03-26 21:41 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-19 22:56 UTC (permalink / raw) To: git; +Cc: Han Young 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-19 22:56 ` Junio C Hamano @ 2024-03-26 21:41 ` Junio C Hamano 2024-03-27 9:17 ` Jeff King 2024-03-27 22:11 ` Junio C Hamano 2 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-26 21:41 UTC (permalink / raw) To: git; +Cc: Han Young 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-19 22:56 ` Junio C Hamano 2024-03-26 21:41 ` Junio C Hamano @ 2024-03-27 9:17 ` Jeff King 2024-03-27 14:59 ` Junio C Hamano 2024-03-27 22:11 ` Junio C Hamano 2 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2024-03-27 9:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Han Young 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-27 9:17 ` Jeff King @ 2024-03-27 14:59 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-27 14:59 UTC (permalink / raw) To: Jeff King; +Cc: git, Han Young 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-19 22:56 ` Junio C Hamano 2024-03-26 21:41 ` Junio C Hamano 2024-03-27 9:17 ` Jeff King @ 2024-03-27 22:11 ` Junio C Hamano 2024-03-28 10:32 ` Jeff King 2 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-27 22:11 UTC (permalink / raw) To: git; +Cc: Han Young, Jeff King, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-27 22:11 ` Junio C Hamano @ 2024-03-28 10:32 ` Jeff King 2024-03-28 11:40 ` Jeff King 2024-03-28 16:19 ` [PATCH 0/1] quote: quote space Junio C Hamano 0 siblings, 2 replies; 26+ messages in thread From: Jeff King @ 2024-03-28 10:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Han Young, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 10:32 ` Jeff King @ 2024-03-28 11:40 ` Jeff King 2024-03-28 17:05 ` Eric Sunshine 2024-03-28 16:19 ` [PATCH 0/1] quote: quote space Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Jeff King @ 2024-03-28 11:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Han Young, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 11:40 ` Jeff King @ 2024-03-28 17:05 ` Eric Sunshine 2024-03-28 17:31 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Eric Sunshine @ 2024-03-28 17:05 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git, Han Young, Johannes Schindelin 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/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 17:05 ` Eric Sunshine @ 2024-03-28 17:31 ` Junio C Hamano 2024-03-28 21:08 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-28 17:31 UTC (permalink / raw) To: Eric Sunshine; +Cc: Jeff King, git, Han Young, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-03-28 17:31 ` Junio C Hamano @ 2024-03-28 21:08 ` Junio C Hamano 2024-03-29 2:18 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-28 21:08 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, Jeff King, Han Young, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-03-28 21:08 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Junio C Hamano @ 2024-03-29 2:18 ` Junio C Hamano 2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano 2024-03-29 11:27 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Jeff King 0 siblings, 2 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-29 2:18 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, Jeff King, Han Young, Johannes Schindelin 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'. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] t4126: fix "funny directory name" test on Windows (again) 2024-03-29 2:18 ` Junio C Hamano @ 2024-03-29 5:37 ` Junio C Hamano 2024-03-29 12:00 ` Jeff King 2024-03-29 17:21 ` [PATCH v2] " Junio C Hamano 2024-03-29 11:27 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Jeff King 1 sibling, 2 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-29 5:37 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, Jeff King, Han Young, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] t4126: fix "funny directory name" test on Windows (again) 2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano @ 2024-03-29 12:00 ` Jeff King 2024-03-29 17:21 ` [PATCH v2] " Junio C Hamano 1 sibling, 0 replies; 26+ messages in thread From: Jeff King @ 2024-03-29 12:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Sunshine, Han Young, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2] t4126: fix "funny directory name" test on Windows (again) 2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano 2024-03-29 12:00 ` Jeff King @ 2024-03-29 17:21 ` Junio C Hamano 2024-03-29 18:34 ` Jeff King 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-29 17:21 UTC (permalink / raw) To: git; +Cc: Eric Sunshine, Jeff King, Han Young, Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: fix "funny directory name" test on Windows (again) 2024-03-29 17:21 ` [PATCH v2] " Junio C Hamano @ 2024-03-29 18:34 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2024-03-29 18:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Sunshine, Han Young, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-03-29 2:18 ` Junio C Hamano 2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano @ 2024-03-29 11:27 ` Jeff King 2024-03-29 17:01 ` Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Jeff King @ 2024-03-29 11:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Eric Sunshine, Han Young, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-03-29 11:27 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Jeff King @ 2024-03-29 17:01 ` Junio C Hamano 2024-04-27 14:47 ` Johannes Schindelin 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-29 17:01 UTC (permalink / raw) To: Jeff King; +Cc: git, Eric Sunshine, Han Young, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-03-29 17:01 ` Junio C Hamano @ 2024-04-27 14:47 ` Johannes Schindelin 2024-04-27 17:20 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Johannes Schindelin @ 2024-04-27 14:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Eric Sunshine, Han Young 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable 2024-04-27 14:47 ` Johannes Schindelin @ 2024-04-27 17:20 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2024-04-27 17:20 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, git, Eric Sunshine, Han Young 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 10:32 ` Jeff King 2024-03-28 11:40 ` Jeff King @ 2024-03-28 16:19 ` Junio C Hamano 2024-03-28 16:30 ` Jeff King 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2024-03-28 16:19 UTC (permalink / raw) To: Jeff King; +Cc: git, Han Young, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 16:19 ` [PATCH 0/1] quote: quote space Junio C Hamano @ 2024-03-28 16:30 ` Jeff King 2024-03-28 16:53 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2024-03-28 16:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Han Young, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/1] quote: quote space 2024-03-28 16:30 ` Jeff King @ 2024-03-28 16:53 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2024-03-28 16:53 UTC (permalink / raw) To: Jeff King; +Cc: git, Han Young, Johannes Schindelin 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 ;-) ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2024-04-27 17:21 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-03-19 9:52 [PATCH 0/1] quote: quote space Han Young 2024-03-19 9:52 ` [PATCH 1/1] " Han Young 2024-03-19 9:59 ` Kristoffer Haugsbakk 2024-03-19 15:15 ` [PATCH 0/1] " Junio C Hamano 2024-03-19 22:56 ` Junio C Hamano 2024-03-26 21:41 ` Junio C Hamano 2024-03-27 9:17 ` Jeff King 2024-03-27 14:59 ` Junio C Hamano 2024-03-27 22:11 ` Junio C Hamano 2024-03-28 10:32 ` Jeff King 2024-03-28 11:40 ` Jeff King 2024-03-28 17:05 ` Eric Sunshine 2024-03-28 17:31 ` Junio C Hamano 2024-03-28 21:08 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Junio C Hamano 2024-03-29 2:18 ` Junio C Hamano 2024-03-29 5:37 ` [PATCH] t4126: fix "funny directory name" test on Windows (again) Junio C Hamano 2024-03-29 12:00 ` Jeff King 2024-03-29 17:21 ` [PATCH v2] " Junio C Hamano 2024-03-29 18:34 ` Jeff King 2024-03-29 11:27 ` [PATCH v2] t4126: make sure a directory with SP at the end is usable Jeff King 2024-03-29 17:01 ` Junio C Hamano 2024-04-27 14:47 ` Johannes Schindelin 2024-04-27 17:20 ` Junio C Hamano 2024-03-28 16:19 ` [PATCH 0/1] quote: quote space Junio C Hamano 2024-03-28 16:30 ` Jeff King 2024-03-28 16:53 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.