[PATCH] send-pack: improve error reporting for total remote unpack

[PATCH] send-pack: improve error reporting for total remote unpack

[Jeff King]

If the remote doesn't give us per-ref status reports, then
we pessimistically assume that all refs failed. However,
instead of just exiting the function, we now mark them
individually as failed.

This lets us print the usual status table, but refs which
failed in this way are marked individually (alongside refs
which may have failed for other reasons, like being rejected
for non-ff status).

Signed-off-by: Jeff King <peff@peff.net>
---
On Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:

> Hmm.  When we did not receive status, we cannot tell what
> succeeded or failed, but what we _can_ tell the user is which
> refs we attempted to push.  I wonder if robbing that information
> from the user with this "return -1" is a good idea.  Perhaps we
> would instead want to set the status of all the refs to error
> and call print_push_status() anyway?  I dunno.

I think this is a bit nicer. However, I noticed that it's hard to follow
these error paths, anyway. packet_read_line likes to die on bad input
anyway. And I couldn't get receive_pack to give me a bad unpack status;
instead, it just died with a fatal error and reported nothing.

> > OK. Since it is already in next, do you want a style fixup patch [for
> > the comment]?
> I do not think it is particularly a big deal -- perhaps clean it
> before we touch the vicinity of the code next time.  The same
> goes for the "} else {" stuff.

Well, it fixed itself here (and it looks like you tweaked the else
cuddling when you applied to next).

One other thing I noticed while doing this patch is that our remote ref
status reporting isn't foolproof.  We set every ref we send to 'OK' with
the nice effect that if status reporting isn't enabled, we just assume
that it worked. However, if the status coming back is truncated (i.e.,
some refs are missing in receive_status), we will just fail to notice and
assume all is well. So to be perfect, we would need a
REF_STATUS_EXPECTING_REPORT.

I can implement this if desired. OTOH, this isn't a new bug at all, and it
would be pretty tricky to trigger it (you would need to die exactly on a
line boundary).

 builtin-send-pack.c |   31 +++++++++++++++++++++----------
 cache.h             |    1 +
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5fadd0b..602e196 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -165,9 +165,14 @@ static struct ref *set_ref_error(struct ref *refs, const char *line)
 	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 void set_ref_error_all(struct ref *refs)
+{
+	struct ref *ref;
+	for (ref = refs; ref; ref = ref->next)
+		if (ref->status == REF_STATUS_OK)
+			ref->status = REF_STATUS_REMOTE_FAILURE;
+}
+
 static int receive_status(int in, struct ref *refs)
 {
 	struct ref *hint;
@@ -175,11 +180,15 @@ static int receive_status(int in, struct ref *refs)
 	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;
+		error("did not receive remote status");
+		set_ref_error_all(refs);
+		return -1;
 	}
 	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;
@@ -329,6 +338,11 @@ static void print_push_status(const char *dest, struct ref *refs)
 			else
 				print_ref_status('!', "[remote rejected]", ref, ref->peer_ref, ref->error);
 			break;
+		case REF_STATUS_REMOTE_FAILURE:
+			print_ref_status('!', "[remote failure]", ref,
+					ref->deletion ? ref->peer_ref : NULL,
+					NULL);
+			break;
 		case REF_STATUS_OK:
 			print_ok_ref_status(ref);
 			break;
@@ -462,11 +476,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 ba9178f..de011bf 100644
--- a/cache.h
+++ b/cache.h
@@ -511,6 +511,7 @@ struct ref {
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
+		REF_STATUS_REMOTE_FAILURE,
 	} status;
 	char *error;
 	struct ref *peer_ref; /* when renaming */
-- 
1.5.3.5.1815.g9445b-dirty

Re: [PATCH] send-pack: improve error reporting for total remote unpack

[Jeff King]

> Subject: [PATCH] send-pack: improve error reporting for total remote unpack

Oops, the subject was supposed to be "...total remote _failure_".

-Peff

Re: [PATCH] send-pack: improve error reporting for total remote unpack

[Jeff King]

On Sun, Nov 18, 2007 at 12:58:06AM -0500, Jeff King wrote:

> One other thing I noticed while doing this patch is that our remote ref
> status reporting isn't foolproof.  We set every ref we send to 'OK' with
> the nice effect that if status reporting isn't enabled, we just assume
> that it worked. However, if the status coming back is truncated (i.e.,
> some refs are missing in receive_status), we will just fail to notice and
> assume all is well. So to be perfect, we would need a
> REF_STATUS_EXPECTING_REPORT.

