git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Brandon Williams <bmwill@google.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com, sbeller@google.com
Subject: Re: [PATCH] submodule--helper: teach push-check to handle HEAD
Date: Fri, 23 Jun 2017 15:51:16 -0700	[thread overview]
Message-ID: <xmqqk242b9nf.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170623200427.26803-1-bmwill@google.com> (Brandon Williams's message of "Fri, 23 Jun 2017 13:04:27 -0700")

Brandon Williams <bmwill@google.com> writes:

> 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"?

> +
> +	/*
> +	 * 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 ...

> +	/* 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?

>  	/*
>  	 * 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.

>  		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..."

> +			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?

> +			default:
>  				die("src refspec '%s' must name a ref",
>  				    rs->src);
> +			}
>  		}
>  		free_refspec(refspec_nr, refspec);
>  	}
> +	free(head);
>  
>  	return 0;
>  }

  parent reply	other threads:[~2017-06-23 22:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-23 20:04 [PATCH] submodule--helper: teach push-check to handle HEAD Brandon Williams
2017-06-23 20:26 ` Stefan Beller
2017-06-23 22:51 ` Junio C Hamano [this message]
2017-06-26 18:12   ` Brandon Williams
2017-07-20 17:40 ` [PATCH v2 1/1] " Brandon Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk242b9nf.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).