receive-pack: gently write messages to proc-receive

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.

Add more "--die-*" options to test helper to test different stages of
the protocol between "receive-pack" and "proc-receive" hook.

Reported-by: Johannes Schindelin <[email protected]>
Suggested-by: Jeff King <[email protected]>
Signed-off-by: Jiang Xin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: jiangxin

builtin/receive-pack.c

@@ -977,15 +977,25 @@ static int read_proc_receive_report(struct packet_reader *reader,
 	int new_report = 0;
 	int code = 0;
 	int once = 0;
+	int response = 0;
 
 	for (;;) {
 		struct object_id old_oid, new_oid;
 		const char *head;
 		const char *refname;
 		char *p;
-
-		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
+		enum packet_read_status status;
+
+		status = packet_reader_read(reader);
+		if (status != PACKET_READ_NORMAL) {
+			/* Check whether proc-receive exited abnormally */
+			if (status == PACKET_READ_EOF && !response) {
+				strbuf_addstr(errmsg, "proc-receive exited abnormally");
+				return -1;
+			}
 			break;
+		}
+		response++;
 
 		head = reader->line;
 		p = strchr(head, ' ');
@@ -1145,28 +1155,41 @@ static int run_proc_receive_hook(struct command *commands,
 	if (use_push_options)
 		strbuf_addstr(&cap, " push-options");
 	if (cap.len) {
-		packet_write_fmt(proc.in, "version=1%c%s\n", '\0', cap.buf + 1);
+		code = packet_write_fmt_gently(proc.in, "version=1%c%s\n", '\0', cap.buf + 1);
 		strbuf_release(&cap);
 	} else {
-		packet_write_fmt(proc.in, "version=1\n");
+		code = packet_write_fmt_gently(proc.in, "version=1\n");
 	}
-	packet_flush(proc.in);
+	if (!code)
+		code = packet_flush_gently(proc.in);
 
-	for (;;) {
-		int linelen;
+	if (!code)
+		for (;;) {
+			int linelen;
+			enum packet_read_status status;
 
-		if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
-			break;
+			status = packet_reader_read(&reader);
+			if (status != PACKET_READ_NORMAL) {
+				/* Check whether proc-receive exited abnormally */
+				if (status == PACKET_READ_EOF)
+					code = -1;
+				break;
+			}
 
-		if (reader.pktlen > 8 && starts_with(reader.line, "version=")) {
-			version = atoi(reader.line + 8);
-			linelen = strlen(reader.line);
-			if (linelen < reader.pktlen) {
-				const char *feature_list = reader.line + linelen + 1;
-				if (parse_feature_request(feature_list, "push-options"))
-					hook_use_push_options = 1;
+			if (reader.pktlen > 8 && starts_with(reader.line, "version=")) {
+				version = atoi(reader.line + 8);
+				linelen = strlen(reader.line);
+				if (linelen < reader.pktlen) {
+					const char *feature_list = reader.line + linelen + 1;
+					if (parse_feature_request(feature_list, "push-options"))
+						hook_use_push_options = 1;
+				}
 			}
 		}
+
+	if (code) {
+		strbuf_addstr(&errmsg, "fail to negotiate version with proc-receive hook");
+		goto cleanup;
 	}
 
 	if (version != 1) {
@@ -1180,20 +1203,36 @@ static int run_proc_receive_hook(struct command *commands,
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->run_proc_receive || cmd->skip_update || cmd->error_string)
 			continue;
-		packet_write_fmt(proc.in, "%s %s %s",
-				 oid_to_hex(&cmd->old_oid),
-				 oid_to_hex(&cmd->new_oid),
-				 cmd->ref_name);
+		code = packet_write_fmt_gently(proc.in, "%s %s %s",
+					       oid_to_hex(&cmd->old_oid),
+					       oid_to_hex(&cmd->new_oid),
+					       cmd->ref_name);
+		if (code)
+			break;
+	}
+	if (!code)
+		code = packet_flush_gently(proc.in);
+	if (code) {
+		strbuf_addstr(&errmsg, "fail to write commands to proc-receive hook");
+		goto cleanup;
 	}
-	packet_flush(proc.in);
 
 	/* Send push options */
 	if (hook_use_push_options) {
 		struct string_list_item *item;
 
-		for_each_string_list_item(item, push_options)
-			packet_write_fmt(proc.in, "%s", item->string);
-		packet_flush(proc.in);
+		for_each_string_list_item(item, push_options) {
+			code = packet_write_fmt_gently(proc.in, "%s", item->string);
+			if (code)
+				break;
+		}
+		if (!code)
+			code = packet_flush_gently(proc.in);
+		if (code) {
+			strbuf_addstr(&errmsg,
+				      "fail to write push-options to proc-receive hook");
+			goto cleanup;
+		}
 	}
 
 	/* Read result from proc-receive */

t/helper/test-proc-receive.c

@@ -10,8 +10,11 @@ static const char *proc_receive_usage[] = {
 	NULL
 };
 
-static int die_version;
-static int die_readline;
+static int die_read_version;
+static int die_write_version;
+static int die_read_commands;
+static int die_read_push_options;
+static int die_write_report;
 static int no_push_options;
 static int use_atomic;
 static int use_push_options;
@@ -33,6 +36,9 @@ struct command {
 static void proc_receive_verison(struct packet_reader *reader) {
 	int server_version = 0;
 
+	if (die_read_version)
+		die("die with the --die-read-version option");
+
 	for (;;) {
 		int linelen;
 
@@ -52,9 +58,12 @@ static void proc_receive_verison(struct packet_reader *reader) {
 		}
 	}
 
-	if (server_version != 1 || die_version)
+	if (server_version != 1)
 		die("bad protocol version: %d", server_version);
 
+	if (die_write_version)
+		die("die with the --die-write-version option");
+
 	packet_write_fmt(1, "version=%d%c%s\n",
 			 version, '\0',
 			 use_push_options && !no_push_options ? "push-options": "");
@@ -75,11 +84,13 @@ static void proc_receive_read_commands(struct packet_reader *reader,
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
 
+		if (die_read_commands)
+			die("die with the --die-read-commands option");
+
 		if (parse_oid_hex(reader->line, &old_oid, &p) ||
 		    *p++ != ' ' ||
 		    parse_oid_hex(p, &new_oid, &p) ||
-		    *p++ != ' ' ||
-		    die_readline)
+		    *p++ != ' ')
 			die("protocol error: expected 'old new ref', got '%s'",
 			    reader->line);
 		refname = p;
@@ -99,6 +110,9 @@ static void proc_receive_read_push_options(struct packet_reader *reader,
 	if (no_push_options || !use_push_options)
 	       return;
 
+	if (die_read_push_options)
+		die("die with the --die-read-push-options option");
+
 	while (1) {
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
@@ -117,10 +131,16 @@ int cmd__proc_receive(int argc, const char **argv)
 	struct option options[] = {
 		OPT_BOOL(0, "no-push-options", &no_push_options,
 			 "disable push options"),
-		OPT_BOOL(0, "die-version", &die_version,
-			 "die during version negotiation"),
-		OPT_BOOL(0, "die-readline", &die_readline,
-			 "die when readline"),
+		OPT_BOOL(0, "die-read-version", &die_read_version,
+			 "die when reading version"),
+		OPT_BOOL(0, "die-write-version", &die_write_version,
+			 "die when writing version"),
+		OPT_BOOL(0, "die-read-commands", &die_read_commands,
+			 "die when reading commands"),
+		OPT_BOOL(0, "die-read-push-options", &die_read_push_options,
+			 "die when reading push-options"),
+		OPT_BOOL(0, "die-write-report", &die_write_report,
+			 "die when writing report"),
 		OPT_STRING_LIST('r', "return", &returns, "old/new/ref/status/msg",
 				"return of results"),
 		OPT__VERBOSE(&verbose, "be verbose"),
@@ -136,7 +156,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);
@@ -166,6 +186,8 @@ int cmd__proc_receive(int argc, const char **argv)
 				fprintf(stderr, "proc-receive> %s\n", item->string);
 	}
 
+	if (die_write_report)
+		die("die with the --die-write-report option");
 	if (returns.nr)
 		for_each_string_list_item(item, &returns)
 			packet_write_fmt(1, "%s\n", item->string);