Re: [PATCH 02/49] builtin/repack.c: avoid "the_repository" in existing packs API
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Oct 10, 2025, 22:48 UTC
- Message-ID
- <aOmNWlwG3WPNChKi@nand.local>
- In-Reply-To
- <20251010051955.GA1965904@coredump.intra.peff.net>
On Fri, Oct 10, 2025 at 01:19:55AM -0400, Jeff King wrote:
Show 18 quoted lines
> On Sun, Sep 28, 2025 at 06:07:18PM -0400, Taylor Blau wrote:
>
> > struct existing_packs {
> > + struct repository *repo;
> > struct string_list kept_packs;
> > struct string_list non_kept_packs;
> > struct string_list cruft_packs;
> > @@ -265,7 +266,7 @@ static void existing_packs_release(struct existing_packs *existing)
> > static void collect_pack_filenames(struct existing_packs *existing,
> > const struct string_list *extra_keep)
> > {
> > - struct packfile_store *packs = the_repository->objects->packfiles;
> > + struct packfile_store *packs = existing->repo->objects->packfiles;
>
> I found it a little funny to pass around a repository struct as part of
> existing_packs, since they're not directly related. But I think it is
> mostly just a convenience to do so, and "existing_packs" is really
> storing an overall context.Yeah, the patch as-is suggests that we could have gotten away with just adding a pointer to the packfile_store within existing_packs. I tried to do that locally, and it works up until "builtin/repack.c: rename many 'struct existing_packs' functions", where we start to want to use "repo", or "repo->hash_algo" :-<.
Show 11 quoted lines
> It could probably be scoped down to pass around an object_database > instead, or maybe even a packfile store, which somehow feels a little > more "correct" to me as context. But I have trouble imagining that being > helpful to any new code (why would it have an object_store but a not a > repo object?). And I can easily imagine having the repo available being > useful for future refactorings. > > So I think what you've written here is probably a good path forward. I > wanted to outline my thinking because I suspect the same will apply to > other patches in this series (i.e., they don't always need a repo > object, but it's simplest to pass them one).
Yeah, exactly. In general for this series I am trying to do as few changes to the actual functions as possible. There are some (like the renaming in the later patch I mentioned above), but only in service of trying to have a clean repack.h (at least from a naming standpoint).
There is lots of opportunity to clean up the repack code further. I agree it would be nice to have existing_packs only take a packfile_store, and pass around the repository in other places as needed. I'm happy to investigate that, but as a follow-up to this series, since I want to change as few things as possible here.
Thanks, Taylor