Re: [PATCH 4/4] branch: add --recurse-submodules option for branch creation

From: Glen Choo <chooglen@google.com>
Date: Nov 24, 2021, 18:18 UTC
Message-ID: <kl6lmtltto1l.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20211124013153.3619998-1-jonathantanmy@google.com>

Thanks! The feedback is really useful.

Jonathan Tan <jonathantanmy@google.com> writes:

Show 19 quoted lines

>> +static int submodule_create_branch(struct repository *r, const char *name,
>> +				   const char *start_oid,
>> +				   const char *start_name, int force,
>> +				   int reflog, int quiet,
>> +				   enum branch_track track, char **err_msg)
>> +{
>> +	int ret = 0;
>> +	struct child_process child = CHILD_PROCESS_INIT;
>> +	struct strbuf child_err = STRBUF_INIT;
>> +	child.git_cmd = 1;
>> +	child.err = -1;
>> +
>> +	prepare_other_repo_env(&child.env_array, r->gitdir);
>> +	strvec_pushl(&child.args, "submodule--helper", "create-branch", NULL);
>
> Before this function is a function that calls "git branch" directly -
> couldn't this function do the same? (And then you can make both of them
> into one function.) The functionality should be exactly the same except
> that one has "--dry-run" and the other doesn't.

I see two somewhat valid interpretations, so I will address both.

If you are suggesting that I should call "git branch" instead of a new "git submodule--helper create-branch", that unfortunately does not work. Because of how we've defined the semantics, we want the submodule to branch off the commit in the superproject tree (which is a bare object id), but we want to set up tracking based off the ref that the user gave (evaluating it in the context of the submodule). This is why submodule--helper.c:module_create_branch() makes two calls like so:

	create_branch(the_repository, "<branch name>", "<object id>", force, 0, reflog, quiet,
		      BRANCH_TRACK_NEVER);
	setup_tracking("<branch name>", "<tracking name>", track, quiet, 0);

On the other hand, you might be suggesting that I should just add --dry-run to "git submodule--helper create-branch". That way, the dry-run form of the command is validating the non dry-run form (instead of using "git branch --dry-run" to validate "git submodule--helper create-branch"). That's a reasonable suggestion that avoids bikeshedding around "git branch --dry-run".

Show 20 quoted lines

>> @@ -874,6 +894,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  			FREE_AND_NULL(unused_full_ref);
>>  			return 0;
>>  		}
>> +		if (recurse_submodules) {
>> +			create_submodule_branches(the_repository, branch_name,
>> +						  start_name, force, reflog,
>> +						  quiet, track);
>> +			return 0;
>> +		}
>>  		create_branch(the_repository, branch_name, start_name, force, 0,
>>  			      reflog, quiet, track);
>>  	} else
>
> create_submodule_branches() here is a bit misleading since it also
> creates branches in the_repository. Might be better to write explicitly
> check in submodules -> create in main repository -> create in
> submodules. Or, if you want to combine all the submodule code in one
> function, (check in submodules -> create in submodules) -> create in
> main repository.

Philippe had a similar comment, I will rename it.

Show 16 quoted lines

>> +test_expect_success 'setup superproject and submodule' '
>> +	git init super &&
>> +	test_commit foo &&
>> +	git init sub-upstream &&
>> +	test_commit -C sub-upstream foo &&
>> +	git -C super submodule add ../sub-upstream sub &&
>> +	git -C super commit -m "add submodule" &&
>> +	git -C super config submodule.propagateBranches true
>> +'
>
> If making each test independent is important (which seems like a good
> goal to me, although I know that the Git tests are inconsistent on
> that), we could make this into a bash function (with test_when_finished)
> that gets called in every test. It doesn't violate the t/README request
> to put all test code inside test_expect_success assertions (since the
> function is still being run inside an assertion).

That's an interesting idea and it's more likely to be correct than my approach. I think it lines up better with testing best practices. However...

> In the general case, it will make test code slower to run, but if you're
> going to have to cleanup branches, I think it's better to just recreate
> the repo. In any case, for the general case, I can start a separate
> email thread for this discussion.

I'm concerned about the same thing and I suspect that recreating the repo won't be kindly received by some reviewers, even if they might be able to stomach cleanup_branches(). I think your suggestion is a better long-term direction, but I'd like to see discussion on the general case before changing the tests. A separate thread sounds good.

Show 13 quoted lines

