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

Karthik Nayak <[email protected]> Wed, 19 Aug 2026 15:13:50 +0200
Newsgroups	org.kernel.vger.git
Message-ID	<CAOLa=ZTtOJLXkfZ8jKpuA9REg5CP_xxD8+kDxPAYLeRz_xR1Wg@mail.gmail.com>
Patrick Steinhardt <[email protected]> writes:

> On Tue, Aug 18, 2026 at 09:55:55AM +0200, 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.
>
> I think it would have been useful to add context why none of the
> preexisting hooks work for us:
>
>   - The pre-receive hook runs too early, as we haven't updated
>     references at that point yet and we need to have the full view of
>     all resulting updates (both objects and references).
>
>   - The update hook is too inefficient as it runs once per reference,
>     and we cannot trivially determine the last update.
>
>   - The reference-transaction hook cannot be used by us because we care
>     about the phase where it was committed already. And while the hook
>     fires in that phase, it does not allow the caller to modify the
>     result in any capacity.
>
>   - The post-receive and post-update hooks cannot be used as they run
>     too late, at the point where we have already reported success to the
>     client.
>

Yeah, this is worthwhile mentioning, I already have made the commit
message a lot more descriptive, so it does become bloated. I think it is
justified though, since more information is always more useful than less.

>> 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
>
> I'd drop "remote" here -- from the point of view of git-receive-pack(1)
> it really is the local repository.
>

Yeah, makes sense.

>
>> +the status report is sent back to the client.
>> +
>> +The hook receives the pkt-line encoded status report on standard input
>> +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.
>
> This assumes a bit too much about the implementation of the client, as
> it may not even be git-push(1) in the first place. We could still
> mention this, but we should say that this depends on the client.
>

Yeah I'll make it specific to git-push(1).

>> 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.
>
> We should probably document that we expect the hook to never return
> non-zero, even if it rejects reference updates, and that doing so
> indicates a bug. This is mostly because git-receive-pack(1) shouldn't
> ever just die on the client without giving it a proper status.
>

Yeah, this is a part I was thinking about but wasn't sure if it should
be added in because, we could also do an implementation where we simply
ignore the exit code of the hook.

>> +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.
>
> Good thing to call out.
>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 86933d8d7e..bc22b3ec31 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1004,6 +1004,41 @@ static int run_update_hook(struct command *cmd)
>>  	return code;
>>  }
>>
>> +static int run_report_hook(struct strbuf *report)
>> +{
>> +	struct child_process proc = CHILD_PROCESS_INIT;
>> +	struct async sideband_async;
>> +	int sideband_async_started = 0;
>> +	int saved_stderr = -1;
>> +	struct strbuf out = STRBUF_INIT;
>> +	const char *hook_path;
>> +	int code;
>
> Nit: I think it's more commont to call this `ret` rather than `code`.
>

I copied over another hook to start, and left the naming as is. I'm okay
with changing it to ret.

>> diff --git a/t/t5412-report-hook.sh b/t/t5412-report-hook.sh
>> new file mode 100755
>> index 0000000000..47f20e8d67
>> --- /dev/null
>> +++ b/t/t5412-report-hook.sh
>> @@ -0,0 +1,176 @@
>> +#!/bin/sh
>> +
>> +test_description='test report hook'
>> +
>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>> +
>> +. ./test-lib.sh
>> +
>> +. "$TEST_DIRECTORY"/t5411/common-functions.sh
>> +
>> +URL_PREFIX="\.\."
>
> I was about to say that this looks unused, but it's used by
> "common-functions.sh".
>

I didn't know about this too before.

> [snip]
>> +test_expect_success "hook stderr is relayed to client via sideband" '
>> +	test_when_finished "rm -rf upstream" &&
>> +	test_when_finished "git -C workbench remote remove origin" &&
>> +
>> +	git init --bare upstream &&
>> +	git -C workbench remote add origin ../upstream &&
>> +	git -C workbench push origin $A:refs/heads/main &&
>> +
>> +	test_hook -C upstream --setup report <<-\EOF &&
>> +	echo "hook-stderr-message" >&2
>> +	exit 1
>
> Should we maybe not exit abnormally here to see that the push succeeds?
>

The test right above 'hook can report a custom failure message', does
that exactly.

>> +	EOF
>> +
>> +	test_must_fail git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
>> +	test_grep "hook-stderr-message" out
>> +'
>
> This should have the "remote: " prefix, right? If so, should we verify
> that?
>

Yeah makes sense. let me add that.

Thanks for the review.


> Patrick
signature.asc (application/pgp-signature, 690 B)