Re: [PATCH 26/49] builtin/repack.c: remove "repack_promisor_objects()" from the builtin

Patrick Steinhardt
2025-09-29T23:22:04Z

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