Merge branch 'jk/push-deadlock-regression-fix' into maint

"git push" had a handful of codepaths that could lead to a deadlock
when unexpected error happened, which has been fixed.

* jk/push-deadlock-regression-fix:
  send-pack: report signal death of pack-objects
  send-pack: read "unpack" status even on pack-objects failure
  send-pack: improve unpack-status error messages
  send-pack: use skip_prefix for parsing unpack status
  send-pack: extract parsing of "unpack" response
  receive-pack: fix deadlock when we cannot create tmpdir

2 files changed, 40 insertions, 11 deletions

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1dbb8a0692..8672825e2e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1664,8 +1664,11 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 	}
 
 	tmp_objdir = tmp_objdir_create();
-	if (!tmp_objdir)
+	if (!tmp_objdir) {
+		if (err_fd > 0)
+			close(err_fd);
 		return "unable to create temporary object directory";
+	}
 	child.env = tmp_objdir_env(tmp_objdir);
 
 	/*
diff --git a/send-pack.c b/send-pack.c
index 6195b43e9a..d2d2a49a02 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -72,6 +72,7 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 	struct child_process po = CHILD_PROCESS_INIT;
 	FILE *po_in;
 	int i;
+	int rc;
 
 	i = 4;
 	if (args->use_thin_pack)
@@ -125,27 +126,44 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
 		po.out = -1;
 	}
 
-	if (finish_command(&po))
+	rc = finish_command(&po);
+	if (rc) {
+		/*
+		 * For a normal non-zero exit, we assume pack-objects wrote
+		 * something useful to stderr. For death by signal, though,
+		 * we should mention it to the user. The exception is SIGPIPE
+		 * (141), because that's a normal occurence if the remote end
+		 * hangs up (and we'll report that by trying to read the unpack
+		 * status).
+		 */
+		if (rc > 128 && rc != 141)
+			error("pack-objects died of signal %d", rc - 128);
 		return -1;
+	}
+	return 0;
+}
+
+static int receive_unpack_status(int in)
+{
+	const char *line = packet_read_line(in, NULL);
+	if (!skip_prefix(line, "unpack ", &line))
+		return error(_("unable to parse remote unpack status: %s"), line);
+	if (strcmp(line, "ok"))
+		return error(_("remote unpack failed: %s"), line);
 	return 0;
 }
 
 static int receive_status(int in, struct ref *refs)
 {
 	struct ref *hint;
-	int ret = 0;
-	char *line = packet_read_line(in, NULL);
-	if (!starts_with(line, "unpack "))
-		return error("did not receive remote status");
-	if (strcmp(line, "unpack ok")) {
-		error("unpack failed: %s", line + 7);
-		ret = -1;
-	}
+	int ret;
+
 	hint = NULL;
+	ret = receive_unpack_status(in);
 	while (1) {
 		char *refname;
 		char *msg;
-		line = packet_read_line(in, NULL);
+		char *line = packet_read_line(in, NULL);
 		if (!line)
 			break;
 		if (!starts_with(line, "ok ") && !starts_with(line, "ng ")) {
@@ -557,6 +575,14 @@ int send_pack(struct send_pack_args *args,
 				close(out);
 			if (git_connection_is_socket(conn))
 				shutdown(fd[0], SHUT_WR);
+
+			/*
+			 * Do not even bother with the return value; we know we
+			 * are failing, and just want the error() side effects.
+			 */
+			if (status_report)
+				receive_unpack_status(in);
+
 			if (use_sideband) {
 				close(demux.out);
 				finish_async(&demux);