Re: [PATCH v2 08/24] midx: respect 'core.multiPackIndex' when writing

On Tue, Jul 27, 2021 at 01:47:40PM -0400, Taylor Blau wrote:

quoted

BTW, yet another weirdness: close_object_store() will call close_midx()
on the outermost midx struct, ignoring o->multi_pack_index->next
entirely. So that's a leak, but also means we may not be closing the
midx we're interested in (since write_midx_internal() takes an
object-dir parameter, and we could be pointing to some other
object-dir's midx).

Yuck, this is a mess. I'm tempted to say that we should be closing the
MIDX that we're operating on inside of write_midx_internal() so we can
write, but then declaring the whole object store to be bunk and calling
close_object_store() before leaving the function. Of course, one of
those steps should be closing the inner-most MIDX before closing the
next one and so on.

That gets even weirder when you look at other callers of
write_midx_internal(). For instance, expire_midx_packs() is calling
load_multi_pack_index() directly, and then passing it to
write_midx_internal().

So closing the whole object store there is likewise weird.

I actually think having write_midx_internal() open up a new midx is
reasonable-ish. It's just that:

  - it's weird when it stuffs duplicate packs into the
    r->objects->packed_git list. But AFAICT that's not actually hurting
    anything?

  - we do need to make sure that the midx is closed (not just our copy,
    but any other open copies that happen to be in the same process) in
    order for things to work on Windows.

So I guess because of the second point, the internal midx write probably
needs to be calling close_object_store(). But because other callers use
load_multi_pack_index(), it probably needs to be closing the one that is
passed in, too! But of course not double-closing it if it did come from
the regular object store. One easy easy way to avoid that is to just
open a separate one.

I have some spicy takes on how midx's should have been designed, but I
think it's probably not productive to rant about it at this point. ;)

-Peff