Subject: [RFC PATCH] t5411: fix broken pipe write error on proc-receive

From: Jiang Xin <[email-masked]>
Date: Thu, 5 Nov 2020 10:23:29 -0500

From: Jiang Xin <[email-masked]>

Johannes found a flaky hang in t5411 in the osx-clang job of the CI/PR
builds, and ran into this issue using `--stress` option with the
following errror 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 there is no such error message in output. It seems
that the expected error message is overridden by the broken pipe error
message. The broken pipe error is because "receive-pack" sends other
commands to the "proc-receive" hook, but the hook dies earlier.

To fix this issue, these changes are made:

1. In "receive-pack", close the input stream for the "proc-receive" hook
   before reading result from "proc-receive".

2. The test helper for "proc-receive" consumes the input stream before
   it die earlier.

Reported-by: Johannes Schindelin <[email-masked]>
Signed-off-by: Jiang Xin <[email-masked]>
---
 builtin/receive-pack.c            | 4 +++-
 t/helper/test-proc-receive.c      | 8 +++++---
 t/t5411/test-0013-bad-protocol.sh | 3 +--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bb9909c52e..6bd402897c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1172,6 +1172,7 @@ static int run_proc_receive_hook(struct command *commands,
 	if (version != 1) {
 		strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
 			    version);
+		close(proc.in);
 		code = -1;
 		goto cleanup;
 	}
@@ -1196,11 +1197,12 @@ static int run_proc_receive_hook(struct command *commands,
 		packet_flush(proc.in);
 	}
 
+	close(proc.in);
+
 	/* Read result from proc-receive */
 	code = read_proc_receive_report(&reader, commands, &errmsg);
 
 cleanup:
-	close(proc.in);
 	close(proc.out);
 	if (use_sideband)
 		finish_async(&muxer);
diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
index 42164d9898..132c74db52 100644
--- a/t/helper/test-proc-receive.c
+++ b/t/helper/test-proc-receive.c
@@ -79,9 +79,11 @@ static void proc_receive_read_commands(struct packet_reader *reader,
 		    *p++ != ' ' ||
 		    parse_oid_hex(p, &new_oid, &p) ||
 		    *p++ != ' ' ||
-		    die_readline)
+		    die_readline) {
+			while (packet_reader_read(reader) != PACKET_READ_EOF);
 			die("protocol error: expected 'old new ref', got '%s'",
-			    reader->line);
+				die_readline? "<call with --die-readline>": reader->line);
+		}
 		refname = p;
 		FLEX_ALLOC_STR(cmd, ref_name, refname);
 		oidcpy(&cmd->old_oid, &old_oid);
@@ -136,7 +138,7 @@ int cmd__proc_receive(int argc, const char **argv)
 		usage_msg_opt("Too many arguments.", proc_receive_usage, options);
 	packet_reader_init(&reader, 0, NULL, 0,
 			   PACKET_READ_CHOMP_NEWLINE |
-			   PACKET_READ_DIE_ON_ERR_PACKET);
+			   PACKET_READ_GENTLE_ON_EOF);
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 	proc_receive_verison(&reader);
diff --git a/t/t5411/test-0013-bad-protocol.sh b/t/t5411/test-0013-bad-protocol.sh
index c5fe4cb37b..ee75515430 100644
--- a/t/t5411/test-0013-bad-protocol.sh
+++ b/t/t5411/test-0013-bad-protocol.sh
@@ -91,8 +91,7 @@ test_expect_success "proc-receive: bad protocol (hook --die-readline, $PROTOCOL)
 		HEAD:refs/for/master/topic \
 		>out 2>&1 &&
 	make_user_friendly_and_stable_output <out >actual &&
-
-	grep "remote: fatal: protocol error: expected \"old new ref\", got \"<ZERO-OID> <COMMIT-A> refs/for/master/topic\"" actual &&
+	grep "remote: fatal: protocol error: expected \"old new ref\", got \"<call with --die-readline>\"" actual &&
 
 	git -C "$upstream" show-ref >out &&
 	make_user_friendly_and_stable_output <out >actual &&
-- 
2.29.0.dirty