From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 07 Oct 2025 20:36:57 GMT
Subject: Re: [PATCH 26/49] builtin/repack.c: remove "repack_promisor_objects()" from the builtin
Message-ID: <aOV56Uzley6ebswk@nand.local>
In-Reply-To: <aNsUl2afoigZZiLm@pks.im>

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