From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 10 Oct 2025 22:57:08 GMT
Subject: Re: [PATCH 19/49] repack: remove 'existing_packs' API from the builtin
Message-ID: <aOmPROWM95FX57wC@nand.local>
In-Reply-To: <20251010060836.GG1965904@coredump.intra.peff.net>

On Fri, Oct 10, 2025 at 02:08:36AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:08:23PM -0400, Taylor Blau wrote:
>
> > --- a/repack.h
> > +++ b/repack.h
> > @@ -2,6 +2,7 @@
> >  #define REPACK_H
> >
> >  #include "list-objects-filter-options.h"
> > +#include "string-list.h"
> >
> >  struct pack_objects_args {
> >  	char *window;
> > @@ -31,4 +32,38 @@ void pack_objects_args_release(struct pack_objects_args *args);
> >  void repack_remove_redundant_pack(struct repository *repo, const char *dir_name,
> >  				  const char *base_name);
> >
> > +struct repository;
> > +struct packed_git;
> > +
> > +struct existing_packs {
> > +	struct repository *repo;
> > +	struct string_list kept_packs;
> > +	struct string_list non_kept_packs;
> > +	struct string_list cruft_packs;
> > +};
>
> Earlier, I saw remove_redundant_pack() got namespaced with a "repack_"
> prefix. Which I think makes sense, since it's a pretty generic name and
> the symbol now has global visibility. But none of this existing_packs
> stuff got the same treatment.
>
> I don't know how much it matters. I certainly have no plans to call
> something existing_packs elsewhere in the code, but I do wonder if the
> naming should make it more clear this is all repack-related.

I think this is a similar case as `struct pack_objects_args`. We could
prefix this as `repack_existing_packs`, but it seems a bit verbose, and
existing_packs is a semi-useful API by itself.

Thanks,
Taylor