Re: [PATCH] hook: introduce the report hook for git-receive-pack(1)
"Kristoffer Haugsbakk" <[email protected]>
| Newsgroups | org.kernel.vger.git |
|---|---|
| Message-ID | <[email protected]> |
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. > --- > 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)) > > 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). > +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 ;) ). > + > 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. > + > 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)? > + git init --bare upstream && >[snip]