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

Taylor Blau
2025-10-07T20:37:00Z

On Tue, Sep 30, 2025 at 01:21:59AM +0200, Patrick Steinhardt wrote:
> > 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?

Yup, definitely. Fixed.

> > 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.

I think we have precedence for this in other places throughout the code
(for e.g., "midx.h" => "midx.c" and "midx-write.c", "pack-bitmap.h" =>
"pack-bitmap.c" and "pack-bitmap-write.c", etc), so I think it's OK
here.

If you change your mind and do feel strongly about it later on, I'm
happy to adjust it ;-).

Thanks,
Taylor