Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
>> Although this commit does not introduce breaking changes, it is
>> incompatible with existing --recurse-submodules commands because "git
>> branch --recurse-submodules" writes to the submodule ref store, but most
>> commands only consider the superproject gitlink and ignore the submodule
>> ref store. For example, "git checkout --recurse-submodules" will check
>> out the commits in the superproject gitlinks (and put the submodules in
>> detached HEAD) instead of checking out the submodule branches.
>
> The usual meaning of "incompatible" is "cannot be used together", I
> think, which is not what's happening here - it's just that the user
> would expect them to work together in a specific way but they don't do
> that.
Ok, I'll replace "incompatible with" with "does not interact well with".
>> + */
>> +static int submodule_create_branch(struct repository *r,
>> + const struct submodule *submodule,
>> + const char *name, const char *start_oid,
>> + const char *start_name, int force,
>> + int reflog, int quiet,
>> + enum branch_track track, int dry_run)
>> +{
>> + int ret = 0;
>> + struct child_process child = CHILD_PROCESS_INIT;
>> + struct strbuf child_err = STRBUF_INIT;
>> + struct strbuf out_buf = STRBUF_INIT;
>> + char *out_prefix = xstrfmt("submodule '%s': ", submodule->name);
>> + child.git_cmd = 1;
>> + child.err = -1;
>> + child.stdout_to_stderr = 1;
>> +
>> + prepare_other_repo_env(&child.env_array, r->gitdir);
>> + /*
>> + * submodule_create_branch() is indirectly invoked by "git
>> + * branch", but we cannot invoke "git branch" in the child
>> + * process because it does not let us set start_name and
>> + * start_oid separately (see create_branches_recursively()).
>
> Probably clearer to enumerate the 3 different pieces of information
> needed: the name of the branch to be created, the OID, and the name of
> the branch that would be used for tracking purposes.
Makes sense. Also the comment on create_branches_recursively() doesn't
do a great job of explaining why we need the additional piece of
information (tracking name), so I'll tidy them up together.
> An argument could be made that "git branch" should be extended to be
> able to take in these 3 different pieces of information, but it also
> makes sense to put this functionality in submodule--helper for now,
> since the whole thing is marked as experimental.
Yes, I considered this, but I can't think of a good reason to expose
this functionality to users - users can set their upstream with the more
intuitive "git branch --set-upstream-to".
>> +/*
>> + * Creates a new branch in repository and its submodules (and its
>> + * submodules, recursively). Besides these exceptions, the parameters
>> + * function identically to create_branch():
>> + *
>> + * - start_name is the name of the ref, in repository r, that the new
>> + * branch should start from. In submodules, branches will start from
>> + * the respective gitlink commit ids in start_name's tree.
>> + *
>> + * - tracking_name is the name used of the ref that will be used to set
>> + * up tracking, e.g. origin/main. This is propagated to submodules so
>> + * that tracking information will appear as if the branch branched off
>> + * tracking_name instead of start_name (which is a plain commit id for
>> + * submodules). If omitted, start_name is used for tracking (just like
>> + * create_branch()).
>> + *
>> + */
>> +void create_branches_recursively(struct repository *r, const char *name,
>> + const char *start_name,
>> + const char *tracking_name, int force,
>> + int reflog, int quiet, enum branch_track track,
>> + int dry_run);
>
> Instead of taking in "name", "start_name", and "tracking_name", could we
> take in "name", "oid", and "tracking_name"? That way, it's clearer what
> each parameter is used for.
I used start_name to mirror create_branches(), but start_name makes less
sense here because it's ambiguous.
Since the start_name is always an oid or NULL, this makes sense. I'll do
this.
>> +test_expect_success 'should not create branches in inactive submodules' '
>> + test_when_finished "reset_test" &&
>> + test_config -C super submodule.sub.active false &&
>> + (
>> + cd super &&
>> + git branch --recurse-submodules branch-a &&
>> + git rev-parse branch-a &&
>> + test_must_fail git -C sub branch-a
>> + )
>> +'
>
> The "test_must_fail" line doesn't make sense - there is no such command
> branch-a. To avoid errors like this, maybe make sure that all
> test_must_fail invocations are accompanied by assertions on the error
> message. (And for "test_must_fail git rev-parse", we could have a helper
> function here that asserts the "object not found".)
Ah good catch. I'll make the test helper.
>
>> +test_expect_success 'should set up tracking of local branches with track=always' '
>> + test_when_finished "reset_test" &&
>> + (
>> + cd super &&
>> + git -c branch.autoSetupMerge=always branch --recurse-submodules branch-a main &&
>> + git -C sub rev-parse main &&
>> + test "$(git -C sub config branch.branch-a.remote)" = . &&
>> + test "$(git -C sub config branch.branch-a.merge)" = refs/heads/main
>> + )
>> +'
>
> As described in t/README line 671, this means that the inner command
> could silently fail. You can do `REMOTE=$(...) &&` <newline> `test ...`.
> Same comment throughout this file.
Thanks for the catch and double thanks for the recommended fix.
Will reroll soon.
