From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 07 Oct 2025 20:17:19 GMT
Subject: Re: [PATCH 07/49] builtin/repack.c: avoid "the_hash_algo" in `write_oid()`
Message-ID: <aOV1TzXOizrK3d4Q@nand.local>
In-Reply-To: <aNsUZtGst4MguEXq@pks.im>

On Tue, Sep 30, 2025 at 01:21:10AM +0200, Patrick Steinhardt wrote:
> On Sun, Sep 28, 2025 at 06:07:37PM -0400, Taylor Blau wrote:
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index 094f5a0cc2..7d62959dc2 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -413,6 +419,7 @@ static void repack_promisor_objects(struct repository *repo,
> >  				    const struct pack_objects_args *args,
> >  				    struct string_list *names)
> >  {
> > +	struct write_oid_context ctx;
> >  	struct child_process cmd = CHILD_PROCESS_INIT;
> >  	FILE *out;
> >  	struct strbuf line = STRBUF_INIT;
> > @@ -427,7 +434,9 @@ static void repack_promisor_objects(struct repository *repo,
> >  	 * {type -> existing pack order} ordering when computing deltas instead
> >  	 * of a {type -> size} ordering, which may produce better deltas.
> >  	 */
> > -	for_each_packed_object(repo, write_oid, &cmd,
> > +	ctx.cmd = &cmd;
> > +	ctx.algop = repo->hash_algo;
> > +	for_each_packed_object(repo, write_oid, &ctx,
> >  			       FOR_EACH_OBJECT_PROMISOR_ONLY);
>
> As this one is essentially new code, and we have all of the info
> available right at the start of the problem. Do we maybe want to use
> designated initializers?

I considered it, but ultimately decided against it here because I find
it clearer to initialize "ctx" right before it is used to avoid
ambiguity.

Alternatively, you could accomplish this with a compound literal, but
that's (a) still a test balloon and (b) more verbose than necessary.

Thanks,
Taylor