Re: [GSoC][PATCH 1/5] builtin/pack-refs: factor out core logic into a helper

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 03, 2025 at 09:26:37AM +0530, Meet Soni wrote:
> On Tue, 2 Sept 2025 at 15:48, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Tue, Aug 26, 2025 at 01:06:41PM +0530, Meet Soni wrote:
> > > The implementation of `git pack-refs` is monolithic within
> > > `cmd_pack_refs()`, making it impossible to share its logic with other
> > > commands. To enable code reuse for the upcoming `git refs optimize`
> > > subcommand, refactor the core logic into a shared helper function.
> > >
> > > Introduce a new `pack-refs.h` header to define the public interface
> > > for this shared logic. It contains the declaration for a new helper
> > > function, `pack_refs_core()`, and a macro for the common usage
> > > options.
> > >
> > > Move the option parsing and packing logic from `cmd_pack_refs()` into a
> > > new helper function named `pack_refs_core()`. This helper is made
> > > generic by accepting the command's usage string as a parameter.
> > >
> > > The original `cmd_pack_refs()` is simplified to a thin wrapper that
> > > is only responsible for defining its specific usage array and calling
> > > the shared helper.
> > >
> > > Mentored-by: Patrick Steinhardt <ps@pks.im>
> > > Mentored-by: shejialuo <shejialuo@gmail.com>
> > > Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
> > > ---
> > >  builtin/pack-refs.c | 31 ++++++++++++++++++++-----------
> > >  pack-refs.h         | 22 ++++++++++++++++++++++
> > >  2 files changed, 42 insertions(+), 11 deletions(-)
> > >  create mode 100644 pack-refs.h
> >
> > Shouldn't that header live in "builtin/pack-refs.h"? Makes it way more
> > obvious that it exposes functions from "builtin/pack-refs.c".
> 
> I couldn't find any header files in the builtin/ directory. Also, since we
> placed the for-each-ref.h file in the root directory in our previous series, I
> decided to do the same here.

Hm. Honestly, I'd much rather also move "for-each-ref.h" into
"builtin/", as well. The logic is not part of libgit.a and specific to
the builtins, so I think it's preferable to have it in that directory.

Patrick