Re: [PATCH] hook: introduce the report hook for git-receive-pack(1)

Karthik Nayak <[email protected]> Wed, 19 Aug 2026 14:11:19 +0200
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZSN+h4TkZrqPPRNZ58Pyfamv9_tM=m7W8_RYhUU0p0q0w@mail.gmail.com>
"Kristoffer Haugsbakk" <[email protected]> writes:

> On Tue, Aug 18, 2026, at 09:55, Karthik Nayak wrote:
>> When running 'git-receive-pack(1)', there is currently no way for the
>> server to intercept and modify the status report before it is sent back
>> to the client. This is useful for servers with custom logic that need
>> to transform or gate the report based on the outcome of external logic
>> post reference updates.
>>
>> Introduce a new 'report' hook which receives the pkt-line encoded
>> status report on stdin and whose stdout replaces the report sent to the
>> client. A non-zero exit status causes `receive-pack` to die and the
>> client to treat the push as failed.
>>
>> Similar to the 'proc-receive' hook, this does not use the config-based
>> hook infrastructure. That infrastructure is designed for parallelizable
>> notification hooks. As this hook is a bidirectional filter, it would
>> require significant modifications to that infrastructure and this hook
>> cannot be parallelized anyway.
>>
>> Signed-off-by: Karthik Nayak <[email protected]>
>> ---
>> To give some context, we at GitLab are building a custom MVCC around
>> Git. Each git-push would initialize a new version which is then
>> committed as the default post some operations. These operations take
>> place after the reference transaction and based on the output status of
>> those operations, we want to propagate the status to the user. There
>> currently exists no good mechanism to do so.
>>
>> Having a report hook which allows us to modify the report being
>> propagated to the user, allows us to modify the report based on the
>> status of our MVCC commit phase.
>
> Personally I think understanding concrete things is easier than
> understanding general things. And discussing the concrete case in the
> commit message would help with that as well as provide the context for
> git-log(1) rather than just the people who have read these emails.
>

I was conflicted about it, since while it does provide some context, it
doesn't apply to most usecases. I will add a little more context in the
commit message.

>> ---
>>  Documentation/githooks.adoc |  23 ++++++
>>  builtin/receive-pack.c      |  41 +++++++++++
>>  t/meson.build               |   1 +
>>  t/t5412-report-hook.sh      | 176 ++++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 241 insertions(+)
>
> Should the git-receive-pack(1) doc be updated to mention that this hook
> exists? I don’t understand the setup here. The existing
> git-receive-pack(1) doc has sections for these hooks:
>
> • `update`
> • `pre-receive`
> • `post-receive`
> • `post-update`
>
> But not these:
>
> • `push-to-checkout`
> • `proc-receive`
>
> (referenced against githooks(5))
>

I didn't know about this. I wonder why we have two sources of truth for
the same. As you see, it's already starting to diverge.

I will add both of them with links to githooks(5), but perhaps a cleanup
there is in order. I would say making githooks(5) the canonical location
with git-receive-pack(1) referencing it makes sense.

>>
>> diff --git a/Documentation/githooks.adoc b/Documentation/githooks.adoc
>> index ed045940d1..7e6643ad89 100644
>> --- a/Documentation/githooks.adoc
>> +++ b/Documentation/githooks.adoc
>> @@ -527,6 +527,29 @@ The exit status of the hook is ignored for any
>> state except for the
>>  status will cause the transaction to be aborted. The hook will not be
>>  called with "aborted" state in that case.
>>
>> +report
>> +~~~~~~
>> +
>> +This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
>> +`git push` and updates reference(s) in its repository. It executes on
>> +the remote repository once after all refs have been updated, but before
>> +the status report is sent back to the client.
>> +
>> +The hook receives the pkt-line encoded status report on standard input
>
> Another naive question (I have never used any of this). Should this link
> to some gitprotocol-X(5) after `pkt-line` in order to have a link that
> explains what it is? I don’t see any mention of `pkt-line` on
> git-receive-pack(1) or a mention of a gitprotocol-X(5).
>

We could link to 'Documentation/gitprotocol-common.adoc', but I'm not
sure if it is erring on the side of being too verbose. I'll leave it out
since its already existing and assumed to be common knowledge for users
of such hooks. But happy to add it in if others disagree :)

>> +and its standard output replaces the report sent to the client. Any
>> +output written to standard error is forwarded to the client over the
>> +sideband channel and will appear as `remote:` lines on the client's
>> +terminal. To reject individual ref updates, rewrite the corresponding
>> +`ok` lines to `ng` lines in the output report (with an explanatory
>> +error string) and exit zero; standard error can accompany this to
>> +provide a human-readable explanation. A non-zero exit status causes
>> +`receive-pack` to die.
>> +
>> +Note that by the time this hook runs, all ref updates have already been
>> +applied to the repository. A non-zero exit causes the client to see the
>> +push as failed, but does *not* roll back any ref changes that were
>> +already committed server-side.
>
> To my naive eyes this description looks good and without any obvious
> errors (typos ;) ).
>

Thanks for reading through

>> +
>>  push-to-checkout
>>  ~~~~~~~~~~~~~~~~
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>>[snip]
>> @@ -2592,6 +2630,9 @@ static void report_v2(struct command *commands,
>> const char *unpack_status)
>>  	}
>>  	packet_buf_flush(&buf);
>>
>> +	if (run_report_hook(&buf))
>> +		die("report hook failed");
>
> Okay, it seems typical for this command to use regular strings (not
> translated) for errors. Which makes sense given the application. There
> does seem to be translated error strings but one example is “refusing to
> update current branch”, which seems to be more of a non-bare, end-user
> error than a server error.
>

Yeah, since these are generally less user-facing (I say less because
this can be propagated to the user, if the hook exists with a non-zero
error code) I choose not to translate it. As you mentioned, this seems
to be the way for such error messages.

>> +
>>  	if (use_sideband)
>>  		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
>>  	else
>>[snip]
>> diff --git a/t/t5412-report-hook.sh b/t/t5412-report-hook.sh
>>[snip]
>> +test_expect_success "no report hook, push succeeds" '
>> +	test_when_finished "rm -rf upstream" &&
>> +	test_when_finished "git -C workbench remote remove origin" &&
>
> This teardown routine is common to all the tests. Is it better style
> here to write it out compared to using a helper function (test code is
> different from “normal” code)?
>

Since tests are self-contained, I usually keep the teardowns within
them if they're simple enough.

>> +	git init --bare upstream &&
>>[snip]
signature.asc (application/pgp-signature, 690 B)