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

[Taylor Blau <me@ttaylorr.com>]

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
> > > @@ -278,7 +278,7 @@ static int unuse_one_window(struct packed_git *current)
> > >
> > >  	if (current)
> > >  		scan_windows(current, &lru_p, &lru_w, &lru_l);
> > > -	for (p = current->repo->objects->packed_git; p; p = p->next)
> > > +	for (p = current->repo->objects->packfiles->packs; p; p = p->next)
> >
> > Not a huge deal, but I do find "current->repo->objects->packfiles->packs"
> > to be a bit unfortunate. I wonder if we should rename "packs" to "head"
> > or "list_head" or similar since it's clear from
> > "current->repo->objects->packfiles" that this is a list of packfiles.
>
> I'd like to keep this part as-is for now if you don't mind. This is
> mostly because I've got a follow-up patch series that _does_ introduce
> `head` as part of making the `->next` pointer go away.

I'm OK with that approach provided that you have a plan to introduce
"head" here ;-).

> > > @@ -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.

Thanks,
Taylor