Glen Choo <chooglen@xxxxxxxxxx> writes:
> +submodule.propagateBranches::
> + [EXPERIMENTAL] A boolean that enables branching support with
> + submodules. This allows certain commands to accept
> + `--recurse-submodules` (`git branch --recurse-submodules` will
> + create branches recursively), and certain commands that already
> + accept `--recurse-submodules` will now consider branches (`git
> + switch --recurse-submodules` will switch to the correct branch
> + in all submodules).
After some thought, I think that the way in this patch is the best way
to do it, even if it's unfortunate that a natural-looking "git branch
--recurse-submodules" wouldn't work without
"submodule.propagateBranches" (or whatever we decide to call the
variable).
Right now, as far as I know, "--recurse-submodules" in all Git commands
(and equivalently, the configuration variable "submodule.recurse") does
the same thing but in submodules too. For example, "fetch" fetches in
submodules, "switch" switches in submodules, and so on. It would be
plausible for "branch --recurse-submodules" to also create a branch in
submodules, but if a user just used it as-is, it would be surprising if
subsequent commands like "switch --recurse-submodules" or "-c
submodule.recurse=true switch" didn't use the branches that were just
created.
So OK, this makes sense.
> +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.
> @@ -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.
> +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).
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.
> +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.
> +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.
> +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.
> +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?
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? I would say that the user
should be able to recursively create the branch in this case, but am
open to other opinions. In any case, that case should be tested.
