Re: [PATCH v4 05/25] midx: clear auxiliary .rev after replacing the MIDX

[Taylor Blau <me@ttaylorr.com>]

On Sun, Aug 29, 2021 at 03:56:31PM -0700, Junio C Hamano wrote:
> My recollection is that "--object-dir" is mostly about the alternate
> odb usecase---am I correct?

That matches my understanding. The documentation refers to the value of
this flag as `<alt>`, making me think that supporting non-alternates is
a historical accident.

> I wonder if it is safe to assume that in practice a directory given
> to the "--object-dir" option is always the "objects" subdirectory in
> a repository, and it is an error if there is no "config" file next
> to the directory.  Then, we could check ../config relative to the
> given directory and error out if they use different hash.

Maybe... although I have to admit to not being very excited about it. Is
the idea to read ../config to try and check for any incompatibilities
between the in-core state and the target repository's settings? If so,
this seems like a recipe for catching bugs too late.

For e.g., catching the_hash_algo != target_repository->hash would
definitely squash the bug you saw when integrating, but we would have to
remember to update this spot later on if, say, the target repository
started using a different reference storage backend (since bitmap
generation necessarily iterates the references to figure out which
commits should receive coverage).

> I do not recall offhand how careful link_alt_odb_entries() is, but I
> suspect it isn't at all (back when I invented it, there weren't need
> for configuration to switch between hashes, and since then I do not
> recall seeing any heavy update to the alternate odb code).  Perhaps
> we should tighten it so that we check the accompanying "config" file
> first and ignore the entry with incompatible "hash" (and we may
> later discover other trait on a repository that is incompatible with
> the current one)?

Or are you saying you're concerned about an alternates chain which
don't all use the same object format?

If the former, then I would say:

    "Supporting arbitrary --object-dir when invoked from outside a
    repository is a bug that happened to not cause any problems, but
    is surprising, error-prone, and should fall outside of the burden of
    backwards compatibility, so we should get rid of it."

If the latter, then I agree we could and should do better at detecting
it and providing a helpful error message, but I don't see how doing so
now or later would affect this series. Even if we just disallow
--object-dir pointing at a non-alternate repository, we would still have
the issue of having alternate chains which don't all have the same
object format.

So that makes me feel like the latter is a problem outside of this
series that can be dealt with later.

I'm admittedly a little unsure of how to progress here. Given that this
series has received positive review over the complicated parts, it seems
that it is getting stuck on how to deal with `--object-dir`, especially
when invoked outside of a Git repository. My inclination would be to
send a new version that simply requires the MIDX builtin to be run from
within a repository (as well as the cleanups from Johannes).

Does that seem like a good direction forward to you? If not, let me know
if there's another issue that we should deal with first and I'd be happy
to start there.

> Thanks.

Thanks,
Taylor