Re: [PATCH v2 02/16] odb: move list of packfiles into `struct packfile_store`

> On Tue, Sep 02, 2025 at 10:50:41AM +0200, Patrick Steinhardt wrote:
> > > > diff --git a/packfile.c b/packfile.c
> > > > index 8fbf1cfc2d..6478e4cc30 100644
> > > > --- a/packfile.c
> > > > +++ b/packfile.c
> > > > @@ -2344,5 +2339,23 @@ struct packfile_store *packfile_store_new(struct object_database *odb)
> > > >
> > > >  void packfile_store_free(struct packfile_store *store)
> > > >  {
> > > > +	packfile_store_close(store);
> > >
> > > Seeing a call to packfile_store_close() here was a little surprising to
> > > me. The code that you are moving has a comment that says:
> > >
> > >    * `close_object_store()` only closes the packfiles, but doesn't free
> > >    * them. We thus have to do this manually.
> > >
> > > , so I would have expected to preserve that behavior.
> >
> > This behaviour is preserved though. Calling `packfile_store_close()`
> > does not free the packfiles, it only closes them. And we continue to
> > call `packfile_store_close()` in `close_object_store()`, so nothing
> > changes.
> >
> > The only change in behaviour is that we now also know to close packfiles
> > when freeing the packfile store.
> 
> Why is that change necessary? I am not trying to be overly pedantic
> here, but as you note above this code is extremely fragile, so I am
> trying to avoid any changes that are not strictly necessary.