Hi Glen,

Le 2022-08-29 à 16:54, Glen Choo via GitGitGadget a écrit :
> From: Glen Choo <chooglen@google.com>
> 
> Teach "git clone" the "--detach" option, which leaves the cloned repo in
> detached HEAD (like "git checkout --detach"). If the clone is not bare,
> the remote's HEAD branch is also not created (bare clones always copy
> all remote branches directly to local branches, so the branch is still
> created in the bare case).

At first reading I thought you meant the 'origin/HEAD' symref, which is 
not the case here. I think something like this would maybe be clearer:

If the clone is not bare, skip the creation of a local branch corresponding to 
the branch pointed to by the remote's HEAD symref (bare clones...
to local branches, so that branch ...

(OK this is very verbose but in my opinion it's clearer.)

> This is especially useful in the "submodule.propagateBranches" workflow,
> where the submodule branch names match the superproject's branch names,
> so it makes no sense to name the branches after the submodule's remote's
> branches.

We are just skipping the creation of a single branch here, so it's unclear 
to me which other branches are being talked about in this last paragraph.
All remote-tracking branches are unaffected by this flag, no?

> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  Documentation/git-clone.txt |  7 ++++++-
>  builtin/clone.c             | 12 +++++++++---
>  t/t5601-clone.sh            | 22 ++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 632bd1348ea..a3af90824b6 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -16,7 +16,7 @@ SYNOPSIS
>  	  [--depth <depth>] [--[no-]single-branch] [--no-tags]
>  	  [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
>  	  [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
> -	  [--filter=<filter> [--also-filter-submodules]] [--] <repository>
> +	  [--filter=<filter> [--also-filter-submodules] [--detach]] [--] <repository>
>  	  [<directory>]
>  
>  DESCRIPTION
> @@ -210,6 +210,11 @@ objects from the source repository into a pack in the cloned repository.
>  	`--branch` can also take tags and detaches the HEAD at that commit
>  	in the resulting repository.
>  
> +--detach::
> +	If the cloned repository's HEAD points to a branch, point the newly
> +	created HEAD to the branch's commit instead of the branch itself. In a
> +	non-bare repository, the branch will not be created.

Again, I think the wording could be improved, maybe something along those lines:

If the cloned repository's HEAD points to a branch, detach the newly created HEAD
at the commit at the tip of that branch. Additionnally, in a non-bare repository,
skip creating a corresponding local branch.

> +
>  -u <upload-pack>::
>  --upload-pack <upload-pack>::
>  	When given, and the repository to clone from is accessed
> diff --git a/builtin/clone.c b/builtin/clone.c
> index c4ff4643ecd..1bc1807360e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -77,6 +77,7 @@ static int option_filter_submodules = -1;    /* unspecified */
>  static int config_filter_submodules = -1;    /* unspecified */
>  static struct string_list server_options = STRING_LIST_INIT_NODUP;
>  static int option_remote_submodules;
> +static int option_detach;
>  
>  static int recurse_submodules_cb(const struct option *opt,
>  				 const char *arg, int unset)
> @@ -160,6 +161,8 @@ static struct option builtin_clone_options[] = {
>  		    N_("any cloned submodules will use their remote-tracking branch")),
>  	OPT_BOOL(0, "sparse", &option_sparse_checkout,
>  		    N_("initialize sparse-checkout file to include only files at root")),
> +	OPT_BOOL(0, "detach", &option_detach,
> +		 N_("detach HEAD and don't create branch")),

maybe "don't create any local branch" ?

>  	OPT_END()
>  };
>  
> @@ -607,10 +610,12 @@ static void update_remote_refs(const struct ref *refs,
>  }
>  
>  static void update_head(const struct ref *our, const struct ref *remote,
> -			const char *unborn, const char *msg)
> +			const char *unborn, int should_detach,
> +			const char *msg)
>  {
>  	const char *head;
> -	if (our && skip_prefix(our->name, "refs/heads/", &head)) {
> +	if (our && !should_detach &&
> +	    skip_prefix(our->name, "refs/heads/", &head)) {
>  		/* Local default branch link */
>  		if (create_symref("HEAD", our->name, NULL) < 0)
>  			die(_("unable to update HEAD"));

OK, so the addition of that condition means that if --detach was given, we now
go into the 'else if (our)' branch, as long as 'our' is non-null, which means
that the remote's HEAD points to a branch or we gave --branch. This makes sense.
If the remote's HEAD does not point to a branch and we did not give --branch,
then we go into 'else if (remote)', as before. 

> @@ -1339,7 +1344,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  			   branch_top.buf, reflog_msg.buf, transport,
>  			   !is_local);
>  
> -	update_head(our_head_points_at, remote_head, unborn_head, reflog_msg.buf);
> +	update_head(our_head_points_at, remote_head, unborn_head,
> +		    option_detach, reflog_msg.buf);
>  
>  	/*
>  	 * We want to show progress for recursive submodule clones iff
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index cf3be0584f4..1e7e5143a76 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -333,6 +333,28 @@ test_expect_success 'clone checking out a tag' '
>  	test_cmp fetch.expected fetch.actual
>  '
>  
> +test_expect_success '--detach detaches and does not create branch' '
> +	test_when_finished "rm -fr dst" &&
> +	git clone --detach src dst &&
> +	(
> +		cd dst &&
> +		test_must_fail git rev-parse main &&
> +		test_must_fail git symbolic-ref HEAD &&
> +		test_cmp_rev HEAD refs/remotes/origin/HEAD
> +	)
> +'
> +
> +test_expect_success '--detach with --bare detaches but creates branch' '
> +	test_when_finished "rm -fr dst" &&
> +	git clone --bare --detach src dst &&
> +	(
> +		cd dst &&
> +		git rev-parse main &&
> +		test_must_fail git symbolic-ref HEAD &&
> +		test_cmp_rev HEAD refs/heads/main
> +	)
> +'
> +

Tests look good.

Le 2022-08-29 à 16:54, Glen Choo via GitGitGadget a écrit :
> From: Glen Choo <chooglen@google.com>
> 
> When processes recurse into submodules, the child processes have to
> use the same value of "submodule.propagateBranches" as the parent
> process regardless of whether the process is spawned in the superproject
> or submodule, otherwise the behavior may be inconsistent if the
> repositories don't agree on the config.
> 
> We haven't needed a way to propagate the config because because the only

"because because"

> command that reads "submodule.propagateBranches" is "git branch", which
> only has one mode of operation with "--recurse-submodules". However, a
> future commit will teach "submodule.propagateBranches" to "git submodule
> update", making this necessary.
> 
> Propagate "submodule.propagateBranches" to child processes by adding a
> corresponding GIT_INTERNAL_* environment variable and repository
> setting, and setting the environment variable inside
> prepare_submodule_repo_env(). Then, refactor builtin/branch.c to read
> the repository setting.
> 
> Using an internal environment variable is a potentially leaky
> abstraction because environment variables can come from sources besides
> the parent process. A more robust solution would be to teach Git that
> the repository is a submodule and to only read
> "submodule.propagateBranches" from the superproject config. There is WIP
> for this on the ML [1].
> 
> Another alternative would be to pass "-c submodule.propagateBranches" to
> all child processes. This is error-prone because many different
> processes are invoked directly or indirectly by "git submodule update"
> (e.g. "git submodule--helper clone", "git clone", "git checkout"). With
> an environment variable, we can avoid this work because
> prepare_submodule_repo_env() is already called for submodule child
> processes.

I think this is a good justification. I agree adding '-c' everywhere would be
error-prone.

Hi Glen,

Le 2022-08-29 à 16:54, Glen Choo via GitGitGadget a écrit :
> From: Glen Choo <chooglen@google.com>
> 
> It has included submodule cloning tests without remote-tracking branches
> tests since f05da2b48b (clone, submodule: pass partial clone filters to
> submodules, 2022-02-04) at least. Rename it accordingly so that we can
> put future submodule cloning tests there.

I think it was named "*-remote" because it was introduced when 
'git clone --remote-submodules' was added in 4c6910163a 
(clone: add `--remote-submodules` flag, 2019-05-19).

In any case, nice clean-up!

Hi Glen,

Le 2022-08-29 à 16:54, Glen Choo via GitGitGadget a écrit :
> 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 clone --recurse-submodules"
> also learns this trick because it is implemented with "git submodule
> update --recursive".

OK. Maybe a more descriptive title would then be:

clone, submodule update: create and check out submodule branches

?

Another thing, 'git pull --recurse-submodules' is also implemented using
'git submodule update --recursive'. But I don't think we want 'git pull'
to start creating new branches in submodules, even with submodule.propagateBranches=true
(though I haven't thought about it very hard). So maybe adding a word about
that would be nice.

> 
> With "submodule.propagateBranches=true", submodules are cloned with
> "--detach" so that they do not contain branches from their upstream.

We usually use the present tense to talk about the current state of the code base,
and then the imperative to order to codebase to improve itself;
here you already used the imperative "teach" in the previous paragraph,
so I'm assuming you are now talking about the new state of the code.
Maybe just adding "now" i.e. "submodules are now cloned" would help
readers ?

> 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. But, to be
> conservative, this commit does not change the behavior of
> "submodule.propagateBranches=false".

I agree that it would be "cleaner" to make the change also for
"submodule.propagateBranches=false" eventually, but... let's not
change things just to change things :)

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

I guess you mean "in a future series" for the last sentence ? FWIW I still have
your RFC from last Febryary about that [1] in my "unread Git mailing list" folder,
I always seem to lack the time to sit down and read it through, sorry!
Incidentally, I notice you did not link to it in the cover letter, 
any reasoon why?

[1] https://lore.kernel.org/git/20220209065236.36494-1-chooglen@google.com/

Stepping back a bit, you write "thus it will fail if the submodule has the
branch, but it is not checked out." If I read your patch correctly, this is
implicit in that 'git checkout -b super-branch' that is ran by 'run_update_command'
will error out if the branch already exists, right ? 

Is there anything more we should do in that case ? 
Should we remind the user, something like
"you have submodule.propagateBranches set, but the branch 'super-branch' already
exists in submodule 'that-sub'" ? 

I'm trying to think of a scenario in which this could happen...

Say a user:
1. clones a superproject with --recurse-submodules, but without 'submodule.propagateBranches'
2. runs 'git checkout -b topic' in the superproject
3. runs 'git branch topic' in the submodule
4. runs 'git submodule update' with 'submodule.propagateBranches' in the superproject

This fails:

fatal: a branch named 'topic' already exists
fatal: Unable to checkout 'deadbeef' in submodule path 'sub'

Do we need a more specific message ? I'm not sure.

> 
> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++--
>  t/t5617-clone-submodules.sh | 34 ++++++++++++++++++++++++++++++++++
>  t/t7406-submodule-update.sh | 22 ++++++++++++++++++++++
>  3 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index cbf6bda4850..7eb2c45900e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1695,6 +1695,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  			strvec_push(&cp.args, clone_data->single_branch ?
>  				    "--single-branch" :
>  				    "--no-single-branch");
> +		if (the_repository->settings.submodule_propagate_branches)
> +			strvec_push(&cp.args, "--detach");
> +
>  
>  		strvec_push(&cp.args, "--");
>  		strvec_push(&cp.args, clone_data->url);
> @@ -1733,6 +1736,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>  	if (error_strategy)
>  		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
>  				       error_strategy);
> +	if (the_repository->settings.submodule_propagate_branches)
> +		git_config_set_in_file(p, "submodule.propagateBranches",
> +				       "true");

Why do we need to set that in the config of the submodule ? I'm guessing this 
is so that the new code also works for nested submodules, right ? 

I'm thinking about a user that would alternate between 'submodule.propagateBranches=true' and 'false'.
Maybe they sometimes have to work on the superproject and the submodule(s), sometimes 
only in the superproject. If they want to deactivate submodule.propagateBranches, would they have to
remember to also deactivate it in all submodules, in case of nested submodules ?...  if so,
this is a little unfortunate. But I _think_ they wouldn't have to, because as long as 
it's false in the superproject config, then we won't get into the new code at all when running
in the top level superproject...

>  	free(sm_alternate);
>  	free(error_strategy);
> @@ -1792,6 +1798,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>  	memset(&filter_options, 0, sizeof(filter_options));
>  	argc = parse_options(argc, argv, prefix, module_clone_options,
>  			     git_submodule_helper_usage, 0);
> +	prepare_repo_settings(the_repository);
>  
>  	clone_data.dissociate = !!dissociate;
>  	clone_data.quiet = !!quiet;
> @@ -1872,6 +1879,7 @@ struct submodule_update_clone {
>  struct update_data {
>  	const char *prefix;
>  	const char *displaypath;
> +	const char *super_branch;
>  	enum submodule_update_type update_default;
>  	struct object_id suboid;
>  	struct string_list references;
> @@ -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);
>  		break;
>  	case SM_UPDATE_REBASE:
>  		cp.git_cmd = 1;
> @@ -2456,6 +2466,7 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
>  static int update_submodule(struct update_data *update_data)
>  {
>  	int submodule_up_to_date;
> +	const char *submodule_head = NULL;
>  
>  	ensure_core_worktree(update_data->sm_path);
>  
> @@ -2469,7 +2480,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))
>  		die(_("Unable to find current revision in submodule path '%s'"),
>  			update_data->displaypath);
>  
> @@ -2493,7 +2504,13 @@ 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_head &&
> +	    !skip_prefix(submodule_head, "refs/heads/", &submodule_head))
> +		submodule_up_to_date = !strcmp(update_data->super_branch, submodule_head);

I'm not sure I understand this logic. We want to change the 'submodule_up_to_date' boolean,
so that we compare branch names instead of oid's, and we do that only if:

1. we are running with 'propagateBranches=true' (so update_data->super_branch will be set to the superproject's branch)
2. a ref is checked out in the submodule (so submodule_head will hold its name)
3. it's not a branch (so skip_prefix will return 0, and !skip_prefix will be 1). 
   In that case it must be simply "HEAD", i.e. the submodule's HEAD is detached.

Why do we need (2. + 3.) ? 

If branch 'foo' is currently checked out in the superproject, and
branch 'bar' is currently checked out in the submodule, and someone
runs 'git -c propagateBranches=true submodule update', wouldn't they expect
that 'bar' be checked out in the submodule ? Maybe not, but the commit message
and the tests should be more explicit about the expected behaviour in this case, I think.

And thinking about it more, won't this:

    submodule_up_to_date = !strcmp(update_data->super_branch, submodule_head);

always be false, since we already know that submodule_head is "HEAD" ?... 
Unless I'm confused...
 
> +	else
> +		submodule_up_to_date = oideq(&update_data->oid, &update_data->suboid);
> +
>  	if (!submodule_up_to_date || update_data->force)
>  		if (run_update_procedure(update_data))
>  			return 1;
> @@ -2551,6 +2568,12 @@ static int update_submodules(struct update_data *update_data)
>  		goto cleanup;
>  	}
>  
> +	if (the_repository->settings.submodule_propagate_branches) {
> +		struct branch *current_branch = branch_get(NULL);
> +		if (current_branch)
> +			update_data->super_branch = current_branch->name;

OK, so this condition means that super_branch won't get set if we are not
currently on a branch, i.e. we are in detached HEAD. This makes sense as there
would be no branch to propagate. Do we need a test for this ? maybe a case where
we clone with '--recurse-submodules --branch some-tag' ?

> +	}
> +
>  	for (i = 0; i < suc.update_clone_nr; i++) {
>  		struct update_clone_data ucd = suc.update_clone[i];
>  
> @@ -2634,6 +2657,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
>  	memset(&filter_options, 0, sizeof(filter_options));
>  	argc = parse_options(argc, argv, prefix, module_update_options,
>  			     git_submodule_helper_usage, 0);
> +	prepare_repo_settings(the_repository);
>  
>  	if (opt.require_init)
>  		opt.init = 1;
> diff --git a/t/t5617-clone-submodules.sh b/t/t5617-clone-submodules.sh
> index b5c66cb18cb..215fb02e9fb 100755
> --- a/t/t5617-clone-submodules.sh
> +++ b/t/t5617-clone-submodules.sh
> @@ -12,10 +12,17 @@ pwd=$(pwd)
>  test_expect_success 'setup' '
>  	git checkout -b main &&
>  	test_commit commit1 &&
> +	mkdir subsub &&
> +	(
> +		cd subsub &&
> +		git init &&
> +		test_commit subsubcommit1
> +	) &&
>  	mkdir sub &&
>  	(
>  		cd sub &&
>  		git init &&
> +		git submodule add "file://$pwd/subsub" subsub &&
>  		test_commit subcommit1 &&
>  		git tag sub_when_added_to_super &&
>  		git branch other
> @@ -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
> +'
> +
>  test_done
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 6cc07460dd2..00a6fec8912 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -1178,4 +1178,26 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=
>  	test_cmp expect.err actual.err
>  '
>  
> +test_expect_success 'submodule update with submodule.propagateBranches checks out branches' '
> +	test_when_finished "rm -fr top-cloned" &&
> +	cp -r top-clean top-cloned &&
> +
> +	# Create a new upstream submodule
> +	git init middle2 &&
> +	test_commit -C middle2 "middle2" &&
> +	git -C top submodule add ../middle2 middle2 &&
> +	git -C top commit -m "add middle2" &&
> +
> +	git -C top-cloned checkout -b "new-branch" &&
> +	git -C top-cloned pull origin main &&
> +	test_config -C top-cloned submodule.propagateBranches true &&
> +	git -C top-cloned submodule update --recursive &&
> +
> +	for REPO in "top-cloned/middle2" "top-cloned/middle" "top-cloned/middle/bottom"
> +	do
> +	    HEAD_BRANCH=$(git -C $REPO symbolic-ref HEAD) &&
> +	    test $HEAD_BRANCH = "refs/heads/new-branch" || return 1
> +	done
> +'
> +
>  test_done
> 

These tests look good, but maybe more tests would be needed in 
the light of my comments above... 

Thanks again for working on improving submodules!

Cheers,

Philippe.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Glen,
>
> Le 2022-08-29 à 16:54, Glen Choo via GitGitGadget a écrit :
>> 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 clone --recurse-submodules"
>> also learns this trick because it is implemented with "git submodule
>> update --recursive".
>
> OK. Maybe a more descriptive title would then be:
>
> clone, submodule update: create and check out submodule branches
>
> ?

Ah, thanks. Your other wording suggestions upthread are also very
helpful.

>
> Another thing, 'git pull --recurse-submodules' is also implemented using
> 'git submodule update --recursive'. But I don't think we want 'git pull'
> to start creating new branches in submodules, even with submodule.propagateBranches=true
> (though I haven't thought about it very hard). So maybe adding a word about
> that would be nice.

Good point. I thought that `git pull --recurse-submodules` used the
`--merge` strategy (in which case, it wouldn't matter), but looks like
it uses the `--checkout` strategy.

I'm quite certain we want to replace this `git pull
--recurse-submodules` implementation, aka non recursive `git merge` +
`git submodule update`, with a recursive `git merge` (and possibly
updating the worktrees with `git checkout --recurse-submodules`). Since
this flag is still experimental and incomplete, I think we have the
freedom to say that we won't care about this for now, but either way
I'll mention this somewhere.

>> "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.
>
> I guess you mean "in a future series" for the last sentence ? FWIW I still have
> your RFC from last Febryary about that [1] in my "unread Git mailing list" folder,
> I always seem to lack the time to sit down and read it through, sorry!
> Incidentally, I notice you did not link to it in the cover letter, 
> any reasoon why?
>
> [1] https://lore.kernel.org/git/20220209065236.36494-1-chooglen@google.com/

Ah, yes, I meant "in a future series". I didn't think that the RFC would
be very useful to reviewers since the non-RFC version is likely change a
lot (I've done a lot of tinkering between then and now), and it didn't
gain much traction in the first place anyway.

>
> Stepping back a bit, you write "thus it will fail if the submodule has the
> branch, but it is not checked out." If I read your patch correctly, this is
> implicit in that 'git checkout -b super-branch' that is ran by 'run_update_command'
> will error out if the branch already exists, right ? 
>
> Is there anything more we should do in that case ? 
> Should we remind the user, something like
> "you have submodule.propagateBranches set, but the branch 'super-branch' already
> exists in submodule 'that-sub'" ? 
>
> I'm trying to think of a scenario in which this could happen...
>
> Say a user:
> 1. clones a superproject with --recurse-submodules, but without 'submodule.propagateBranches'
> 2. runs 'git checkout -b topic' in the superproject
> 3. runs 'git branch topic' in the submodule
> 4. runs 'git submodule update' with 'submodule.propagateBranches' in the superproject
>
> This fails:
>
> fatal: a branch named 'topic' already exists
> fatal: Unable to checkout 'deadbeef' in submodule path 'sub'
>
> Do we need a more specific message ? I'm not sure.

Hm, you're right, this does seem quite opaque to end users; this means
nothing if they don't know that `git submodule update` uses `git checkout
-b` under the hood, which they obviously shouldn't need to know.

The main simplifying assumption behind `submodule.propagateBranches` (or
at least, this early version of it) is that users won't interact with
branches on the submodules directly outside of very specific scenarios,
e.g. setting submodule-specific tracking info. So maybe the more
comprehensive solution would be to block users from creating branches if
the submodule's superproject uses `submodule.propagateBranches`.

>> 
>> Signed-off-by: Glen Choo <chooglen@google.com>
>> ---
>>  builtin/submodule--helper.c | 28 ++++++++++++++++++++++++++--
>>  t/t5617-clone-submodules.sh | 34 ++++++++++++++++++++++++++++++++++
>>  t/t7406-submodule-update.sh | 22 ++++++++++++++++++++++
>>  3 files changed, 82 insertions(+), 2 deletions(-)
>> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index cbf6bda4850..7eb2c45900e 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -1695,6 +1695,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>>  			strvec_push(&cp.args, clone_data->single_branch ?
>>  				    "--single-branch" :
>>  				    "--no-single-branch");
>> +		if (the_repository->settings.submodule_propagate_branches)
>> +			strvec_push(&cp.args, "--detach");
>> +
>>  
>>  		strvec_push(&cp.args, "--");
>>  		strvec_push(&cp.args, clone_data->url);
>> @@ -1733,6 +1736,9 @@ static int clone_submodule(struct module_clone_data *clone_data)
>>  	if (error_strategy)
>>  		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
>>  				       error_strategy);
>> +	if (the_repository->settings.submodule_propagate_branches)
>> +		git_config_set_in_file(p, "submodule.propagateBranches",
>> +				       "true");
>
> Why do we need to set that in the config of the submodule ? I'm guessing this 
> is so that the new code also works for nested submodules, right ? 

As long as the value is set in the superproject, the new code still
works. This is meant as a way of setting the user's preferred value in
the submodule. Although.. `git clone` doesn't automatically set this
value in the superproject - it would have to be read off
system/global/cli config, so maybe it's more coherent to acknowledge
that the user's preferred value probably isn't in the repo anyway, and
maybe I should just drop this.

>
> I'm thinking about a user that would alternate between 'submodule.propagateBranches=true' and 'false'.
> Maybe they sometimes have to work on the superproject and the submodule(s), sometimes 
> only in the superproject. If they want to deactivate submodule.propagateBranches, would they have to
> remember to also deactivate it in all submodules, in case of nested submodules ?...  if so,
> this is a little unfortunate. But I _think_ they wouldn't have to, because as long as 
> it's false in the superproject config, then we won't get into the new code at all when running
> in the top level superproject...

Hm, would a user want to alternate in the first place? Maybe? e.g. with
`git checkout topic`, "true" would check out the submodule worktree at
the branch (including any WIP you have) but "false" would give you the
worktree specified by the superproject. Both are useful.

The way it's written now, "submodule.propagateBranches" is only passed
to submodule processes if it is "true", so it can be overrided by
submodule config if superproject says "false" but submodule says "true".
I should fix that..

>>  	free(sm_alternate);
>>  	free(error_strategy);
>> @@ -1792,6 +1798,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>  	memset(&filter_options, 0, sizeof(filter_options));
>>  	argc = parse_options(argc, argv, prefix, module_clone_options,
>>  			     git_submodule_helper_usage, 0);
>> +	prepare_repo_settings(the_repository);
>>  
>>  	clone_data.dissociate = !!dissociate;
>>  	clone_data.quiet = !!quiet;
>> @@ -1872,6 +1879,7 @@ struct submodule_update_clone {
>>  struct update_data {
>>  	const char *prefix;
>>  	const char *displaypath;
>> +	const char *super_branch;
>>  	enum submodule_update_type update_default;
>>  	struct object_id suboid;
>>  	struct string_list references;
>> @@ -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);
>>  		break;
>>  	case SM_UPDATE_REBASE:
>>  		cp.git_cmd = 1;
>> @@ -2456,6 +2466,7 @@ static void update_data_to_args(struct update_data *update_data, struct strvec *
>>  static int update_submodule(struct update_data *update_data)
>>  {
>>  	int submodule_up_to_date;
>> +	const char *submodule_head = NULL;
>>  
>>  	ensure_core_worktree(update_data->sm_path);
>>  
>> @@ -2469,7 +2480,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))
>>  		die(_("Unable to find current revision in submodule path '%s'"),
>>  			update_data->displaypath);
>>  
>> @@ -2493,7 +2504,13 @@ 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_head &&
>> +	    !skip_prefix(submodule_head, "refs/heads/", &submodule_head))
>> +		submodule_up_to_date = !strcmp(update_data->super_branch, submodule_head);
>
> I'm not sure I understand this logic. We want to change the 'submodule_up_to_date' boolean,
> so that we compare branch names instead of oid's, and we do that only if:
>
> 1. we are running with 'propagateBranches=true' (so update_data->super_branch will be set to the superproject's branch)
> 2. a ref is checked out in the submodule (so submodule_head will hold its name)
> 3. it's not a branch (so skip_prefix will return 0, and !skip_prefix will be 1). 
>    In that case it must be simply "HEAD", i.e. the submodule's HEAD is detached.
>
> Why do we need (2. + 3.) ? 

Oops I got skip_prefix() backwards, 3. should read "if a branch is
checked out". I'll add a test case for this (I could've sworn I had one
at some point).

>
> If branch 'foo' is currently checked out in the superproject, and
> branch 'bar' is currently checked out in the submodule, and someone
> runs 'git -c propagateBranches=true submodule update', wouldn't they expect
> that 'bar' be checked out in the submodule ? Maybe not, but the commit message
> and the tests should be more explicit about the expected behaviour in this case, I think.

Yeah, I'll call it out. I think this case is better addressed by having
`git checkout topic --recurse-submodules` automatically create "topic"
in the submodules that don't have it. This is one of the reasons why the
`git checkout` RFC isn't so relevant any more ;).

>> +	else
>> +		submodule_up_to_date = oideq(&update_data->oid, &update_data->suboid);
>> +
>>  	if (!submodule_up_to_date || update_data->force)
>>  		if (run_update_procedure(update_data))
>>  			return 1;
>> @@ -2551,6 +2568,12 @@ static int update_submodules(struct update_data *update_data)
>>  		goto cleanup;
>>  	}
>>  
>> +	if (the_repository->settings.submodule_propagate_branches) {
>> +		struct branch *current_branch = branch_get(NULL);
>> +		if (current_branch)
>> +			update_data->super_branch = current_branch->name;
>
> OK, so this condition means that super_branch won't get set if we are not
> currently on a branch, i.e. we are in detached HEAD. This makes sense as there
> would be no branch to propagate. Do we need a test for this ? maybe a case where
> we clone with '--recurse-submodules --branch some-tag' ?

Good point, I'll add a test for this.

>> +	}
>> +
>>  	for (i = 0; i < suc.update_clone_nr; i++) {
>>  		struct update_clone_data ucd = suc.update_clone[i];
>>  
>> @@ -2634,6 +2657,7 @@ static int module_update(int argc, const char **argv, const char *prefix)
>>  	memset(&filter_options, 0, sizeof(filter_options));
>>  	argc = parse_options(argc, argv, prefix, module_update_options,
>>  			     git_submodule_helper_usage, 0);
>> +	prepare_repo_settings(the_repository);
>>  
>>  	if (opt.require_init)
>>  		opt.init = 1;
>> diff --git a/t/t5617-clone-submodules.sh b/t/t5617-clone-submodules.sh
>> index b5c66cb18cb..215fb02e9fb 100755
>> --- a/t/t5617-clone-submodules.sh
>> +++ b/t/t5617-clone-submodules.sh
>> @@ -12,10 +12,17 @@ pwd=$(pwd)
>>  test_expect_success 'setup' '
>>  	git checkout -b main &&
>>  	test_commit commit1 &&
>> +	mkdir subsub &&
>> +	(
>> +		cd subsub &&
>> +		git init &&
>> +		test_commit subsubcommit1
>> +	) &&
>>  	mkdir sub &&
>>  	(
>>  		cd sub &&
>>  		git init &&
>> +		git submodule add "file://$pwd/subsub" subsub &&
>>  		test_commit subcommit1 &&
>>  		git tag sub_when_added_to_super &&
>>  		git branch other
>> @@ -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
>> +'
>> +
>>  test_done
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index 6cc07460dd2..00a6fec8912 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -1178,4 +1178,26 @@ test_expect_success 'submodule update --recursive skip submodules with strategy=
>>  	test_cmp expect.err actual.err
>>  '
>>  
>> +test_expect_success 'submodule update with submodule.propagateBranches checks out branches' '
>> +	test_when_finished "rm -fr top-cloned" &&
>> +	cp -r top-clean top-cloned &&
>> +
>> +	# Create a new upstream submodule
>> +	git init middle2 &&
>> +	test_commit -C middle2 "middle2" &&
>> +	git -C top submodule add ../middle2 middle2 &&
>> +	git -C top commit -m "add middle2" &&
>> +
>> +	git -C top-cloned checkout -b "new-branch" &&
>> +	git -C top-cloned pull origin main &&
>> +	test_config -C top-cloned submodule.propagateBranches true &&
>> +	git -C top-cloned submodule update --recursive &&
>> +
>> +	for REPO in "top-cloned/middle2" "top-cloned/middle" "top-cloned/middle/bottom"
>> +	do
>> +	    HEAD_BRANCH=$(git -C $REPO symbolic-ref HEAD) &&
>> +	    test $HEAD_BRANCH = "refs/heads/new-branch" || return 1
>> +	done
>> +'
>> +
>>  test_done
>> 
>
> These tests look good, but maybe more tests would be needed in 
> the light of my comments above... 
>
> Thanks again for working on improving submodules!

Thanks for lending your time and attention :)
>
> Cheers,
>
> Philippe.

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/refs.c b/refs.c
> index 90bcb271687..d72015c95e9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1784,19 +1784,21 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>  }
>  
>  int resolve_gitlink_ref(const char *submodule, const char *refname,
> -			struct object_id *oid)
> +			struct object_id *oid, const char **referent_out)

s/referent/target/ throughout this patch, I think.

Jonathan Tan <jonathantanmy@google.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> diff --git a/refs.c b/refs.c
>> index 90bcb271687..d72015c95e9 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1784,19 +1784,21 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
>>  }
>>  
>>  int resolve_gitlink_ref(const char *submodule, const char *refname,
>> -			struct object_id *oid)
>> +			struct object_id *oid, const char **referent_out)
>
> s/referent/target/ throughout this patch, I think.

I prefer the word "target", but this is a break from existing
conventions, e.g.

  int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
          struct strbuf *referent);

We can do this change, but I think we should also change this everywhere
if we do.