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