>> +test_expect_success '--recurse-submodules should be ignored if submodule.propagateBranches is false' '
>> +	test_when_finished "cleanup_branches super branch-a" &&
>> +	(
>> +		cd super &&
>> +		git -c submodule.propagateBranches=false branch --recurse-submodules branch-a &&
>> +		git rev-parse branch-a &&
>> +		test_must_fail git -C sub rev-parse branch-a
>> +	)
>> +'
>
> This doesn't sound like the right behavior to me - I think it's fine if
> it was the config "submodule.recurse" instead of "--recurse-submodules",
> but if the argument is given on CLI, it should be a fatal error.

Philippe mentioned the same thing, which sounds right to me.

Show 20 quoted lines

>> +test_expect_success 'should create branch when submodule is in .git/modules but not .gitmodules' '
>> +	test_when_finished "cleanup_branches super branch-a branch-b branch-c" &&
>> +	(
>> +		cd super &&
>> +		git branch branch-a &&
>> +		git checkout -b branch-b &&
>> +		git submodule add ../sub-upstream sub2 &&
>> +		# branch-b now has a committed submodule not in branch-a
>> +		git commit -m "add second submodule" &&
>> +		git checkout branch-a &&
>> +		git branch --recurse-submodules branch-c branch-b &&
>> +		git rev-parse branch-c &&
>> +		git -C sub rev-parse branch-c &&
>> +		git checkout --recurse-submodules branch-c &&
>> +		git -C sub2 rev-parse branch-c
>> +	)
>> +'
>
> Hmm...how is this submodule in .git/modules but not .gitmodules? It
> looks like a normal submodule to me.

The test title is probably too terse - the submodule is not in the working tree's .gitmodules, but it is in branch-b's .gitmodules. I'll reword the title.

>> +test_expect_success 'should not create branch when submodule is not in .git/modules' '
>
> The title of this test contradicts the title of the test that I quoted
> previously.

I'm not sure how this is a contradiction, from before..

  should create branch when submodule is in .git/modules but not
  [the working tree's] .gitmodules

meaning "we should create the branch if we can find the submodule in .git/modules", i.e. the implication is: presence in .git/modules => can create branch.

Whereas

  should not create branch when submodule is not in .git/modules

meaning "we should not create the branch if we cannot find the submodule in .git/modules", i.e. the implication is: absence in .git/modules => cannot create branch.

Taken together, they assert that presence in .git/modules is a necessary condition for the subodule branch to be created.

Show 9 quoted lines

>> +test_expect_success 'should not fail when unable to set up tracking in submodule' '
>> +	test_when_finished "cleanup_branches super-clone branch-b" &&
>> +	(
>> +		cd super-clone &&
>> +		git branch --recurse-submodules branch-b origin/branch-b
>> +	)
>> +'
>
> Is there a warning printed that we can check?

"git branch" does not warn if tracking is not set up when it is not explicitly required, so this does not warn. However, I can imagine that if the superproject branch has tracking set up, a user might expect that all submodules would also have tracking set up, and thus a warning might be useful. I don't think it will be _that_ useful for most users, but at least some users would probably appreciate it.

For slightly unrelated reasons, I tried to get the tracking info of a newly created branch and it is tedious. For this reason and the fact that I'm not sure if the benefit is that great, I'm tempted *not* to add the warning, but perhaps you feel more strongly than I do?

> Also, this patch set doesn't discuss the case in which the branch in a
> submodule already exists, but it points to the exact commit that we
> want. What is the functionality in that case?

The behavior is identical to the general case where the branch already exists - branch validation (git branch --dry-run) fails.

> I would say that the user should be able to recursively create the
> branch in this case, but am open to other opinions.

I'm inclined to disagree. To illustrate this in the real world, say a user wants to create a 'new-feature' branch recursively, but there is already a 'new-feature in a submodule. Here are two possible sets of events that could lead to this situation:

1) the 'new-feature' branch was intended for the same work as the new
   branch but the user just happened to create the 'new-feature'
   subomdule branch first
2) the existing 'new-feature' branch doesn't actually contain the same
   work (maybe it's an overloaded term, or maybe the user used a branch
   to bookmark a commit before starting work on it)

What would happen if we allowed the branch to be created? In case 1, the user would be more than happy (because we read their mind! woohoo!) But in case 2, the user might not realize that their bookmark is getting clobbered by the new recursive branch - they might try revisiting their bookmark only to realize that they've accidentally committed on top of it.

I think the incidence of case 2 is far lower than case 1's, but I think it's reasonable to be defensive by default. In any case, we can change the default after more user testing.

> In any case, that case should be tested.

Testing this case would make the intended behavior clearer, so I will add this test.