send-pack: tighten remote error reporting

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 5fadd0bc95272f453ef68e194c540b777db443ee..14447bb7cd301bd693273734ce6b291f11bfcef6 100644 (file)
--- 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);
 }
 
        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));
 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)) {
        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) {
                ret = -1;
        }
        hint = NULL;
        while (1) {

+               char *refname;
+               char *msg;

                len = packet_read_line(in, line, sizeof(line));
                if (!len)
                        break;
                if (len < 3 ||
                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;
                }
                        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)
                if (hint)

-                       hint = set_ref_error(hint, line + 3);
+                       hint = find_ref_by_name(hint, refname);

                if (!hint)
                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;
 }
        }
        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:
                                        "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 ? NULL : ref->peer_ref,
+                                       ref->remote_status);
+                       break;
+               case REF_STATUS_EXPECTING_REPORT:
+                       print_ref_status('!', "[remote failure]", ref,
+                                       ref->deletion ? NULL : ref->peer_ref,
+                                       "remote failed to report status");

                        break;
                case REF_STATUS_OK:
                        print_ok_ref_status(ref);
                        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++;
                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);
 
                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);
                }
                                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);
        }
 
        packet_flush(out);


@@ -462,11 +475,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
        }
        close(out);
 
        }
        close(out);
 

-       if (expect_status_report) {
+       if (expect_status_report)

                ret = receive_status(in, remote_refs);
                ret = receive_status(in, remote_refs);

-               if (ret == -2)
-                       return -1;
-       }

        else
                ret = 0;
 
        else
                ret = 0;
 

diff --git a/cache.h b/cache.h
index 8d601dd6f79d75b8c3b8ac65a56f73cf9b20cedf..e0c1cc3fe86944e7b45926e0fff2ba4a4377ecf2 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -504,8 +504,9 @@ struct ref {
                REF_STATUS_REJECT_NODELETE,
                REF_STATUS_UPTODATE,
                REF_STATUS_REMOTE_REJECT,
                REF_STATUS_REJECT_NODELETE,
                REF_STATUS_UPTODATE,
                REF_STATUS_REMOTE_REJECT,

+               REF_STATUS_EXPECTING_REPORT,

        } status;
        } status;

-       char *error;
+       char *remote_status;

        struct ref *peer_ref; /* when renaming */
        char name[FLEX_ARRAY]; /* more */
 };
        struct ref *peer_ref; /* when renaming */
        char name[FLEX_ARRAY]; /* more */
 };