Robin Jarry <robin@xxxxxxxx> writes: > When a remote client exits while the pre-receive hook is running, > receive-pack is not killed by SIGPIPE because the signal is ignored. > This is a side effect of commit ec7dbd145bd8 (receive-pack: allow hooks > to ignore its standard input stream, 2014-09-12). > > The pre-receive hook itself is not interrupted and does not receive any > error since its stdout is a pipe which is read in an async thread and > output back to the client socket in a side band channel. > > After the pre-receive has exited the SIGPIPE default handler is restored > and if the hook did not report any error, objects are migrated from > temporary to permanent storage. All of the above talks about the pre-receive hook, but it is unclear how that is relevant to this change. The first paragraph says "... is not killed", and if that was a bad thing (in other words, it should be killed but is not, and that is a bug worth fixing), and if this patch changes the behaviour, then that paragraph is worth saying. Similarly for the other two. > Before running the post-receive hook, status info is reported back to > the client. Since the client has died, receive-pack is killed by SIGPIPE > and post-receive is never executed. In other words, regardless of what happens (or does not happen) to the pre-receive hook, which may not even exist, if "git push" dies before the post-receive hook runs, this paragraph applies, no? What I am getting at is that this can (and should) be the first paragraph of the description without losing clarity. > Ignore SIGPIPE before reporting status to the client to increase the > chances of post-receive running if pre-receive was successful. This does > not guarantee 100% consistency but it should resist early disconnection > by the client. OK. > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 49b846d96052..5fe7992028d4 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -2564,12 +2564,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) > use_keepalive = KEEPALIVE_ALWAYS; > execute_commands(commands, unpack_status, &si, > &push_options); > + sigchain_push(SIGPIPE, SIG_IGN); > if (pack_lockfile) > unlink_or_warn(pack_lockfile); Shouldn't we start ignoring SIGPIPE here, not before we try to unlink the lockfile? > if (report_status_v2) > report_v2(commands, unpack_status); > else if (report_status) > report(commands, unpack_status); > + sigchain_pop(SIGPIPE); In other words, push/pop pair should surround the part that reports the status, as the proposed commit log message said. > run_receive_hook(commands, "post-receive", 1, > &push_options); > run_update_post_hook(commands); Other than these, looks good to me. Thanks.
