Re: [PATCH v3 0/6] odb/files: reload alternates on retry again

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

H
Patrick Steinhardt <ps@pks.im> writes:

> 3:  da5a24624e8 = 3:  8882555124e odb/source-files: reload alternates on retry
> 4:  13e4e857f68 ! 4:  64d76b0ac94 odb/source-files: simplify directory duplication detection
>     @@ Commit message
>              Wall Time    266.5 ms  ±     2.8 ms     262.6 ms  …   276.7 ms            -7.8%
>              Memory         5.9 MiB ±     0.1 MiB      5.6 MiB …     6.1 MiB           +5.1%
>      
>     +    Rename `dirs_paths_icase` to `dir_paths_iscase` to match the naming of
>     +    the newly added set.
>     +
>          Signed-off-by: Patrick Steinhardt <ps@pks.im>
>      
>       ## odb/source-files.c ##
>     @@ odb/source-files.c: static bool odb_files_dir_is_usable(struct odb_source_files
>       	/*
>      -	 * Prevent the common mistake of listing the same
>      -	 * thing twice, or object directory itself.
>     -+	 * Prevent the common mistake of listing the same thing twice, or
>     -+	 * object directory itself.
>     ++	 * Prevent the common mistake of listing the same thing twice,
>     ++	 * or object directory itself.
>     ++	 *
>     ++	 * 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.

Good thought process.  "Mountpoint" is misspelled, by the way.

>     ++	 *
>     ++	 *   - The value may be lying, in which case we might
>     ++	 *     deduplicate alternates that are in fact not mapping to the
>     ++	 *     same directory.

This one is much more serious than the previous two.  As an
optimization to avoid repeating work known to produce no new
results, deduplication can tolerate false negatives (e.g., we
thought "DIR" and "Dir" were different and ended up scanning the
same thing twice).  It cannot, however, allow false positives, which
would lead to "missing" objects that only the mistakenly
deduplicated object store could have provided.

>     ++	 * We should investigate whether we can remove this whole
>     ++	 * mechanism outright.
>       	 */

Sure.

I guess the previous round was already almost ready with Christian's
review, and this round addressing his comments, we are pretty much
done with the topic and can mark it as ready for 'next'?

Thanks, all.