Re: [PATCH v3 0/8] clone, submodule update: check out submodule branches

[Glen Choo <chooglen@google.com>]

Hi Philippe.

Thanks for spending time on the high level ideas (and not just the
code); it really helps to keep this logical and consistent.

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>> = Description
>> 
>> This series teaches "git clone --recurse-submodules" and "git submodule
>> update" to understand "submodule.propagateBranches" (see Further Reading for
>> context), i.e. if the superproject has a branch checked out and a submodule
>> is cloned, the submodule will have the same branch checked out.
>> 
>> To do this, "git submodule update" checks if "submodule.propagateBranches"
>> is true. If so, and if the superproject has the branch 'topic' checked out,
>> then:
>> 
>>  * Submodules are cloned without their upstream branches
>>  * The 'topic' branch is created in the submodule
>>  * The submodule is updated via "git checkout topic" instead of checking out
>>    the gitlink's OID.
>> 
>
> Currently, the description of submodule.propagateBranches is:
>
>     [EXPERIMENTAL] A boolean that enables branching support when 
>     using --recurse-submodules or submodule.recurse=true. Enabling this 
>     will allow certain commands to accept --recurse-submodules and certain 
>     commands that already accept --recurse-submodules will now consider branches. 
>     Defaults to false.
>
> I think with this series that description must be tweaked, because "git submodule update"
> does not qualify as a command that "now accepts --recurse-submodules", neither does
> it "already accept --recurse-submodules" but now changes behaviour to consider branches.
>
> It does change behaviour to "now consider branches", but never had anything to do with
> "--recurse-submodules".

Yes that's a good point, thanks.

>
> --8<--
>
>> 
>> = Future work
>> 
>>  * Patch 5, which refactors resolve_gitlink_ref(), notes that a better
>>    interface would be to return the refname instead of using an "out"
>>    parameter, but we use an "out" parameter so that any new callers trying
>>    to use the old function signature will get stopped by the compiler. The
>>    refactor can be finished at a later time.
>> 
>>  * Patch 5 uses the name "target" when we are talking about what a symref
>>    points to, instead of "referent" like the other functions. "target" is
>>    the better choice, since "referent" could also apply to non-symbolic
>>    refs, but that cleanup is quite big.
>> 
>>  * Patch 8 notes that for already cloned submodules, the branch may not
>>    point to the same OID as the superproject gitlink, and it may not even
>>    exist. This will be addressed in a more comprehensive manner when we add
>>    support for checking out branches with "git checkout
>>    --recurse-submodules".
>
>
> A few points I thought about while reading this version:

This is a good reminder for me to check in that doc I promised that
describes how branches in submodules would work. This is great feedback
for that work, though :)

>
> 1. There is always a possibility that the branch name in the superproject already exists
> in the submodule remote, but is a completely different topic (think of a branch named "refactor"
> for example). With this series (and propagateBranches=true), this would mean that 
> the initial submodule clone would create a local branch "refactor" that points to the gitlink
> in the superproject, and a remote-tracking branch "origin/refactor" that points to the unrelated
> submodule topic branch from the submodule remote. This could be confusing... but I don't
> really know what Git could do about it !

Yes.. I think submodule.propagateBranches requires a change in mental
model where submodule branch names have nothing to do with the
submodule's remote's branch names. It _might_ not be so confusing once
users have a grasp on that, but I recognize that that's quite optimistic
of me.

The biggest sources of confusion I see comes from remote-tracking,
either implicitly (e.g. "git checkout topic" when "topic" doesn't exist,
but "origin/topic" does) or manually (e.g. "git branch
--set-upstream-to"), because they'll see similarly-named remote and
local branches that have nothing to do with each other.

I can see some ways to address this, though they're not perfect:

- Remove the need for users to set up manual remote-tracking and disable
  implicit remote-tracking (possibly by using .gitmodules). 
- Block or warn on possibly confusing operations (e.g. don't allow users
  to create branches in the submodule directly)

> In patch 3/8 'git branch' is used to create the submodule branch from an oid, and so 
> it does not track any branch, and is not affected by 'branch.autoSetupMerge' as far as I 
> could test. But maybe this should be explicitely mentioned somewhere? 

I could mention it. For now, I don't see any reasonable way to respect
branch.autoSetupMerge, since there's no good way to map from 'submodule
remote branch' -> 'local submodule branch that's part of a superproject
branch'.

Between this and the previous section, perhaps we should think of
submodule branches as being fundamentally different from "regular"
branches..

> 2. The new "git submodule update" behaviour seems to only make sense with "--checkout", 
> which is the default "git submodue update" mode. But what if one uses "git submodule
> update --merge", or "--rebase", or has submodule.<name>.update set to a custom command
> or "none" ? Is the idea that these modes are incompatible with submodule branching ?
> I can understand that this gets really complex and might change when 'git merge' and 
> 'git rebase' themselves are taught to update submodule worktrees (and probably also submodule
> branches from what you refer to as future work), but in any case I think we should at 
> least test the new code when these options are used (if only to error out outright as
> incompatible)...

Ah, I should have been more explicit. Yes, I intend for the other modes
to be incompatible, so this should error out outright.

"git submodule update" is overdue for some refactoring IMO (at least for
internal use cases). It is quite badly overloaded - it peforms 3
different actions (clone missing submodules, fetch missing commits,
"update" worktree) and supports 4 different update strategies. In this
series, I was concerned mainly with "git clone --recurse-submodules",
and teaching "git submodule update --checkout" turned out to be a nice
side-effect, but because the "submodule update" API surface is so large,
we end up with this extra noise of how this new setting will interact
with the rest of API.

One goal of the Submodule UX (branches aside) is to remove the need for
manual submodule lifecycle management via "git submodule". For "git
submodule update", this would be:

- Teach fetch to clone missing submodules
- Teach fetch to fetch missing commits (I've done this already with my
  work on fetching in out-of-tree submodules)
- Teach checkout/reset to update working trees as we expect

I'm still not sure what to think of the merge/rebase/custom command
update strategies since they aren't simply "updating" the working tree.
My suspicion is that merge/rebase are more like partial implementations
of merge/rebase --recurse-submodules. The custom command is perhaps more
ofa way for users to orchestrate nonnative Git workflows on top of
submodules, so I don't feel bad about not supporting that at least.

> Thanks and cheers,
>
> Philippe.