Re: [PATCH 3/6] builtin/receive-pack: lift global state out of unpack()
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 7, 2026, 07:03 UTC
- Message-ID
- <anWDSt155Y9hzHGM@pks.im>
- In-Reply-To
- <20260806213859.816157-4-jltobler@gmail.com>
On Thu, Aug 06, 2026 at 04:38:56PM -0500, Justin Tobler wrote:
Show 6 quoted lines
> 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".
Show 36 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 7 quoted lines
> @@ -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