From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 10 Oct 2025 22:51:16 GMT
Subject: Re: [PATCH 09/49] builtin/repack.c: avoid "the_hash_algo" in `finish_pack_objects_cmd()`
Message-ID: <aOmN5Hj1xT4Qc6Ed@nand.local>
In-Reply-To: <20251010053115.GC1965904@coredump.intra.peff.net>

On Fri, Oct 10, 2025 at 01:31:15AM -0400, Jeff King wrote:
> On Sun, Sep 28, 2025 at 06:07:45PM -0400, Taylor Blau wrote:
>
> > @@ -1150,7 +1151,8 @@ static int write_filtered_pack(const struct pack_objects_args *args,
> >  		fprintf(in, "%s%s.pack\n", caret, item->string);
> >  	fclose(in);
> >
> > -	return finish_pack_objects_cmd(&cmd, names, local);
> > +	return finish_pack_objects_cmd(existing->repo->hash_algo, &cmd, names,
> > +				       local);
> >  }
>
> OK, my last musing on which types to pass around, I promise. ;) This is
> a great counter-example to my initial comment on patch 2: if we had put
> an object_source into the "existing" struct back then, we wouldn't be
> able to pull the hash_algo out now.
>
> (Well, sort of; object_database does have a pointer back to its
> containing repo, which itself feels kind of weird. I'd just as soon not
> rely on that, here, though).

Yeah, that feels like a tinier example of what we're talking about here,
just within the object_database struct itself ;-). I agree that it does
not make sense to rely on that, since I think the existence of that
pointer is equally something that we could consider changing.

> > -static int finish_pack_objects_cmd(struct child_process *cmd,
> > +static int finish_pack_objects_cmd(const struct git_hash_algo *algop,
> > +				   struct child_process *cmd,
>
> I am mildly surprised at some of these spots that _only_ need a
> hash_algo and nothing else, but the proof of the pudding is in the
> compiling. As they say. Well, as I say.

I would like that on a t-shirt, please.

Thanks,
Taylor