{"thread":{"id":"46244","subject":"[PATCH] submodule--helper: teach push-check to handle HEAD","startedAt":"2017-06-23T20:04:37Z","lastAt":"2017-07-20T17:45:55Z","messageCount":5,"participants":["Brandon Williams","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323144","messageId":"20170623200427.26803-1-bmwill@google.com","threadId":"46244","inReplyTo":null,"subject":"[PATCH] submodule--helper: teach push-check to handle HEAD","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-23T20:04:27Z","receivedAt":"2017-06-23T20:04:37Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In 06bf4ad1d (push: propagate remote and refspec with\n--recurse-submodules) push was taught how to propagate a refspec down to\nsubmodules when the '--recurse-submodules' flag is given.  The only refspecs\nthat are allowed to be propagated are ones which name a ref which exists\nin both the superproject and the submodule, with the caveat that 'HEAD'\nwas disallowed.\n\nThis patch teaches push-check (the submodule helper which determines if\na refspec can be propagated to a submodule) to permit propagating 'HEAD'\nif and only if the superproject and the submodule both have the same\nnamed branch checked out and the submodule is not in a detached head\nstate.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n builtin/submodule--helper.c    | 57 +++++++++++++++++++++++++++++++-----------\n submodule.c                    | 18 ++++++++++---\n t/t5531-deep-submodule-push.sh | 25 +++++++++++++++++-\n 3 files changed, 82 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1b4d2b346..fd5020036 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n static int push_check(int argc, const char **argv, const char *prefix)\n {\n \tstruct remote *remote;\n+\tconst char *superproject_head;\n+\tchar *head;\n+\tint detached_head = 0;\n+\tstruct object_id head_oid;\n \n-\tif (argc < 2)\n-\t\tdie(\"submodule--helper push-check requires at least 1 argument\");\n+\tif (argc < 3)\n+\t\tdie(\"submodule--helper push-check requires at least 2 argument\");\n+\n+\t/*\n+\t * superproject's resolved head ref.\n+\t * if HEAD then the superproject is in a detached head state, otherwise\n+\t * it will be the resolved head ref.\n+\t */\n+\tsuperproject_head = argv[1];\n+\t/* Get the submodule's head ref and determine if it is detached */\n+\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n+\tif (!head)\n+\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\tif (!strcmp(head, \"HEAD\"))\n+\t\tdetached_head = 1;\n \n \t/*\n \t * The remote must be configured.\n \t * This is to avoid pushing to the exact same URL as the parent.\n \t */\n-\tremote = pushremote_get(argv[1]);\n+\tremote = pushremote_get(argv[2]);\n \tif (!remote || remote->origin == REMOTE_UNCONFIGURED)\n-\t\tdie(\"remote '%s' not configured\", argv[1]);\n+\t\tdie(\"remote '%s' not configured\", argv[2]);\n \n \t/* Check the refspec */\n-\tif (argc > 2) {\n-\t\tint i, refspec_nr = argc - 2;\n+\tif (argc > 3) {\n+\t\tint i, refspec_nr = argc - 3;\n \t\tstruct ref *local_refs = get_local_heads();\n \t\tstruct refspec *refspec = parse_push_refspec(refspec_nr,\n-\t\t\t\t\t\t\t     argv + 2);\n+\t\t\t\t\t\t\t     argv + 3);\n \n \t\tfor (i = 0; i < refspec_nr; i++) {\n \t\t\tstruct refspec *rs = refspec + i;\n@@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv, const char *prefix)\n \t\t\tif (rs->pattern || rs->matching)\n \t\t\t\tcontinue;\n \n-\t\t\t/*\n-\t\t\t * LHS must match a single ref\n-\t\t\t * NEEDSWORK: add logic to special case 'HEAD' once\n-\t\t\t * working with submodules in a detached head state\n-\t\t\t * ceases to be the norm.\n-\t\t\t */\n-\t\t\tif (count_refspec_match(rs->src, local_refs, NULL) != 1)\n+\t\t\t/* LHS must match a single ref */\n+\t\t\tswitch(count_refspec_match(rs->src, local_refs, NULL)) {\n+\t\t\tcase 1:\n+\t\t\t\tbreak;\n+\t\t\tcase 0:\n+\t\t\t\t/*\n+\t\t\t\t * If LHS matches 'HEAD' then we need to ensure\n+\t\t\t\t * that it matches the same named branch\n+\t\t\t\t * checked out in the superproject.\n+\t\t\t\t */\n+\t\t\t\tif (!strcmp(rs->src, \"HEAD\")) {\n+\t\t\t\t\tif (!detached_head &&\n+\t\t\t\t\t    !strcmp(head, superproject_head))\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\tdie(\"HEAD does not match the named branch in the superproject\");\n+\t\t\t\t}\n+\t\t\tdefault:\n \t\t\t\tdie(\"src refspec '%s' must name a ref\",\n \t\t\t\t    rs->src);\n+\t\t\t}\n \t\t}\n \t\tfree_refspec(refspec_nr, refspec);\n \t}\n+\tfree(head);\n \n \treturn 0;\n }\ndiff --git a/submodule.c b/submodule.c\nindex 1b8a3b575..920b49328 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1003,7 +1003,8 @@ static int push_submodule(const char *path,\n  * Perform a check in the submodule to see if the remote and refspec work.\n  * Die if the submodule can't be pushed.\n  */\n-static void submodule_push_check(const char *path, const struct remote *remote,\n+static void submodule_push_check(const char *path, const char *head,\n+\t\t\t\t const struct remote *remote,\n \t\t\t\t const char **refspec, int refspec_nr)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1011,6 +1012,7 @@ static void submodule_push_check(const char *path, const struct remote *remote,\n \n \targv_array_push(&cp.args, \"submodule--helper\");\n \targv_array_push(&cp.args, \"push-check\");\n+\targv_array_push(&cp.args, head);\n \targv_array_push(&cp.args, remote->name);\n \n \tfor (i = 0; i < refspec_nr; i++)\n@@ -1049,10 +1051,20 @@ int push_unpushed_submodules(struct oid_array *commits,\n \t * won't be propagated due to the remote being unconfigured (e.g. a URL\n \t * instead of a remote name).\n \t */\n-\tif (remote->origin != REMOTE_UNCONFIGURED)\n+\tif (remote->origin != REMOTE_UNCONFIGURED) {\n+\t\tchar *head;\n+\t\tstruct object_id head_oid;\n+\n+\t\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n+\t\tif (!head)\n+\t\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\n \t\tfor (i = 0; i < needs_pushing.nr; i++)\n \t\t\tsubmodule_push_check(needs_pushing.items[i].string,\n-\t\t\t\t\t     remote, refspec, refspec_nr);\n+\t\t\t\t\t     head, remote,\n+\t\t\t\t\t     refspec, refspec_nr);\n+\t\tfree(head);\n+\t}\n \n \t/* Actually push the submodules */\n \tfor (i = 0; i < needs_pushing.nr; i++) {\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex beff65b8a..0f84a5314 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -533,7 +533,8 @@ test_expect_success 'push propagating refspec to a submodule' '\n \t# Fails when refspec includes an object id\n \ttest_must_fail git -C work push --recurse-submodules=on-demand origin \\\n \t\t\"$(git -C work rev-parse branch2):refs/heads/branch2\" &&\n-\t# Fails when refspec includes 'HEAD' as it is unsupported at this time\n+\t# Fails when refspec includes HEAD and parent and submodule do not\n+\t# have the same named branch checked out\n \ttest_must_fail git -C work push --recurse-submodules=on-demand origin \\\n \t\tHEAD:refs/heads/branch2 &&\n \n@@ -548,4 +549,26 @@ test_expect_success 'push propagating refspec to a submodule' '\n \ttest_cmp expected_pub actual_pub\n '\n \n+test_expect_success 'push propagating HEAD refspec to a submodule' '\n+\tgit -C work/gar/bage checkout branch2 &&\n+\t> work/gar/bage/junk12 &&\n+\tgit -C work/gar/bage add junk12 &&\n+\tgit -C work/gar/bage commit -m \"Twelfth junk\" &&\n+\n+\tgit -C work checkout branch2 &&\n+\tgit -C work add gar/bage &&\n+\tgit -C work commit -m \"updating gar/bage in branch2\" &&\n+\n+\t# Passes since the superproject and submodules HEAD are both on branch2\n+\tgit -C work push --recurse-submodules=on-demand origin \\\n+\t\tHEAD:refs/heads/branch2 &&\n+\n+\tgit -C submodule.git rev-parse branch2 >actual_submodule &&\n+\tgit -C pub.git rev-parse branch2 >actual_pub &&\n+\tgit -C work/gar/bage rev-parse branch2 >expected_submodule &&\n+\tgit -C work rev-parse branch2 >expected_pub &&\n+\ttest_cmp expected_submodule actual_submodule &&\n+\ttest_cmp expected_pub actual_pub\n+'\n+\n test_done\n-- \n2.13.1.704.gde00cce3c-goog\n\n"},{"id":"323151","messageId":"CAGZ79kZqjaXSrqawgDnxu58sSPWVm8kAhRxn69Sbo8_aP4rr6w@mail.gmail.com","threadId":"46244","inReplyTo":"20170623200427.26803-1-bmwill@google.com","subject":"Re: [PATCH] submodule--helper: teach push-check to handle HEAD","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-23T20:26:30Z","receivedAt":"2017-06-23T20:26:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jun 23, 2017 at 1:04 PM, Brandon Williams <bmwill@google.com> wrote:\n> In 06bf4ad1d (push: propagate remote and refspec with\n> --recurse-submodules) push was taught how to propagate a refspec down to\n> submodules when the '--recurse-submodules' flag is given.  The only refspecs\n> that are allowed to be propagated are ones which name a ref which exists\n> in both the superproject and the submodule, with the caveat that 'HEAD'\n> was disallowed.\n>\n> This patch teaches push-check (the submodule helper which determines if\n> a refspec can be propagated to a submodule) to permit propagating 'HEAD'\n> if and only if the superproject and the submodule both have the same\n> named branch checked out and the submodule is not in a detached head\n> state.\n\ncont'd:\n\nWe need this use case because Gerrit's documentation ingrains\nthis workflow in its users to use\n\n    git push <remote> HEAD:refs/for/<target-branch>\n\nAnd when both the submodule as well as the superproject\nare still on a branch with the same name (and not detached) we'd\nnot be misunderstood by the user on the syntax.\n\nMore on the code later.\n"},{"id":"323167","messageId":"xmqqk242b9nf.fsf@gitster.mtv.corp.google.com","threadId":"46244","inReplyTo":"20170623200427.26803-1-bmwill@google.com","subject":"Re: [PATCH] submodule--helper: teach push-check to handle HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-23T22:51:16Z","receivedAt":"2017-06-23T22:51:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> In 06bf4ad1d (push: propagate remote and refspec with\n> --recurse-submodules) push was taught how to propagate a refspec down to\n> submodules when the '--recurse-submodules' flag is given.  The only refspecs\n> that are allowed to be propagated are ones which name a ref which exists\n> in both the superproject and the submodule, with the caveat that 'HEAD'\n> was disallowed.\n>\n> This patch teaches push-check (the submodule helper which determines if\n> a refspec can be propagated to a submodule) to permit propagating 'HEAD'\n> if and only if the superproject and the submodule both have the same\n> named branch checked out and the submodule is not in a detached head\n> state.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  builtin/submodule--helper.c    | 57 +++++++++++++++++++++++++++++++-----------\n>  submodule.c                    | 18 ++++++++++---\n>  t/t5531-deep-submodule-push.sh | 25 +++++++++++++++++-\n>  3 files changed, 82 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 1b4d2b346..fd5020036 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n>  static int push_check(int argc, const char **argv, const char *prefix)\n>  {\n>  \tstruct remote *remote;\n> +\tconst char *superproject_head;\n> +\tchar *head;\n> +\tint detached_head = 0;\n> +\tstruct object_id head_oid;\n>  \n> -\tif (argc < 2)\n> -\t\tdie(\"submodule--helper push-check requires at least 1 argument\");\n> +\tif (argc < 3)\n> +\t\tdie(\"submodule--helper push-check requires at least 2 argument\");\n\n\"arguments\"?\n\n> +\n> +\t/*\n> +\t * superproject's resolved head ref.\n> +\t * if HEAD then the superproject is in a detached head state, otherwise\n> +\t * it will be the resolved head ref.\n> +\t */\n> +\tsuperproject_head = argv[1];\n\nThe above makes it sound like the caller gives either \"HEAD\" (when\ndetached) or \"refs/heads/branch\" (when on 'branch') in argv[1] and\nyou are stashing it away, but ...\n\n> +\t/* Get the submodule's head ref and determine if it is detached */\n> +\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n> +\tif (!head)\n> +\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n> +\tif (!strcmp(head, \"HEAD\"))\n> +\t\tdetached_head = 1;\n\n... the work to see which branch we are on and if we are detached is\ndone by this code without consulting argv[1].  I cannot tell what is\ngoing on.  Is argv[1] assigned to superproject_head a red herring?\n\n>  \t/*\n>  \t * The remote must be configured.\n>  \t * This is to avoid pushing to the exact same URL as the parent.\n>  \t */\n> -\tremote = pushremote_get(argv[1]);\n> +\tremote = pushremote_get(argv[2]);\n>  \tif (!remote || remote->origin == REMOTE_UNCONFIGURED)\n> -\t\tdie(\"remote '%s' not configured\", argv[1]);\n> +\t\tdie(\"remote '%s' not configured\", argv[2]);\n>  \n>  \t/* Check the refspec */\n> -\tif (argc > 2) {\n> -\t\tint i, refspec_nr = argc - 2;\n> +\tif (argc > 3) {\n> +\t\tint i, refspec_nr = argc - 3;\n>  \t\tstruct ref *local_refs = get_local_heads();\n>  \t\tstruct refspec *refspec = parse_push_refspec(refspec_nr,\n> -\t\t\t\t\t\t\t     argv + 2);\n> +\t\t\t\t\t\t\t     argv + 3);\n\nIf you have no need for argv[1] (and you don't, as you have stashed\nit away in superproject_head), it may be less damage to the code if\nyou shifted argv upfront after grabbing superproject_head.\n\n>  \t\tfor (i = 0; i < refspec_nr; i++) {\n>  \t\t\tstruct refspec *rs = refspec + i;\n> @@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv, const char *prefix)\n>  \t\t\tif (rs->pattern || rs->matching)\n>  \t\t\t\tcontinue;\n>  \n> -\t\t\t/*\n> -\t\t\t * LHS must match a single ref\n> -\t\t\t * NEEDSWORK: add logic to special case 'HEAD' once\n> -\t\t\t * working with submodules in a detached head state\n> -\t\t\t * ceases to be the norm.\n> -\t\t\t */\n> -\t\t\tif (count_refspec_match(rs->src, local_refs, NULL) != 1)\n> +\t\t\t/* LHS must match a single ref */\n> +\t\t\tswitch(count_refspec_match(rs->src, local_refs, NULL)) {\n\n\"switch (count...\"\n\n> +\t\t\tcase 1:\n> +\t\t\t\tbreak;\n> +\t\t\tcase 0:\n> +\t\t\t\t/*\n> +\t\t\t\t * If LHS matches 'HEAD' then we need to ensure\n> +\t\t\t\t * that it matches the same named branch\n> +\t\t\t\t * checked out in the superproject.\n> +\t\t\t\t */\n> +\t\t\t\tif (!strcmp(rs->src, \"HEAD\")) {\n> +\t\t\t\t\tif (!detached_head &&\n> +\t\t\t\t\t    !strcmp(head, superproject_head))\n> +\t\t\t\t\t\tbreak;\n> +\t\t\t\t\tdie(\"HEAD does not match the named branch in the superproject\");\n> +\t\t\t\t}\n\nHmph, so earlier people can \"push --recurse-submodules HEAD:$dest\"\nand $dest can be anything, but now we are tightening the rule?\n\n> +\t\t\tdefault:\n>  \t\t\t\tdie(\"src refspec '%s' must name a ref\",\n>  \t\t\t\t    rs->src);\n> +\t\t\t}\n>  \t\t}\n>  \t\tfree_refspec(refspec_nr, refspec);\n>  \t}\n> +\tfree(head);\n>  \n>  \treturn 0;\n>  }\n"},{"id":"323285","messageId":"20170626181258.GA177061@google.com","threadId":"46244","inReplyTo":"xmqqk242b9nf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] submodule--helper: teach push-check to handle HEAD","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-06-26T18:12:58Z","receivedAt":"2017-06-26T18:13:17Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/23, Junio C Hamano wrote:\n> Brandon Williams <bmwill@google.com> writes:\n> \n> > In 06bf4ad1d (push: propagate remote and refspec with\n> > --recurse-submodules) push was taught how to propagate a refspec down to\n> > submodules when the '--recurse-submodules' flag is given.  The only refspecs\n> > that are allowed to be propagated are ones which name a ref which exists\n> > in both the superproject and the submodule, with the caveat that 'HEAD'\n> > was disallowed.\n> >\n> > This patch teaches push-check (the submodule helper which determines if\n> > a refspec can be propagated to a submodule) to permit propagating 'HEAD'\n> > if and only if the superproject and the submodule both have the same\n> > named branch checked out and the submodule is not in a detached head\n> > state.\n> >\n> > Signed-off-by: Brandon Williams <bmwill@google.com>\n> > ---\n> >  builtin/submodule--helper.c    | 57 +++++++++++++++++++++++++++++++-----------\n> >  submodule.c                    | 18 ++++++++++---\n> >  t/t5531-deep-submodule-push.sh | 25 +++++++++++++++++-\n> >  3 files changed, 82 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> > index 1b4d2b346..fd5020036 100644\n> > --- a/builtin/submodule--helper.c\n> > +++ b/builtin/submodule--helper.c\n> > @@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n> >  static int push_check(int argc, const char **argv, const char *prefix)\n> >  {\n> >  \tstruct remote *remote;\n> > +\tconst char *superproject_head;\n> > +\tchar *head;\n> > +\tint detached_head = 0;\n> > +\tstruct object_id head_oid;\n> >  \n> > -\tif (argc < 2)\n> > -\t\tdie(\"submodule--helper push-check requires at least 1 argument\");\n> > +\tif (argc < 3)\n> > +\t\tdie(\"submodule--helper push-check requires at least 2 argument\");\n> \n> \"arguments\"?\n \nYou're right, I'll fix the typo.\n\n> > +\n> > +\t/*\n> > +\t * superproject's resolved head ref.\n> > +\t * if HEAD then the superproject is in a detached head state, otherwise\n> > +\t * it will be the resolved head ref.\n> > +\t */\n> > +\tsuperproject_head = argv[1];\n> \n> The above makes it sound like the caller gives either \"HEAD\" (when\n> detached) or \"refs/heads/branch\" (when on 'branch') in argv[1] and\n> you are stashing it away, but ...\n> \n> > +\t/* Get the submodule's head ref and determine if it is detached */\n> > +\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n> > +\tif (!head)\n> > +\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n> > +\tif (!strcmp(head, \"HEAD\"))\n> > +\t\tdetached_head = 1;\n> \n> ... the work to see which branch we are on and if we are detached is\n> done by this code without consulting argv[1].  I cannot tell what is\n> going on.  Is argv[1] assigned to superproject_head a red herring?\n\nThe idea is that 'git submodule--helper push-check' is called by a\nsuperproject on every submodule that may be pushed.  So this command is\ninvoked on the submodule itself.  This change requires knowing what\n'HEAD' refers to in the superproject (either detached or a named branch)\nso the superproject passes that information to the submodule via\nargv[1].  This snippet of code is responsible for finding what 'HEAD'\nrefers to in the submodule so that later we can compare the\nsuperproject's and submodule's 'HEAD' ref to see if they match the same\nnamed branch.\n\n> \n> >  \t/*\n> >  \t * The remote must be configured.\n> >  \t * This is to avoid pushing to the exact same URL as the parent.\n> >  \t */\n> > -\tremote = pushremote_get(argv[1]);\n> > +\tremote = pushremote_get(argv[2]);\n> >  \tif (!remote || remote->origin == REMOTE_UNCONFIGURED)\n> > -\t\tdie(\"remote '%s' not configured\", argv[1]);\n> > +\t\tdie(\"remote '%s' not configured\", argv[2]);\n> >  \n> >  \t/* Check the refspec */\n> > -\tif (argc > 2) {\n> > -\t\tint i, refspec_nr = argc - 2;\n> > +\tif (argc > 3) {\n> > +\t\tint i, refspec_nr = argc - 3;\n> >  \t\tstruct ref *local_refs = get_local_heads();\n> >  \t\tstruct refspec *refspec = parse_push_refspec(refspec_nr,\n> > -\t\t\t\t\t\t\t     argv + 2);\n> > +\t\t\t\t\t\t\t     argv + 3);\n> \n> If you have no need for argv[1] (and you don't, as you have stashed\n> it away in superproject_head), it may be less damage to the code if\n> you shifted argv upfront after grabbing superproject_head.\n> \n> >  \t\tfor (i = 0; i < refspec_nr; i++) {\n> >  \t\t\tstruct refspec *rs = refspec + i;\n> > @@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv, const char *prefix)\n> >  \t\t\tif (rs->pattern || rs->matching)\n> >  \t\t\t\tcontinue;\n> >  \n> > -\t\t\t/*\n> > -\t\t\t * LHS must match a single ref\n> > -\t\t\t * NEEDSWORK: add logic to special case 'HEAD' once\n> > -\t\t\t * working with submodules in a detached head state\n> > -\t\t\t * ceases to be the norm.\n> > -\t\t\t */\n> > -\t\t\tif (count_refspec_match(rs->src, local_refs, NULL) != 1)\n> > +\t\t\t/* LHS must match a single ref */\n> > +\t\t\tswitch(count_refspec_match(rs->src, local_refs, NULL)) {\n> \n> \"switch (count...\"\n> \n> > +\t\t\tcase 1:\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase 0:\n> > +\t\t\t\t/*\n> > +\t\t\t\t * If LHS matches 'HEAD' then we need to ensure\n> > +\t\t\t\t * that it matches the same named branch\n> > +\t\t\t\t * checked out in the superproject.\n> > +\t\t\t\t */\n> > +\t\t\t\tif (!strcmp(rs->src, \"HEAD\")) {\n> > +\t\t\t\t\tif (!detached_head &&\n> > +\t\t\t\t\t    !strcmp(head, superproject_head))\n> > +\t\t\t\t\t\tbreak;\n> > +\t\t\t\t\tdie(\"HEAD does not match the named branch in the superproject\");\n> > +\t\t\t\t}\n> \n> Hmph, so earlier people can \"push --recurse-submodules HEAD:$dest\"\n> and $dest can be anything, but now we are tightening the rule?\n\nI don't believe that anything is changing w.r.t. what $dest can be\n(unless I'm missing something).  This is more about enabling 'HEAD' to\nbe used on the LHS of a refspec as before it wasn't permitted.  With\nthis change a user can use 'push --recurse-submodules HEAD:$dest' and\nthe refspec which includes 'HEAD' on the LHS will be propagated to the\nsubmodules if and only if 'HEAD' is not detached in the superproject or\nsubmodule and 'HEAD' refers to the same named branch.\n\n> \n> > +\t\t\tdefault:\n> >  \t\t\t\tdie(\"src refspec '%s' must name a ref\",\n> >  \t\t\t\t    rs->src);\n> > +\t\t\t}\n> >  \t\t}\n> >  \t\tfree_refspec(refspec_nr, refspec);\n> >  \t}\n> > +\tfree(head);\n> >  \n> >  \treturn 0;\n> >  }\n\n-- \nBrandon Williams\n"},{"id":"324816","messageId":"20170720174037.45305-1-bmwill@google.com","threadId":"46244","inReplyTo":"20170623200427.26803-1-bmwill@google.com","subject":"[PATCH v2 1/1] submodule--helper: teach push-check to handle HEAD","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-07-20T17:40:37Z","receivedAt":"2017-07-20T17:45:55Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"In 06bf4ad1d (push: propagate remote and refspec with\n--recurse-submodules) push was taught how to propagate a refspec down to\nsubmodules when the '--recurse-submodules' flag is given.  The only refspecs\nthat are allowed to be propagated are ones which name a ref which exists\nin both the superproject and the submodule, with the caveat that 'HEAD'\nwas disallowed.\n\nThis patch teaches push-check (the submodule helper which determines if\na refspec can be propagated to a submodule) to permit propagating 'HEAD'\nif and only if the superproject and the submodule both have the same\nnamed branch checked out and the submodule is not in a detached head\nstate.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n\nChanges in V2:\n * fixed a few style issues\n * shifted argv/argc to prevent more damage to the code than is necessary.\n\n builtin/submodule--helper.c    | 49 ++++++++++++++++++++++++++++++++++--------\n submodule.c                    | 18 +++++++++++++---\n t/t5531-deep-submodule-push.sh | 25 ++++++++++++++++++++-\n 3 files changed, 79 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6abdad329..0939e3912 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1108,9 +1108,28 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n static int push_check(int argc, const char **argv, const char *prefix)\n {\n \tstruct remote *remote;\n+\tconst char *superproject_head;\n+\tchar *head;\n+\tint detached_head = 0;\n+\tstruct object_id head_oid;\n \n-\tif (argc < 2)\n-\t\tdie(\"submodule--helper push-check requires at least 1 argument\");\n+\tif (argc < 3)\n+\t\tdie(\"submodule--helper push-check requires at least 2 arguments\");\n+\n+\t/*\n+\t * superproject's resolved head ref.\n+\t * if HEAD then the superproject is in a detached head state, otherwise\n+\t * it will be the resolved head ref.\n+\t */\n+\tsuperproject_head = argv[1];\n+\targv++;\n+\targc--;\n+\t/* Get the submodule's head ref and determine if it is detached */\n+\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n+\tif (!head)\n+\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\tif (!strcmp(head, \"HEAD\"))\n+\t\tdetached_head = 1;\n \n \t/*\n \t * The remote must be configured.\n@@ -1133,18 +1152,30 @@ static int push_check(int argc, const char **argv, const char *prefix)\n \t\t\tif (rs->pattern || rs->matching)\n \t\t\t\tcontinue;\n \n-\t\t\t/*\n-\t\t\t * LHS must match a single ref\n-\t\t\t * NEEDSWORK: add logic to special case 'HEAD' once\n-\t\t\t * working with submodules in a detached head state\n-\t\t\t * ceases to be the norm.\n-\t\t\t */\n-\t\t\tif (count_refspec_match(rs->src, local_refs, NULL) != 1)\n+\t\t\t/* LHS must match a single ref */\n+\t\t\tswitch (count_refspec_match(rs->src, local_refs, NULL)) {\n+\t\t\tcase 1:\n+\t\t\t\tbreak;\n+\t\t\tcase 0:\n+\t\t\t\t/*\n+\t\t\t\t * If LHS matches 'HEAD' then we need to ensure\n+\t\t\t\t * that it matches the same named branch\n+\t\t\t\t * checked out in the superproject.\n+\t\t\t\t */\n+\t\t\t\tif (!strcmp(rs->src, \"HEAD\")) {\n+\t\t\t\t\tif (!detached_head &&\n+\t\t\t\t\t    !strcmp(head, superproject_head))\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\tdie(\"HEAD does not match the named branch in the superproject\");\n+\t\t\t\t}\n+\t\t\tdefault:\n \t\t\t\tdie(\"src refspec '%s' must name a ref\",\n \t\t\t\t    rs->src);\n+\t\t\t}\n \t\t}\n \t\tfree_refspec(refspec_nr, refspec);\n \t}\n+\tfree(head);\n \n \treturn 0;\n }\ndiff --git a/submodule.c b/submodule.c\nindex 6531c5d60..36f45f5a5 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1015,7 +1015,8 @@ static int push_submodule(const char *path,\n  * Perform a check in the submodule to see if the remote and refspec work.\n  * Die if the submodule can't be pushed.\n  */\n-static void submodule_push_check(const char *path, const struct remote *remote,\n+static void submodule_push_check(const char *path, const char *head,\n+\t\t\t\t const struct remote *remote,\n \t\t\t\t const char **refspec, int refspec_nr)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -1023,6 +1024,7 @@ static void submodule_push_check(const char *path, const struct remote *remote,\n \n \targv_array_push(&cp.args, \"submodule--helper\");\n \targv_array_push(&cp.args, \"push-check\");\n+\targv_array_push(&cp.args, head);\n \targv_array_push(&cp.args, remote->name);\n \n \tfor (i = 0; i < refspec_nr; i++)\n@@ -1061,10 +1063,20 @@ int push_unpushed_submodules(struct oid_array *commits,\n \t * won't be propagated due to the remote being unconfigured (e.g. a URL\n \t * instead of a remote name).\n \t */\n-\tif (remote->origin != REMOTE_UNCONFIGURED)\n+\tif (remote->origin != REMOTE_UNCONFIGURED) {\n+\t\tchar *head;\n+\t\tstruct object_id head_oid;\n+\n+\t\thead = resolve_refdup(\"HEAD\", 0, head_oid.hash, NULL);\n+\t\tif (!head)\n+\t\t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n+\n \t\tfor (i = 0; i < needs_pushing.nr; i++)\n \t\t\tsubmodule_push_check(needs_pushing.items[i].string,\n-\t\t\t\t\t     remote, refspec, refspec_nr);\n+\t\t\t\t\t     head, remote,\n+\t\t\t\t\t     refspec, refspec_nr);\n+\t\tfree(head);\n+\t}\n \n \t/* Actually push the submodules */\n \tfor (i = 0; i < needs_pushing.nr; i++) {\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex beff65b8a..0f84a5314 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -533,7 +533,8 @@ test_expect_success 'push propagating refspec to a submodule' '\n \t# Fails when refspec includes an object id\n \ttest_must_fail git -C work push --recurse-submodules=on-demand origin \\\n \t\t\"$(git -C work rev-parse branch2):refs/heads/branch2\" &&\n-\t# Fails when refspec includes 'HEAD' as it is unsupported at this time\n+\t# Fails when refspec includes HEAD and parent and submodule do not\n+\t# have the same named branch checked out\n \ttest_must_fail git -C work push --recurse-submodules=on-demand origin \\\n \t\tHEAD:refs/heads/branch2 &&\n \n@@ -548,4 +549,26 @@ test_expect_success 'push propagating refspec to a submodule' '\n \ttest_cmp expected_pub actual_pub\n '\n \n+test_expect_success 'push propagating HEAD refspec to a submodule' '\n+\tgit -C work/gar/bage checkout branch2 &&\n+\t> work/gar/bage/junk12 &&\n+\tgit -C work/gar/bage add junk12 &&\n+\tgit -C work/gar/bage commit -m \"Twelfth junk\" &&\n+\n+\tgit -C work checkout branch2 &&\n+\tgit -C work add gar/bage &&\n+\tgit -C work commit -m \"updating gar/bage in branch2\" &&\n+\n+\t# Passes since the superproject and submodules HEAD are both on branch2\n+\tgit -C work push --recurse-submodules=on-demand origin \\\n+\t\tHEAD:refs/heads/branch2 &&\n+\n+\tgit -C submodule.git rev-parse branch2 >actual_submodule &&\n+\tgit -C pub.git rev-parse branch2 >actual_pub &&\n+\tgit -C work/gar/bage rev-parse branch2 >expected_submodule &&\n+\tgit -C work rev-parse branch2 >expected_pub &&\n+\ttest_cmp expected_submodule actual_submodule &&\n+\ttest_cmp expected_pub actual_pub\n+'\n+\n test_done\n-- \n2.14.0.rc0.284.gd933b75aa4-goog\n\n"}]}