git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

Previous: Junio C HamanoNext: Junio C Hamano
Message 22 of 26 in “quote: quote space”
  1. 0/1 quote: quote spaceHan Young, Mar 19, 2024
  2. 1/1 quote: quote spaceHan Young, Mar 19, 2024
  3. Kristoffer HaugsbakkMar 19, 2024
  4. Junio C HamanoMar 19, 2024
  5. Junio C HamanoMar 19, 2024
  6. Junio C HamanoMar 26, 2024
  7. Jeff KingMar 27, 2024
  8. Junio C HamanoMar 27, 2024
  9. Junio C HamanoMar 27, 2024
  10. Jeff KingMar 28, 2024
  11. Jeff KingMar 28, 2024
  12. Eric SunshineMar 28, 2024
  13. Junio C HamanoMar 28, 2024
  14. t4126: make sure a directory with SP at the end is usableJunio C Hamano, Mar 28, 2024
  15. Junio C HamanoMar 29, 2024
  16. t4126: fix "funny directory name" test on Windows (again)Junio C Hamano, Mar 29, 2024
  17. Jeff KingMar 29, 2024
  18. t4126: fix "funny directory name" test on Windows (again)Junio C Hamano, Mar 29, 2024
  19. Jeff KingMar 29, 2024
  20. Jeff KingMar 29, 2024
  21. Junio C HamanoMar 29, 2024
  22. Johannes SchindelinApr 27, 2024
  23. Junio C HamanoApr 27, 2024
  24. Junio C HamanoMar 28, 2024
  25. Jeff KingMar 28, 2024
  26. Junio C HamanoMar 28, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.