git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>, Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v3 8/8] clone, submodule update: create and check out branches
Date: Tue, 8 Nov 2022 08:53:17 -0500	[thread overview]
Message-ID: <e6949ea0-8053-3487-81bb-6fe50fd34814@gmail.com> (raw)
In-Reply-To: <7cdd6c4184da2d3109498589167f10ecf972edc9.1666988096.git.gitgitgadget@gmail.com>

Hi Glen,

Le 2022-10-28 à 16:14, Glen Choo via GitGitGadget a écrit :
> From: Glen Choo <chooglen@google.com>
> 
> Teach "git submodule update" to:
> 
> - create the branch with the same name as the current superproject
>   branch when cloning a submodule
> - check out that branch (instead of the commit OID) when updating
>   the submodule worktree
> 
> when submodule branching is enabled (submodule.propagateBranches = true)
> on the superproject and a branch is checked out. "git clone
> --recurse-submodules" also learns this trick because it is implemented
> with "git submodule update --recursive".
> 
> 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. In other cases, it
> does not work as well, but we can handle them incrementally:
> 
> - "git pull --recurse-submodules" merges the superproject tree,
>   (changing the gitlink 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.

OK, here you mean that 'git -c submodule.propagateBranches=true submodule update'
would just checkout the submodule branch (or do nothing if it's already checked out),
but that branch would most likely not be up to date with the newly updated gitlink
in the superproject, resulting in a dirty worktree.

> 
>   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).
> 
>   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>
> ---
>  builtin/submodule--helper.c |  37 ++++++++-
>  t/t5617-clone-submodules.sh |  38 +++++++++
>  t/t7406-submodule-update.sh | 156 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 227 insertions(+), 4 deletions(-)
> 

--8<--

> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f094e3d7f36..b749d35f784 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh

--8<--

> +# Test the behavior of an already-cloned submodule.
> +# NEEDSWORK When updating with branches, we always use the branch instead of the
> +# gitlink's OID. This results in some imperfect behavior:
> +#
> +# - If the gitlink's OID disagrees with the branch OID, updating with branches
> +#   may result in a dirty worktree
> +# - If the branch does not exist, the update fails.
> +#
> +# We will reevaluate when "git checkout --recurse-submodules" supports branches
> +# For now, just test for this imperfect behavior.

OK. Maybe it would be a good idea to explicitely flag which tests below have
an imperfect behaviour ? This way it's easier to spot where the NEEDSWORK applies...

--8<--

This following test show the imperfect behaviour:

> +test_expect_success 'branches - correct branch checked out, OIDs disagree' '
> +	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 new-branch &&
> +	git -C branch-super-cloned/sub1 checkout new-branch &&
> +	test_commit -C branch-super-cloned/sub1 new-commit &&
> +	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" &&
> +	test_clean_submodule ! branch-super-cloned sub1
> +'

--8<--

This one also:

> +
> +test_expect_success 'branches - other branch checked out, correct branch exists, OIDs disagree' '
> +	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 new-branch &&
> +	git -C branch-super-cloned/sub1 checkout new-branch &&
> +	test_commit -C branch-super-cloned/sub1 new-commit &&
> +	git -C branch-super-cloned/sub1 checkout 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" &&
> +	test_clean_submodule ! branch-super-cloned sub1
> +'
> +

This one I'm not sure about: with this series 'git submodule update' does not create
submodule branches at other times than at clone time, so I think this behviour would not change.
I think this one is really an edge case where the user mixes submodule branching on/off.

> +test_expect_success 'branches - other branch checked out, correct branch does not exist' '
> +	test_when_finished "rm -fr branch-super-cloned" &&
> +	cp -r branch-super-clean branch-super-cloned &&
> +
> +	git -C branch-super-cloned branch new-branch &&
> +	git -C branch-super-cloned checkout new-branch &&
> +	test_must_fail git -C branch-super-cloned submodule update
> +'
> +
>  test_done

  reply	other threads:[~2022-11-08 13:53 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
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 [this message]
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=e6949ea0-8053-3487-81bb-6fe50fd34814@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@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).