From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Mon, 08 Sep 2025 17:15:54 GMT
Subject: Re: [PATCH v2 07/10] submodule: error out if gitdir name is too long
Message-ID: <874itcoofp.fsf@collabora.com>
In-Reply-To: <20250908155146.GA1308482@coredump.intra.peff.net>

On Mon, 08 Sep 2025, Jeff King <peff@peff.net> wrote:
> On Mon, Sep 08, 2025 at 05:01:14PM +0300, Adrian Ratiu wrote: 
> 
>> Encoding submodule names increases their name size, so there is 
>> an increased risk to hit the max filename length in the gitdir 
>> path.  (the likelihood is still rather small, so it's an 
>> acceptable risk)  This gitdir file-name-too-long corner case 
>> can be be addressed in multiple ways, including sharding or 
>> trimming, however for now, just add the portable logic 
>> (suggested by Peff) to detect the corner case then error out to 
>> avoid comitting to a specific policy (or policies). 
> 
> Thanks, the compat logic here looks reasonable to me. 
> 
> As somebody who has not really been looking into or thought 
> about the topic at all, though, I wondered how necessary 
> pathconf() is here. That is, I can imagine two alternatives: 
> 
>  - just try to use the path, and we either get an error from 
>    open()/mkdir() or we don't. This would end up with roughly 
>    the same outcome as the current code which calls die(), 
>    though it would not help with eventually fulfilling your 
>    TODO. 
> 
>  - set some arbitrary but sane limit (say, 255?). That would 
>  make the 
>    behavior consistent across platforms, though it does mean you 
>    might be prevented from using very long submodule names on 
>    systems that could support it. 
> 
> I dunno. Like I said, this is not a problem I thought a lot 
> about, so feel free to ignore. Mostly I just notice that we have 
> lived for 20+ years without pathconf, I think mostly by 
> following the philosophy of the first bullet point above. 
> 
> -Peff 

I can go either (thrice?) way with this. :) No strong opinion.

1. Just let the OS/filesystem default do its own thing: not doing 
anything is the easiest and perhaps the best approach sometimes.

2. Benefit of hardcoding: we already hardcode NAME_MAX in 
compat/posix.h for platforms which don't have it and fallback to 
it. Might as well use it and print a nice error.

3. The compat layer allows us to at least try and detect the 
corner-case because we slightly increase the risk of hitting it 
(say you have a module name with length >200, encoding it might 
push beyond the limit).

Right now it's worth it mostly for the error msg telling the user 
why the command fails because a default -ENAMETOOLONG might be 
confusing (the plain-text name could be just 200 < NAME_MAX), 
while also opening the door to avoid this situation entirely by 
addressing the TODO.

Again, I'm fine either way.

Since I split this logic into a separate commit, we could even 
drop it now and bring it back later when (if?) we decide to tackle 
the TODO (series is big enough already).

Thanks,
Adrian