Re: [PATCH v4 4/4] submodule: fix case-folding gitdir filesystem colisions

[Adrian Ratiu <adrian.ratiu@collabora.com>]

Hi Junio,

On Mon, 10 Nov 2025, Junio C Hamano <gitster@pobox.com> wrote:
> Adrian Ratiu <adrian.ratiu@collabora.com> writes: 
> 
>> On Sat, 08 Nov 2025, Aaron Schrab <aaron@schrab.com> wrote: 
>>> At 17:05 +0200 07 Nov 2025, Adrian Ratiu 
>>> <adrian.ratiu@collabora.com> wrote:  
>>>>Add a new check in validate_submodule_git_dir() to detect and 
>>>>prevent case-folding filesystem colisions. When this new check 
>>>>is triggered, a stricter casefolding aware URI encoding is 
>>>>used  to percent-encode uppercase characters, e.g. Foo becomes 
>>>>%46oo.  By using this check/retry mechanism the uppercase 
>>>>encoding is  only applied when necessary, so case-sensitive 
>>>>filesystems are  not affected.  
> 
> The .gitdir name munging is a local thing, so it makes sense to 
> do the casefold mitigation only the filesystem is case folding 
> one,

That is correct. Case-sensitive filesystems will stop before 
reaching the "Case 2.2" attempt, because their gitdirs will pass 
validation in the earlier steps.
 
> 
> Your code seems to compare directory names textually, and 
> downcasing the proposed name for some reason, but I am not sure 
> why we need any of these complexity.  Wouldn't it be the matter 
> of actually trying to mkdir(2) the name presented (either "foo" 
> or "Foo") and see if that fails?  If it fails (most likely with 
> EEXIST if case folding is getting in the way, but for any 
> reason), the name is unusable and we need to "tweak" the name to 
> a usable one at that point by retrying.  Once we find a usable 
> name, we can remember the fact that we already created a 
> directory for it and reuse that empty directory in the code 
> where we used to do mkdir(2), no?

I tried the mkdir approach. Unfortunately it does not work because 
submodule_name_to_gitdir() is called on all submodule paths: both 
existing or new, valid or non-valid.

So if a dir already exists, we do not know if there is a 
conflict. It might just be a normal valid path verification of an 
existing module.

This is why I had to come up with the double-test using to_lower() 
to compute a common path, canonicalization and checking for 
existence when there is a difference via is_git_directory(). In 
this case there is always a conflict and is the only reliable way 
I colud think of.

I am very much in favor for finding a simpler check, however we 
need something at least as reliable as the current double-test, 
which passes all cases I could think of and all CI tests.
 
> 
>> Maybe we could derive a new path automatically (eg foo2 or 
>> foo_,  suggestions welcome) and use it if valid. This way, 
>> there is no  user intervention. 
>> 
>> Do you have any preference? 
> 
> If adding 'foo' and then an attempt to add 'Foo' will 
> automatically assign a name that does not conflict with 'foo' to 
> the newly added submodule, then the users would expect the same 
> to happen if the order to add them are swapped, wouldn't they?

Yes and it's a valid expectation. :)

I just missed this corner-case which Aaron brought up (many thanks 
again Aaron!).

It's a simple fix which I'll do in v5: if validation fails, we 
just need to come up with another name and retry.

The probability of conflicts decreases exponentially with each 
retry. By the 4-5 retry the probability is so small, at that 
point, if we're that desperate, we might just hash the name or use 
a random string.

> 
> IOW, I do not see why the code wants to treat uppercase and 
> lowercase letters any differently, and suspect that it might be 
> the source of additional complication.  Also, if there is an 
> existing module with a funny path "%46oo", you cannot just 
> encode "Foo" into "%46oo" to avoid crashes with 'foo' and be 
> done anyway, so it feels like we are inviting more bugs by 
> special casing certain paths (and not encoding or checking 
> others).  Don't we have an issue similar to "case folding" in 
> macOS wrt UTF-8 canonicalization, too?  An identical Unicode 
> string may be canonicalized in two ways, so in a presence of a 
> submodule named one way, the other submodule named in the other 
> canonicalization, while their names may be with different byte 
> sequences, cannot co-exist in the same directory next to each 
> other.  "Try to mkdir(2) the new name, and see if it succeeds, 
> and if so use the resulting empty directory" approach would 
> cover that case with the same mechanism as you need to use for 
> case folding filesystems, I would imagine. 

Foo and "%46oo" is another possible conflict, yes, but only when 
both "foo" and "%46oo" already exist. The deeper you go in the 
retry cycles, the more unlikely conflicts become.

We just need to detect this conflict, come up with another name 
and retry. Retry until one sticks.

Again, the probability of conflicts decreases exponentially with 
each retry. :) If we're desperate: hash the name or try a random 
string before giving up and throwing an error, which is "Case 3" 
in the current patch.

(I wrote above why mkdir cannot solve the detection problem, hope 
it makes sense).

I think it's also worth mentioning a key idea of this design: we 
don't need to detect & fix all corner cases. We can iterate and 
improve both the conflict detection and path generation over time, 
since they are opaque to users, who rely only on the resulting 
submodule.name.gitdir config we publish.

I know it seems like a game of "whack-a-mole", but even if we fix 
just half of the possible conflicts, the likeliest ones, we still 
are in a much better situation than before. At some point we're 
approaching diminishing returns, so extremely rare conflicts might 
not even be worth fixing.

I will add a test for this "Foo" + "foo" + "%46oo" in v5 as 
well. :)