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

[Patrick Steinhardt <ps@pks.im>]

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".

> 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.

> @@ -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.

> @@ -2441,7 +2461,8 @@ static const char *unpack_with_sideband(struct odb_transaction *transaction,
>  	if (start_async(&muxer))
>  		return NULL;
>  
> -	ret = unpack(transaction, shallow_file, muxer.in);
> +	opts.err_fd = muxer.in;
> +	ret = unpack(transaction, &opts);

Hm, okay. I guess this here is because we only want to manually read
stderr in case we use the sideband. It's a bit unfortunate that this
requires us to modify the passed-in options structure, but I guess I can
live with that.

Patrick