From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 29 Sep 2025 23:21:59 GMT
Subject: Re: [PATCH 26/49] builtin/repack.c: remove "repack_promisor_objects()" from the builtin
Message-ID: <aNsUl2afoigZZiLm@pks.im>
In-Reply-To: <3ba6e86f6116cc42fc8f7d31707dd514f4bc16a4.1759097191.git.me@ttaylorr.com>

On Sun, Sep 28, 2025 at 06:08:51PM -0400, Taylor Blau wrote:
> Now that we have properly factored the portion of the builtin which is
> responsible for repacking promisor objects, we can move that function
> (and associated dependencies) out of the builtin entirely.
> 
> Similar to previous extractions, this function is declared in repack.h,
> but implemented in a separate repack-promisor.c file. This is done to
> separate promisor-specific repacking functionality from generic repack
> utilities (like "existing_packs", and "generated_pack" APIs).

Feels sensible.

> diff --git a/repack-promisor.c b/repack-promisor.c
> new file mode 100644
> index 0000000000..8bf42fc715
> --- /dev/null
> +++ b/repack-promisor.c
> @@ -0,0 +1,102 @@
> +#include "git-compat-util.h"
> +#include "repack.h"
> +#include "run-command.h"
> +#include "hex.h"
> +#include "repository.h"
> +#include "packfile.h"
> +#include "path.h"
> +#include "pack.h"

Nit: these headers should probably be sorted lexicographically?

> diff --git a/repack.h b/repack.h
> index f37eb49524..19dc4fd738 100644
> --- a/repack.h
> +++ b/repack.h
> @@ -74,4 +74,8 @@ int generated_pack_has_ext(const struct generated_pack *pack, const char *ext);
>  void generated_pack_install(struct generated_pack *pack, const char *name,
>  			    const char *packdir, const char *packtmp);
>  
> +void repack_promisor_objects(struct repository *repo,
> +			     const struct pack_objects_args *args,
> +			     struct string_list *names, const char *packtmp);
> +
>  #endif /* REPACK_H */

I'm not a huge fan of the code bein gin "repack-promisor.c" while the
declarations are in "repack.h", as it makes things harder to find from
my point of view. But I don't feel too strongly about this.

Patrick