From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 07 Aug 2026 07:03:43 GMT
Subject: Re: [PATCH 4/6] builtin/receive-pack: report unpack errors via strbuf
Message-ID: <anWDTwCwMn5wEdIQ@pks.im>
In-Reply-To: <20260806213859.816157-5-jltobler@gmail.com>

On Thu, Aug 06, 2026 at 04:38:57PM -0500, Justin Tobler wrote:
> When writing packfiles via `unpack()`, error messages are returned
> directly by the function. In preparation for `unpack()` logic being
> moved behind a generic ODB transaction interface, update the function to
> instead write any error messages to a caller provided strbuf and return
> a negative value on error. Call sites are updated to use the error
> strbuf accordingly.

If only Git had a structured error type, than we wouldn't have to have
such ugly workarounds. Anyway, this is a deeper issue and nothing we can
blame on this patch series.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 8c2d6e5789..7635b82bd3 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2344,8 +2344,8 @@ struct unpack_opts {
>  	int quiet;
>  };
>  
> -static const char *unpack(struct odb_transaction *transaction,
> -			  const struct unpack_opts *opts)
> +static int unpack(struct odb_transaction *transaction, struct strbuf *err_msg,
> +		  const struct unpack_opts *opts)
>  {
>  	struct pack_header hdr;
>  	const char *hdr_err;

While I'm not a huge fan of error message parameters like this, this
change does make the calling convention more straight-forward. A reader
probably wouldn't have known beforehand what to do with the return value
without reading through docs.

Also, we cannot just return the equivalent of `return error("msg")`, as
we do want to use and munge the error message as part of the status
report we send to the client.

> @@ -2551,13 +2559,13 @@ static void update_shallow_info(struct command *commands,
>  	free(ref_status);
>  }
>  
> -static void report(struct command *commands, const char *unpack_status)
> +static void report(struct command *commands, struct strbuf *unpack_status)

Should we mark this parameter as `const`?

> @@ -2575,14 +2583,14 @@ static void report(struct command *commands, const char *unpack_status)
>  	strbuf_release(&buf);
>  }
>  
> -static void report_v2(struct command *commands, const char *unpack_status)
> +static void report_v2(struct command *commands, struct strbuf *unpack_status)

And here, as well?

> @@ -2711,8 +2719,8 @@ int cmd_receive_pack(int argc,
>  			   PACKET_READ_DIE_ON_ERR_PACKET);
>  
>  	if ((commands = read_head_info(&reader, &shallow))) {
> -		const char *unpack_status = NULL;
>  		struct string_list push_options = STRING_LIST_INIT_DUP;
> +		struct strbuf unpack_status = STRBUF_INIT;

Can't we reuse this buffer and reset it on every run to save some memory
allocations?

Patrick