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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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