{"thread":{"id":"29305","subject":"[PATCH 2/3] show_ref(): remove unused \"flag\" and \"cb_data\" arguments","startedAt":"2012-01-06T14:12:30Z","lastAt":"2012-01-07T05:08:23Z","messageCount":7,"participants":["mhagger@alum.mit.edu","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"182021","messageId":"1325859153-31016-1-git-send-email-mhagger@alum.mit.edu","threadId":"29305","inReplyTo":null,"subject":"[PATCH 0/3] Eliminate one user of extra_refs","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-06T14:12:30Z","receivedAt":"2012-01-06T14:12:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nReceive pack currently uses \"extra refs\" to keep track of \".have\"\nreferences, which in turn are used to tell the source the SHA1s of\nreferences that are already known to the repository via alternates.\n\nBut the code already creates an array holding the alternate SHA1s.  So\njust read the SHA1s out of this array rather then round-tripping them\nthrough the extra_refs mechanism.\n\nThis is one step towards hopefully abolishing extra_refs altogether.\nI still have to examine the other user.\n\nMichael Haggerty (3):\n  receive-pack: move more work into write_head_info()\n  show_ref(): remove unused \"flag\" and \"cb_data\" arguments\n  write_head_info(): handle \"extra refs\" locally\n\n builtin/receive-pack.c |   51 ++++++++++++++++++++---------------------------\n 1 files changed, 22 insertions(+), 29 deletions(-)\n\n-- \n1.7.8.2\n"},{"id":"182019","messageId":"1325859153-31016-2-git-send-email-mhagger@alum.mit.edu","threadId":"29305","inReplyTo":"1325859153-31016-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/3] receive-pack: move more work into write_head_info()","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-06T14:12:31Z","receivedAt":"2012-01-06T14:12:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nMove some more code from the calling site into write_head_info(), and\ninline add_alternate_refs() there.  (Some more simplification is\ncoming, and it is easier if all this code is in the same place.)\n\nMove some helper functions to avoid the need for forward declarations.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/receive-pack.c |   42 ++++++++++++++++++------------------------\n 1 files changed, 18 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d2dcb7e..08df17e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -144,12 +144,30 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo\n \treturn show_ref(path, sha1, flag, cb_data);\n }\n \n+static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)\n+{\n+\tadd_extra_ref(\".have\", sha1, 0);\n+}\n+\n+static void collect_one_alternate_ref(const struct ref *ref, void *data)\n+{\n+\tstruct sha1_array *sa = data;\n+\tsha1_array_append(sa, ref->old_sha1);\n+}\n+\n static void write_head_info(void)\n {\n+\tstruct sha1_array sa = SHA1_ARRAY_INIT;\n+\tfor_each_alternate_ref(collect_one_alternate_ref, &sa);\n+\tsha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);\n+\tsha1_array_clear(&sa);\n \tfor_each_ref(show_ref_cb, NULL);\n \tif (!sent_capabilities)\n \t\tshow_ref(\"capabilities^{}\", null_sha1, 0, NULL);\n+\tclear_extra_refs();\n \n+\t/* EOF */\n+\tpacket_flush(1);\n }\n \n struct command {\n@@ -869,25 +887,6 @@ static int delete_only(struct command *commands)\n \treturn 1;\n }\n \n-static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)\n-{\n-\tadd_extra_ref(\".have\", sha1, 0);\n-}\n-\n-static void collect_one_alternate_ref(const struct ref *ref, void *data)\n-{\n-\tstruct sha1_array *sa = data;\n-\tsha1_array_append(sa, ref->old_sha1);\n-}\n-\n-static void add_alternate_refs(void)\n-{\n-\tstruct sha1_array sa = SHA1_ARRAY_INIT;\n-\tfor_each_alternate_ref(collect_one_alternate_ref, &sa);\n-\tsha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);\n-\tsha1_array_clear(&sa);\n-}\n-\n int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n {\n \tint advertise_refs = 0;\n@@ -937,12 +936,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\tunpack_limit = receive_unpack_limit;\n \n \tif (advertise_refs || !stateless_rpc) {\n-\t\tadd_alternate_refs();\n \t\twrite_head_info();\n-\t\tclear_extra_refs();\n-\n-\t\t/* EOF */\n-\t\tpacket_flush(1);\n \t}\n \tif (advertise_refs)\n \t\treturn 0;\n-- \n1.7.8.2\n"},{"id":"182018","messageId":"1325859153-31016-3-git-send-email-mhagger@alum.mit.edu","threadId":"29305","inReplyTo":"1325859153-31016-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/3] show_ref(): remove unused \"flag\" and \"cb_data\" arguments","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-06T14:12:32Z","receivedAt":"2012-01-06T14:12:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThe function is not used as a callback, so it doesn't need these\narguments.  Also change its return type to void.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nI suppose, though I didn't verify, that the old function signature was\na vestige of its earlier having been used as a callback function.  But\nit doesn't really matter; the point is that the extra arguments are\ncurrently not needed.\n\n builtin/receive-pack.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 08df17e..65d129c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -115,7 +115,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n-static int show_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n+static void show_ref(const char *path, const unsigned char *sha1)\n {\n \tif (sent_capabilities)\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), path);\n@@ -125,10 +125,9 @@ static int show_ref(const char *path, const unsigned char *sha1, int flag, void\n \t\t\t     \" report-status delete-refs side-band-64k\",\n \t\t\t     prefer_ofs_delta ? \" ofs-delta\" : \"\");\n \tsent_capabilities = 1;\n-\treturn 0;\n }\n \n-static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n+static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, void *unused)\n {\n \tpath = strip_namespace(path);\n \t/*\n@@ -141,7 +140,8 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo\n \t */\n \tif (!path)\n \t\tpath = \".have\";\n-\treturn show_ref(path, sha1, flag, cb_data);\n+\tshow_ref(path, sha1);\n+\treturn 0;\n }\n \n static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)\n@@ -163,7 +163,7 @@ static void write_head_info(void)\n \tsha1_array_clear(&sa);\n \tfor_each_ref(show_ref_cb, NULL);\n \tif (!sent_capabilities)\n-\t\tshow_ref(\"capabilities^{}\", null_sha1, 0, NULL);\n+\t\tshow_ref(\"capabilities^{}\", null_sha1);\n \tclear_extra_refs();\n \n \t/* EOF */\n-- \n1.7.8.2\n"},{"id":"182020","messageId":"1325859153-31016-4-git-send-email-mhagger@alum.mit.edu","threadId":"29305","inReplyTo":"1325859153-31016-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/3] write_head_info(): handle \"extra refs\" locally","fromName":"","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-06T14:12:33Z","receivedAt":"2012-01-06T14:12:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Michael Haggerty <mhagger@alum.mit.edu>\n\nThe old code basically did:\n\n     generate array of SHA1s for alternate refs\n     for each unique SHA1 in array:\n         add_extra_ref(\".have\", sha1)\n     for each ref (including real refs and extra refs):\n         show_ref(refname, sha1)\n\nBut there is no need to stuff the alternate refs in extra_refs; we can\ncall show_ref() directly when iterating over the array, then handle\nreal refs separately.  So change the code to:\n\n     generate array of SHA1s for alternate refs\n     for each unique SHA1 in array:\n         show_ref(\".have\", sha1)\n     for each ref (this now only includes real refs):\n         show_ref(refname, sha1)\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/receive-pack.c |    7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 65d129c..8c9e91e 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -144,9 +144,9 @@ static int show_ref_cb(const char *path, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n-static void add_one_alternate_sha1(const unsigned char sha1[20], void *unused)\n+static void show_one_alternate_sha1(const unsigned char sha1[20], void *unused)\n {\n-\tadd_extra_ref(\".have\", sha1, 0);\n+\tshow_ref(\".have\", sha1);\n }\n \n static void collect_one_alternate_ref(const struct ref *ref, void *data)\n@@ -159,12 +159,11 @@ static void write_head_info(void)\n {\n \tstruct sha1_array sa = SHA1_ARRAY_INIT;\n \tfor_each_alternate_ref(collect_one_alternate_ref, &sa);\n-\tsha1_array_for_each_unique(&sa, add_one_alternate_sha1, NULL);\n+\tsha1_array_for_each_unique(&sa, show_one_alternate_sha1, NULL);\n \tsha1_array_clear(&sa);\n \tfor_each_ref(show_ref_cb, NULL);\n \tif (!sent_capabilities)\n \t\tshow_ref(\"capabilities^{}\", null_sha1);\n-\tclear_extra_refs();\n \n \t/* EOF */\n \tpacket_flush(1);\n-- \n1.7.8.2\n"},{"id":"182025","messageId":"20120106145356.GB4525@sigill.intra.peff.net","threadId":"29305","inReplyTo":"1325859153-31016-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/3] Eliminate one user of extra_refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T14:53:56Z","receivedAt":"2012-01-06T14:53:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2012 at 03:12:30PM +0100, mhagger@alum.mit.edu wrote:\n\n> Receive pack currently uses \"extra refs\" to keep track of \".have\"\n> references, which in turn are used to tell the source the SHA1s of\n> references that are already known to the repository via alternates.\n> \n> But the code already creates an array holding the alternate SHA1s.  So\n> just read the SHA1s out of this array rather then round-tripping them\n> through the extra_refs mechanism.\n> \n> This is one step towards hopefully abolishing extra_refs altogether.\n> I still have to examine the other user.\n\nThanks, this is a nice simplification. The patches look good to me, and\nthey produce the same output for a simple test (I happened to be fiddling\nwith receive-pack and alternates yesterday, so I had a nice test case\nright at hand :) ).\n\n>   receive-pack: move more work into write_head_info()\n\nBTW, I have a patch to make sending \".have\" refs configurable[1] (it\nadds a receive.advertiseAlternates config variable), which this patch\nconflicts with. I don't think that is your problem, but I thought I\nwould mention it since you are working in the area. Is that something we\nwant in git?\n\n-Peff\n\n[1] We are using it at GitHub because our alternates repos are so huge\n    that the overhead of advertising the refs outweighs the minor\n    benefits you get from avoiding object transfer.\n"},{"id":"182037","messageId":"7vobugk42i.fsf@alter.siamese.dyndns.org","threadId":"29305","inReplyTo":"1325859153-31016-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/3] write_head_info(): handle \"extra refs\" locally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-06T19:45:41Z","receivedAt":"2012-01-06T19:45:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> From: Michael Haggerty <mhagger@alum.mit.edu>\n>\n> The old code basically did:\n>\n>      generate array of SHA1s for alternate refs\n>      for each unique SHA1 in array:\n>          add_extra_ref(\".have\", sha1)\n>      for each ref (including real refs and extra refs):\n>          show_ref(refname, sha1)\n>\n> But there is no need to stuff the alternate refs in extra_refs; we can\n> call show_ref() directly when iterating over the array, then handle\n> real refs separately.  So change the code to:\n>\n>      generate array of SHA1s for alternate refs\n>      for each unique SHA1 in array:\n>          show_ref(\".have\", sha1)\n>      for each ref (this now only includes real refs):\n>          show_ref(refname, sha1)\n\nThis updated logic should be equivalent to the old one as long as nobody\nelse called add_extra_ref() before we come to write_head_info() function,\nwhich should hold true.\n\nThe entire series looks good. Thanks.\n"},{"id":"182056","messageId":"7vobuggkvs.fsf@alter.siamese.dyndns.org","threadId":"29305","inReplyTo":"1325859153-31016-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/3] show_ref(): remove unused \"flag\" and \"cb_data\" arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-07T05:08:23Z","receivedAt":"2012-01-07T05:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mhagger@alum.mit.edu writes:\n\n> I suppose, though I didn't verify, that the old function signature was\n> a vestige of its earlier having been used as a callback function.  But\n> it doesn't really matter; the point is that the extra arguments are\n> currently not needed.\n\nYeah, since 8a65ff7 (Generalize the \"show each ref\" code in receice-pack,\n2005-07-02) the function has always been fed to for_each_ref(), but when\n6b01ecf (ref namespaces: Support remote repositories via upload-pack and\nreceive-pack, 2011-07-08) introduced show_ref_cb() as the callback that\nuses show_ref() as a helper, it forgot to do the clean-up in this patch.\n"}]}