{"thread":{"id":"10911","subject":"[PATCH] send-pack: improve error reporting for total remote unpack","startedAt":"2007-11-18T05:58:06Z","lastAt":"2007-11-18T08:08:22Z","messageCount":7,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60204","messageId":"20071118055804.GA19313@sigill.intra.peff.net","threadId":"10911","inReplyTo":null,"subject":"[PATCH] send-pack: improve error reporting for total remote unpack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T05:58:06Z","receivedAt":"2007-11-18T05:58:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the remote doesn't give us per-ref status reports, then\nwe pessimistically assume that all refs failed. However,\ninstead of just exiting the function, we now mark them\nindividually as failed.\n\nThis lets us print the usual status table, but refs which\nfailed in this way are marked individually (alongside refs\nwhich may have failed for other reasons, like being rejected\nfor non-ff status).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:\n\n> Hmm.  When we did not receive status, we cannot tell what\n> succeeded or failed, but what we _can_ tell the user is which\n> refs we attempted to push.  I wonder if robbing that information\n> from the user with this \"return -1\" is a good idea.  Perhaps we\n> would instead want to set the status of all the refs to error\n> and call print_push_status() anyway?  I dunno.\n\nI think this is a bit nicer. However, I noticed that it's hard to follow\nthese error paths, anyway. packet_read_line likes to die on bad input\nanyway. And I couldn't get receive_pack to give me a bad unpack status;\ninstead, it just died with a fatal error and reported nothing.\n\n> > OK. Since it is already in next, do you want a style fixup patch [for\n> > the comment]?\n> I do not think it is particularly a big deal -- perhaps clean it\n> before we touch the vicinity of the code next time.  The same\n> goes for the \"} else {\" stuff.\n\nWell, it fixed itself here (and it looks like you tweaked the else\ncuddling when you applied to next).\n\nOne other thing I noticed while doing this patch is that our remote ref\nstatus reporting isn't foolproof.  We set every ref we send to 'OK' with\nthe nice effect that if status reporting isn't enabled, we just assume\nthat it worked. However, if the status coming back is truncated (i.e.,\nsome refs are missing in receive_status), we will just fail to notice and\nassume all is well. So to be perfect, we would need a\nREF_STATUS_EXPECTING_REPORT.\n\nI can implement this if desired. OTOH, this isn't a new bug at all, and it\nwould be pretty tricky to trigger it (you would need to die exactly on a\nline boundary).\n\n builtin-send-pack.c |   31 +++++++++++++++++++++----------\n cache.h             |    1 +\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 5fadd0b..602e196 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -165,9 +165,14 @@ static struct ref *set_ref_error(struct ref *refs, const char *line)\n \treturn NULL;\n }\n \n-/* a return value of -1 indicates that an error occurred,\n- * but we were able to set individual ref errors. A return\n- * value of -2 means we couldn't even get that far. */\n+static void set_ref_error_all(struct ref *refs)\n+{\n+\tstruct ref *ref;\n+\tfor (ref = refs; ref; ref = ref->next)\n+\t\tif (ref->status == REF_STATUS_OK)\n+\t\t\tref->status = REF_STATUS_REMOTE_FAILURE;\n+}\n+\n static int receive_status(int in, struct ref *refs)\n {\n \tstruct ref *hint;\n@@ -175,11 +180,15 @@ static int receive_status(int in, struct ref *refs)\n \tint ret = 0;\n \tint len = packet_read_line(in, line, sizeof(line));\n \tif (len < 10 || memcmp(line, \"unpack \", 7)) {\n-\t\tfprintf(stderr, \"did not receive status back\\n\");\n-\t\treturn -2;\n+\t\terror(\"did not receive remote status\");\n+\t\tset_ref_error_all(refs);\n+\t\treturn -1;\n \t}\n \tif (memcmp(line, \"unpack ok\\n\", 10)) {\n-\t\tfputs(line, stderr);\n+\t\tchar *p = line + strlen(line) - 1;\n+\t\tif (*p == '\\n')\n+\t\t\t*p = '\\0';\n+\t\terror(\"unpack failed: %s\", line + 7);\n \t\tret = -1;\n \t}\n \thint = NULL;\n@@ -329,6 +338,11 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t\t\telse\n \t\t\t\tprint_ref_status('!', \"[remote rejected]\", ref, ref->peer_ref, ref->error);\n \t\t\tbreak;\n+\t\tcase REF_STATUS_REMOTE_FAILURE:\n+\t\t\tprint_ref_status('!', \"[remote failure]\", ref,\n+\t\t\t\t\tref->deletion ? ref->peer_ref : NULL,\n+\t\t\t\t\tNULL);\n+\t\t\tbreak;\n \t\tcase REF_STATUS_OK:\n \t\t\tprint_ok_ref_status(ref);\n \t\t\tbreak;\n@@ -462,11 +476,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t}\n \tclose(out);\n \n-\tif (expect_status_report) {\n+\tif (expect_status_report)\n \t\tret = receive_status(in, remote_refs);\n-\t\tif (ret == -2)\n-\t\t\treturn -1;\n-\t}\n \telse\n \t\tret = 0;\n \ndiff --git a/cache.h b/cache.h\nindex ba9178f..de011bf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -511,6 +511,7 @@ struct ref {\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n+\t\tREF_STATUS_REMOTE_FAILURE,\n \t} status;\n \tchar *error;\n \tstruct ref *peer_ref; /* when renaming */\n-- \n1.5.3.5.1815.g9445b-dirty\n"},{"id":"60205","messageId":"20071118055920.GA19539@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118055804.GA19313@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: improve error reporting for total remote unpack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T05:59:21Z","receivedAt":"2007-11-18T05:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"> Subject: [PATCH] send-pack: improve error reporting for total remote unpack\n\nOops, the subject was supposed to be \"...total remote _failure_\".\n\n-Peff\n"},{"id":"60206","messageId":"20071118070910.GA23410@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118055804.GA19313@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: improve error reporting for total remote unpack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T07:09:11Z","receivedAt":"2007-11-18T07:09:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 18, 2007 at 12:58:06AM -0500, Jeff King wrote:\n\n> One other thing I noticed while doing this patch is that our remote ref\n> status reporting isn't foolproof.  We set every ref we send to 'OK' with\n> the nice effect that if status reporting isn't enabled, we just assume\n> that it worked. However, if the status coming back is truncated (i.e.,\n> some refs are missing in receive_status), we will just fail to notice and\n> assume all is well. So to be perfect, we would need a\n> REF_STATUS_EXPECTING_REPORT.\n\nThis turned out to be pretty easy, given the other recent changes, and I\nthink it actually makes the code a bit clearer. Please scrap the patch\nto which I am replying, and I will post a replacement series in a\nsecond.\n\n-Peff\n"},{"id":"60207","messageId":"20071118071308.GA18467@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118055804.GA19313@sigill.intra.peff.net","subject":"[PATCH 1/2] make \"find_ref_by_name\" a public function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T07:13:10Z","receivedAt":"2007-11-18T07:13:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This was a static in remote.c, but is generally useful.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h  |    2 ++\n refs.c   |    8 ++++++++\n remote.c |    8 --------\n 3 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ba9178f..1f3f113 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -521,6 +521,8 @@ struct ref {\n #define REF_HEADS\t(1u << 1)\n #define REF_TAGS\t(1u << 2)\n \n+extern struct ref *find_ref_by_name(struct ref *list, const char *name);\n+\n #define CONNECT_VERBOSE       (1u << 0)\n extern struct child_process *git_connect(int fd[2], const char *url, const char *prog, int flags);\n extern int finish_connect(struct child_process *conn);\ndiff --git a/refs.c b/refs.c\nindex ae53254..54ec98d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1433,3 +1433,11 @@ int update_ref(const char *action, const char *refname,\n \t}\n \treturn 0;\n }\n+\n+struct ref *find_ref_by_name(struct ref *list, const char *name)\n+{\n+\tfor ( ; list; list = list->next)\n+\t\tif (!strcmp(list->name, name))\n+\t\t\treturn list;\n+\treturn NULL;\n+}\ndiff --git a/remote.c b/remote.c\nindex 09b7aad..bb01059 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -696,14 +696,6 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n \treturn -errs;\n }\n \n-static struct ref *find_ref_by_name(struct ref *list, const char *name)\n-{\n-\tfor ( ; list; list = list->next)\n-\t\tif (!strcmp(list->name, name))\n-\t\t\treturn list;\n-\treturn NULL;\n-}\n-\n static const struct refspec *check_pattern_match(const struct refspec *rs,\n \t\t\t\t\t\t int rs_nr,\n \t\t\t\t\t\t const struct ref *src)\n-- \n1.5.3.5.1817.g37c1a-dirty\n"},{"id":"60208","messageId":"20071118071651.GB18467@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118055804.GA19313@sigill.intra.peff.net","subject":"[PATCH 2/2] send-pack: tighten remote error reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T07:16:52Z","receivedAt":"2007-11-18T07:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously, we set all ref pushes to 'OK', and then marked\nthem as errors if the remote reported so. This has the\nproblem that if the remote dies or fails to report a ref, we\njust assume it was OK.\n\nInstead, we use a new non-OK state to indicate that we are\nexpecting status (if the remote doesn't support the\nreport-status feature, we fall back on the old behavior).\nThus we can flag refs for which we expected a status, but\ngot none (conversely, we now also print a warning for refs\nfor which we get a status, but weren't expecting one).\n\nThis also allows us to simplify the receive_status exit\ncode, since each ref is individually marked with failure\nuntil we get a success response. We can just print the usual\nstatus table, so the user still gets a sense of what we were\ntrying to do when the failure happened.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a lot more robust, and I think the code is easier to follow. The\nref->error member is now ref->remote_status, which is hopefully a bit\nmore obvious as to when it is set.\n\nThe ref matching is also slightly more micro-optimized, Junio. :)\n\n builtin-send-pack.c |   94 ++++++++++++++++++++++++++++-----------------------\n cache.h             |    3 +-\n 2 files changed, 54 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 5fadd0b..349e02f 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -146,60 +146,67 @@ static void get_local_heads(void)\n \tfor_each_ref(one_local_ref, NULL);\n }\n \n-static struct ref *set_ref_error(struct ref *refs, const char *line)\n-{\n-\tstruct ref *ref;\n-\n-\tfor (ref = refs; ref; ref = ref->next) {\n-\t\tconst char *msg;\n-\t\tif (prefixcmp(line, ref->name))\n-\t\t\tcontinue;\n-\t\tmsg = line + strlen(ref->name);\n-\t\tif (*msg++ != ' ')\n-\t\t\tcontinue;\n-\t\tref->status = REF_STATUS_REMOTE_REJECT;\n-\t\tref->error = xstrdup(msg);\n-\t\tref->error[strlen(ref->error)-1] = '\\0';\n-\t\treturn ref;\n-\t}\n-\treturn NULL;\n-}\n-\n-/* a return value of -1 indicates that an error occurred,\n- * but we were able to set individual ref errors. A return\n- * value of -2 means we couldn't even get that far. */\n static int receive_status(int in, struct ref *refs)\n {\n \tstruct ref *hint;\n \tchar line[1000];\n \tint ret = 0;\n \tint len = packet_read_line(in, line, sizeof(line));\n-\tif (len < 10 || memcmp(line, \"unpack \", 7)) {\n-\t\tfprintf(stderr, \"did not receive status back\\n\");\n-\t\treturn -2;\n-\t}\n+\tif (len < 10 || memcmp(line, \"unpack \", 7))\n+\t\treturn error(\"did not receive remote status\");\n \tif (memcmp(line, \"unpack ok\\n\", 10)) {\n-\t\tfputs(line, stderr);\n+\t\tchar *p = line + strlen(line) - 1;\n+\t\tif (*p == '\\n')\n+\t\t\t*p = '\\0';\n+\t\terror(\"unpack failed: %s\", line + 7);\n \t\tret = -1;\n \t}\n \thint = NULL;\n \twhile (1) {\n+\t\tchar *refname;\n+\t\tchar *msg;\n \t\tlen = packet_read_line(in, line, sizeof(line));\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (len < 3 ||\n-\t\t    (memcmp(line, \"ok\", 2) && memcmp(line, \"ng\", 2))) {\n+\t\t    (memcmp(line, \"ok \", 3) && memcmp(line, \"ng \", 3))) {\n \t\t\tfprintf(stderr, \"protocol error: %s\\n\", line);\n \t\t\tret = -1;\n \t\t\tbreak;\n \t\t}\n-\t\tif (!memcmp(line, \"ok\", 2))\n-\t\t\tcontinue;\n+\n+\t\tline[strlen(line)-1] = '\\0';\n+\t\trefname = line + 3;\n+\t\tmsg = strchr(refname, ' ');\n+\t\tif (msg)\n+\t\t\t*msg++ = '\\0';\n+\n+\t\t/* first try searching at our hint, falling back to all refs */\n \t\tif (hint)\n-\t\t\thint = set_ref_error(hint, line + 3);\n+\t\t\thint = find_ref_by_name(hint, refname);\n \t\tif (!hint)\n-\t\t\thint = set_ref_error(refs, line + 3);\n-\t\tret = -1;\n+\t\t\thint = find_ref_by_name(refs, refname);\n+\t\tif (!hint) {\n+\t\t\twarning(\"remote reported status on unknown ref: %s\",\n+\t\t\t\t\trefname);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (hint->status != REF_STATUS_EXPECTING_REPORT) {\n+\t\t\twarning(\"remote reported status on unexpected ref: %s\",\n+\t\t\t\t\trefname);\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (line[0] == 'o' && line[1] == 'k')\n+\t\t\thint->status = REF_STATUS_OK;\n+\t\telse {\n+\t\t\thint->status = REF_STATUS_REMOTE_REJECT;\n+\t\t\tret = -1;\n+\t\t}\n+\t\tif (msg)\n+\t\t\thint->remote_status = xstrdup(msg);\n+\t\t/* start our next search from the next ref */\n+\t\thint = hint->next;\n \t}\n \treturn ret;\n }\n@@ -324,10 +331,14 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t\t\t\t\t\"non-fast forward\");\n \t\t\tbreak;\n \t\tcase REF_STATUS_REMOTE_REJECT:\n-\t\t\tif (ref->deletion)\n-\t\t\t\tprint_ref_status('!', \"[remote rejected]\", ref, NULL, ref->error);\n-\t\t\telse\n-\t\t\t\tprint_ref_status('!', \"[remote rejected]\", ref, ref->peer_ref, ref->error);\n+\t\t\tprint_ref_status('!', \"[remote rejected]\", ref,\n+\t\t\t\t\tref->deletion ? ref->peer_ref : NULL,\n+\t\t\t\t\tref->remote_status);\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_EXPECTING_REPORT:\n+\t\t\tprint_ref_status('!', \"[remote failure]\", ref,\n+\t\t\t\t\tref->deletion ? ref->peer_ref : NULL,\n+\t\t\t\t\t\"remote failed to report status\");\n \t\t\tbreak;\n \t\tcase REF_STATUS_OK:\n \t\t\tprint_ok_ref_status(ref);\n@@ -434,7 +445,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\thashcpy(ref->new_sha1, new_sha1);\n \t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n-\t\tref->status = REF_STATUS_OK;\n \n \t\tif (!args.dry_run) {\n \t\t\tchar *old_hex = sha1_to_hex(ref->old_sha1);\n@@ -451,6 +461,9 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t\t\tpacket_write(out, \"%s %s %s\",\n \t\t\t\t\told_hex, new_hex, ref->name);\n \t\t}\n+\t\tref->status = expect_status_report ?\n+\t\t\tREF_STATUS_EXPECTING_REPORT :\n+\t\t\tREF_STATUS_OK;\n \t}\n \n \tpacket_flush(out);\n@@ -462,11 +475,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t}\n \tclose(out);\n \n-\tif (expect_status_report) {\n+\tif (expect_status_report)\n \t\tret = receive_status(in, remote_refs);\n-\t\tif (ret == -2)\n-\t\t\treturn -1;\n-\t}\n \telse\n \t\tret = 0;\n \ndiff --git a/cache.h b/cache.h\nindex 1f3f113..0dff61a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -511,8 +511,9 @@ struct ref {\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n+\t\tREF_STATUS_EXPECTING_REPORT,\n \t} status;\n-\tchar *error;\n+\tchar *remote_status;\n \tstruct ref *peer_ref; /* when renaming */\n \tchar name[FLEX_ARRAY]; /* more */\n };\n-- \n1.5.3.5.1817.g37c1a-dirty\n"},{"id":"60209","messageId":"20071118075908.GA24972@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118071651.GB18467@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] send-pack: tighten remote error reporting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T07:59:09Z","receivedAt":"2007-11-18T07:59:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 18, 2007 at 02:16:52AM -0500, Jeff King wrote:\n\n> +\t\t\tprint_ref_status('!', \"[remote rejected]\", ref,\n> +\t\t\t\t\tref->deletion ? ref->peer_ref : NULL,\n> +\t\t\t\t\tref->remote_status);\n\nGah, that should of course be:\n\n  ref->deletion ? NULL : ref->peer_ref\n\n> +\t\tcase REF_STATUS_EXPECTING_REPORT:\n> +\t\t\tprint_ref_status('!', \"[remote failure]\", ref,\n> +\t\t\t\t\tref->deletion ? ref->peer_ref : NULL,\n> +\t\t\t\t\t\"remote failed to report status\");\n\nAnd the same here.\n\nI had resisted making a test that checked the exact output format,\nbecause such things are often a pain to keep up to date. But that's two\nregressions in patches I've submitted that would have been caught. Maybe\nI should just pay more attention.\n\n-Peff\n"},{"id":"60210","messageId":"20071118080820.GA25358@sigill.intra.peff.net","threadId":"10911","inReplyTo":"20071118055804.GA19313@sigill.intra.peff.net","subject":"[PATCH 3/2] send-pack: fix \"everything up-to-date\" message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T08:08:22Z","receivedAt":"2007-11-18T08:08:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This has always been slightly inaccurate, since it used the\nnew_refs counter, which really meant \"did we send any\nobjects,\" so deletions were not counted.\n\nIt has gotten even worse with recent patches, since we no\nlonger look at the 'ret' value, meaning we would say \"up to\ndate\" if non-ff pushes were rejected.\n\nInstead, we now claim up to date iff every ref is either\nunmatched or up to date. Any other case should already have\ngenerated a status line.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c |   16 +++++++++++++++-\n 1 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 14447bb..3aab89c 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -347,6 +347,20 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t}\n }\n \n+static int refs_pushed(struct ref *ref)\n+{\n+\tfor (; ref; ref = ref->next) {\n+\t\tswitch(ref->status) {\n+\t\tcase REF_STATUS_NONE:\n+\t\tcase REF_STATUS_UPTODATE:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static int do_send_pack(int in, int out, struct remote *remote, const char *dest, int nr_refspec, const char **refspec)\n {\n \tstruct ref *ref;\n@@ -487,7 +501,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t\tupdate_tracking_ref(remote, ref);\n \t}\n \n-\tif (!new_refs)\n+\tif (!refs_pushed(remote_refs))\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n \tif (ret < 0)\n \t\treturn ret;\n-- \n1.5.3.5.1817.gd2b4b-dirty\n"}]}