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 v2 7/7] clone, submodule update: create and check out branches
Date: Tue, 25 Oct 2022 10:56:27 -0700 [thread overview]
Message-ID: <20221025175628.913542-1-jonathantanmy@google.com> (raw)
In-Reply-To: <3f98b0d17397dbce85aa87a9591981c790f8f7a2.1666297239.git.gitgitgadget@gmail.com>
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Teach "git submodule update" to:
>
> - create the branch with the same name as the current superproject
> branch when cloning a submodule
Ah, this is when "git submodule update" needs to clone a submodule (as
opposed to doing something with a submodule that is already cloned).
> - check out that branch (instead of the commit OID) when updating
> the submodule worktree
So whenever we run "git submodule update" and a submodule already has a
branch of the same name as what's currently checked out in the
superproject, we check out the submodule's branch, ignoring the gitlink
in the superproject?
> when submodule branching is enabled (submodule.propagateBranches = true)
> on the superproject and a branch is checked out.
OK.
> "git clone
> --recurse-submodules" also learns this trick because it is implemented
> with "git submodule update --recursive".
Is this sentence redundant now that you're specifically calling out the
cloning part above?
> This approach of checking out the branch will not result in a dirty
> worktree for freshly cloned submodules because we can ensure that the
> submodule branch points to the superproject gitlink.
Makes sense.
> In other cases, it
> does not work as well, but we can handle them incrementally:
>
> - "git pull --recurse-submodules" merges the superproject tree (without
> updating the submodule branches), and runs "git submodule update" to
> update the worktrees, so it is almost guaranteed to result in a dirty
> worktree.
Is this because when we "git pull", the superproject likely now has a
different gitlink, but the branch in the submodule hasn't changed?
> The implementation of "git pull --recurse-submodules" is likely to
> change drastically as submodule.propagateBranches work progresses
> (e.g. "git merge" learns to recurse in to submodules), and we may be
> able to replace the "git submodule update" invocation, or teach it new
> tricks that make the update behave well.
>
> - The user might make changes to the submodule branch without committing
> them back to superproject. This is primarily affects "git checkout
> --recurse-submodules", since that is the primary way of switching away
> from a branch and leaving behind WIP (as opposed to "git submodule
> update", which is run post-checkout).
Makes sense - so in summary, there are (at least) two ways of the
superproject's gitlink and the submodule's branch can go out of sync:
the user changing the submodule branch (here) or the user changing the
superproject's gitlink (above).
> In a future series, "git checkout --recurse-submodules" will learn to
> consider submodule branches. We can introduce appropriate guardrails
> then, e.g. requiring that the superproject working tree is not dirty
> before switching away.
>
> Signed-off-by: Glen Choo <chooglen@google.com>
[snip]
> @@ -2521,6 +2532,7 @@ static int update_submodule(struct update_data *update_data)
> {
> int submodule_up_to_date;
> int ret;
> + const char *submodule_head = "HEAD";
I think it's clearer if this is initialized to NULL. I don't think the
submodule head is detached in all code paths when this is HEAD.
> ret = determine_submodule_update_strategy(the_repository,
> update_data->just_cloned,
> @@ -2533,7 +2545,7 @@ static int update_submodule(struct update_data *update_data)
> if (update_data->just_cloned)
> oidcpy(&update_data->suboid, null_oid());
> else if (resolve_gitlink_ref(update_data->sm_path, "HEAD",
> - &update_data->suboid, NULL))
> + &update_data->suboid, &submodule_head))
> return die_message(_("Unable to find current revision in submodule path '%s'"),
> update_data->displaypath);
>
> @@ -2568,7 +2580,14 @@ static int update_submodule(struct update_data *update_data)
> free(remote_ref);
> }
>
> - submodule_up_to_date = oideq(&update_data->oid, &update_data->suboid);
> + if (!update_data->super_branch)
> + submodule_up_to_date = oideq(&update_data->oid, &update_data->suboid);
> + else if (skip_prefix(submodule_head, "refs/heads/", &submodule_head))
> + submodule_up_to_date = !strcmp(update_data->super_branch, submodule_head);
> + /* submodule_branch is "HEAD"; the submodule is in detached HEAD */
> + else
> + submodule_up_to_date = 0;
I think there needs to be a better comment here. It doesn't matter that
the submodule is in detached HEAD; what matters is that the submodule
doesn't have the superproject's branch checked out. So maybe something
like:
if (update_data->super_branch) {
/* (format this appropriately) We also need to check that the
submodule's HEAD points to super_branch. */
const char *submodule_head;
submodule_up_to_date = skip_prefix(...) && !strcmp(...)
} else {
submodule_up_to_date = oideq(...)
}
> diff --git a/t/t5617-clone-submodules.sh b/t/t5617-clone-submodules.sh
[snip]
> @@ -107,4 +114,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 &&
As I said in my earlier review [1], could we use a branch name that is
not "main"? That way, we also check that the clone *creates* the
branches, not just reuses something already there.
[1] https://lore.kernel.org/git/20220901200047.515294-1-jonathantanmy@google.com/
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
[snip]
> +test_expect_success 'submodule.propagateBranches - detached HEAD' '
> + test_when_finished "rm -fr branch-super-cloned" &&
> + cp -r branch-super-clean branch-super-cloned &&
> +
> + git -C branch-super-cloned checkout --detach &&
> + git -C branch-super-cloned pull origin main &&
If the behavior of "pull" is going to change soon (as stated in the
commit message), can we avoid using it in tests here?
> + git -C branch-super-cloned submodule update &&
> +
> + # sub2 should be in detached HEAD
> + git -C branch-super-cloned/sub2 rev-parse --verify HEAD &&
> + test_must_fail git -C branch-super-cloned/sub2 symbolic-ref HEAD
> +'
> +
> +test_expect_success 'submodule.propagateBranches - branch checked out' '
> + test_when_finished "rm -fr branch-super-cloned" &&
> + cp -r branch-super-clean branch-super-cloned &&
> +
> + git -C branch-super-cloned branch --recurse-submodules new-branch &&
> + git -C branch-super-cloned checkout --recurse-submodules new-branch &&
> + git -C branch-super-cloned pull origin main &&
> + git -C branch-super-cloned submodule update &&
> +
> + HEAD_BRANCH1=$(git -C branch-super-cloned/sub1 symbolic-ref HEAD) &&
> + test $HEAD_BRANCH1 = "refs/heads/new-branch" &&
> + HEAD_BRANCH2=$(git -C branch-super-cloned/sub2 symbolic-ref HEAD) &&
> + test $HEAD_BRANCH2 = "refs/heads/new-branch"
I'm not sure of the behavior of "pull" here, so I didn't look too
closely into what these tests test, but I think that you should cover
these situations:
- submodule has correct OID and correct branch checked out
- submodule has correct OID, but wrong branch checked out, and correct
branch doesn't exist
- submodule has correct OID, but wrong branch checked out, and correct
branch exists at correct OID
- submodule has correct OID, but wrong branch checked out, and correct
branch exists at wrong OID
- submodule has incorrect OID but correct branch checked out (if this
currently doesn't work, maybe add a test_must_fail with a NEEDSWORK)
There are other combinations but I think these are the most important
ones.
Also, if there is a situation where you ignore the superproject's
gitlink in favor of the submodule's branch, you should check the OIDs
in all cases too, not just what the HEAD symbolic ref points to.
next prev parent reply other threads:[~2022-10-25 17:56 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
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 [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221025175628.913542-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.
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 NNTP newsgroup(s).