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

Patrick Steinhardt [off-list ref] writes:

On Tue, Aug 18, 2026 at 09:55:55AM +0200, Karthik Nayak wrote:

quoted

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.

quoted

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.

quoted

+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).

quoted

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.

quoted

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

quoted

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.

quoted

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]

quoted

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

quoted

+	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