Previously, we set all ref pushes to 'OK', and then marked
them as errors if the remote reported so. This has the
problem that if the remote dies or fails to report a ref, we
just assume it was OK.

Instead, we use a new non-OK state to indicate that we are
expecting status (if the remote doesn't support the
report-status feature, we fall back on the old behavior).
Thus we can flag refs for which we expected a status, but
got none (conversely, we now also print a warning for refs
for which we get a status, but weren't expecting one).

This also allows us to simplify the receive_status exit
code, since each ref is individually marked with failure
until we get a success response. We can just print the usual
status table, so the user still gets a sense of what we were
trying to do when the failure happened.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a lot more robust, and I think the code is easier to follow. The
ref->error member is now ref->remote_status, which is hopefully a bit
more obvious as to when it is set.

The ref matching is also slightly more micro-optimized, Junio. :)

 builtin-send-pack.c |   94 ++++++++++++++++++++++++++++-----------------------
 cache.h             |    3 +-
 2 files changed, 54 insertions(+), 43 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5fadd0b..349e02f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -146,60 +146,67 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static struct ref *set_ref_error(struct ref *refs, const char *line)
-{
-	struct ref *ref;
-
-	for (ref = refs; ref; ref = ref->next) {
-		const char *msg;
-		if (prefixcmp(line, ref->name))
-			continue;
-		msg = line + strlen(ref->name);
-		if (*msg++ != ' ')
-			continue;
-		ref->status = REF_STATUS_REMOTE_REJECT;
-		ref->error = xstrdup(msg);
-		ref->error[strlen(ref->error)-1] = '\0';
-		return ref;
-	}
-	return NULL;
-}
-
-/* a return value of -1 indicates that an error occurred,
- * but we were able to set individual ref errors. A return
- * value of -2 means we couldn't even get that far. */
 static int receive_status(int in, struct ref *refs)
 {
 	struct ref *hint;
 	char line[1000];
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
-	if (len < 10 || memcmp(line, "unpack ", 7)) {
-		fprintf(stderr, "did not receive status back\n");
-		return -2;
-	}
+	if (len < 10 || memcmp(line, "unpack ", 7))
+		return error("did not receive remote status");
 	if (memcmp(line, "unpack ok\n", 10)) {
-		fputs(line, stderr);
+		char *p = line + strlen(line) - 1;
+		if (*p == '\n')
+			*p = '\0';
+		error("unpack failed: %s", line + 7);
 		ret = -1;
 	}
 	hint = NULL;
 	while (1) {
+		char *refname;
+		char *msg;
 		len = packet_read_line(in, line, sizeof(line));
 		if (!len)
 			break;
 		if (len < 3 ||
-		    (memcmp(line, "ok", 2) && memcmp(line, "ng", 2))) {
+		    (memcmp(line, "ok ", 3) && memcmp(line, "ng ", 3))) {
 			fprintf(stderr, "protocol error: %s\n", line);
 			ret = -1;
 			break;
 		}
-		if (!memcmp(line, "ok", 2))
-			continue;
+
+		line[strlen(line)-1] = '\0';
+		refname = line + 3;
+		msg = strchr(refname, ' ');
+		if (msg)
+			*msg++ = '\0';
+
+		/* first try searching at our hint, falling back to all refs */
 		if (hint)
-			hint = set_ref_error(hint, line + 3);
+			hint = find_ref_by_name(hint, refname);
 		if (!hint)
-			hint = set_ref_error(refs, line + 3);
-		ret = -1;
+			hint = find_ref_by_name(refs, refname);
+		if (!hint) {
+			warning("remote reported status on unknown ref: %s",
+					refname);
+			continue;
+		}
+		if (hint->status != REF_STATUS_EXPECTING_REPORT) {
+			warning("remote reported status on unexpected ref: %s",
+					refname);
+			continue;
+		}
+
+		if (line[0] == 'o' && line[1] == 'k')
+			hint->status = REF_STATUS_OK;
+		else {
+			hint->status = REF_STATUS_REMOTE_REJECT;
+			ret = -1;
+		}
+		if (msg)
+			hint->remote_status = xstrdup(msg);
+		/* start our next search from the next ref */
+		hint = hint->next;
 	}
 	return ret;
 }
@@ -324,10 +331,14 @@ static void print_push_status(const char *dest, struct ref *refs)
 					"non-fast forward");
 			break;
 		case REF_STATUS_REMOTE_REJECT:
-			if (ref->deletion)
-				print_ref_status('!', "[remote rejected]", ref, NULL, ref->error);
-			else
-				print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
+			print_ref_status('!', "[remote rejected]", ref,
+					ref->deletion ? ref->peer_ref : NULL,
+					ref->remote_status);
+			break;
+		case REF_STATUS_EXPECTING_REPORT:
+			print_ref_status('!', "[remote failure]", ref,
+					ref->deletion ? ref->peer_ref : NULL,
+					"remote failed to report status");
 			break;
 		case REF_STATUS_OK:
 			print_ok_ref_status(ref);
@@ -434,7 +445,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		hashcpy(ref->new_sha1, new_sha1);
 		if (!ref->deletion)
 			new_refs++;
-		ref->status = REF_STATUS_OK;
 
 		if (!args.dry_run) {
 			char *old_hex = sha1_to_hex(ref->old_sha1);
@@ -451,6 +461,9 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 				packet_write(out, "%s %s %s",
 					old_hex, new_hex, ref->name);
 		}
+		ref->status = expect_status_report ?
+			REF_STATUS_EXPECTING_REPORT :
+			REF_STATUS_OK;
 	}
 
 	packet_flush(out);
@@ -462,11 +475,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	}
 	close(out);
 
-	if (expect_status_report) {
+	if (expect_status_report)
 		ret = receive_status(in, remote_refs);
-		if (ret == -2)
-			return -1;
-	}
 	else
 		ret = 0;
 
diff --git a/cache.h b/cache.h
index 1f3f113..0dff61a 100644
--- a/cache.h
+++ b/cache.h
@@ -511,8 +511,9 @@ struct ref {
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
+		REF_STATUS_EXPECTING_REPORT,
 	} status;
-	char *error;
+	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
 };
-- 
1.5.3.5.1817.g37c1a-dirty
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html