Re: [PATCH] submodule--helper: teach push-check to handle HEAD
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 23, 2017, 22:51 UTC
- Message-ID
- <xmqqk242b9nf.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170623200427.26803-1-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
Show 37 quoted lines
> In 06bf4ad1d (push: propagate remote and refspec with
> --recurse-submodules) push was taught how to propagate a refspec down to
> submodules when the '--recurse-submodules' flag is given. The only refspecs
> that are allowed to be propagated are ones which name a ref which exists
> in both the superproject and the submodule, with the caveat that 'HEAD'
> was disallowed.
>
> This patch teaches push-check (the submodule helper which determines if
> a refspec can be propagated to a submodule) to permit propagating 'HEAD'
> if and only if the superproject and the submodule both have the same
> named branch checked out and the submodule is not in a detached head
> state.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> builtin/submodule--helper.c | 57 +++++++++++++++++++++++++++++++-----------
> submodule.c | 18 ++++++++++---
> t/t5531-deep-submodule-push.sh | 25 +++++++++++++++++-
> 3 files changed, 82 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1b4d2b346..fd5020036 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1107,24 +1107,41 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
> static int push_check(int argc, const char **argv, const char *prefix)
> {
> struct remote *remote;
> + const char *superproject_head;
> + char *head;
> + int detached_head = 0;
> + struct object_id head_oid;
>
> - if (argc < 2)
> - die("submodule--helper push-check requires at least 1 argument");
> + if (argc < 3)
> + die("submodule--helper push-check requires at least 2 argument");"arguments"?
Show 7 quoted lines
> + > + /* > + * superproject's resolved head ref. > + * if HEAD then the superproject is in a detached head state, otherwise > + * it will be the resolved head ref. > + */ > + superproject_head = argv[1];
The above makes it sound like the caller gives either "HEAD" (when detached) or "refs/heads/branch" (when on 'branch') in argv[1] and you are stashing it away, but ...
Show 6 quoted lines
> + /* Get the submodule's head ref and determine if it is detached */
> + head = resolve_refdup("HEAD", 0, head_oid.hash, NULL);
> + if (!head)
> + die(_("Failed to resolve HEAD as a valid ref."));
> + if (!strcmp(head, "HEAD"))
> + detached_head = 1;... the work to see which branch we are on and if we are detached is done by this code without consulting argv[1]. I cannot tell what is going on. Is argv[1] assigned to superproject_head a red herring?
Show 19 quoted lines
> /*
> * The remote must be configured.
> * This is to avoid pushing to the exact same URL as the parent.
> */
> - remote = pushremote_get(argv[1]);
> + remote = pushremote_get(argv[2]);
> if (!remote || remote->origin == REMOTE_UNCONFIGURED)
> - die("remote '%s' not configured", argv[1]);
> + die("remote '%s' not configured", argv[2]);
>
> /* Check the refspec */
> - if (argc > 2) {
> - int i, refspec_nr = argc - 2;
> + if (argc > 3) {
> + int i, refspec_nr = argc - 3;
> struct ref *local_refs = get_local_heads();
> struct refspec *refspec = parse_push_refspec(refspec_nr,
> - argv + 2);
> + argv + 3);If you have no need for argv[1] (and you don't, as you have stashed it away in superproject_head), it may be less damage to the code if you shifted argv upfront after grabbing superproject_head.
Show 15 quoted lines
> for (i = 0; i < refspec_nr; i++) {
> struct refspec *rs = refspec + i;
> @@ -1132,18 +1149,30 @@ static int push_check(int argc, const char **argv, const char *prefix)
> if (rs->pattern || rs->matching)
> continue;
>
> - /*
> - * LHS must match a single ref
> - * NEEDSWORK: add logic to special case 'HEAD' once
> - * working with submodules in a detached head state
> - * ceases to be the norm.
> - */
> - if (count_refspec_match(rs->src, local_refs, NULL) != 1)
> + /* LHS must match a single ref */
> + switch(count_refspec_match(rs->src, local_refs, NULL)) {"switch (count..."
Show 14 quoted lines
> + case 1:
> + break;
> + case 0:
> + /*
> + * If LHS matches 'HEAD' then we need to ensure
> + * that it matches the same named branch
> + * checked out in the superproject.
> + */
> + if (!strcmp(rs->src, "HEAD")) {
> + if (!detached_head &&
> + !strcmp(head, superproject_head))
> + break;
> + die("HEAD does not match the named branch in the superproject");
> + }Hmph, so earlier people can "push --recurse-submodules HEAD:$dest" and $dest can be anything, but now we are tightening the rule?
Show 11 quoted lines
> + default:
> die("src refspec '%s' must name a ref",
> rs->src);
> + }
> }
> free_refspec(refspec_nr, refspec);
> }
> + free(head);
>
> return 0;
> }