From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 15 Oct 2025 21:18:13 GMT
Subject: Re: [PATCH 40/49] builtin/repack.c: introduce `struct write_pack_opts`
Message-ID: <aPAPlb1VvhKfcEJb@nand.local>
In-Reply-To: <20251015102833.GB2250228@coredump.intra.peff.net>

On Wed, Oct 15, 2025 at 06:28:33AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:09:51PM -0400, Taylor Blau wrote:
>
> > Instead of repeating those arguments for each function, let's extract an
> > options struct called "write_pack_opts" which has these three parameters
> > as member fields. While we're at it, add fields for "packdir," and
> > "packtmp", both of which are static variables within the builtin, and
> > need to be read from within these two functions.
>
> Makes sense, although...
>
> > +		struct write_pack_opts opts = {
> > +			.po_args = &po_args,
> > +			.destination = filter_to,
> > +			.pack_prefix = find_pack_prefix(packdir, packtmp),
> > +			.packdir = packdir,
> > +			.packtmp = packtmp,
> > +		};
>
> ...since we are now passing packdir and packtmp anyway, and pack_prefix
> is derived from those, should the called function just do that
> derivation itself? Or do we expect that some callers may eventually use
> a different prefix?
>
> Probably not a huge deal either way, but maybe an easy way to tighten up
> the interface a bit.

We do this a little further down in "repack: move `find_pack_prefix()`
out of the builtin", which I tried to separate since I wanted to go
incrementally here. But I should mention that that patch is upcoming to
avoid readers wondering why we're not making the change sooner.

Thanks,
Taylor