Re: [PATCH v2] t4126: make sure a directory with SP at the end is usable
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Apr 27, 2024, 14:47 UTC
- Message-ID
- <386aecc6-d94b-3f76-9a11-f05c68fb6767@gmx.de>
- In-Reply-To
- <xmqqplvd0y6c.fsf@gitster.g>
Hi Junio,
On Fri, 29 Mar 2024, Junio C Hamano wrote:
Show 55 quoted lines
> 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.
Show 6 quoted lines
> 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