Re: [PATCH v3 2/2] receive-pack: gently write messages to proc-receive

[Jeff King <peff@peff.net>]

On Tue, Nov 10, 2020 at 08:01:35PM +0800, Jiang Xin wrote:

> Johannes found a flaky hang in `t5411/test-0013-bad-protocol.sh` in the
> osx-clang job of the CI/PR builds, and ran into an issue when using
> the `--stress` option with the following error messages:
> 
>     fatal: unable to write flush packet: Broken pipe
>     send-pack: unexpected disconnect while reading sideband packet
>     fatal: the remote end hung up unexpectedly
> 
> In this test case, the "proc-receive" hook sends an error message and
> dies earlier. While "receive-pack" on the other side of the pipe
> should forward the error message of the "proc-receive" hook to the
> client side, but it fails to do so. This is because "receive-pack"
> uses `packet_write_fmt()` and `packet_flush()` to write pkt-line
> message to "proc-receive" hook, and these functions die immediately
> when pipe is broken. Using "gently" forms for these functions will get
> more predicable output.

The changes to use gently() in the code looked good to me, and I think
you got all of the relevant spots.

I was surprised by a few bits:

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bb9909c52e..697a4e8802 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -974,9 +974,10 @@ static int read_proc_receive_report(struct packet_reader *reader,
>  	struct command *cmd;
>  	struct command *hint = NULL;
>  	struct ref_push_report *report = NULL;
> -	int new_report = 0;
>  	int code = 0;
> +	int new_report = 0;

This is just noise in the diff, I think. It does not matter either way.

> @@ -984,8 +985,14 @@ static int read_proc_receive_report(struct packet_reader *reader,
>  		const char *refname;
>  		char *p;
>  
> -		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
> +		if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
> +			if (!response) {
> +				strbuf_addstr(errmsg, "no response from proc-receive hook");
> +				return -1;
> +			}
>  			break;
> +		}
> +		response++;

This extra check seems orthogonal to the rest of the commit. It does
seem like it might be a reasonable thing to check, but I wondered:

  - if the hook has nothing to report, wouldn't it just send a flush
    packet? Does that break protocol or not?

  - if not, then I guess we're expecting a response for every ref we
    mentioned (and presumably we would not trigger the hook at all if
    there are no refs). But in that case, shouldn't we be checking that
    we counted up the number of responses we expected? But we already do
    that, by annotating the items in the commands list that didn't get
    RUN_PROC_RECEIVE_RETURNED set.

So at best, it seems like this check is redundant (and at worst it may
complain unnecessarily about a corner case).

> @@ -1100,7 +1107,7 @@ static int run_proc_receive_hook(struct command *commands,
>  	struct strbuf cap = STRBUF_INIT;
>  	struct strbuf errmsg = STRBUF_INIT;
>  	int hook_use_push_options = 0;
> -	int version = 0;
> +	int version = -1;
> [...]
> -	if (version != 1) {
> +	if (version == -1) {
> +		strbuf_addstr(&errmsg, "fail to negotiate version with proc-receive hook");
> +		code = -1;
> +		goto cleanup;
> +	} else if (version != 1) {
>  		strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
>  			    version);

Likewise this seems orthogonal to the main point of the patch. Though it
seems like a good idea in general to check when the other side doesn't
report a version (assuming it is a protocol breakage not to report the
version, and we're not simply supposed to default).

-Peff