Re: [PATCH 43/49] repack: extract `write_pack_opts_is_local()`

Taylor Blau
2025-10-15T21:21:34Z

On Fri, Oct 10, 2025 at 02:28:57PM +0200, Patrick Steinhardt wrote:
> On Sun, Sep 28, 2025 at 06:10:08PM -0400, Taylor Blau wrote:
> > diff --git a/repack.c b/repack.c
> > index c4326a532d..7af297ae48 100644
> > --- a/repack.c
> > +++ b/repack.c
> > @@ -77,6 +77,12 @@ const char *write_pack_opts_pack_prefix(struct write_pack_opts *opts)
> > return pack_prefix;
> > }
> >
> > +int write_pack_opts_is_local(struct write_pack_opts *opts)
> > +{
> > + const char *scratch;
> > + return skip_prefix(opts->destination, opts->packdir, &scratch);
> > +}
> > +
> > #define DELETE_PACK 1
> > #define RETAIN_PACK 2
> >
>
> Now that we accept booleans in our codebase, should this function return
> a `bool` instead?

Yeah, I agree that it should eventually return "bool", but I don't want
to make the change in this patch, at least within the context of this
series where I'm trying to keep the changes-per-patch as minimal as
possible.

> > diff --git a/repack.h b/repack.h
> > index 46d2312fa9..16f2de2ea9 100644
> > --- a/repack.h
> > +++ b/repack.h
> > @@ -40,6 +40,7 @@ struct write_pack_opts {
> > };
> >
> > const char *write_pack_opts_pack_prefix(struct write_pack_opts *opts);
> > +int write_pack_opts_is_local(struct write_pack_opts *opts);
>
> It would be good if all of these these new library functions could come
> with some documentation what they actually do.

Fully agreed. I think there is enough here that I'd rather pursue this
as a separate step, but at least this series moves us in a good
direction to document things by putting the functions in a clean header
that is more amenable to documentation than the builtin is.

Thanks,
Taylor