git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>
Cc: Jonathan Tan <jonathantanmy@google.com>,
	git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH 6/6] clone, submodule update: check out branches
Date: Thu,  1 Sep 2022 13:00:47 -0700	[thread overview]
Message-ID: <20220901200047.515294-1-jonathantanmy@google.com> (raw)
In-Reply-To: <6f7f2f9a3f19b6d874d644b7fb7feb3a72fc6227.1661806456.git.gitgitgadget@gmail.com>

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Glen Choo <chooglen@google.com>
> 
> Teach "git submodule update" to update submodules by creating and
> checking out the current superproject branch when
> "submodule.propagateBranches=true".

"git submodule update" already knows how to update submodules; probably
better to say:

  Teach "git submodule update" to create and check out a branch of the
  same name as the current superproject branch when updating a submodule
  if "submodule.propagateBranches=true" is set on the superproject.

> With "submodule.propagateBranches=true", submodules are cloned with
> "--detach" so that they do not contain branches from their upstream.
> This prevents conflicts between branch names from the superproject and
> the branch names from the submodule's upstream. Arguably, "--detach"
> should also be the default for "submodule.propagateBranches=false"
> since it doesn't make sense to create a submodule branch when the
> submodule is always expected to be in detached HEAD.

This paragraph made me think of the use case in which we cloned a
submodule-using repo, made a commit in a submodule (thus advancing a
branch) without a corresponding commit in a superproject, and then
recloned our clone, hoping that the state will persist. It would not
persist, but as stated here, the existing behavior is already that
branches in submodules are not cloned, so retaining this existing
behavior is not a problem.

> "git submodule update" tries to create the branch as long as it is not
> currently checked out, thus it will fail if the submodule has the
> branch, but it is not checked out. This is fine because the main purpose
> of "git submodule update" is to clone new submodules (which have no
> branches, and will never have this problem). "git checkout" with
> "submodule.propagateBranches" will cover the use case of recursively
> checking out an existing branch.

In regular usage, the user will, as you say, run "git checkout". So when
"git submodule update" is run, a submodule will either have no branches
(because it was just cloned or because we have never switched to that
branch before in the superproject) or it will have the correct branch
already checked out, so it would already be considered up to date (no
matter whether the commit matches with the superproject's gitlink: only
the name of the branch matters).

I'm concerned about the case in which the user, say, has created a
branch in a submodule for some reason. E.g.:

  (cd sub; git branch my-branch)
  git checkout my-branch

so this would fail because we wouldn't be able to create "my-branch" in
the "sub" submodule. We might need a message explaining what can be done
to fix this situation, but for now, maybe a NEEDSWORK will suffice.

> @@ -2206,6 +2214,8 @@ static int run_update_command(struct update_data *ud, int subforce)
>  		strvec_pushl(&cp.args, "checkout", "-q", NULL);
>  		if (subforce)
>  			strvec_push(&cp.args, "-f");
> +		if (ud->super_branch)
> +			strvec_pushl(&cp.args, "-b", ud->super_branch, NULL);

Here is where the NEEDSWORK would go.

> @@ -106,4 +113,31 @@ test_expect_success '--no-also-filter-submodules overrides clone.filterSubmodule
>  	test_cmp_config -C super_clone3/sub false --default false remote.origin.promisor
>  '
>  
> +test_expect_success 'submodule.propagateBranches checks out branches at correct commits' '
> +	git -C sub checkout -b not-main &&
> +	git -C subsub checkout -b not-main &&
> +	git clone --recurse-submodules \
> +		-c submodule.propagateBranches=true \
> +		"file://$pwd/." super_clone4 &&
> +
> +	# Assert that each repo is pointing to "main"
> +	for REPO in "super_clone4" "super_clone4/sub" "super_clone4/sub/subsub"
> +	do
> +	    HEAD_BRANCH=$(git -C $REPO symbolic-ref HEAD) &&
> +	    test $HEAD_BRANCH = "refs/heads/main" || return 1
> +	done &&
> +
> +	# Assert that the submodule branches are pointing to the right revs
> +	EXPECT_SUB_OID="$(git -C super_clone4 rev-parse :sub)" &&
> +	ACTUAL_SUB_OID="$(git -C super_clone4/sub rev-parse refs/heads/main)" &&
> +	test $EXPECT_SUB_OID = $ACTUAL_SUB_OID &&
> +	EXPECT_SUBSUB_OID="$(git -C super_clone4/sub rev-parse :subsub)" &&
> +	ACTUAL_SUBSUB_OID="$(git -C super_clone4/sub/subsub rev-parse refs/heads/main)" &&
> +	test $EXPECT_SUBSUB_OID = $ACTUAL_SUBSUB_OID &&
> +
> +	# Assert that the submodules do not have branches from their upstream
> +	test_must_fail git -C super_clone4/sub rev-parse not-main &&
> +	test_must_fail git -C super_clone4/sub/subsub rev-parse not-main
> +'

Instead of reusing "main", can we use a branch name that exists in the
superproject but not the submodule? Here, we cannot tell the difference
between git reusing the referent of submodule's "main" versus git using
the gitlink in superproject's "main".

