Re: [PATCH 3/6] builtin/receive-pack: lift global state out of unpack()

On 26/08/07 09:03AM, Patrick Steinhardt wrote:
> On Thu, Aug 06, 2026 at 04:38:56PM -0500, Justin Tobler wrote:
> > In git-receive-pack(1), writing the packfile to the transaction is
> > handled via `unpack()` which relies on global variables to decide how to
> > invoke the underlying git-index-pack(1) or git-unpack-objects(1) child
> > processes. In a subsequent commit, the `unpack()` logic is moved behind
> > a generic ODB transaction interface to handle writing packfiles and thus
> > can no rely on these globals.
> 
> Nit: either "can not" or "can no longer".

Will fix.

> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index 6da854fca2..8c2d6e5789 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -2333,18 +2333,25 @@ static void push_header_arg(struct strvec *args, struct pack_header *hdr)
> >  		     ntohl(hdr->hdr_version), ntohl(hdr->hdr_entries));
> >  }
> >  
> > +struct unpack_opts {
> > +	const char *fsck_msg_types;
> > +	const char *shallow_file;
> > +	off_t max_input_size;
> > +	int fsck_objects;
> > +	int unpack_limit;
> > +	int reject_thin;
> > +	int err_fd;
> > +	int quiet;
> > +};
> > +
> >  static const char *unpack(struct odb_transaction *transaction,
> > -			  const char *shallow_file, int err_fd)
> > +			  const struct unpack_opts *opts)
> >  {
> >  	struct pack_header hdr;
> >  	const char *hdr_err;
> >  	int status;
> >  	struct child_process child = CHILD_PROCESS_INIT;
> > -	int fsck_objects = (receive_fsck_objects >= 0
> > -			    ? receive_fsck_objects
> > -			    : transfer_fsck_objects >= 0
> > -			    ? transfer_fsck_objects
> > -			    : 0);
> > +	int err_fd = opts->err_fd;
> >  
> >  	hdr_err = parse_pack_header(&hdr);
> >  	if (hdr_err) {
> 
> It's quite hard to see that the function indeed doesn't rely on the
> global variables anymore, and I'm quite certain that I'd not spot cases
> that you forgot to convert to use the options structure instead. But I
> assume that the function will move into a different file in a subsequent
> commit, so we'd notice in that patch.

Ya, that is indeed the plan. :)

> > @@ -2428,11 +2435,24 @@ static const char *unpack(struct odb_transaction *transaction,
> >  static const char *unpack_with_sideband(struct odb_transaction *transaction,
> >  					const char *shallow_file)
> >  {
> > +	struct unpack_opts opts = {
> > +		.fsck_objects = (receive_fsck_objects >= 0
> > +				 ? receive_fsck_objects
> > +				 : transfer_fsck_objects >= 0
> > +				 ? transfer_fsck_objects
> > +				 : 0),
> 
> This looks quite ugly, but it's no more ugly than the previous code it
> replaces.

In a different version of this patch, I modified the fsck objects field
after initialization and dropped the single statement here, but I
ultimately didn't think that looked much better either.

-Justin