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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: Nov 8, 2022, 14:23 UTC
Message-ID: <b9e7e6b7-43f8-3b80-79ba-3ffc53bec7f8@gmail.com>
In-Reply-To: <pull.1321.v3.git.git.1666988096.gitgitgadget@gmail.com>

Hi Glen,

Le 2022-10-28 à 16:14, Glen Choo via GitGitGadget a écrit :

> This version has relatively few changes, and should address all of
> Jonathan's comments (thanks!).
>

I was not able to take a look before now, but I think the suggestions by Jonathan on v2 make a lot of sense, especially adding more tests in the last patch. Thanks for these additional tests.

I have a few comments/questions on the overall design, which I'll write up at the end of this reply since they are more general.

Show 16 quoted lines

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

--8<--

Show 19 quoted lines

> 
> = 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:

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 !

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?

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

Thanks and cheers,

Philippe.