Re: [PATCH v4 2/3] receive-pack: move message generation to separate function

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 31, 2026, 06:44 UTC
Message-ID: <apUi62Q_0CFBbBVO@pks.im>
In-Reply-To: <20260826-758-introduce-hook-v4-2-6b14975ad957@gmail.com>

On Wed, Aug 26, 2026 at 12:19:38PM +0200, Karthik Nayak wrote:

Show 8 quoted lines

> Post the reference transaction, both `report()` and `report_v2()`
> generate the message to be sent to the client. In v2, we also add
> reports for each reference if available.
> 
> Since they share common code,
> move them to a common function. This will also help the following
> commit, where we will need to regenerate the message during hook
> failure.

How about this instead:

  After git-receive-pack(1) has committed the reference updates, we call
  either `report()` or `report_v2()` to report to the client which of
  the references we have updated successfully and which updates have
  failed. The only difference between those two functions is that the
  latter also knows to provide a more detailed report about how exactly
  a given reference was updated.

  In the next commit we're about to add another site that wants to
  generate these reports. Refactor the logic into a shared function that
  can easily be reused.

Show 16 quoted lines

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 86933d8d7e..70a686c142 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2530,67 +2530,71 @@ static void update_shallow_info(struct command *commands,
>  	free(ref_status);
>  }
>  
> -static void report(struct command *commands, const char *unpack_status)
> +/*
> + * Generate the response to be sent to the client invoking 'git-receive-pack(1)'.
> + * For v2 protocol, set `add_reports` to true, which will also add additional
> + * report per reference update.
> + */
> +static void generate_response(struct strbuf *buf, struct command *commands,
> +			      const char *unpack_status, bool add_reports)

Response sounds quite generic, so should this be renamed to `generate_report()` instead? If so, we could adapt the parameter to `detailed_reports` or somesuch thing.

Other than that this patch looks good to me.

Patrick