git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 7/7] clone, submodule update: create and check out branches

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Oct 25, 2022, 17:56 UTC
Message-ID
<20221025175628.913542-1-jonathantanmy@google.com>
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.
Show 7 quoted lines
> 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?

Show 11 quoted lines
>   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).
Show 6 quoted lines
>   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]
Show 5 quoted lines
> @@ -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.

Show 23 quoted lines
>  	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]
Show 17 quoted lines
> @@ -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]
Show 6 quoted lines
> +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?

Show 20 quoted lines
> +	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.

Previous: Glen Choo via GitGitGadgetNext: Glen Choo
Message 26 of 56 in “clone, submodule update: check out submodule branches”
  1. 0/6 clone, submodule update: check out submodule branchesGlen Choo via GitGitGadget, Aug 29, 2022
  2. 1/6 clone: teach --detach optionGlen Choo via GitGitGadget, Aug 29, 2022
  3. Philippe BlainAug 30, 2022
  4. 2/6 repo-settings: add submodule_propagate_branchesGlen Choo via GitGitGadget, Aug 29, 2022
  5. Philippe BlainAug 30, 2022
  6. 3/6 t5617: drop references to remote-tracking branchesGlen Choo via GitGitGadget, Aug 29, 2022
  7. Philippe BlainAug 30, 2022
  8. 4/6 submodule: return target of submodule symrefGlen Choo via GitGitGadget, Aug 29, 2022
  9. Jonathan TanSep 1, 2022
  10. Glen ChooSep 1, 2022
  11. 6/6 clone, submodule update: check out branchesGlen Choo via GitGitGadget, Aug 29, 2022
  12. Philippe BlainAug 30, 2022
  13. Glen ChooAug 30, 2022
  14. Jonathan TanSep 1, 2022
  15. 5/6 submodule--helper: refactor up-to-date criterionGlen Choo via GitGitGadget, Aug 29, 2022
  16. 0/7 clone, submodule update: check out submodule branchesGlen Choo via GitGitGadget, Oct 20, 2022
  17. 1/7 clone: teach --detach optionGlen Choo via GitGitGadget, Oct 20, 2022
  18. 2/7 repo-settings: add submodule_propagate_branchesGlen Choo via GitGitGadget, Oct 20, 2022
  19. Jonathan TanOct 25, 2022
  20. 3/7 submodule--helper clone: create named branchGlen Choo via GitGitGadget, Oct 20, 2022
  21. Jonathan TanOct 25, 2022
  22. 4/7 t5617: drop references to remote-tracking branchesGlen Choo via GitGitGadget, Oct 20, 2022
  23. 5/7 submodule: return target of submodule symrefGlen Choo via GitGitGadget, Oct 20, 2022
  24. 6/7 submodule update: refactor update targetsGlen Choo via GitGitGadget, Oct 20, 2022
  25. 7/7 clone, submodule update: create and check out branchesGlen Choo via GitGitGadget, Oct 20, 2022
  26. Jonathan TanOct 25, 2022
  27. Glen ChooOct 25, 2022
  28. Junio C HamanoOct 20, 2022
  29. Glen ChooOct 20, 2022
  30. Junio C HamanoOct 21, 2022
  31. 0/8 clone, submodule update: check out submodule branchesGlen Choo via GitGitGadget, Oct 28, 2022
  32. 1/8 clone: teach --detach optionGlen Choo via GitGitGadget, Oct 28, 2022
  33. Junio C HamanoOct 28, 2022
  34. Junio C HamanoOct 28, 2022
  35. Glen ChooOct 28, 2022
  36. Taylor BlauOct 30, 2022
  37. Glen ChooOct 31, 2022
  38. Philippe BlainNov 8, 2022
  39. 2/8 repo-settings: add submodule_propagate_branchesGlen Choo via GitGitGadget, Oct 28, 2022
  40. 3/8 submodule--helper clone: create named branchGlen Choo via GitGitGadget, Oct 28, 2022
  41. 5/8 submodule: return target of submodule symrefGlen Choo via GitGitGadget, Oct 28, 2022
  42. Junio C HamanoOct 28, 2022
  43. Glen ChooOct 28, 2022
  44. 4/8 t5617: drop references to remote-tracking branchesGlen Choo via GitGitGadget, Oct 28, 2022
  45. 8/8 clone, submodule update: create and check out branchesGlen Choo via GitGitGadget, Oct 28, 2022
  46. Philippe BlainNov 8, 2022
  47. Jonathan TanNov 15, 2022
  48. Glen ChooNov 22, 2022
  49. Jonathan TanNov 23, 2022
  50. Junio C HamanoNov 23, 2022
  51. 6/8 submodule update: refactor update targetsGlen Choo via GitGitGadget, Oct 28, 2022
  52. 7/8 submodule--helper: remove update_data.suboidGlen Choo via GitGitGadget, Oct 28, 2022
  53. Jonathan TanNov 14, 2022
  54. Taylor BlauOct 30, 2022
  55. Philippe BlainNov 8, 2022
  56. Glen ChooNov 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.