Git development
 help / color / mirror / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Karthik Nayak <karthik.188@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] hook: introduce the report hook for git-receive-pack(1)
Date: Thu, 20 Aug 2026 10:50:06 +0100	[thread overview]
Message-ID: <29f39d8b-6cf0-4811-afb3-0a1656877f31@gmail.com> (raw)
In-Reply-To: <20260818-758-introduce-hook-v1-1-8a8d89e65838@gmail.com>

Hi Karthik

On 18/08/2026 08:55, Karthik Nayak wrote:
> 
> 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.

Config based hooks are about running more than one script to run per 
hook event, they're not about parallel execution per-se. Indeed the 
documentation for git hook notes

     Some hooks always run sequentially regardless of this flag or the
     hook.jobs config, because Git knows they cannot safely run in
     parallel: applypatch-msg, pre-commit, prepare-commit-msg, commit-
     msg, post-commit, post-checkout, and push-to-checkout.

I think the question the commit message should be answering is, whether 
a design like proc-receive that predates config based hooks and only 
allows a single hook script, makes sense now that we have config based 
hooks, or, if we were adding that functionality now, would we design it 
differently? I think the answer for server side hooks is that a design 
around a single script is probably reasonable but it would be worth 
discussing that in the commit message.

Thanks

Phillip



> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> 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.
> ---
>   Documentation/githooks.adoc |  23 ++++++
>   builtin/receive-pack.c      |  41 +++++++++++
>   t/meson.build               |   1 +
>   t/t5412-report-hook.sh      | 176 ++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 241 insertions(+)
> 
> 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
> +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.
> +
>   push-to-checkout
>   ~~~~~~~~~~~~~~~~
>   
> 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;
> +
> +	hook_path = find_hook(the_repository, "report");
> +	if (!hook_path)
> +		return 0;
> +
> +	strvec_push(&proc.args, hook_path);
> +	proc.trace2_hook_name = "report";
> +
> +	prepare_sideband_async(&sideband_async, &saved_stderr,
> +			       &sideband_async_started);
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	code = pipe_command(&proc, report->buf, report->len, &out,
> +			    report->len, NULL, 0);
> +	sigchain_pop(SIGPIPE);
> +
> +	finish_sideband_async(&sideband_async, saved_stderr,
> +			      sideband_async_started);
> +
> +	if (!code)
> +		strbuf_swap(&out, report);
> +
> +	strbuf_release(&out);
> +	return code;
> +}
> +
>   static struct command *find_command_by_refname(struct command *list,
>   					       const char *refname)
>   {
> @@ -2547,6 +2582,9 @@ static void report(struct command *commands, const char *unpack_status)
>   	}
>   	packet_buf_flush(&buf);
>   
> +	if (run_report_hook(&buf))
> +		die("report hook failed");
> +
>   	if (use_sideband)
>   		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
>   	else
> @@ -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");
> +
>   	if (use_sideband)
>   		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
>   	else
> diff --git a/t/meson.build b/t/meson.build
> index a25f37d2f5..7056e31326 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -651,6 +651,7 @@ integration_tests = [
>     't5409-colorize-remote-messages.sh',
>     't5410-receive-pack.sh',
>     't5411-proc-receive-hook.sh',
> +  't5412-report-hook.sh',
>     't5500-fetch-pack.sh',
>     't5501-fetch-push-alternates.sh',
>     't5502-quickfetch.sh',
> 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="\.\."
> +
> +test_expect_success "setup workbench" '
> +	git init workbench &&
> +	create_commits_in workbench A B
> +'
> +
> +test_expect_success "no report hook, push succeeds" '
> +	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 &&
> +	git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +
> +	make_user_friendly_and_stable_output <out >actual &&
> +	cat >expect <<-\EOF &&
> +	To ../upstream
> +	   <COMMIT-A>..<COMMIT-B>  <COMMIT-B> -> main
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success "passthrough does not alter report" '
> +	test_when_finished "rm -rf upstream" &&
> +	test_when_finished "git -C workbench remote remove origin" &&
> +	git init --bare upstream &&
> +
> +	test_hook -C upstream --setup report <<-\EOF &&
> +	cat
> +	EOF
> +
> +	git -C workbench remote add origin ../upstream &&
> +	git -C workbench push origin $A:refs/heads/main &&
> +	git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +
> +	make_user_friendly_and_stable_output <out >actual &&
> +	cat >expect <<-\EOF &&
> +	To ../upstream
> +	   <COMMIT-A>..<COMMIT-B>  <COMMIT-B> -> main
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success "non-zero exit causes receive-pack to die" '
> +	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 &&
> +	exit 1
> +	EOF
> +
> +	test_must_fail git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +	make_user_friendly_and_stable_output <out >actual &&
> +	cat >expect <<-\EOF &&
> +	fatal: report hook failed
> +	send-pack: unexpected disconnect while reading sideband packet
> +	fatal: the remote end hung up unexpectedly
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success "hook is invoked and receives report on stdin" '
> +	test_when_finished "rm -rf upstream" &&
> +	test_when_finished "git -C workbench remote remove origin" &&
> +
> +	git init --bare upstream &&
> +	test_hook -C upstream --setup report <<-EOF &&
> +	tee raw
> +	EOF
> +
> +	git -C workbench remote add origin ../upstream &&
> +	git -C workbench push origin $A:refs/heads/main &&
> +	git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +
> +	make_user_friendly_and_stable_output <out >actual &&
> +	cat >expect <<-EOF &&
> +	To ../upstream
> +	   <COMMIT-A>..<COMMIT-B>  <COMMIT-B> -> main
> +	EOF
> +	test_cmp expect actual &&
> +
> +	test-tool pkt-line unpack <upstream/raw >actual-report &&
> +	cat >expect-report <<-EOF &&
> +	unpack ok
> +	ok refs/heads/main
> +	0000
> +	EOF
> +	test_cmp expect-report actual-report
> +'
> +
> +test_expect_success "hook can modify the report sent to client" '
> +	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 &&
> +	test-tool pkt-line unpack |
> +	sed "s/^ok /ng /" |
> +	test-tool pkt-line pack
> +	EOF
> +
> +	test_must_fail git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +	make_user_friendly_and_stable_output <out >actual &&
> +	cat >expect <<-\EOF &&
> +	To ../upstream
> +	 ! [remote rejected] <COMMIT-B> -> main (failed)
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success "hook can report a custom failure message" '
> +	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 "push rejected: service X is down" >&2
> +	test-tool pkt-line unpack |
> +	sed "s/^ok \(.*\)/ng \1 service-x-is-down/" |
> +	test-tool pkt-line pack |
> +	tee raw
> +	EOF
> +
> +	test_must_fail git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +	test_grep "push rejected: service X is down" out &&
> +
> +	test-tool pkt-line unpack <upstream/raw >actual-report &&
> +	cat >expect-report <<-\EOF &&
> +	unpack ok
> +	ng refs/heads/main service-x-is-down
> +	0000
> +	EOF
> +	test_cmp expect-report actual-report
> +'
> +
> +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
> +	EOF
> +
> +	test_must_fail git -C workbench push origin $B:refs/heads/main >out 2>&1 &&
> +	test_grep "hook-stderr-message" out
> +'
> +
> +test_done
> 
> ---
> base-commit: 11c6700f10234578d10523faf35656ca491425c9
> change-id: 20260812-758-introduce-hook-5b3af9f1a7e8
> 
> 
> Thanks
> - Karthik
> 
> 


  parent reply	other threads:[~2026-08-20  9:50 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-18  7:55 [PATCH] hook: introduce the report hook for git-receive-pack(1) Karthik Nayak
2026-08-18 20:54 ` Junio C Hamano
2026-08-19  7:03 ` Kristoffer Haugsbakk
2026-08-19 12:11   ` Karthik Nayak
2026-08-19 14:47     ` Kristoffer Haugsbakk
2026-08-19  7:39 ` Patrick Steinhardt
2026-08-19 13:13   ` Karthik Nayak
2026-08-19 13:20     ` Patrick Steinhardt
2026-08-19 13:24       ` Karthik Nayak
2026-08-20  9:50 ` Phillip Wood [this message]
2026-08-20 15:43   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=29f39d8b-6cf0-4811-afb3-0a1656877f31@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=karthik.188@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox