From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 15 Sep 2025 07:44:32 GMT
Subject: Re: [PATCH v2 02/16] odb: move list of packfiles into `struct packfile_store`
Message-ID: <aMfD4JuwJPHmh5WI@pks.im>
In-Reply-To: <aMNYaPSnf6JP7bNj@nand.local>

On Thu, Sep 11, 2025 at 07:16:56PM -0400, Taylor Blau wrote:
> 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.

Hm, okay. I'll drop that part then. We can revisit this at a later point
in time once we move the logic to close the object database into the
respective sources.

Patrick