Re: [PATCH v3 2/5] odb: decouple source path comparisons from `the_repository`

[Patrick Steinhardt <ps@pks.im>]

On Thu, Aug 20, 2026 at 08:59:46AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > 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.
> >
> >   - They derive case-sensitivity from `repo_ignore_case()`, which
> >     returns a default value in case the repository's configuration has
> >     not been parsed yet. Object database sources may be registered
> >     before that is the case, so the answer may flip depending on when a
> >     source gets registered.
> 
> As you later mention, we can always hash case-insensitively with
> the downside of additional possibilities of hash collisions.  I
> would not be too worried about the hash side, but the above makes
> me wonder what should happen in the eq() function when a repository
> uses object databases living on separate filesystems, some being
> case-insensitive and others being case-sensitive.
> 
> In any case, I wonder if 'core.ignoreCase' should even be a part of
> the repository configuration.  Do we need to support
> configurations where some parts of the repository are backed by a
> case-insensitive filesystem while others are not?  And if so,
> how?  It almost feels as if each of these object database sources
> needs to report "This is the path to my filesystem location, and
> the path may have case-different aliases" and "My path is on a
> case-sensitive filesystem so you do not have to worry about it
> clashing", and we need to compare them accordingly.

In theory you can of course construct cases where we'd need that. But
this whole mechanism is very old already, and I don't know about a
single reported case where it caused problems. So yes, it is imperfect,
but I think we're overcomplicating things that have been working just
fine for the last 20 years in practice.

> > Overall it's quite debatable whether all of this complexity really is
> > worth it, out of two reasons:
> >
> >   - We could linearly search through all sources to find duplicates. But
> >     the mentioned commit cares about cases with thousands of alternates,
> >     and a linear search would of course regress performance quite a bit.
> >     This doesn't really feel like a reasonable case to care about, but I
> >     don't feel comfortable regressing it anyway.
> 
> Linear or hashed, the issue of what the definition of eq() should be
> remains.  Discarding the hash map does not help at all, I suspect.
> Am I missing something?

No, you're not missing anything here. This was more of a "using a
hashmap in general feels overengineered" statement, as you'd typically
only have at most a handful of them anyway.

Doing a couple of string comparisons would likely be more efficient
compared to spinning up the whole hashmap machinery if you really only
have one or two alternates, which is going to be 99% of all the use
cases out there. Having the hashmap probably only starts to make sense
once you have a couple dozen or even hundreds of alternates, so I have a
feeling that we overoptimized for a mostly theoretical scenario here.

I don't feel comfortable removing that mechanism though. There's always
that one person relying on those weird edge cases.

> >   - It's dubious whether we should handle "core.ignoreCase" in the first
> >     place. The downside would be that we might add the same alternate
> >     multiple times with different casing. But this is an edge case, and
> >     it's not even fully fixed because we don't resolve symlinks or
> >     mountpoints, either.
> 
> Do we know if these all come directly from the way the user spelled
> these paths?

We have two different sources, "objects/info/alternates" and
"GIT_ALTERNATE_OBJECT_DIRECTORIES", both of which are parsed via
`parse_alternates()`. That function knows to translate relative paths
into absolute ones and it normalizes the result via `realpath()`. So no,
they're not exactly the same as what the user has provided. But
unfortunately we cannot assume that `realpath()` provides a canonical
representation of the path name, either.

> Unless there is a demon that randomly flips the character case in a
> pathname once it is obtained from the user or readdir() before it
> gets to this code path, an easy way out may be to tell users "don't
> spell the pathnames inconsistently" or its equivalent, "do spell
> them exactly the way readdir() would report on your system", with "if
> you fail to do so, bad things will happen".  I suspect that the bad
> thing in this particular case is merely that a search in the
> alternates is made unnecessarily inefficient due to duplicates, so it
> may be a reasonable alternative.

Yeah. All of this is really just caused by the fact that there is no
platform-agnostic way to check whether two directories are the same
thing. Which is kind of surprising, if you ask me.

> Alternatively, we can even say "your repository cannot span
> filesystems with different case sensitivities"; I am sure there
> would be some users affected by such a declaration, but I do not
> know how much we should care.

I'm hesitant to go there, as that would retroactively introduce
limitations that could break existing use cases that we supported just
fine until now. The proposed patch is carefully trying to not alter any
user-visible behaviour at all, so both before and after this patch the
behaviour with regards to case sensitivity should be exactly the same.
And that current behaviour seems to be working just fine, or otherwise
I assume we'd have seen bug reports in this area.

So I think we shouldn't throw the baby out with the bathwater.

> > +/*
> > + * NEEDSWORK: we're using "core.ignoreCase" to deduplicate alternates that
> > + * _may_ be the same. This requires quite a bit of boilerplate for dubious
> > + * benefit:
> > + *
> > + *   - Duplicating alternates should really only lead to regressed performance.
> > + *
> > + *   - We don't properly resolve symlinks or mointpoints, so we may still end
> > + *     up duplicating alternates.
> > + *
> > + *   - The value may be lying, in which case we might deduplicate alternates
> > + *     that are in fact not mapping to the same directory.
> > + *
> > + * We should investigate whether we can remove this whole mechanism outright.
> > + */
> > +static int odb_source_paths_cmp(struct object_database *o,
> > +				const char *a, const char *b)
> > +{
> > +	if (o->source_paths_icase < 0) {
> > +		int icase = 0;
> > +		repo_config_get_bool(o->repo, "core.ignorecase", &icase);
> 
> I suspect accessing o->repo should be safe even in the
> initialization sequence, simply because "o->repo = repo" is done as
> the first thing in odb_new(), but do we know o->repo->initialized is
> true in this code path?  Refraining from making that call and
> assuming a case senstivie comparison may be necessary when o->repo
> is not yet initialized.

Part of the motivation for why I did all the refactorings in "setup.c"
was to ensure that we always set up the object database (and reference
database) after the repository was fully initialized, including all of
its extensions. So yes, we know that it's fully initialized at this
point in time.

One way to prove this is by doing the following:

diff --git a/odb.c b/odb.c
index 115957e983..2f1cdfd592 100644
--- a/odb.c
+++ b/odb.c
@@ -1063,6 +1063,9 @@ struct object_database *odb_new(struct repository *repo,
 	char *primary_source = NULL, *secondary_sources = NULL;
 	struct object_database *o;
 
+	if (!repo->initialized || !repo->commondir)
+		BUG("repository is not initialized");
+
 	CALLOC_ARRAY(o, 1);
 	o->repo = repo;
 	pthread_mutex_init(&o->replace_mutex, NULL);

That _does_ trigger a test failure, but only in our unit tests because
we don't fully initialize the environment there for t-odb-inmemory. All
the other tests are passing.

I could add that to the patch series as a safety mechanism, but I'm not
sure that's worth it.

Patrick