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

Patrick Steinhardt <[email protected]> writes:

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

I do not, either, and more importantly, removing the hashing
mechanism does not help an iota here to deal with case insensitive
filesystems.

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

Yup.  Which means the simplest way out would be to do a "best
effort" case-insensitive match when there is a hint that the
platform might be using a case insensitive filesystem.

And that in turn gives us a direction to solve this part ...

>> > +/*
>> > + * NEEDSWORK: we're using "core.ignoreCase" to deduplicate alternates that
>> > + * _may_ be the same. This requires quite a bit of boilerplate for dubious
>> > ...
>> > +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.

...which is that, since case-insensitivity support is at most best
effort, we do not really care if o->repo is not initialized.  The
code can stay as-is, and if the user spelled the path to a single
alternate object store using two different cases in two places,
causing the code to treat them as two different entities, the effect
is merely an extra search of the "second copy" (which is guaranteed
to find nothing, after a search in the first copy finds no object
they are looking for)s, a minor performance penalty