{"thread":{"id":"23484","subject":"[PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs","startedAt":"2010-04-15T20:54:07Z","lastAt":"2010-04-18T23:11:04Z","messageCount":5,"participants":["Jay Soffian","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"139626","messageId":"1271364848-32805-1-git-send-email-jaysoffian@gmail.com","threadId":"23484","inReplyTo":null,"subject":"[PATCH 1/2] receive-pack: Switch global variable 'commands' to a parameter","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-04-15T20:54:07Z","receivedAt":"2010-04-15T20:54:07Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"Receive pack is inconsistent in its usage of the 'commands'\nvariable; though it is setup as a global and accessed that way by\nexecute_commands(), report(), and run_receive_hook(), it is also\npassed as a parameter to delete_only() and run_update_post_hook().\n\nFor consistency, make it local to cmd_receive_pack and pass it as a\nparameter. As long as we're cleaning up, also make our use of the\nnames 'commands' and 'cmd' consistent.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\n builtin/receive-pack.c |   63 ++++++++++++++++++++++--------------------------\n 1 files changed, 29 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0559fcc..3fc73cf 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -134,8 +134,6 @@ struct command {\n \tchar ref_name[FLEX_ARRAY]; /* more */\n };\n \n-static struct command *commands;\n-\n static const char pre_receive_hook[] = \"hooks/pre-receive\";\n static const char post_receive_hook[] = \"hooks/post-receive\";\n \n@@ -188,7 +186,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n \treturn 0;\n }\n \n-static int run_receive_hook(const char *hook_name)\n+static int run_receive_hook(struct command *commands, const char *hook_name)\n {\n \tstatic char buf[sizeof(commands->old_sha1) * 2 + PATH_MAX + 4];\n \tstruct command *cmd;\n@@ -447,15 +445,15 @@ static const char *update(struct command *cmd)\n \n static char update_post_hook[] = \"hooks/post-update\";\n \n-static void run_update_post_hook(struct command *cmd)\n+static void run_update_post_hook(struct command *commands)\n {\n-\tstruct command *cmd_p;\n+\tstruct command *cmd;\n \tint argc;\n \tconst char **argv;\n \tstruct child_process proc;\n \n-\tfor (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n-\t\tif (cmd_p->error_string)\n+\tfor (argc = 0, cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (cmd->error_string)\n \t\t\tcontinue;\n \t\targc++;\n \t}\n@@ -464,12 +462,12 @@ static void run_update_post_hook(struct command *cmd)\n \targv = xmalloc(sizeof(*argv) * (2 + argc));\n \targv[0] = update_post_hook;\n \n-\tfor (argc = 1, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n+\tfor (argc = 1, cmd = commands; cmd; cmd = cmd->next) {\n \t\tchar *p;\n-\t\tif (cmd_p->error_string)\n+\t\tif (cmd->error_string)\n \t\t\tcontinue;\n-\t\tp = xmalloc(strlen(cmd_p->ref_name) + 1);\n-\t\tstrcpy(p, cmd_p->ref_name);\n+\t\tp = xmalloc(strlen(cmd->ref_name) + 1);\n+\t\tstrcpy(p, cmd->ref_name);\n \t\targv[argc] = p;\n \t\targc++;\n \t}\n@@ -488,38 +486,34 @@ static void run_update_post_hook(struct command *cmd)\n \t}\n }\n \n-static void execute_commands(const char *unpacker_error)\n+static void execute_commands(struct command *commands, const char *unpacker_error)\n {\n-\tstruct command *cmd = commands;\n+\tstruct command *cmd;\n \tunsigned char sha1[20];\n \n \tif (unpacker_error) {\n-\t\twhile (cmd) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\t\tcmd->error_string = \"n/a (unpacker error)\";\n-\t\t\tcmd = cmd->next;\n-\t\t}\n \t\treturn;\n \t}\n \n-\tif (run_receive_hook(pre_receive_hook)) {\n-\t\twhile (cmd) {\n+\tif (run_receive_hook(commands, pre_receive_hook)) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\t\tcmd->error_string = \"pre-receive hook declined\";\n-\t\t\tcmd = cmd->next;\n-\t\t}\n \t\treturn;\n \t}\n \n \thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n \n-\twhile (cmd) {\n+\tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tcmd->error_string = update(cmd);\n-\t\tcmd = cmd->next;\n-\t}\n }\n \n-static void read_head_info(void)\n+static struct command *read_head_info(void)\n {\n+\tstruct command *commands = NULL;\n \tstruct command **p = &commands;\n+\n \tfor (;;) {\n \t\tstatic char line[1000];\n \t\tunsigned char old_sha1[20], new_sha1[20];\n@@ -557,6 +551,7 @@ static void read_head_info(void)\n \t\t*p = cmd;\n \t\tp = &cmd->next;\n \t}\n+\treturn commands;\n }\n \n static const char *parse_pack_header(struct pack_header *hdr)\n@@ -643,10 +638,10 @@ static const char *unpack(void)\n \t}\n }\n \n-static void report(const char *unpack_status)\n+static void report(struct command *commands, const char *unpack_status)\n {\n-\tstruct command *cmd;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct command *cmd;\n \n \tpacket_buf_write(&buf, \"unpack %s\\n\",\n \t\t\t unpack_status ? unpack_status : \"ok\");\n@@ -667,12 +662,12 @@ static void report(const char *unpack_status)\n \tstrbuf_release(&buf);\n }\n \n-static int delete_only(struct command *cmd)\n+static int delete_only(struct command *commands)\n {\n-\twhile (cmd) {\n+\tstruct command *cmd;\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!is_null_sha1(cmd->new_sha1))\n \t\t\treturn 0;\n-\t\tcmd = cmd->next;\n \t}\n \treturn 1;\n }\n@@ -722,6 +717,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tint stateless_rpc = 0;\n \tint i;\n \tchar *dir = NULL;\n+\tstruct command *commands;\n \n \targv++;\n \tfor (i = 1; i < argc; i++) {\n@@ -772,18 +768,17 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \tif (advertise_refs)\n \t\treturn 0;\n \n-\tread_head_info();\n-\tif (commands) {\n+\tif ((commands = read_head_info()) != NULL) {\n \t\tconst char *unpack_status = NULL;\n \n \t\tif (!delete_only(commands))\n \t\t\tunpack_status = unpack();\n-\t\texecute_commands(unpack_status);\n+\t\texecute_commands(commands, unpack_status);\n \t\tif (pack_lockfile)\n \t\t\tunlink_or_warn(pack_lockfile);\n \t\tif (report_status)\n-\t\t\treport(unpack_status);\n-\t\trun_receive_hook(post_receive_hook);\n+\t\t\treport(commands, unpack_status);\n+\t\trun_receive_hook(commands, post_receive_hook);\n \t\trun_update_post_hook(commands);\n \t\tif (auto_gc) {\n \t\t\tconst char *argv_gc_auto[] = {\n-- \n1.7.0.3.436.g2b878\n"},{"id":"139625","messageId":"1271364848-32805-2-git-send-email-jaysoffian@gmail.com","threadId":"23484","inReplyTo":"1271364848-32805-1-git-send-email-jaysoffian@gmail.com","subject":"[PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-04-15T20:54:08Z","receivedAt":"2010-04-15T20:54:08Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"When pushing to a remote repo, normally the sending side tries to\nfilter out any aliased updates (e.g, foo:baz bar:baz). However, it\nis impossible for the sender to know if two refs are aliased on the\nreceiving side via symrefs. Here is one such scenario:\n\n$ git init origin\n(cd origin && touch file && git add file && git commit -a -m intial)\ngit clone --bare origin origin.git\nrm -rf origin\n\ngit clone origin.git client\n\ngit clone --mirror client backup.git &&\n(cd backup.git && git remote set-head origin --auto)\n\n(cd client &&\n\tgit remote add --mirror backup ../backup.git &&\n\techo change1 > file && git commit -a -m change1 &&\n\tgit push origin &&\n\tgit push backup\n)\n\nThe push to backup fails with:\n\nCounting objects: 5, done.\nWriting objects: 100% (3/3), 244 bytes, done.\nTotal 3 (delta 0), reused 0 (delta 0)\nUnpacking objects: 100% (3/3), done.\nerror: Ref refs/remotes/origin/master is at ef307ff6d0026900f84bae7bfe2f5d695238ca66 but expected 262cd5762e76e0aca2c185a3995095318772e2f2\nremote: error: failed to lock refs/remotes/origin/master\nTo ../backup.git\n   262cd57..ef307ff  master -> master\n   262cd57..ef307ff  origin/HEAD -> origin/HEAD\n ! [remote rejected] origin/master -> origin/master (failed to lock)\nerror: failed to push some refs to '../backup.git'\n\nThe reason is that refs/remotes/origin/HEAD is a symref to\nrefs/remotes/origin/master, but it is impossible for the sending\nside to unambiguously know this in all cases.\n\nThis commit fixes the issue by having receive pack ignore updates to\nany symrefs IFF the target of the symref is being identically\nupdated.\n\nSigned-off-by: Jay Soffian <jaysoffian@gmail.com>\n---\n builtin/receive-pack.c |   35 +++++++++++++++++++++++++++++++++--\n t/t5516-fetch-push.sh  |   20 ++++++++++++++++++++\n 2 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 3fc73cf..a2e3bc8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -9,6 +9,7 @@\n #include \"object.h\"\n #include \"remote.h\"\n #include \"transport.h\"\n+#include \"string-list.h\"\n \n static const char receive_pack_usage[] = \"git receive-pack <git-dir>\";\n \n@@ -486,10 +487,30 @@ static void run_update_post_hook(struct command *commands)\n \t}\n }\n \n+static int aliased_ref(struct command *cmd, struct string_list *list)\n+{\n+\tstruct string_list_item *item;\n+\tunsigned char sha1[20];\n+\tint flag;\n+\n+\tconst char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);\n+\n+\tif (!(flag & REF_ISSYMREF))\n+\t\treturn 0;\n+\n+\tif ((item = string_list_lookup(dst_name, list)) != NULL) {\n+\t\tstruct command *other_cmd = (struct command *) item->util;\n+\t\treturn (!(hashcmp(cmd->old_sha1, other_cmd->old_sha1) &&\n+\t\t\thashcmp(cmd->new_sha1, other_cmd->new_sha1)));\n+\t}\n+\treturn 0;\n+}\n+\n static void execute_commands(struct command *commands, const char *unpacker_error)\n {\n \tstruct command *cmd;\n \tunsigned char sha1[20];\n+\tstruct string_list ref_list = { NULL, 0, 0, 0 };\n \n \tif (unpacker_error) {\n \t\tfor (cmd = commands; cmd; cmd = cmd->next)\n@@ -505,8 +526,18 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n \n-\tfor (cmd = commands; cmd; cmd = cmd->next)\n-\t\tcmd->error_string = update(cmd);\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tstruct string_list_item *item =\n+\t\t\tstring_list_append(cmd->ref_name, &ref_list);\n+\t\titem->util = (void *)cmd;\n+\t}\n+\tsort_string_list(&ref_list);\n+\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (!aliased_ref(cmd, &ref_list))\n+\t\t\tcmd->error_string = update(cmd);\n+\t}\n+\tstring_list_clear(&ref_list, 0);\n }\n \n static struct command *read_head_info(void)\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 2de98e6..c384529 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -660,6 +660,26 @@ test_expect_success 'push with branches containing #' '\n \tgit checkout master\n '\n \n+test_expect_success 'push into aliased refs' '\n+\tmk_test heads/master &&\n+\tmk_child child1 &&\n+\tmk_child child2 &&\n+\t(cd child1 &&\n+\t\tgit branch foo &&\n+\t\tgit symbolic-ref refs/heads/bar refs/heads/foo\n+\t\tgit config receive.denyCurrentBranch false\n+\t) &&\n+\t(cd child2 &&\n+\t\t: >path2 &&\n+\t\tgit add path2 &&\n+\t\ttest_tick &&\n+\t\tgit commit -a -m child2 &&\n+\t\tgit branch foo &&\n+\t\tgit branch bar &&\n+\t\tgit push ../child1 foo bar\n+\t)\n+'\n+\n test_expect_success 'push --porcelain' '\n \tmk_empty &&\n \techo >.git/foo  \"To testrepo\" &&\n-- \n1.7.0.3.436.g2b878\n"},{"id":"139634","messageId":"7v39ywiat9.fsf@alter.siamese.dyndns.org","threadId":"23484","inReplyTo":"1271364848-32805-2-git-send-email-jaysoffian@gmail.com","subject":"Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-15T22:57:06Z","receivedAt":"2010-04-15T22:57:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> When pushing to a remote repo, normally the sending side tries to\n> filter out any aliased updates (e.g, foo:baz bar:baz). However, it\n> is impossible for the sender to know if two refs are aliased on the\n> receiving side via symrefs. Here is one such scenario:\n>\n> $ git init origin\n\nIndent this line a bit.\n\n> (cd origin && touch file && git add file && git commit -a -m intial)\n> git clone --bare origin origin.git\n> rm -rf origin\n>\n> git clone origin.git client\n>\n> git clone --mirror client backup.git &&\n> (cd backup.git && git remote set-head origin --auto)\n>\n> (cd client &&\n> \tgit remote add --mirror backup ../backup.git &&\n> \techo change1 > file && git commit -a -m change1 &&\n> \tgit push origin &&\n> \tgit push backup\n> )\n\nConsistently use prompt and indent these to match the first \"init\".\n\n>\n> The push to backup fails with:\n>\n> Counting objects: 5, done.\n> Writing objects: 100% (3/3), 244 bytes, done.\n> Total 3 (delta 0), reused 0 (delta 0)\n> Unpacking objects: 100% (3/3), done.\n> error: Ref refs/remotes/origin/master is at ef307ff6d0026900f84bae7bfe2f5d695238ca66 but expected 262cd5762e76e0aca2c185a3995095318772e2f2\n\nIndent this as well, and trim the object names as the exact values do not\nmatter.\n\n> remote: error: failed to lock refs/remotes/origin/master\n> To ../backup.git\n>    262cd57..ef307ff  master -> master\n>    262cd57..ef307ff  origin/HEAD -> origin/HEAD\n>  ! [remote rejected] origin/master -> origin/master (failed to lock)\n> error: failed to push some refs to '../backup.git'\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 3fc73cf..a2e3bc8 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -9,6 +9,7 @@\n>  #include \"object.h\"\n>  #include \"remote.h\"\n>  #include \"transport.h\"\n> +#include \"string-list.h\"\n>  \n>  static const char receive_pack_usage[] = \"git receive-pack <git-dir>\";\n>  \n> @@ -486,10 +487,30 @@ static void run_update_post_hook(struct command *commands)\n>  \t}\n>  }\n>  \n> +static int aliased_ref(struct command *cmd, struct string_list *list)\n> +{\n\nNit; what this does sounds more like \"aliased update\" to me.\n\n> +\tstruct string_list_item *item;\n> +\tunsigned char sha1[20];\n> +\tint flag;\n> +\n> +\tconst char *dst_name = resolve_ref(cmd->ref_name, sha1, 0, &flag);\n> +\n> +\tif (!(flag & REF_ISSYMREF))\n> +\t\treturn 0;\n> +\n> +\tif ((item = string_list_lookup(dst_name, list)) != NULL) {\n> +\t\tstruct command *other_cmd = (struct command *) item->util;\n> +\t\treturn (!(hashcmp(cmd->old_sha1, other_cmd->old_sha1) &&\n> +\t\t\thashcmp(cmd->new_sha1, other_cmd->new_sha1)));\n\nThis will also catch two symrefs that point at the same underlying ref.\nIf all three are updated consistently then all will be fine.  If even one\nof them is inconsistent, we will try the update() and give an error\nmessage.\n\nWe _could_ give even stronger error message to help diagnosing the\nsituation if we wanted to.\n\nVery nice.\n"},{"id":"139635","messageId":"y2g76718491004151607u58021df8naab23b6daf06513d@mail.gmail.com","threadId":"23484","inReplyTo":"7v39ywiat9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-04-15T23:07:52Z","receivedAt":"2010-04-15T23:07:52Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Apr 15, 2010 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jay Soffian <jaysoffian@gmail.com> writes:\n>\n>> When pushing to a remote repo, normally the sending side tries to\n>> filter out any aliased updates (e.g, foo:baz bar:baz). However, it\n>> is impossible for the sender to know if two refs are aliased on the\n>> receiving side via symrefs. Here is one such scenario:\n>>\n\n> [... lots of good feedback ...]\n\nWill incorporate and re-roll.\n\n> This will also catch two symrefs that point at the same underlying ref.\n> If all three are updated consistently then all will be fine.  If even one\n> of them is inconsistent, we will try the update() and give an error\n> message.\n>\n> We _could_ give even stronger error message to help diagnosing the\n> situation if we wanted to.\n\nOkay, I'll see what I can figure out.\n\n> Very nice.\n\nI appreciate the compliment, but it is unexpected after reading your\nreply to \"failed to lock\". I think your suggestion there (fix\nverify_lock()) probably addresses the real problem in which case this\nis just a band-aid.\n\nj.\n"},{"id":"139854","messageId":"7v7ho4uzjr.fsf@alter.siamese.dyndns.org","threadId":"23484","inReplyTo":"y2g76718491004151607u58021df8naab23b6daf06513d@mail.gmail.com","subject":"Re: [PATCH 2/2] receive-pack: ignore duplicated commands which can occur with symrefs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-18T23:11:04Z","receivedAt":"2010-04-18T23:11:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> On Thu, Apr 15, 2010 at 6:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Very nice.\n>\n> I appreciate the compliment, but it is unexpected after reading your\n> reply to \"failed to lock\". I think your suggestion there (fix\n> verify_lock()) probably addresses the real problem in which case this\n> is just a band-aid.\n\nNot necessarily.  I thought I said we may need _both_, and this is a good\nchange regardless.\n\nThanks.\n"}]}