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

[Jonathan Tan <jonathantanmy@google.com>]

Glen Choo <chooglen@google.com> writes:
> > 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);

Ah, I meant this interpretation. In that case, could we do it like this:

 - Add a dry_run parameter to submodule_create_branch(). It should stay
   exactly the same (except passing dry_run), so that the same code path
   is executed with and without dry_run. Delete
   submodule_validate_branchname(). (This makes sense conceptually
   because we're validating that we can later create the branches with
   the exact same parameters.)

 - When actually creating the branches, call submodule_create_branch(),
   then another command to set up tracking. I think it makes more sense
   if this is done directly through a Git command. If you want to use
   setup_tracking() through submodule--helper, I think that needs an
   explanation as to why a Git command wouldn't work.

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

I didn't mean this interpretation, but this idea is reasonable if we
needed such functionality.

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

Ah, I see. I think just delete the "is in .git/modules" part, so
something like "should create branch when submodule is not in HEAD's
.gitmodules".

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

[snip]

Ah...yes you're right.

> >> +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?

In that case, maybe explain why tracking cannot be set up and verify in
the test that no tracking was set up.

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

OK, this makes sense.