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().
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> 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
