Re: [PATCH v4 2/3] receive-pack: move message generation to separate function
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Aug 31, 2026, 19:05 UTC
- Message-ID
- <CAOLa=ZTrj9LRFHDXTYw-fNvmA5qOZrrU-8d-3aY1NB2J_zib5g@mail.gmail.com>
- In-Reply-To
- <apUi62Q_0CFBbBVO@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 23 quoted lines
> 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.
Show 24 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
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