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

Glen Choo [off-list ref] writes:

quoted

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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

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.