Re: [PATCH 2/5] pack-bitmap: tag bitmapped packs with their corresponding MIDX

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 05, 2024 at 05:00:24AM -0400, Jeff King wrote:
> On Thu, Aug 29, 2024 at 02:58:19PM -0400, Taylor Blau wrote:
>
> > On Tue, Aug 27, 2024 at 05:14:14PM -0700, Junio C Hamano wrote:
> > > > diff --git a/midx.c b/midx.c
> > > > index ca98bfd7c6..67e0d64004 100644
> > > > --- a/midx.c
> > > > +++ b/midx.c
> > > > @@ -496,6 +496,7 @@ int nth_bitmapped_pack(struct repository *r, struct multi_pack_index *m,
> > > >  				 MIDX_CHUNK_BITMAPPED_PACKS_WIDTH * local_pack_int_id +
> > > >  				 sizeof(uint32_t));
> > > >  	bp->pack_int_id = pack_int_id;
> > > > +	bp->from_midx = m;
> > >
> > > Do multi_pack_index objects live as long as bitmapped_pack objects
> > > that point at them live?  If m later goes away without letting the
> > > bitmapped_pack know about it, the borrowed pointer in from_midx
> > > would become dangling, which is not what we want to see.
> > >
> > > "None of these objects are released or relocated while we are
> > > running pack-objects, so once the .from_midx member is assigned
> > > here, it will always be pointing at a valid multi_pack_index object"
> > > is a satisfactory answer, I guess.
> >
> > Good question, and good answer ;-).
> >
> > This is only relevant in a read-only path where we're generating a new
> > pack from existing packs and not altering those pack or rewriting /
> > deleting the MIDX attached to them. So I think we're OK here and don't
> > have any lifetime/scope issues.
>
> Do we ever close/reopen the midx? For example, if a simultaneous process
> wrote a new one and we triggered reprepare_packed_git()?
>
> I think the answer is "no"; we might read a new midx, but we never ditch
> the old one (just like we do for packed_git structs). Which I suspect is
> needed even before this patch, since various other parts of the bitmap
> code (and probably others) rely on the struct staying in place across as
> we read many objects.

I think that we *could* close and reopen the MIDX via a call to
close_object_store() (which dispatches a call to close_midx(), before
NULL-ing out o->multi_pack_index) and then calling prepare_packed_git()
or similar.

But I'm not aware of any such codepaths that deal with pack reuse and
calling nth_bitmapped_pack that would do such a thing, so I think that
we're OK here.

Thanks,
Taylor