Glen Choo <chooglen@xxxxxxxxxx> 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.
