On Tue, Jul 27, 2021 at 01:47:40PM -0400, Taylor Blau wrote:
> > 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
