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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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?

>   - 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?

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.

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.

> +/*
> + * 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.

> +		o->source_paths_icase = icase;
> +	}
> +
> +	return o->source_paths_icase ? strcasecmp(a, b) : strcmp(a, b);
> +}