Re: [PATCH v2 15/16] packfile: refactor `get_all_packs()` to work on packfile store

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 27, 2025, 01:45 UTC
Message-ID: <aK5jK2Q3McLnJ9Uw@nand.local>
In-Reply-To: <20250821-b4-pks-packfiles-store-v2-15-d10623355e9f@pks.im>

On Thu, Aug 21, 2025 at 09:39:13AM +0200, Patrick Steinhardt wrote:

> The `get_all_packs()` function prepares the packfile store and then
> returns its packfiles. Refactor it to accept a packfile store instead of
> a repository to clarify its scope.

I think that clarifying the scope here is a good idea. But I am a little sad to see this patch proposing that we drop get_all_packs(), which IMHO is a useful convenience function.

In effect this is pushing out the implementation details of the packfile_store out to every caller that wants to use get_all_packs(), which I am not sure is a win. Should those callers care where the array of packs is found, or have to write

    packfile_store_get_packs(the_repository->objects->packfiles)

each time they want to get the set of packs in a repository?

I could see an argument in the future where we have object stores that aren't packfile-based and thus calling "get_all_packs()" is not meaningful. But I don't think we are there yet, so I think that this patch is pushing the burden of that future hypothetical on all existing callers of get_all_packs().

Show 33 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/cat-file.c          |  2 +-
>  builtin/count-objects.c     |  2 +-
>  builtin/fast-import.c       |  4 ++--
>  builtin/fsck.c              |  8 ++++----
>  builtin/gc.c                |  8 ++++----
>  builtin/pack-objects.c      | 18 +++++++++---------
>  builtin/pack-redundant.c    |  4 ++--
>  builtin/repack.c            |  6 +++---
>  connected.c                 |  2 +-
>  http-backend.c              |  4 ++--
>  http.c                      |  2 +-
>  object-name.c               |  4 ++--
>  pack-bitmap.c               |  4 ++--
>  pack-objects.c              |  2 +-
>  packfile.c                  | 14 +++++++-------
>  packfile.h                  |  7 ++++++-
>  server-info.c               |  2 +-
>  t/helper/test-find-pack.c   |  2 +-
>  t/helper/test-pack-mtimes.c |  2 +-
>  19 files changed, 51 insertions(+), 46 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index fce0b06451c..7124c43fb14 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -854,7 +854,7 @@ static void batch_each_object(struct batch_options *opt,
>  						 batch_one_object_bitmapped, &payload)) {
>  		struct packed_git *pack;
>
> -		for (pack = get_all_packs(the_repository); pack; pack = pack->next) {
> +		for (pack = packfile_store_get_packs(the_repository->objects->packfiles); pack; pack = pack->next) {

If we do go this route, it might be nice to introduce the pattern of having a stack variable to hold the packfile_store pointer, since the line above here is getting a little long at >100 characters just to enumerate packs.

Thanks, Taylor