From: Patrick Steinhardt Date: Tue, 02 Sep 2025 08:50:41 GMT Subject: Re: [PATCH v2 02/16] odb: move list of packfiles into `struct packfile_store` Message-ID: In-Reply-To: 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