| Message ID | xmqqh6gqt674.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 012c8b307d339873bcdbbe95018ccfff904fc501 |
| Headers | show |
| Series | [v2] t4126: make sure a directory with SP at the end is usable | expand |
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'.
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
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.
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
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.
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
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(-)