From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 31 Aug 2026 19:05:35 GMT
Subject: Re: [PATCH v4 2/3] receive-pack: move message generation to separate function
Message-ID: <CAOLa=ZTrj9LRFHDXTYw-fNvmA5qOZrrU-8d-3aY1NB2J_zib5g@mail.gmail.com>
In-Reply-To: <apUi62Q_0CFBbBVO@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Aug 26, 2026 at 12:19:38PM +0200, Karthik Nayak wrote:
>> 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.
>

reads better, will swap in.

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

Yeah, I was thinking about it being too generic but didn't pay much
heed, since you also think the same, I'll change it.

Thanks