Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

On Tue, Aug 02, 2016 at 12:01:57PM -0400, Stephen Morton wrote:

Sounds tricky. I do think it's important, almost a 'data integrity' issue,
that IF a commit is received, THEN the post-receive hook must be run. Too
much mission-critical stuff is based on post-receive hooks.

I agree it would be a good property to have. I think it's hard to do
atomically, though. Certainly we can wait to tell the other side "your
push has been recorded" until after the hook is run. But we would
already have updated the refs locally at that point (and we must -- that
is part of the interface to the post-receive hooks, that the refs are
already in place). So would we roll-back the ref update then? Even that
suffers from power failures, etc.

So I'm not sure if making it truly atomic is all the feasible. However,
we could certainly make things more robust than they are now.

The simplest thing may be to just bump the post-receive hook before the
status report. That opens up the question of whether clients are
actually waiting already for the post-receive to finish. Looking at the
code in send-pack, it looks like the network is hooked up to the
sideband demuxer thread, which will read until EOF on the network. So we
are waiting either way for the post-receive to run. It doesn't really
matter if it happens before or after the report to the client.

So I _think_ something like this would work:

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 15c323a..91d01f0 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c

@@ -1767,9 +1767,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 		execute_commands(commands, unpack_status, &si);
 		if (pack_lockfile)
 			unlink_or_warn(pack_lockfile);
+		run_receive_hook(commands, "post-receive", 1);
 		if (report_status)
 			report(commands, unpack_status);
-		run_receive_hook(commands, "post-receive", 1);
 		run_update_post_hook(commands);
 		if (auto_gc) {
 			const char *argv_gc_auto[] = {

but maybe there are other gotchas.

-Peff