Re: [PATCH v2] receive-pack: ignore SIGPIPE while reporting status to client

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2021, 21:10 UTC
Message-ID: <xmqqzgqd11dp.fsf@gitster.g>
In-Reply-To: <20211106220358.144886-1-robin@jarry.cc>

Robin Jarry <robin@jarry.cc> writes:

Show 12 quoted lines

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

Show 11 quoted lines

> 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?

Show 5 quoted lines

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