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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Aug 25, 2025 at 07:42:53PM -0400, Taylor Blau wrote:
> On Thu, Aug 21, 2025 at 09:39:00AM +0200, Patrick Steinhardt wrote:
> > The object database tracks the list of packfiles it currently knows
> > about. With the introduction of the `struct packfile_store` we have a
> > better place to host this list though.
> >
> > Move the list accordingly. Extract the logic from `odb_clear()` that
> > knows to close all such packfiles and move it into the new subsystem, as
> > well.
> 
> Not a comment on this patch itself, but as a meta-comment, I really
> appreciate you taking such an incremental approach here. The packfile
> machinery is quite fragile in my experience, so breaking it up into (what
> are so far) easily review-able chunks makes it much easier to build
> confidence in the correctness of these changes.

It certainly is fragile overall. I stared at code for way longer than I
really want to admit in some cases.

> > diff --git a/odb.c b/odb.c
> > index 34b70d0074..17a9135cbd 100644
> > --- a/odb.c
> > +++ b/odb.c
> > @@ -1038,16 +1038,7 @@ void odb_clear(struct object_database *o)
> >
> >  	INIT_LIST_HEAD(&o->packed_git_mru);
> >  	close_object_store(o);
> > -
> > -	/*
> > -	 * `close_object_store()` only closes the packfiles, but doesn't free
> > -	 * them. We thus have to do this manually.
> > -	 */
> > -	for (struct packed_git *p = o->packed_git, *next; p; p = next) {
> > -		next = p->next;
> > -		free(p);
> > -	}
> > -	o->packed_git = NULL;
> > +	packfile_store_free(o->packfiles);
> 
> Interesting. The movement of the for-loop here all looks correct to me.
> But I think the new packfile_store is creating a new awkardness here
> that we should consider.
> 
> In existing implementation, all of the ->next pointers here point to
> heap locations that have already been free()'d. But that's OK, since
> they aren't reachable at the moment that we do "o-packed_store = NULL".
> 
> Having a separate packfile_store changes that, since (from my reading of
> the code) o->packfiles will still be non-NULL even after calling
> odb_clear(), *and* those pointers will refer to free'd heap locations.
> 
> That seems like a potential footgun to me. I think that we could either:
> 
>  * Change packfile_store_free() to take in an object_database pointer,
>    and NULL out the ->packs pointer after free'ing all of the packfiles.
>    That would make it more similar to the existing behavior.
> 
>  * Leave packfile_store_free() as-is, document that it does NOT clear
>    out the top-level pointer, and so callers are encouraged to NULL it
>    out themselves after calling it. Likewise, we should change
>    odb_clear() to do:
> 
>        packfile_store_free(o->packfiles);
>        o->packfiles = NULL;
> 
> Let me know what you think.

Good point. I think it's unlikely to ever become a problem, but I don't
see a reason why we shouldn't NULL out `o->packfiles`, either. So I'll
do the second approach.

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

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

> I *think* that
> this happens to be OK, since close_pack() is a noop if it is called more
> than once (though I had to double check through all of its leaf
> functions that that was indeed the case).
> 
> I would probably strike this from the new function, since the sole
> caller above already calls close_object_store() before calling
> packfile_store_free().

Calling `packfile_store_close()` is idempotent indeed, so it shouldn't
be an issue to call this function twice. To me the question is whether
there's ever a use case where you want to free the packfile store, but
don't want to close the packfiles stored in it.

From all I've seen that is never the case, so I think it's sensible to
ensure that we always close the packfile store before we free it to make
things a tiny bit more self-contained.

> > +void packfile_store_close(struct packfile_store *store)
> > +{
> > +	struct packed_git *p;
> > +
> > +	for (p = store->packs; p; p = p->next)
> > +		if (p->do_not_close)
> > +			BUG("want to close pack marked 'do-not-close'");
> > +		else
> > +			close_pack(p);
> > +}
> 
> And likewise this looks good to me. I do find the braceless for-loop a
> little hard to read, but it's (a) correct, and (b) consistent with the
> original implementation, so I don't feel strongly about changing it.

Agreed, it is a bit awkward. I feel like our coding style should be
amended to say that we only do braceless bodies in case the body is a
single statement.

> As a side-note, you could inline the declaration of "p" here into the
> for-loop, but I can understand not wanting to to make the diff more
> readable with --color-moved.

I wouldn't mind adapting this while at it, too.

Patrick