{"thread":{"id":"10904","subject":"[PATCH v3 0/3] tracking per-ref errors on push","startedAt":"2007-11-17T12:53:24Z","lastAt":"2007-11-18T05:00:34Z","messageCount":20,"participants":["Jeff King","Alex Riesen","Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"60135","messageId":"20071117125323.GA23125@sigill.intra.peff.net","threadId":"10904","inReplyTo":null,"subject":"[PATCH v3 0/3] tracking per-ref errors on push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-17T12:53:24Z","receivedAt":"2007-11-17T12:53:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are cleaned-up versions of the previous patches I sent. The\nimprovements are:\n\n  1/3 send-pack: track errors for each ref\n    - there was a t5404 regression because I removed the is_null_sha1()\n      check before updating a tracking ref (but the replacement fix\n      doesn't come until 2/3). Even though this check is not correct,\n      it's better to fix it all at once correctly in 2/3.\n    - clarified the desired git-push exit code in t5404\n    - I renamed the struct elements to (hopefully) be a bit more obvious\n    - readability cleanups (fixed some very long lines, hoisted some\n      code into its own functions)\n\n  2/3 send-pack: check ref->status before updating tracking refs\n    - moved in fix from 1/3 mentioned above\n    - add test for deleting tracking branches, which was broken in next\n      but fixed by this patch\n\n  3/3 send-pack: assign remote errors to each ref\n    - squashed optimization patch\n    - remove bogus parsing drawback in commit message\n    - add test\n\nI'm hoping to get feedback from the cc'd people:\n  - Alex: please OK the modifications to t5404\n  - Pierre: this should fix the tracking ref update issues you reported\n  - Daniel: a general OK, since I am mangling your code :)\n\n-Peff\n"},{"id":"60136","messageId":"20071117125426.GA23186@sigill.intra.peff.net","threadId":"10904","inReplyTo":"20071117125323.GA23125@sigill.intra.peff.net","subject":"[PATCH 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-17T12:54:27Z","receivedAt":"2007-11-17T12:54:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Instead of keeping the 'ret' variable, we instead have a\nstatus flag for each ref that tracks what happened to it.\nWe then print the ref status after all of the refs have\nbeen examined.\n\nThis paves the way for three improvements:\n  - updating tracking refs only for non-error refs\n  - incorporating remote rejection into the printed status\n  - printing errors in a different order than we processed\n    (e.g., consolidating non-ff errors near the end with\n    a special message)\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c          |  225 +++++++++++++++++++++++++-----------------\n cache.h                      |   13 ++-\n t/t5404-tracking-branches.sh |   14 ++-\n 3 files changed, 157 insertions(+), 95 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 418925e..90ca2d3 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -207,8 +207,9 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)\n \t}\n }\n \n-static const char *prettify_ref(const char *name)\n+static const char *prettify_ref(const struct ref *ref)\n {\n+\tconst char *name = ref->name;\n \treturn name + (\n \t\t!prefixcmp(name, \"refs/heads/\") ? 11 :\n \t\t!prefixcmp(name, \"refs/tags/\") ? 10 :\n@@ -218,15 +219,105 @@ static const char *prettify_ref(const char *name)\n \n #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n \n+static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n+{\n+\tfprintf(stderr, \" %c %-*s \", flag, SUMMARY_WIDTH, summary);\n+\tif (from)\n+\t\tfprintf(stderr, \"%s -> %s\", prettify_ref(from), prettify_ref(to));\n+\telse\n+\t\tfputs(prettify_ref(to), stderr);\n+\tif (msg) {\n+\t\tfputs(\" (\", stderr);\n+\t\tfputs(msg, stderr);\n+\t\tfputc(')', stderr);\n+\t}\n+\tfputc('\\n', stderr);\n+}\n+\n+static const char *status_abbrev(unsigned char sha1[20])\n+{\n+\tconst char *abbrev;\n+\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n+\treturn abbrev ? abbrev : sha1_to_hex(sha1);\n+}\n+\n+static void print_ok_ref_status(struct ref *ref)\n+{\n+\tif (ref->deletion)\n+\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n+\telse if (is_null_sha1(ref->old_sha1))\n+\t\tprint_ref_status('*',\n+\t\t\t(!prefixcmp(ref->name, \"refs/tags/\") ? \"[new tag]\" :\n+\t\t\t  \"[new branch]\"),\n+\t\t\tref, ref->peer_ref, NULL);\n+\telse {\n+\t\tchar quickref[83];\n+\t\tchar type;\n+\t\tconst char *msg;\n+\n+\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n+\t\tif (ref->nonfastforward) {\n+\t\t\tstrcat(quickref, \"...\");\n+\t\t\ttype = '+';\n+\t\t\tmsg = \" (forced update)\";\n+\t\t}\n+\t\telse {\n+\t\t\tstrcat(quickref, \"..\");\n+\t\t\ttype = ' ';\n+\t\t\tmsg = NULL;\n+\t\t}\n+\t\tstrcat(quickref, status_abbrev(ref->new_sha1));\n+\n+\t\tprint_ref_status(type, quickref, ref, ref->peer_ref, msg);\n+\t}\n+}\n+\n+static void print_push_status(const char *dest, struct ref *refs)\n+{\n+\tstruct ref *ref;\n+\tint shown_dest = 0;\n+\n+\tfor (ref = refs; ref; ref = ref->next) {\n+\t\tif (!ref->status)\n+\t\t\tcontinue;\n+\t\tif (ref->status == REF_STATUS_UPTODATE && !args.verbose)\n+\t\t\tcontinue;\n+\n+\t\tif (!shown_dest) {\n+\t\t\tfprintf(stderr, \"To %s\\n\", dest);\n+\t\t\tshown_dest = 1;\n+\t\t}\n+\n+\t\tswitch(ref->status) {\n+\t\tcase REF_STATUS_NONE:\n+\t\t\tprint_ref_status('X', \"[no match]\", ref, NULL, NULL);\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_REJECT_NODELETE:\n+\t\t\tprint_ref_status('!', \"[rejected]\", ref, NULL,\n+\t\t\t\t\t\"remote does not support deleting refs\");\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_UPTODATE:\n+\t\t\tprint_ref_status('=', \"[up to date]\", ref,\n+\t\t\t\t\tref->peer_ref, NULL);\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n+\t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\"non-fast forward\");\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_OK:\n+\t\t\tprint_ok_ref_status(ref);\n+\t\t\tbreak;\n+\t\t}\n+\t}\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 \tint new_refs;\n-\tint ret = 0;\n \tint ask_for_status_report = 0;\n \tint allow_deleting_refs = 0;\n \tint expect_status_report = 0;\n-\tint shown_dest = 0;\n \tint flags = MATCH_REFS_NONE;\n \n \tif (args.send_all)\n@@ -262,10 +353,6 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t */\n \tnew_refs = 0;\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n-\t\tchar old_hex[60], *new_hex;\n-\t\tint will_delete_ref;\n-\t\tconst char *pretty_ref;\n-\t\tconst char *pretty_peer = NULL; /* only used when not deleting */\n \t\tconst unsigned char *new_sha1;\n \n \t\tif (!ref->peer_ref) {\n@@ -276,29 +363,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\telse\n \t\t\tnew_sha1 = ref->peer_ref->new_sha1;\n \n-\t\tif (!shown_dest) {\n-\t\t\tfprintf(stderr, \"To %s\\n\", dest);\n-\t\t\tshown_dest = 1;\n-\t\t}\n-\n-\t\twill_delete_ref = is_null_sha1(new_sha1);\n \n-\t\tpretty_ref = prettify_ref(ref->name);\n-\t\tif (!will_delete_ref)\n-\t\t\tpretty_peer = prettify_ref(ref->peer_ref->name);\n-\n-\t\tif (will_delete_ref && !allow_deleting_refs) {\n-\t\t\tfprintf(stderr, \" ! %-*s %s (remote does not support deleting refs)\\n\",\n-\t\t\t\t\tSUMMARY_WIDTH, \"[rejected]\", pretty_ref);\n-\t\t\tret = -2;\n+\t\tref->deletion = is_null_sha1(new_sha1);\n+\t\tif (ref->deletion && !allow_deleting_refs) {\n+\t\t\tref->status = REF_STATUS_REJECT_NODELETE;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!will_delete_ref &&\n+\t\tif (!ref->deletion &&\n \t\t    !hashcmp(ref->old_sha1, new_sha1)) {\n-\t\t\tif (args.verbose)\n-\t\t\t\tfprintf(stderr, \" = %-*s %s -> %s\\n\",\n-\t\t\t\t\tSUMMARY_WIDTH, \"[up to date]\",\n-\t\t\t\t\tpretty_peer, pretty_ref);\n+\t\t\tref->status = REF_STATUS_UPTODATE;\n \t\t\tcontinue;\n \t\t}\n \n@@ -321,33 +394,26 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t\t *     always allowed.\n \t\t */\n \n-\t\tif (!args.force_update &&\n-\t\t    !will_delete_ref &&\n+\t\tref->nonfastforward =\n+\t\t    !ref->deletion &&\n \t\t    !is_null_sha1(ref->old_sha1) &&\n-\t\t    !ref->force) {\n-\t\t\tif (!has_sha1_file(ref->old_sha1) ||\n-\t\t\t    !ref_newer(new_sha1, ref->old_sha1)) {\n-\t\t\t\t/* We do not have the remote ref, or\n-\t\t\t\t * we know that the remote ref is not\n-\t\t\t\t * an ancestor of what we are trying to\n-\t\t\t\t * push.  Either way this can be losing\n-\t\t\t\t * commits at the remote end and likely\n-\t\t\t\t * we were not up to date to begin with.\n-\t\t\t\t */\n-\t\t\t\tfprintf(stderr, \" ! %-*s %s -> %s (non-fast forward)\\n\",\n-\t\t\t\t\t\tSUMMARY_WIDTH, \"[rejected]\",\n-\t\t\t\t\t\tpretty_peer, pretty_ref);\n-\t\t\t\tret = -2;\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t    (!has_sha1_file(ref->old_sha1)\n+\t\t      || !ref_newer(new_sha1, ref->old_sha1));\n+\n+\t\tif (ref->nonfastforward && !ref->force && !args.force_update) {\n+\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n+\t\t\tcontinue;\n \t\t}\n+\n \t\thashcpy(ref->new_sha1, new_sha1);\n-\t\tif (!will_delete_ref)\n+\t\tif (!ref->deletion)\n \t\t\tnew_refs++;\n-\t\tstrcpy(old_hex, sha1_to_hex(ref->old_sha1));\n-\t\tnew_hex = sha1_to_hex(ref->new_sha1);\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+\t\t\tchar *new_hex = sha1_to_hex(ref->new_sha1);\n+\n \t\t\tif (ask_for_status_report) {\n \t\t\t\tpacket_write(out, \"%s %s %s%c%s\",\n \t\t\t\t\told_hex, new_hex, ref->name, 0,\n@@ -359,64 +425,43 @@ 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\tif (will_delete_ref)\n-\t\t\tfprintf(stderr, \" - %-*s %s\\n\",\n-\t\t\t\tSUMMARY_WIDTH, \"[deleting]\",\n-\t\t\t\tpretty_ref);\n-\t\telse if (is_null_sha1(ref->old_sha1)) {\n-\t\t\tconst char *msg;\n-\n-\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n-\t\t\t\tmsg = \"[new tag]\";\n-\t\t\telse\n-\t\t\t\tmsg = \"[new branch]\";\n-\t\t\tfprintf(stderr, \" * %-*s %s -> %s\\n\",\n-\t\t\t\tSUMMARY_WIDTH, msg,\n-\t\t\t\tpretty_peer, pretty_ref);\n-\t\t}\n-\t\telse {\n-\t\t\tchar quickref[83];\n-\t\t\tchar type = ' ';\n-\t\t\tconst char *msg = \"\";\n-\t\t\tconst char *old_abb;\n-\t\t\told_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);\n-\t\t\tstrcpy(quickref, old_abb ? old_abb : old_hex);\n-\t\t\tif (ref_newer(ref->peer_ref->new_sha1, ref->old_sha1))\n-\t\t\t\tstrcat(quickref, \"..\");\n-\t\t\telse {\n-\t\t\t\tstrcat(quickref, \"...\");\n-\t\t\t\ttype = '+';\n-\t\t\t\tmsg = \" (forced update)\";\n-\t\t\t}\n-\t\t\tstrcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));\n-\n-\t\t\tfprintf(stderr, \" %c %-*s %s -> %s%s\\n\",\n-\t\t\t\ttype,\n-\t\t\t\tSUMMARY_WIDTH, quickref,\n-\t\t\t\tpretty_peer, pretty_ref,\n-\t\t\t\tmsg);\n-\t\t}\n \t}\n \n \tpacket_flush(out);\n-\tif (new_refs && !args.dry_run)\n-\t\tret = pack_objects(out, remote_refs);\n+\tif (new_refs && !args.dry_run) {\n+\t\tif (pack_objects(out, remote_refs) < 0) {\n+\t\t\tclose(out);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n \tclose(out);\n \n+\tprint_push_status(dest, remote_refs);\n+\n \tif (expect_status_report) {\n \t\tif (receive_status(in))\n-\t\t\tret = -4;\n+\t\t\treturn -1;\n \t}\n \n-\tif (!args.dry_run && remote && ret == 0) {\n+\tif (!args.dry_run && remote) {\n \t\tfor (ref = remote_refs; ref; ref = ref->next)\n \t\t\tif (!is_null_sha1(ref->new_sha1))\n \t\t\t\tupdate_tracking_ref(remote, ref);\n \t}\n \n-\tif (!new_refs && ret == 0)\n+\tif (!new_refs)\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n-\treturn ret;\n+\tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tswitch (ref->status) {\n+\t\tcase REF_STATUS_NONE:\n+\t\tcase REF_STATUS_UPTODATE:\n+\t\tcase REF_STATUS_OK:\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\treturn 0;\n }\n \n static void verify_remote_names(int nr_heads, const char **heads)\ndiff --git a/cache.h b/cache.h\nindex e9b3ce3..2768342 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -500,8 +500,17 @@ struct ref {\n \tstruct ref *next;\n \tunsigned char old_sha1[20];\n \tunsigned char new_sha1[20];\n-\tunsigned char force;\n-\tunsigned char merge;\n+\tunsigned char force : 1;\n+\tunsigned char merge : 1;\n+\tunsigned char nonfastforward : 1;\n+\tunsigned char deletion : 1;\n+\tenum {\n+\t\tREF_STATUS_NONE = 0,\n+\t\tREF_STATUS_OK,\n+\t\tREF_STATUS_REJECT_NONFASTFORWARD,\n+\t\tREF_STATUS_REJECT_NODELETE,\n+\t\tREF_STATUS_UPTODATE,\n+\t} status;\n \tstruct ref *peer_ref; /* when renaming */\n \tchar name[FLEX_ARRAY]; /* more */\n };\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex 20718d4..799e47e 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -19,7 +19,7 @@ test_expect_success 'setup' '\n \tgit commit -a -m b2\n '\n \n-test_expect_success 'check tracking branches updated correctly after push' '\n+test_expect_success 'prepare pushable branches' '\n \tcd aa &&\n \tb1=$(git rev-parse origin/b1) &&\n \tb2=$(git rev-parse origin/b2) &&\n@@ -31,8 +31,16 @@ test_expect_success 'check tracking branches updated correctly after push' '\n \tgit commit -a -m aa-b2 &&\n \tgit checkout master &&\n \techo aa-master >>file &&\n-\tgit commit -a -m aa-master &&\n-\tgit push &&\n+\tgit commit -a -m aa-master\n+'\n+\n+test_expect_success 'mixed-success push returns error' '! git push'\n+\n+test_expect_success 'check tracking branches updated correctly after push' '\n+\ttest \"$(git rev-parse origin/master)\" = \"$(git rev-parse master)\"\n+'\n+\n+test_expect_success 'check tracking branches not updated for failed refs' '\n \ttest \"$(git rev-parse origin/b1)\" = \"$b1\" &&\n \ttest \"$(git rev-parse origin/b2)\" = \"$b2\"\n '\n-- \n1.5.3.5.1795.gf2a4e-dirty\n"},{"id":"60137","messageId":"20071117125515.GB23186@sigill.intra.peff.net","threadId":"10904","inReplyTo":"20071117125323.GA23125@sigill.intra.peff.net","subject":"[PATCH 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-17T12:55:15Z","receivedAt":"2007-11-17T12:55:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously, we manually checked the 'NONE' and 'UPTODATE'\nconditions. Now that we have ref->status, we can easily\nsay \"only update if we pushed successfully\".\n\nThis adds a test for and fixes a regression introduced in\ned31df31 where deleted refs did not have their tracking\nbranches removed. This was due to a bogus per-ref error test\nthat is superseded by the more accurate ref->status flag.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c          |   18 +++++-------------\n t/t5404-tracking-branches.sh |    5 +++++\n 2 files changed, 10 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 90ca2d3..c7d07aa 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -180,24 +180,17 @@ static int receive_status(int in)\n static void update_tracking_ref(struct remote *remote, struct ref *ref)\n {\n \tstruct refspec rs;\n-\tint will_delete_ref;\n \n-\trs.src = ref->name;\n-\trs.dst = NULL;\n-\n-\tif (!ref->peer_ref)\n+\tif (ref->status != REF_STATUS_OK)\n \t\treturn;\n \n-\twill_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);\n-\n-\tif (!will_delete_ref &&\n-\t\t\t!hashcmp(ref->old_sha1, ref->peer_ref->new_sha1))\n-\t\treturn;\n+\trs.src = ref->name;\n+\trs.dst = NULL;\n \n \tif (!remote_find_tracking(remote, &rs)) {\n \t\tif (args.verbose)\n \t\t\tfprintf(stderr, \"updating local tracking ref '%s'\\n\", rs.dst);\n-\t\tif (is_null_sha1(ref->peer_ref->new_sha1)) {\n+\t\tif (ref->deletion) {\n \t\t\tif (delete_ref(rs.dst, NULL))\n \t\t\t\terror(\"Failed to delete\");\n \t\t} else\n@@ -445,8 +438,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \n \tif (!args.dry_run && remote) {\n \t\tfor (ref = remote_refs; ref; ref = ref->next)\n-\t\t\tif (!is_null_sha1(ref->new_sha1))\n-\t\t\t\tupdate_tracking_ref(remote, ref);\n+\t\t\tupdate_tracking_ref(remote, ref);\n \t}\n \n \tif (!new_refs)\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex 799e47e..1493a92 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -45,4 +45,9 @@ test_expect_success 'check tracking branches not updated for failed refs' '\n \ttest \"$(git rev-parse origin/b2)\" = \"$b2\"\n '\n \n+test_expect_success 'deleted branches have their tracking branches removed' '\n+\tgit push origin :b1 &&\n+\ttest \"$(git rev-parse origin/b1)\" = \"origin/b1\"\n+'\n+\n test_done\n-- \n1.5.3.5.1795.gf2a4e-dirty\n"},{"id":"60138","messageId":"20071117125602.GC23186@sigill.intra.peff.net","threadId":"10904","inReplyTo":"20071117125323.GA23125@sigill.intra.peff.net","subject":"[PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-17T12:56:03Z","receivedAt":"2007-11-17T12:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This lets us show remote errors (e.g., a denied hook) along\nwith the usual push output.\n\nThere is a slightly clever optimization in receive_status\nthat bears explanation. We need to correlate the returned\nstatus and our ref objects, which naively could be an O(m*n)\noperation. However, since the current implementation of\nreceive-pack returns the errors to us in the same order that\nwe sent them, we optimistically look for the next ref to be\nlooked up to come after the last one we have found. So it\nshould be an O(m+n) merge if the receive-pack behavior\nholds, but we fall back to a correct but slower behavior if\nit should change.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c       |   51 +++++++++++++++++++++++++++++++++++++++-----\n cache.h                   |    2 +\n t/t5406-remote-rejects.sh |   24 +++++++++++++++++++++\n 3 files changed, 71 insertions(+), 6 deletions(-)\n create mode 100755 t/t5406-remote-rejects.sh\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex c7d07aa..bcf7143 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -146,19 +146,43 @@ static void get_local_heads(void)\n \tfor_each_ref(one_local_ref, NULL);\n }\n \n-static int receive_status(int in)\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 -1;\n+\t\treturn -2;\n \t}\n \tif (memcmp(line, \"unpack ok\\n\", 10)) {\n \t\tfputs(line, stderr);\n \t\tret = -1;\n \t}\n+\thint = NULL;\n \twhile (1) {\n \t\tlen = packet_read_line(in, line, sizeof(line));\n \t\tif (!len)\n@@ -171,7 +195,10 @@ static int receive_status(int in)\n \t\t}\n \t\tif (!memcmp(line, \"ok\", 2))\n \t\t\tcontinue;\n-\t\tfputs(line, stderr);\n+\t\tif (hint)\n+\t\t\thint = set_ref_error(hint, line + 3);\n+\t\tif (!hint)\n+\t\t\thint = set_ref_error(refs, line + 3);\n \t\tret = -1;\n \t}\n \treturn ret;\n@@ -297,6 +324,12 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\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\tbreak;\n \t\tcase REF_STATUS_OK:\n \t\t\tprint_ok_ref_status(ref);\n \t\t\tbreak;\n@@ -312,6 +345,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \tint allow_deleting_refs = 0;\n \tint expect_status_report = 0;\n \tint flags = MATCH_REFS_NONE;\n+\tint ret;\n \n \tif (args.send_all)\n \t\tflags |= MATCH_REFS_ALL;\n@@ -429,12 +463,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \t}\n \tclose(out);\n \n-\tprint_push_status(dest, remote_refs);\n-\n \tif (expect_status_report) {\n-\t\tif (receive_status(in))\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+\n+\tprint_push_status(dest, remote_refs);\n \n \tif (!args.dry_run && remote) {\n \t\tfor (ref = remote_refs; ref; ref = ref->next)\n@@ -443,6 +480,8 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \n \tif (!new_refs)\n \t\tfprintf(stderr, \"Everything up-to-date\\n\");\n+\tif (ret < 0)\n+\t\treturn ret;\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\ndiff --git a/cache.h b/cache.h\nindex 2768342..ba9178f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -510,7 +510,9 @@ struct ref {\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n+\t\tREF_STATUS_REMOTE_REJECT,\n \t} status;\n+\tchar *error;\n \tstruct ref *peer_ref; /* when renaming */\n \tchar name[FLEX_ARRAY]; /* more */\n };\ndiff --git a/t/t5406-remote-rejects.sh b/t/t5406-remote-rejects.sh\nnew file mode 100755\nindex 0000000..46b2cb4\n--- /dev/null\n+++ b/t/t5406-remote-rejects.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='remote push rejects are reported by client'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir .git/hooks &&\n+\t(echo \"#!/bin/sh\" ; echo \"exit 1\") >.git/hooks/update &&\n+\tchmod +x .git/hooks/update &&\n+\techo 1 >file &&\n+\tgit add file &&\n+\tgit commit -m 1 &&\n+\tgit clone . child &&\n+\tcd child &&\n+\techo 2 >file &&\n+\tgit commit -a -m 2\n+'\n+\n+test_expect_success 'push reports error' '! git push 2>stderr'\n+\n+test_expect_success 'individual ref reports error' 'grep rejected stderr'\n+\n+test_done\n-- \n1.5.3.5.1795.gf2a4e-dirty\n"},{"id":"60140","messageId":"20071117133444.GB2716@steel.home","threadId":"10904","inReplyTo":"20071117125426.GA23186@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-17T13:34:44Z","receivedAt":"2007-11-17T13:34:44Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Sat, Nov 17, 2007 13:54:27 +0100:\n> Instead of keeping the 'ret' variable, we instead have a\n> status flag for each ref that tracks what happened to it.\n> We then print the ref status after all of the refs have\n> been examined.\n> \n> This paves the way for three improvements:\n>   - updating tracking refs only for non-error refs\n>   - incorporating remote rejection into the printed status\n>   - printing errors in a different order than we processed\n>     (e.g., consolidating non-ff errors near the end with\n>     a special message)\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nAcked-by: Alex Riesen <raa.lkml@gmail.com>\n\nStill would like to see the deletion of tracing branches checked.\nLike this perhaps?\n\ndiff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\nindex 799e47e..4fe4a07 100755\n--- a/t/t5404-tracking-branches.sh\n+++ b/t/t5404-tracking-branches.sh\n@@ -10,6 +10,7 @@ test_expect_success 'setup' '\n \tgit commit -m 1 &&\n \tgit branch b1 &&\n \tgit branch b2 &&\n+\tgit branch b3 &&\n \tgit clone . aa &&\n \tgit checkout b1 &&\n \techo b1 >>file &&\n@@ -23,6 +24,7 @@ test_expect_success 'prepare pushable branches' '\n \tcd aa &&\n \tb1=$(git rev-parse origin/b1) &&\n \tb2=$(git rev-parse origin/b2) &&\n+\tb3=$(git rev-parse origin/b3) &&\n \tgit checkout -b b1 origin/b1 &&\n \techo aa-b1 >>file &&\n \tgit commit -a -m aa-b1 &&\n@@ -45,4 +47,12 @@ test_expect_success 'check tracking branches not updated for failed refs' '\n \ttest \"$(git rev-parse origin/b2)\" = \"$b2\"\n '\n \n+test_expect_success 'delete remote branch' '\n+\tgit push origin :refs/heads/b3 &&\n+\t{\n+\t\tgit rev-parse --verify origin/b3\n+\t\ttest $? != 0\n+        }\n+'\n+\n test_done\n-- \n1.5.3.5.747.gf06543\n"},{"id":"60141","messageId":"20071117134546.GC2716@steel.home","threadId":"10904","inReplyTo":"20071117125515.GB23186@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-17T13:45:46Z","receivedAt":"2007-11-17T13:45:46Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Sat, Nov 17, 2007 13:55:15 +0100:\n> diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh\n> index 799e47e..1493a92 100755\n> --- a/t/t5404-tracking-branches.sh\n> +++ b/t/t5404-tracking-branches.sh\n> @@ -45,4 +45,9 @@ test_expect_success 'check tracking branches not updated for failed refs' '\n>  \ttest \"$(git rev-parse origin/b2)\" = \"$b2\"\n>  '\n>  \n> +test_expect_success 'deleted branches have their tracking branches removed' '\n> +\tgit push origin :b1 &&\n> +\ttest \"$(git rev-parse origin/b1)\" = \"origin/b1\"\n> +'\n> +\n>  test_done\n\nOh, missed that.\n\nCompletely-Acked-By: Alex \"Sleepy\" Riesen <raa.lkml@gmail.com>\n"},{"id":"60142","messageId":"20071117135322.GA3992@sigill.intra.peff.net","threadId":"10904","inReplyTo":"20071117134546.GC2716@steel.home","subject":"Re: [PATCH 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-17T13:53:22Z","receivedAt":"2007-11-17T13:53:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 02:45:46PM +0100, Alex Riesen wrote:\n\n> > +test_expect_success 'deleted branches have their tracking branches removed' '\n> > +\tgit push origin :b1 &&\n> > +\ttest \"$(git rev-parse origin/b1)\" = \"origin/b1\"\n> > +'\n> > +\n> >  test_done\n> \n> Oh, missed that.\n> \n> Completely-Acked-By: Alex \"Sleepy\" Riesen <raa.lkml@gmail.com>\n\nYes, I didn't put it in the first patch, because it was broken there. ;)\n\n-Peff\n"},{"id":"60155","messageId":"Pine.LNX.4.64.0711171258000.12193@iabervon.org","threadId":"10904","inReplyTo":"20071117125602.GC23186@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-17T18:05:29Z","receivedAt":"2007-11-17T18:05:29Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 17 Nov 2007, Jeff King wrote:\n\n> This lets us show remote errors (e.g., a denied hook) along\n> with the usual push output.\n> \n> There is a slightly clever optimization in receive_status\n> that bears explanation. We need to correlate the returned\n> status and our ref objects, which naively could be an O(m*n)\n> operation. However, since the current implementation of\n> receive-pack returns the errors to us in the same order that\n> we sent them, we optimistically look for the next ref to be\n> looked up to come after the last one we have found. So it\n> should be an O(m+n) merge if the receive-pack behavior\n> holds, but we fall back to a correct but slower behavior if\n> it should change.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin-send-pack.c       |   51 +++++++++++++++++++++++++++++++++++++++-----\n>  cache.h                   |    2 +\n>  t/t5406-remote-rejects.sh |   24 +++++++++++++++++++++\n>  3 files changed, 71 insertions(+), 6 deletions(-)\n>  create mode 100755 t/t5406-remote-rejects.sh\n> \n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index c7d07aa..bcf7143 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -146,19 +146,43 @@ static void get_local_heads(void)\n>  \tfor_each_ref(one_local_ref, NULL);\n>  }\n>  \n> -static int receive_status(int in)\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\nMaybe this should take both the full list and the hint and do both passes \ninternally? IMHO, the logic in receive_status() looks like it might be \nsetting the error twice or not at all, unless you read very carefully.\n\nBut, regardless,\n\nAcked-by: Daniel Barkalow <barkalow@iabervon.org>\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"60153","messageId":"Pine.LNX.4.64.0711171251030.12193@iabervon.org","threadId":"10904","inReplyTo":"20071117125515.GB23186@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-17T18:05:32Z","receivedAt":"2007-11-17T18:05:32Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 17 Nov 2007, Jeff King wrote:\n\n> Previously, we manually checked the 'NONE' and 'UPTODATE'\n> conditions. Now that we have ref->status, we can easily\n> say \"only update if we pushed successfully\".\n> \n> This adds a test for and fixes a regression introduced in\n> ed31df31 where deleted refs did not have their tracking\n> branches removed. This was due to a bogus per-ref error test\n> that is superseded by the more accurate ref->status flag.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n\nAcked-by: Daniel Barkalow <barkalow@iabervon.org>\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"60154","messageId":"Pine.LNX.4.64.0711171217200.12193@iabervon.org","threadId":"10904","inReplyTo":"20071117125426.GA23186@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-11-17T18:05:35Z","receivedAt":"2007-11-17T18:05:35Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 17 Nov 2007, Jeff King wrote:\n\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index 418925e..90ca2d3 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -218,15 +219,105 @@ static const char *prettify_ref(const char *name)\n>  \n>  #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n>  \n> +static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n\nIsn't \"from\" always \"to->peer_ref\"? It'd be nice to make this function \nunable to print something different from what we actually did. (Actually \nit might be \"to->deletion ? NULL : to->peer_ref\", but that would also be \nbetter to have as an explicit feature of how you display \"to\", rather than \nimplicit in the set of callers.\n\n> +static const char *status_abbrev(unsigned char sha1[20])\n> +{\n> +\tconst char *abbrev;\n> +\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n> +\treturn abbrev ? abbrev : sha1_to_hex(sha1);\n> +}\n\nMaybe we should have a find_unique_abbrev()-like function that doesn't \nmind if the requested object doesn't exist?\n\n> +\n> +static void print_ok_ref_status(struct ref *ref)\n> +{\n> +\tif (ref->deletion)\n> +\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n> +\telse if (is_null_sha1(ref->old_sha1))\n> +\t\tprint_ref_status('*',\n> +\t\t\t(!prefixcmp(ref->name, \"refs/tags/\") ? \"[new tag]\" :\n> +\t\t\t  \"[new branch]\"),\n> +\t\t\tref, ref->peer_ref, NULL);\n> +\telse {\n> +\t\tchar quickref[83];\n\nShouldn't this be 40 + 3 + 40 + 1?\n\n> +\t\tchar type;\n> +\t\tconst char *msg;\n> +\n> +\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n> +\t\tif (ref->nonfastforward) {\n> +\t\t\tstrcat(quickref, \"...\");\n> +\t\t\ttype = '+';\n> +\t\t\tmsg = \" (forced update)\";\n> +\t\t}\n> +\t\telse {\n\nCoding style, IIRC.\n\nRegardless of these nits, it all looks good to me.\n\nAcked-by: Daniel Barkalow <barkalow@iabervon.org>\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"60169","messageId":"7v6400twzp.fsf@gitster.siamese.dyndns.org","threadId":"10904","inReplyTo":"20071117125426.GA23186@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-17T20:53:14Z","receivedAt":"2007-11-17T20:53:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Instead of keeping the 'ret' variable, we instead have a\n> status flag for each ref that tracks what happened to it.\n> We then print the ref status after all of the refs have\n> been examined.\n>\n> This paves the way for three improvements:\n>   - updating tracking refs only for non-error refs\n>   - incorporating remote rejection into the printed status\n>   - printing errors in a different order than we processed\n>     (e.g., consolidating non-ff errors near the end with\n>     a special message)\n\nLooks good.\n\nExcept that\n\n> @@ -218,15 +219,105 @@ static const char *prettify_ref(const char *name)\n>  \n>  #define SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n>  \n> +static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n> +{\n> +\tfprintf(stderr, \" %c %-*s \", flag, SUMMARY_WIDTH, summary);\n> +\tif (from)\n> +\t\tfprintf(stderr, \"%s -> %s\", prettify_ref(from), prettify_ref(to));\n> +\telse\n> +\t\tfputs(prettify_ref(to), stderr);\n> +\tif (msg) {\n> +\t\tfputs(\" (\", stderr);\n> +\t\tfputs(msg, stderr);\n> +\t\tfputc(')', stderr);\n> +\t}\n> +\tfputc('\\n', stderr);\n> +}\n\nmsg is parenthesized here, so...\n\n> +static void print_ok_ref_status(struct ref *ref)\n> +{\n> +\tif (ref->deletion)\n> +\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n> +\telse if (is_null_sha1(ref->old_sha1))\n> +\t\tprint_ref_status('*',\n> +\t\t\t(!prefixcmp(ref->name, \"refs/tags/\") ? \"[new tag]\" :\n> +\t\t\t  \"[new branch]\"),\n> +\t\t\tref, ref->peer_ref, NULL);\n> +\telse {\n> +\t\tchar quickref[83];\n> +\t\tchar type;\n> +\t\tconst char *msg;\n> +\n> +\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n> +\t\tif (ref->nonfastforward) {\n> +\t\t\tstrcat(quickref, \"...\");\n> +\t\t\ttype = '+';\n> +\t\t\tmsg = \" (forced update)\";\n\n... you do not need the \" (\" and \")\" here.\n\nAll merged to 'next'.  Thanks.\n"},{"id":"60185","messageId":"20071118000306.GA4000@sigill.intra.peff.net","threadId":"10904","inReplyTo":"Pine.LNX.4.64.0711171258000.12193@iabervon.org","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T00:03:07Z","receivedAt":"2007-11-18T00:03:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 01:05:29PM -0500, Daniel Barkalow wrote:\n\n> Maybe this should take both the full list and the hint and do both passes \n> internally? IMHO, the logic in receive_status() looks like it might be \n> setting the error twice or not at all, unless you read very carefully.\n\nNot unreasonable, though I started in that direction and it ended up\nbeing less readable (which isn't to say that it's not possible). But it\nlooks like Junio has merged to next, so I think it is best to leave it\nas-is.\n\n-Peff\n"},{"id":"60186","messageId":"20071118001312.GB4000@sigill.intra.peff.net","threadId":"10904","inReplyTo":"Pine.LNX.4.64.0711171217200.12193@iabervon.org","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T00:13:14Z","receivedAt":"2007-11-18T00:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:\n\n> > +static void print_ref_status(char flag, const char *summary, struct ref *to, struct ref *from, const char *msg)\n> \n> Isn't \"from\" always \"to->peer_ref\"? It'd be nice to make this function \n> unable to print something different from what we actually did. (Actually \n> it might be \"to->deletion ? NULL : to->peer_ref\", but that would also be \n> better to have as an explicit feature of how you display \"to\", rather than \n> implicit in the set of callers.\n\nYes, I also considered changing \"from\" to \"show peer\" which might have\nbeen nicer. I am not opposed to such a cleanup, but again, not sure if\nit worth it now that we are merged.\n\n> > +static const char *status_abbrev(unsigned char sha1[20])\n> > +{\n> > +\tconst char *abbrev;\n> > +\tabbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);\n> > +\treturn abbrev ? abbrev : sha1_to_hex(sha1);\n> > +}\n> \n> Maybe we should have a find_unique_abbrev()-like function that doesn't \n> mind if the requested object doesn't exist?\n\nI agree, though I don't feel qualified to comment on what other places\nthat should be used (I was a bit surprised to find out that\nfind_unique_abbrev ever returned NULL, but changing the semantics at\nthis point is probably going to cause some subtle bug).\n\n> > +\t\tchar quickref[83];\n> Shouldn't this be 40 + 3 + 40 + 1?\n\nOops, yes. I think it should be hard to trigger (both commits would have\nto either not be in your db, or not be unique to 40 digits). But clearly\nit should be fixed, and it looks like Junio did.\n\nIt was a stupid cut-and-paste from Nicolas' fetch code, but it looks\nlike he correctly allocates 84 bytes for the \"...\" case.\n\n> > +\t\tchar type;\n> > +\t\tconst char *msg;\n> > +\n> > +\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n> > +\t\tif (ref->nonfastforward) {\n> > +\t\t\tstrcat(quickref, \"...\");\n> > +\t\t\ttype = '+';\n> > +\t\t\tmsg = \" (forced update)\";\n> > +\t\t}\n> > +\t\telse {\n> \n> Coding style, IIRC.\n\nSorry, I don't see the style nit you're mentioning here.\n\n-Peff\n"},{"id":"60187","messageId":"20071118001509.GC4000@sigill.intra.peff.net","threadId":"10904","inReplyTo":"7v6400twzp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T00:15:10Z","receivedAt":"2007-11-18T00:15:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 12:53:14PM -0800, Junio C Hamano wrote:\n\n> > +\t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n> > +\t\tif (ref->nonfastforward) {\n> > +\t\t\tstrcat(quickref, \"...\");\n> > +\t\t\ttype = '+';\n> > +\t\t\tmsg = \" (forced update)\";\n> \n> ... you do not need the \" (\" and \")\" here.\n\nOops, good catch.\n\n> All merged to 'next'.  Thanks.\n\nI see you fixed up the off-by-one buffer, too. Thanks.\n\n-Peff\n"},{"id":"60191","messageId":"7vir40z7nm.fsf@gitster.siamese.dyndns.org","threadId":"10904","inReplyTo":"20071117125602.GC23186@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T01:03:57Z","receivedAt":"2007-11-18T01:03:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index c7d07aa..bcf7143 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -146,19 +146,43 @@ static void get_local_heads(void)\n>  \tfor_each_ref(one_local_ref, NULL);\n>  }\n>  \n> -static int receive_status(int in)\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\nIt probably would not matter for sane repositories, but with\nthousands of refs, strlen() and prefixcmp() may start to hurt:\n\n\tstruct ref *ref;\n\tint reflen;\n\tconst char *msg = strchr(line, ' ');\n\n        if (!msg)\n        \treturn NULL;\n\treflen = msg - line;\n\tmsg++;\n\tfor (ref = refs; ref; ref = ref->next) {\n        \tif (strncmp(line, ref->name, reflen) || line[reflen] != ' ')\n\t\t\tcontinue;\n\t\t...\n\t\treturn ref->next;\n\t}\n\treturn NULL;\n\nbut the \"hint\" optimization probably make the above\nmicro-optimization irrelevant.\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\nIt is preferred to have a multi-line comment like this:\n\n\t/*\n         * A return value of -1 ...\n\t * ...\n\t * ... couldn't even get that far.\n\t */\n\n> +static int receive_status(int in, struct ref *refs)\n> ...\n> +\thint = NULL;\n>  \twhile (1) {\n>  \t\tlen = packet_read_line(in, line, sizeof(line));\n>  \t\tif (!len)\n> @@ -171,7 +195,10 @@ static int receive_status(int in)\n>  \t\t}\n>  \t\tif (!memcmp(line, \"ok\", 2))\n>  \t\t\tcontinue;\n> -\t\tfputs(line, stderr);\n> +\t\tif (hint)\n> +\t\t\thint = set_ref_error(hint, line + 3);\n> +\t\tif (!hint)\n> +\t\t\thint = set_ref_error(refs, line + 3);\n\nClever... taking advantage of the order receive-pack reports to\noptimize.\n\nBefore receive_status() is called, can the refs already have the\nerror status and string set?\n\n> @@ -429,12 +463,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n>  \t}\n>  \tclose(out);\n>  \n> -\tprint_push_status(dest, remote_refs);\n> -\n>  \tif (expect_status_report) {\n> -\t\tif (receive_status(in))\n> +\t\tret = receive_status(in, remote_refs);\n> +\t\tif (ret == -2)\n>  \t\t\treturn -1;\n\nHmm.  When we did not receive status, we cannot tell what\nsucceeded or failed, but what we _can_ tell the user is which\nrefs we attempted to push.  I wonder if robbing that information\nfrom the user with this \"return -1\" is a good idea.  Perhaps we\nwould instead want to set the status of all the refs to error\nand call print_push_status() anyway?  I dunno.\n\n>  \t}\n> +\telse\n> +\t\tret = 0;\n> +\n> +\tprint_push_status(dest, remote_refs);\n>  \n>  \tif (!args.dry_run && remote) {\n>  \t\tfor (ref = remote_refs; ref; ref = ref->next)\n"},{"id":"60195","messageId":"7v6400z6tk.fsf@gitster.siamese.dyndns.org","threadId":"10904","inReplyTo":"20071118001312.GB4000@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T01:21:59Z","receivedAt":"2007-11-18T01:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Nov 17, 2007 at 01:05:35PM -0500, Daniel Barkalow wrote:\n>\n>> > +\t\tif (ref->nonfastforward) {\n>> > +\t\t\tstrcat(quickref, \"...\");\n>> > +\t\t\ttype = '+';\n>> > +\t\t\tmsg = \" (forced update)\";\n>> > +\t\t}\n>> > +\t\telse {\n>> \n>> Coding style, IIRC.\n>\n> Sorry, I don't see the style nit you're mentioning here.\n\nI think Daniel is referring to \"Put 'else' on the same line as\nthe brace that closes the corresponding 'if' opened\", iow:\n\n\tif (...) {\n        \t...\n\t} else {\n\t\t...\n\t}\n"},{"id":"60197","messageId":"20071118023942.GA4560@sigill.intra.peff.net","threadId":"10904","inReplyTo":"7vir40z7nm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T02:39:43Z","receivedAt":"2007-11-18T02:39:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:\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> \n> It probably would not matter for sane repositories, but with\n> thousands of refs, strlen() and prefixcmp() may start to hurt:\n\nIt is actually _just_ prefixcmp. Or do you mean the strlen we call in\nprefixcmp? If so, I think the right solution is to make prefixcmp\nfaster.  :)\n\n> but the \"hint\" optimization probably make the above\n> micro-optimization irrelevant.\n\nAgreed.\n\n> It is preferred to have a multi-line comment like this:\n> \n> \t/*\n>          * A return value of -1 ...\n> \t * ...\n> \t * ... couldn't even get that far.\n> \t */\n\nOK. Since it is already in next, do you want a style fixup patch?\n\n> Before receive_status() is called, can the refs already have the\n> error status and string set?\n\nNothing else sets the string, so the latter is not possible (perhaps it\nshould be \"remote_error\" for clarity). It is less clear that we are not\noverwriting another status; however, if you look at do_send_pack, we\nonly actually send the remote refs that are getting REF_STATUS_OK.\n\nA broken or malicious remote could change the push status of an\narbitrary ref to rejection, but I don't really see the point. We could\nexplicitly check that we are changing from OK to REMOTE_REJECTED in\nset_ref_error.\n\n> >  \tif (expect_status_report) {\n> > -\t\tif (receive_status(in))\n> > +\t\tret = receive_status(in, remote_refs);\n> > +\t\tif (ret == -2)\n> >  \t\t\treturn -1;\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\nThat is a reasonable behavior (although they have already seen an\n\"error: \" message, I think). We might also consider returning something\nbesides \"-1\" to differentiate \"ok, but some refs failed\" from \"terribly\nbroken\". The old code used to use \"-2\" and \"-4\", but I checked and all\nof the error checking paths seemed to end up as a boolean.\n\nI can work up a patch if there is consensus.\n\n-Peff\n"},{"id":"60198","messageId":"20071118031215.GB4560@sigill.intra.peff.net","threadId":"10904","inReplyTo":"7v6400z6tk.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T03:12:16Z","receivedAt":"2007-11-18T03:12:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 05:21:59PM -0800, Junio C Hamano wrote:\n\n> > Sorry, I don't see the style nit you're mentioning here.\n> \n> I think Daniel is referring to \"Put 'else' on the same line as\n> the brace that closes the corresponding 'if' opened\", iow:\n> \n> \tif (...) {\n>         \t...\n> \t} else {\n> \t\t...\n> \t}\n\nAh. We are really inconsistent about that (I count about 333 non-cuddled\n(but possible to cuddle) versus 499 cuddled). Should we put something in\nthe CodingGuidelines? I am also happy to roll this change into a style\npatch.\n\nFor reference, I counted with:\n\n# cuddled:\ngrep '} *else' *.c | wc -l\n\n# possible but non-cuddled\n# when we find an 'else', if the previous line had a '}'\n# on it, then it is a hit. There are a few false positives\n# when the \"} else\" is from an outer block scope\n# And no, this sed invocation probably isn't totally portable. :)\nsed -n '/else/{x; /}/{x;p}}; h' *.c | wc -l\n# or to check the accuracy\nsed -n '/else/{x; /}/{p;x;p}}; h' *.c\n\n-Peff\n"},{"id":"60201","messageId":"7vwssgxir4.fsf@gitster.siamese.dyndns.org","threadId":"10904","inReplyTo":"20071118023942.GA4560@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T04:47:11Z","receivedAt":"2007-11-18T04:47:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Nov 17, 2007 at 05:03:57PM -0800, Junio C Hamano wrote:\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>> \n>> It probably would not matter for sane repositories, but with\n>> thousands of refs, strlen() and prefixcmp() may start to hurt:\n>\n> It is actually _just_ prefixcmp. Or do you mean the strlen we call in\n> prefixcmp? If so, I think the right solution is to make prefixcmp\n> faster.  :)\n\nI was referring to strlen(ref->name) taken for all refs during\nthe loop.  Micro-optimized one finds the end of refname on the\n\"ng\" line once before entering the loop.\n\n> OK. Since it is already in next, do you want a style fixup patch?\n\nI do not think it is particularly a big deal -- perhaps clean it\nbefore we touch the vicinity of the code next time.  The same\ngoes for the \"} else {\" stuff.\n\n>> Before receive_status() is called, can the refs already have the\n>> error status and string set?\n>\n> Nothing else sets the string, so the latter is not possible (perhaps it\n> should be \"remote_error\" for clarity). It is less clear that we are not\n> ... if you look at do_send_pack, we\n> only actually send the remote refs that are getting REF_STATUS_OK.\n\nAh, Ok.\n"},{"id":"60202","messageId":"20071118050033.GA13097@sigill.intra.peff.net","threadId":"10904","inReplyTo":"7vwssgxir4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-18T05:00:34Z","receivedAt":"2007-11-18T05:00:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 17, 2007 at 08:47:11PM -0800, Junio C Hamano wrote:\n\n>>> +     for (ref = refs; ref; ref = ref->next) {\n>>> +             const char *msg;\n>>> +             if (prefixcmp(line, ref->name))\n>>> +                     continue;\n>>> +             msg = line + strlen(ref->name);\n>>> +             if (*msg++ != ' ')\n>>> +                     continue;\n>>> +             ref->status = REF_STATUS_REMOTE_REJECT;\n>>> +             ref->error = xstrdup(msg);\n>>> +             ref->error[strlen(ref->error)-1] = '\\0';\n>>> +             return ref;\n>>> +     }\n>>> +     return NULL;\n>>> +}\n> > It is actually _just_ prefixcmp. Or do you mean the strlen we call in\n> > prefixcmp? If so, I think the right solution is to make prefixcmp\n> > faster.  :)\n> I was referring to strlen(ref->name) taken for all refs during\n> the loop.  Micro-optimized one finds the end of refname on the\n> \"ng\" line once before entering the loop.\n\nI don't see such a strlen, except in the implementation of prefixcmp,\nbecause we continue most of the time based on its result. If you have a\nfalse match on the prefixcmp (i.e., a prefix of another ref), you do an\nextra strlen.\n\nBut I don't think this is worth micro-optimizing.\n\n-Peff\n"}]}