Re: [PATCH v2 1/4] odb: decouple source path comparisons from `the_repository`

On Mon, Aug 17, 2026 at 07:39:05AM +0200, Patrick Steinhardt wrote:
> On Fri, Aug 14, 2026 at 01:17:24PM -0400, Jeff King wrote:
> > On Wed, Aug 12, 2026 at 11:13:57AM +0200, Patrick Steinhardt wrote:
> > 
> > > When registering alternates we deduplicate object database sources by
> > > their path so that the same source won't be added twice. Ever since
> > > cf2dc1c238 (speed up alt_odb_usable() with many alternates, 2021-07-07)
> > > this duplicate check is backed by a map keyed by the source's path,
> > > using `fspathhash()` and `fspatheq()` as hash and equality functions,
> > > respectively.
> > > 
> > > These functions are problematic in this context for two reasons:
> > > 
> > >   - They implicitly depend on `the_repository` instead of the
> > >     repository that owns the object database.
> > 
> > I'm not even sure that using core.ignorecase here is strictly correct.
> > It is a property of the containing repository, and the filesystem in
> > which it's stored. But there is no guarantee that the alternate
> > directories are in the same repository, or even the same filesystem!
> > 
> > So it is really just a best guess proxy for "this system tends to use or
> > not use case insensitive filesystems[1]". It can be wrong in both
> > directions (failing to suppress duplicates, and suppressing them when
> > they are not actually duplicates).
> > 
> > I wonder how bad it would be if we just always did case-sensitive
> > comparisons and made it the caller's responsibility to spell things
> > consistently.  I guess some names ultimately come from things like
> > "--reference" command-line arguments, so that would depend on user
> > spelling. But having duplicates at all is kind of unlikely (you can't
> > get it from one --reference clone, but rather a complex tree of
> > interwoven repos with shared roots).
> > 
> > How bad is a duplicate alternate? It's a minor performance issue, I'd
> > think. We would add its packs to the list (though hardly ever look
> > through them, as the "first" copy would satisfy most requests, and the
> > unused second copies end up at the back of the MRU list). You'd only pay
> > the extra lookup cost for an object which we fail to find entirely,
> > which is rare-ish (mostly speculative lookups for fetches).
> 
> A performance regression is definitely the most likely change in
> behaviour we might see because of this. One other part I am a bit
> worried about is housekeeping, but I think we should be fine there as we
> only consider the primary source as special.
> 
> I also had the feeling that case insensitivity is quite a bit lacking,
> too. What we're really after is whether two directories are actually the
> exact same path. And whether the path is case-insensitive is only one
> part of that equation, so it's an imperfect metric by itself already.
> 
> Ideally, we should probably use realpath(3p) to at least also resolve
> symlinks. Unfortunately, it's not guaranteed that this function also
> knows to canonicalize casing.
> 
> > And it would fix the unlikely-but-possible opposite case of suppressing
> > a non-duplicate. If you have a repo on a case-insensitive filesystem
> > with two alternates on a case-sensitive system that differ only in case,
> > we erroneously suppress one of them, and commands may fail to find
> > objects we should have. Of course that's super unlikely, which is why
> > nobody has run into it before.
> > 
> > So I kind of wonder if we could just do away with considering case
> > insensitivity here at all. We'd err on the side of correctness in the
> > ambiguous cases, and this code complexity can just go away.
> 
> You will of course be able to craft edge cases where that would be a
> significant regression. But if your alternates file looks like this you
> may be holding it wrong:
> 
>     /path/to/alternate
>     /PATH/TO/ALTERNATE
>     /pAtH/tO/aLtErNaTe
>     /PaTh/To/AlTeRnAtE
> 
> > Alternatively, I think we could probably make the check more thorough in
> > a similar way. Always consider a pair of case-insensitive matches as
> > possible duplicates, and then for each possible duplicate use stat() to
> > check their st_dev and st_ino values. That keeps things cheap for normal
> > cases, and we pay only the stat() before de-duping. It's correct and
> > doesn't rely on the repo, though it is a bit more somewhat complicated
> > code.
> 
> Hm. Weren't there filesystems where `st_ino` and `st_dev` aren't set at
> all? I think that's the case on Windows, which is unfortunately also the
> one where we see case insensitive filesystems by default. So that makes
> it way less effective, as it only works on systems where we typically
> aren't case-insensitive in the first place (except macOS maybe).
> 
> So if we want to go down this path I'm inclined to just unconditionally
> use case sensitive matching and not introduce any secondary machinery.

Thinking about this a bit more: I'd suggest that we leave this out of
this patch and instead document this as a NEEDSWORK area for now. I
_think_ that this proposed refactoring should be generally fine, and I
quite like the simplification that results from it. But the risk for
regression is quite a bit higher compared to the origanal patch that
I've proposed.

Patrick