This turned out to be pretty easy, given the other recent changes, and I
think it actually makes the code a bit clearer. Please scrap the patch
to which I am replying, and I will post a replacement series in a
second.

-Peff

[PATCH 1/2] make "find_ref_by_name" a public function

[Jeff King]

This was a static in remote.c, but is generally useful.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  |    2 ++
 refs.c   |    8 ++++++++
 remote.c |    8 --------
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index ba9178f..1f3f113 100644
--- a/cache.h
+++ b/cache.h
@@ -521,6 +521,8 @@ struct ref {
 #define REF_HEADS	(1u << 1)
 #define REF_TAGS	(1u << 2)
 
+extern struct ref *find_ref_by_name(struct ref *list, const char *name);
+
 #define CONNECT_VERBOSE       (1u << 0)
 extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);
 extern int finish_connect(struct child_process *conn);
diff --git a/refs.c b/refs.c
index ae53254..54ec98d 100644
--- a/refs.c
+++ b/refs.c
@@ -1433,3 +1433,11 @@ int update_ref(const char *action, const char *refname,
 	}
 	return 0;
 }
+
+struct ref *find_ref_by_name(struct ref *list, const char *name)
+{
+	for ( ; list; list = list->next)
+		if (!strcmp(list->name, name))
+			return list;
+	return NULL;
+}
diff --git a/remote.c b/remote.c
index 09b7aad..bb01059 100644
--- a/remote.c
+++ b/remote.c
@@ -696,14 +696,6 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 	return -errs;
 }
 
-static struct ref *find_ref_by_name(struct ref *list, const char *name)
-{
-	for ( ; list; list = list->next)
-		if (!strcmp(list->name, name))
-			return list;
-	return NULL;
-}
-
 static const struct refspec *check_pattern_match(const struct refspec *rs,
 						 int rs_nr,
 						 const struct ref *src)
-- 
1.5.3.5.1817.g37c1a-dirty

[PATCH 2/2] send-pack: tighten remote error reporting

[Jeff King]

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

Re: [PATCH 2/2] send-pack: tighten remote error reporting

[Jeff King]

On Sun, Nov 18, 2007 at 02:16:52AM -0500, Jeff King wrote:

> +			print_ref_status('!', "[remote rejected]", ref,
> +					ref->deletion ? ref->peer_ref : NULL,
> +					ref->remote_status);

Gah, that should of course be:

  ref->deletion ? NULL : ref->peer_ref

> +		case REF_STATUS_EXPECTING_REPORT:
> +			print_ref_status('!', "[remote failure]", ref,
> +					ref->deletion ? ref->peer_ref : NULL,
> +					"remote failed to report status");

And the same here.

I had resisted making a test that checked the exact output format,
because such things are often a pain to keep up to date. But that's two
regressions in patches I've submitted that would have been caught. Maybe
I should just pay more attention.

-Peff

[PATCH 3/2] send-pack: fix "everything up-to-date" message

[Jeff King]

This has always been slightly inaccurate, since it used the
new_refs counter, which really meant "did we send any
objects," so deletions were not counted.

It has gotten even worse with recent patches, since we no
longer look at the 'ret' value, meaning we would say "up to
date" if non-ff pushes were rejected.

Instead, we now claim up to date iff every ref is either
unmatched or up to date. Any other case should already have
generated a status line.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-send-pack.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 14447bb..3aab89c 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -347,6 +347,20 @@ static void print_push_status(const char *dest, struct ref *refs)
 	}
 }
 
+static int refs_pushed(struct ref *ref)
+{
+	for (; ref; ref = ref->next) {
+		switch(ref->status) {
+		case REF_STATUS_NONE:
+		case REF_STATUS_UPTODATE:
+			break;
+		default:
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)
 {
 	struct ref *ref;
@@ -487,7 +501,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 			update_tracking_ref(remote, ref);
 	}
 
-	if (!new_refs)
+	if (!refs_pushed(remote_refs))
 		fprintf(stderr, "Everything up-to-date\n");
 	if (ret < 0)
 		return ret;
-- 
1.5.3.5.1817.gd2b4b-dirty