{"thread":{"id":"10843","subject":"[RFC/PATCH 0/3] tracking per-ref errors on push","startedAt":"2007-11-13T10:25:01Z","lastAt":"2007-11-16T05:05:16Z","messageCount":20,"participants":["Jeff King","Alex Riesen","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"59647","messageId":"20071113102500.GA2767@sigill.intra.peff.net","threadId":"10843","inReplyTo":null,"subject":"[RFC/PATCH 0/3] tracking per-ref errors on push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T10:25:01Z","receivedAt":"2007-11-13T10:25:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Following is a series to track push errors for individual refs.  This\nshould have three immediate advantages (the first two of which I\nimplement in the series):\n\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\nThe patches are:\n\n  1. send-pack: track errors for each ref\n     This is the groundwork for the other 2. I also think it makes the\n     code a bit more readable by splitting out the print formatting from\n     the push logic, and by explicitly naming the different states that\n     we were previously deducing from the states of other variables.\n\n  2. send-pack: check ref->status before updating tracking refs\n     This should fix the bugs that people are seeing from 334f483\n\n  3. send-pack: assign remote errors to each ref\n     This may have some problems, as I will explain below.\n\nI have a few concerns which make this an RFC and not a real patch\nsubmission:\n\n  - I have done pretty minimal testing, and there are no automated tests\n    (at least 2, which fixes a real bug, should get a test).\n    Unfortunately, I am out of time to work on this and am leaving the\n    country for a week. Between that and Thanksgiving travel, I'm not\n    sure when I'll have time to finish up, so I thought it best to have\n    comments waiting (and others should feel free to pick up the work if\n    they want -- Alex, I think your error consolidation should go nicely\n    on top of this).\n\n  - It looks like the push mirror code just made it into next, which\n    is going to require a conflict-heavy rebase on my part.\n\n  - There is a potential performance bottleneck in patch 3. See the\n    commit message.\n\n  - In patch 3, the 'ng' message sent by the remote are not\n    unambiguously parseable.  See the commit message.\n\n-Peff\n"},{"id":"59648","messageId":"20071113102709.GA2905@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH/RFC 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T10:27:09Z","receivedAt":"2007-11-13T10:27:09Z","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---\n builtin-send-pack.c |  201 ++++++++++++++++++++++++++++-----------------------\n cache.h             |   13 +++-\n 2 files changed, 121 insertions(+), 93 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 5a0f5c6..3ac2615 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,90 @@ 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+\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_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, ref->peer_ref, NULL);\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_NONFF:\n+\t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref, \"non-fast forward\");\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_OK:\n+\t\t\tif (ref->deletion)\n+\t\t\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n+\t\t\telse if (is_null_sha1(ref->old_sha1))\n+\t\t\t\tprint_ref_status('*',\n+\t\t\t\t\t\t(prefixcmp(ref->name, \"refs/tags/\") ?\n+\t\t\t\t\t\t  \"[new branch]\" : \"[new tag]\"),\n+\t\t\t\t\t\tref, ref->peer_ref, NULL);\n+\t\t\telse {\n+\t\t\t\tchar quickref[83];\n+\t\t\t\tchar type = ' ';\n+\t\t\t\tconst char *msg = NULL;\n+\t\t\t\tconst char *old_abb;\n+\n+\t\t\t\told_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);\n+\t\t\t\tstrcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));\n+\t\t\t\tif (ref->nonff) {\n+\t\t\t\t\tstrcat(quickref, \"...\");\n+\t\t\t\t\ttype = '+';\n+\t\t\t\t\tmsg = \" (forced update)\";\n+\t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tstrcat(quickref, \"..\");\n+\t\t\t\tstrcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));\n+\n+\t\t\t\tprint_ref_status(type, quickref, ref, ref->peer_ref, msg);\n+\t\t\t}\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 \n \t/* No funny business with the matcher */\n \tremote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);\n@@ -256,35 +332,17 @@ 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;\n-\n \t\tif (!ref->peer_ref)\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\tpretty_ref = prettify_ref(ref->name);\n-\t\tpretty_peer = prettify_ref(ref->peer_ref->name);\n-\n-\t\twill_delete_ref = is_null_sha1(ref->peer_ref->new_sha1);\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(ref->peer_ref->new_sha1);\n+\t\tif (ref->deletion && !allow_deleting_refs) {\n+\t\t\tref->status = REF_STATUS_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, ref->peer_ref->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@@ -307,34 +365,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->nonff =\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(ref->peer_ref->new_sha1,\n-\t\t\t\t       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(ref->peer_ref->new_sha1, ref->old_sha1));\n+\n+\t\tif (ref->nonff && !ref->force && !args.force_update) {\n+\t\t\tref->status = REF_STATUS_NONFF;\n+\t\t\tcontinue;\n \t\t}\n+\n \t\thashcpy(ref->new_sha1, ref->peer_ref->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@@ -346,63 +396,32 @@ 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\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+\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 5f40e12..ca5d96d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -499,8 +499,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 nonff : 1;\n+\tunsigned char deletion : 1;\n+\tenum {\n+\t\tREF_STATUS_NONE = 0,\n+\t\tREF_STATUS_OK,\n+\t\tREF_STATUS_NONFF,\n+\t\tREF_STATUS_NODELETE,\n+\t\tREF_STATUS_UPTODATE,\n+\t} status;\n \tstruct ref *peer_ref; /* when renaming */\n \tchar name[FLEX_ARRAY]; /* more */\n };\n-- \n1.5.3.5.1704.g24d42-dirty\n"},{"id":"59649","messageId":"20071113102757.GB2905@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH/RFC 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T10:27:57Z","receivedAt":"2007-11-13T10:27:57Z","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---\n builtin-send-pack.c |   15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 3ac2615..2805c92 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-- \n1.5.3.5.1704.g24d42-dirty\n"},{"id":"59650","messageId":"20071113102900.GC2905@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH/RFC 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T10:29:00Z","receivedAt":"2007-11-13T10:29:00Z","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. There are two drawbacks to this\nchange:\n\n  1. cross-referencing the incoming status with the ref list\n     is worst case O(n^2) (where n = number of refs); this\n     can be fixed with a smarter implementation\n\n  2. the status parsing is not foolproof. We get a line like\n\n         ng refs/heads/master arbitrary msg\n\n     which cannot be parsed unambiguously in the face of\n     refnames with spaces. We do a prefix-match so that\n     you will only run into problems if you have two refs,\n     one of which is a prefix match of the other, and the\n     longer having a space right after the prefix.\n---\n builtin-send-pack.c |   44 +++++++++++++++++++++++++++++++++++++-------\n cache.h             |    2 ++\n 2 files changed, 39 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 2805c92..a2307fa 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -146,14 +146,34 @@ 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 void set_ref_error(struct ref *refs, const char *line) {\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;\n+\t}\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 \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@@ -171,7 +191,7 @@ 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\tset_ref_error(refs, line + 3);\n \t\tret = -1;\n \t}\n \treturn ret;\n@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t\tcase REF_STATUS_NONFF:\n \t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref, \"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\tif (ref->deletion)\n \t\t\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n@@ -296,6 +322,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest\n \tint ask_for_status_report = 0;\n \tint allow_deleting_refs = 0;\n \tint expect_status_report = 0;\n+\tint ret;\n \n \t/* No funny business with the matcher */\n \tremote_tail = get_remote_heads(in, &remote_refs, 0, NULL, REF_NORMAL);\n@@ -400,12 +427,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@@ -414,7 +444,7 @@ 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-\treturn 0;\n+\treturn ret;\n }\n \n static void verify_remote_names(int nr_heads, const char **heads)\ndiff --git a/cache.h b/cache.h\nindex ca5d96d..082e03b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -509,7 +509,9 @@ struct ref {\n \t\tREF_STATUS_NONFF,\n \t\tREF_STATUS_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 };\n-- \n1.5.3.5.1704.g24d42-dirty\n"},{"id":"59651","messageId":"20071113102958.GD2905@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102900.GC2905@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T10:29:58Z","receivedAt":"2007-11-13T10:29:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2007 at 05:29:00AM -0500, Jeff King wrote:\n\n> +\t\tref->error = xstrdup(msg);\n\nAnother problem with this patch: this is a leak.\n\n-Peff\n"},{"id":"59656","messageId":"20071113113202.GA15845@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102709.GA2905@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T11:32:03Z","receivedAt":"2007-11-13T11:32:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2007 at 05:27:09AM -0500, Jeff King wrote:\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\nArgh, this fails t5400 quite badly without the following patch:\n\n---\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 3ac2615..eff84e0 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -421,6 +421,16 @@ 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+\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"},{"id":"59657","messageId":"20071113113437.GB15845@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/3] tracking per-ref errors on push","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T11:34:37Z","receivedAt":"2007-11-13T11:34:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2007 at 05:25:01AM -0500, Jeff King wrote:\n\n>   - It looks like the push mirror code just made it into next, which\n>     is going to require a conflict-heavy rebase on my part.\n\nThis turned out not to be too bad. A rebased series (with the extra\npatch I just posted) follows, and it passes the test suite (though I\nstill think more tests are in order).\n\n-Peff\n"},{"id":"59658","messageId":"20071113113623.GA15880@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH v2 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T11:36:23Z","receivedAt":"2007-11-13T11:36:23Z","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 |  213 +++++++++++++++++++++++++++++----------------------\n cache.h             |   13 +++-\n 2 files changed, 132 insertions(+), 94 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 418925e..c01a9c4 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,90 @@ 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+\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_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, ref->peer_ref, NULL);\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_NONFF:\n+\t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref, \"non-fast forward\");\n+\t\t\tbreak;\n+\t\tcase REF_STATUS_OK:\n+\t\t\tif (ref->deletion)\n+\t\t\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n+\t\t\telse if (is_null_sha1(ref->old_sha1))\n+\t\t\t\tprint_ref_status('*',\n+\t\t\t\t\t\t(prefixcmp(ref->name, \"refs/tags/\") ?\n+\t\t\t\t\t\t  \"[new branch]\" : \"[new tag]\"),\n+\t\t\t\t\t\tref, ref->peer_ref, NULL);\n+\t\t\telse {\n+\t\t\t\tchar quickref[83];\n+\t\t\t\tchar type = ' ';\n+\t\t\t\tconst char *msg = NULL;\n+\t\t\t\tconst char *old_abb;\n+\n+\t\t\t\told_abb = find_unique_abbrev(ref->old_sha1, DEFAULT_ABBREV);\n+\t\t\t\tstrcpy(quickref, old_abb ? old_abb : sha1_to_hex(ref->old_sha1));\n+\t\t\t\tif (ref->nonff) {\n+\t\t\t\t\tstrcat(quickref, \"...\");\n+\t\t\t\t\ttype = '+';\n+\t\t\t\t\tmsg = \" (forced update)\";\n+\t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tstrcat(quickref, \"..\");\n+\t\t\t\tstrcat(quickref, find_unique_abbrev(ref->new_sha1, DEFAULT_ABBREV));\n+\n+\t\t\t\tprint_ref_status(type, quickref, ref, ref->peer_ref, msg);\n+\t\t\t}\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 +338,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 +348,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_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 +379,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->nonff =\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->nonff && !ref->force && !args.force_update) {\n+\t\t\tref->status = REF_STATUS_NONFF;\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 +410,42 @@ 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\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 5f40e12..ca5d96d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -499,8 +499,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 nonff : 1;\n+\tunsigned char deletion : 1;\n+\tenum {\n+\t\tREF_STATUS_NONE = 0,\n+\t\tREF_STATUS_OK,\n+\t\tREF_STATUS_NONFF,\n+\t\tREF_STATUS_NODELETE,\n+\t\tREF_STATUS_UPTODATE,\n+\t} status;\n \tstruct ref *peer_ref; /* when renaming */\n \tchar name[FLEX_ARRAY]; /* more */\n };\n-- \n1.5.3.5.1731.g0763\n"},{"id":"59659","messageId":"20071113113651.GB15880@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH v2 2/3] send-pack: check ref->status before updating tracking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T11:36:51Z","receivedAt":"2007-11-13T11:36:51Z","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\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c |   15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex c01a9c4..5c84766 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-- \n1.5.3.5.1731.g0763\n"},{"id":"59660","messageId":"20071113113710.GC15880@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113102500.GA2767@sigill.intra.peff.net","subject":"[PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T11:37:10Z","receivedAt":"2007-11-13T11:37:10Z","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. There are two drawbacks to this\nchange:\n\n  1. cross-referencing the incoming status with the ref list\n     is worst case O(n^2) (where n = number of refs); this\n     can be fixed with a smarter implementation\n\n  2. the status parsing is not foolproof. We get a line like\n\n         ng refs/heads/master arbitrary msg\n\n     which cannot be parsed unambiguously in the face of\n     refnames with spaces. We do a prefix-match so that\n     you will only run into problems if you have two refs,\n     one of which is a prefix match of the other, and the\n     longer having a space right after the prefix.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin-send-pack.c |   44 ++++++++++++++++++++++++++++++++++++++------\n cache.h             |    2 ++\n 2 files changed, 40 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 5c84766..7d466d9 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -146,14 +146,34 @@ 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 void set_ref_error(struct ref *refs, const char *line) {\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;\n+\t}\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 \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@@ -171,7 +191,7 @@ 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\tset_ref_error(refs, line + 3);\n \t\tret = -1;\n \t}\n \treturn ret;\n@@ -258,6 +278,12 @@ static void print_push_status(const char *dest, struct ref *refs)\n \t\tcase REF_STATUS_NONFF:\n \t\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref, \"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\tif (ref->deletion)\n \t\t\t\tprint_ref_status('-', \"[deleted]\", ref, NULL, NULL);\n@@ -297,6 +323,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@@ -414,12 +441,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@@ -428,6 +458,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 ca5d96d..082e03b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -509,7 +509,9 @@ struct ref {\n \t\tREF_STATUS_NONFF,\n \t\tREF_STATUS_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 };\n-- \n1.5.3.5.1731.g0763\n"},{"id":"59693","messageId":"20071113190447.GB3268@steel.home","threadId":"10843","inReplyTo":"20071113102709.GA2905@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/3] send-pack: track errors for each ref","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-13T19:04:47Z","receivedAt":"2007-11-13T19:04:47Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Tue, Nov 13, 2007 11:27:09 +0100:\n> diff --git a/cache.h b/cache.h\n> index 5f40e12..ca5d96d 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -499,8 +499,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 nonff : 1;\n\n        unsigned char fastforward : 1; ? ffwd?\n\n> +\tunsigned char deletion : 1;\n> +\tenum {\n> +\t\tREF_STATUS_NONE = 0,\n> +\t\tREF_STATUS_OK,\n> +\t\tREF_STATUS_NONFF,\n\nisn't it a duplication of nonff?\n"},{"id":"59725","messageId":"20071113201859.GE3268@steel.home","threadId":"10843","inReplyTo":"20071113102709.GA2905@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 1/3] send-pack: track errors for each ref","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-11-13T20:18:59Z","receivedAt":"2007-11-13T20:18:59Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Jeff King, Tue, Nov 13, 2007 11:27:09 +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\nIt wont apply to current master. How ready is built-in send-pack/push?\nShould I fix send-pack.c properly?\n"},{"id":"59760","messageId":"20071113223006.GA19032@sigill.intra.peff.net","threadId":"10843","inReplyTo":"20071113190447.GB3268@steel.home","subject":"Re: [PATCH/RFC 1/3] send-pack: track errors for each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-13T22:30:06Z","receivedAt":"2007-11-13T22:30:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2007 at 08:04:47PM +0100, Alex Riesen wrote:\n\n> > -\tunsigned char force;\n> > -\tunsigned char merge;\n> > +\tunsigned char force : 1;\n> > +\tunsigned char merge : 1;\n> > +\tunsigned char nonff : 1;\n> \n>         unsigned char fastforward : 1; ? ffwd?\n\nI agree it is a little funny to have a negative flag, but it is the\nexceptional case to be nonff, so I think it makes the code flow a little\nbetter (you would just be asking for !ref->fastforward all the time). I\nam fine with a more readable name.\n\n> > +\tunsigned char deletion : 1;\n> > +\tenum {\n> > +\t\tREF_STATUS_NONE = 0,\n> > +\t\tREF_STATUS_OK,\n> > +\t\tREF_STATUS_NONFF,\n> \n> isn't it a duplication of nonff?\n\nNo. The nonff flag is \"is this push a non-fast forward\". The\nREF_STATUS_NONFF state is \"we didn't push because this is a nonff case\".\nSo you can have nonff = 1 and status = REF_STATUS_OK, which means that\nthe push was forced (either by ->force, or args.force).\n\nSo perhaps a better name is REF_STATUS_REJECT_NONFF (or something longer\nif NONFF is too non-obvious).\n\nAn alternative is that the deletion and nonff flags can be folded into\nthe status. But then checking for \"is everything OK\" becomes\nnon-obvious (it's something like REF_STATUS_NONFF_FORCED ||\nREF_STATUS_DELETION_OK || REF_STATUS_PUSH_OK). I tried it that way and\nfound the code is a bit more readable by pulling those features away\nfrom status (so status is \"did we succeed, or did we fail, and if the\nlatter, for what reason?\").\n\n-Peff\n"},{"id":"59807","messageId":"7vbq9xpprg.fsf@gitster.siamese.dyndns.org","threadId":"10843","inReplyTo":"20071113113710.GC15880@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-14T01:41:39Z","receivedAt":"2007-11-14T01:41:39Z","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> This lets us show remote errors (e.g., a denied hook) along\n> with the usual push output. ...\n\nYay!\n\n> ... There are two drawbacks to this\n> change:\n>\n>   1. cross-referencing the incoming status with the ref list\n>      is worst case O(n^2) (where n = number of refs); this\n>      can be fixed with a smarter implementation\n\nSure.\n\n>   2. the status parsing is not foolproof. We get a line like\n>\n>          ng refs/heads/master arbitrary msg\n>\n>      which cannot be parsed unambiguously in the face of\n>      refnames with spaces. We do a prefix-match so that\n>      you will only run into problems if you have two refs,\n>      one of which is a prefix match of the other, and the\n>      longer having a space right after the prefix.\n\nIs it really \"arbitrary msg\", or just a fixed set of strings?\n\nAlso I think we can rely on the order report-status extension\nreports the per-ref result.  It gives back the information the\nsame order send-pack side supplies the head information, no?\n"},{"id":"59808","messageId":"7v4pfppp3b.fsf@gitster.siamese.dyndns.org","threadId":"10843","inReplyTo":"7vbq9xpprg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-14T01:56:08Z","receivedAt":"2007-11-14T01:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>   2. the status parsing is not foolproof. We get a line like\n>>\n>>          ng refs/heads/master arbitrary msg\n>>\n>>      which cannot be parsed unambiguously...\n>\n> Is it really \"arbitrary msg\", or just a fixed set of strings?\n\nThis was a wrong thing to say.  We may add new error message in\nlater versions of receive-pack and we want to make it compatible\nwith today's send-pack with minimum hassle.  So that is a\nproblem.  But...\n\n> Also I think we can rely on the order report-status extension\n> reports the per-ref result.  It gives back the information the\n> same order send-pack side supplies the head information, no?\n\nI was hoping that this can solve both of the problems you\ncited...\n"},{"id":"59809","messageId":"Pine.LNX.4.64.0711140159550.4362@racer.site","threadId":"10843","inReplyTo":"7vbq9xpprg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-14T02:01:14Z","receivedAt":"2007-11-14T02:01:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 13 Nov 2007, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >   2. the status parsing is not foolproof. We get a line like\n> >\n> >          ng refs/heads/master arbitrary msg\n> >\n> >      which cannot be parsed unambiguously in the face of\n> >      refnames with spaces.\n\nSince when can refnames contain spaces?  In my copy of git, bad_ref_char() \nin refs.c returns 1 if ch <= ' '.  It's the first error path, even.\n\nCiao,\nDscho\n"},{"id":"59817","messageId":"7vabphnyo9.fsf@gitster.siamese.dyndns.org","threadId":"10843","inReplyTo":"Pine.LNX.4.64.0711140159550.4362@racer.site","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-14T06:12:06Z","receivedAt":"2007-11-14T06:12:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> >   2. the status parsing is not foolproof. We get a line like\n>> >\n>> >          ng refs/heads/master arbitrary msg\n>> >\n>> >      which cannot be parsed unambiguously in the face of\n>> >      refnames with spaces.\n>\n> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() \n> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.\n\nBah.  You are right.\n"},{"id":"59921","messageId":"20071115044715.GB9794@sigill.intra.peff.net","threadId":"10843","inReplyTo":"7vbq9xpprg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-15T04:47:16Z","receivedAt":"2007-11-15T04:47:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2007 at 05:41:39PM -0800, Junio C Hamano wrote:\n\n> Is it really \"arbitrary msg\", or just a fixed set of strings?\n\nIt is for a fixed version of the remote receive-pack, but I don't want\nto start relying on that.\n\n> Also I think we can rely on the order report-status extension\n> reports the per-ref result.  It gives back the information the\n> same order send-pack side supplies the head information, no?\n\nI considered that, but I didn't want to rely on it without your say-so.\nWe could also just use it as a hint to boost performance. I.e., track\nthe last match, and start our linear search there, but if we fail, drop\nback to searching the whole list. Something like (on top of my other\npatch):\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 7d466d9..8e9580a 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -146,7 +146,8 @@ static void get_local_heads(void)\n \tfor_each_ref(one_local_ref, NULL);\n }\n \n-static void set_ref_error(struct ref *refs, const char *line) {\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@@ -159,8 +160,9 @@ static void set_ref_error(struct ref *refs, const char *line) {\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;\n+\t\treturn ref;\n \t}\n+\treturn NULL;\n }\n \n /* a return value of -1 indicates that an error occurred,\n@@ -168,6 +170,7 @@ static void set_ref_error(struct ref *refs, const char *line) {\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@@ -179,6 +182,7 @@ static int receive_status(int in, struct ref *refs)\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@@ -191,7 +195,10 @@ static int receive_status(int in, struct ref *refs)\n \t\t}\n \t\tif (!memcmp(line, \"ok\", 2))\n \t\t\tcontinue;\n-\t\tset_ref_error(refs, line + 3);\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"},{"id":"59922","messageId":"20071115045414.GC9794@sigill.intra.peff.net","threadId":"10843","inReplyTo":"Pine.LNX.4.64.0711140159550.4362@racer.site","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-15T04:54:17Z","receivedAt":"2007-11-15T04:54:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2007 at 02:01:14AM +0000, Johannes Schindelin wrote:\n\n> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() \n> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.\n\nOops, I'm clearly an idiot. I explicitly looked at bad_ref_char before\nwriting the original message, and somehow read that as strictly less\nthan.\n\nSo the parsing problem goes away, and I think using the sort order as a\nhint takes away the potential performance problem (I don't even know if\nit was a problem -- there may be other O(n^2) behavior).\n\nI will take a look at the tests Alex has been working on, maybe add a\nfew to it, and submit a cleaned-up series.\n\n-Peff\n"},{"id":"60053","messageId":"7v8x4y3hmb.fsf@gitster.siamese.dyndns.org","threadId":"10843","inReplyTo":"20071115045414.GC9794@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] send-pack: assign remote errors to each ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-16T05:05:16Z","receivedAt":"2007-11-16T05:05:16Z","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 Wed, Nov 14, 2007 at 02:01:14AM +0000, Johannes Schindelin wrote:\n>\n>> Since when can refnames contain spaces?  In my copy of git, bad_ref_char() \n>> in refs.c returns 1 if ch <= ' '.  It's the first error path, even.\n>\n> Oops, I'm clearly an idiot. I explicitly looked at bad_ref_char before\n> writing the original message, and somehow read that as strictly less\n> than.\n>\n> So the parsing problem goes away, and I think using the sort order as a\n> hint takes away the potential performance problem (I don't even know if\n> it was a problem -- there may be other O(n^2) behavior).\n>\n> I will take a look at the tests Alex has been working on, maybe add a\n> few to it, and submit a cleaned-up series.\n\nThanks.\n"}]}