From: Jeff King <peff@peff.net>
Date: Fri, 10 Oct 2025 05:19:55 GMT
Subject: Re: [PATCH 02/49] builtin/repack.c: avoid "the_repository" in existing packs API
Message-ID: <20251010051955.GA1965904@coredump.intra.peff.net>
In-Reply-To: <664a67c93620edf6727f0617f4dddce99901fcde.1759097191.git.me@ttaylorr.com>

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.

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

-Peff