I'll write some more comments on the other patches, but overall this
patch set makes sense to me.

  parent reply	other threads:[~2022-09-01 20:01 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-29 20:54 [PATCH 0/6] clone, submodule update: check out submodule branches Glen Choo via GitGitGadget
2022-08-29 20:54 ` [PATCH 1/6] clone: teach --detach option Glen Choo via GitGitGadget
2022-08-30  4:02   ` Philippe Blain
2022-08-29 20:54 ` [PATCH 2/6] repo-settings: add submodule_propagate_branches Glen Choo via GitGitGadget
2022-08-30  4:02   ` Philippe Blain
2022-08-29 20:54 ` [PATCH 3/6] t5617: drop references to remote-tracking branches Glen Choo via GitGitGadget
2022-08-30  4:03   ` Philippe Blain
2022-08-29 20:54 ` [PATCH 4/6] submodule: return target of submodule symref Glen Choo via GitGitGadget
2022-09-01 20:01   ` Jonathan Tan
2022-09-01 20:46     ` Glen Choo
2022-08-29 20:54 ` [PATCH 5/6] submodule--helper: refactor up-to-date criterion Glen Choo via GitGitGadget
2022-08-29 20:54 ` [PATCH 6/6] clone, submodule update: check out branches Glen Choo via GitGitGadget
2022-08-30  4:03   ` Philippe Blain
2022-08-30 22:54     ` Glen Choo
2022-09-01 20:00   ` Jonathan Tan [this message]
2022-10-20 20:20 ` [PATCH v2 0/7] clone, submodule update: check out submodule branches Glen Choo via GitGitGadget
2022-10-20 20:20   ` [PATCH v2 1/7] clone: teach --detach option Glen Choo via GitGitGadget
2022-10-20 20:20   ` [PATCH v2 2/7] repo-settings: add submodule_propagate_branches Glen Choo via GitGitGadget
2022-10-25 18:03     ` Jonathan Tan
2022-10-20 20:20   ` [PATCH v2 3/7] submodule--helper clone: create named branch Glen Choo via GitGitGadget
2022-10-25 18:00     ` Jonathan Tan
2022-10-20 20:20   ` [PATCH v2 4/7] t5617: drop references to remote-tracking branches Glen Choo via GitGitGadget
2022-10-20 20:20   ` [PATCH v2 5/7] submodule: return target of submodule symref Glen Choo via GitGitGadget
2022-10-20 20:20   ` [PATCH v2 6/7] submodule update: refactor update targets Glen Choo via GitGitGadget
2022-10-20 20:20   ` [PATCH v2 7/7] clone, submodule update: create and check out branches Glen Choo via GitGitGadget
2022-10-25 17:56     ` Jonathan Tan
2022-10-25 21:49       ` Glen Choo
2022-10-20 22:40   ` [PATCH v2 0/7] clone, submodule update: check out submodule branches Junio C Hamano
2022-10-20 23:53     ` Glen Choo
2022-10-21  0:01       ` Junio C Hamano
2022-10-28 20:14   ` [PATCH v3 0/8] " Glen Choo via GitGitGadget
2022-10-28 20:14     ` [PATCH v3 1/8] clone: teach --detach option Glen Choo via GitGitGadget
2022-10-28 21:40       ` Junio C Hamano
2022-10-28 21:54         ` Junio C Hamano
2022-10-28 22:55           ` Glen Choo
2022-10-30 18:14             ` Taylor Blau
2022-10-31 17:07               ` Glen Choo
2022-11-08 13:32       ` Philippe Blain
2022-10-28 20:14     ` [PATCH v3 2/8] repo-settings: add submodule_propagate_branches Glen Choo via GitGitGadget
2022-10-28 20:14     ` [PATCH v3 3/8] submodule--helper clone: create named branch Glen Choo via GitGitGadget
2022-10-28 20:14     ` [PATCH v3 4/8] t5617: drop references to remote-tracking branches Glen Choo via GitGitGadget
2022-10-28 20:14     ` [PATCH v3 5/8] submodule: return target of submodule symref Glen Choo via GitGitGadget
2022-10-28 21:49       ` Junio C Hamano
2022-10-28 23:11         ` Glen Choo
2022-10-28 20:14     ` [PATCH v3 6/8] submodule update: refactor update targets Glen Choo via GitGitGadget
2022-10-28 20:14     ` [PATCH v3 7/8] submodule--helper: remove update_data.suboid Glen Choo via GitGitGadget
2022-11-14 23:45       ` Jonathan Tan
2022-10-28 20:14     ` [PATCH v3 8/8] clone, submodule update: create and check out branches Glen Choo via GitGitGadget
2022-11-08 13:53       ` Philippe Blain
2022-11-15 18:15       ` Jonathan Tan
2022-11-22 18:44         ` Glen Choo
2022-11-23  1:33           ` Jonathan Tan
2022-11-23  4:00             ` Junio C Hamano
2022-10-30 18:19     ` [PATCH v3 0/8] clone, submodule update: check out submodule branches Taylor Blau
2022-11-08 14:23     ` Philippe Blain
2022-11-08 20:43       ` Glen Choo

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=20220901200047.515294-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=levraiphilippeblain@gmail.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).