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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 23, 2022, 01:33 UTC
Message-ID: <20221123013319.1597025-1-jonathantanmy@google.com>
In-Reply-To: <kl6lmt8iesmu.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 6 quoted lines

> > 1. Checkout the branch, ignoring OID (as in this patch).
> > 2. Checkout the branch, erroring out if the OID is wrong.
> > 3. 1 + creating the branch if it does not exist.
> > 4. 2 + creating the branch if it does not exist.
> > 5. Always forcibly create the branch at the gitlink's OID and then checking
> >    it out.

[snip]

> > A. Create only the branch that is checked out in the superproject (as in this
> >    patch).
> > B. Create all branches that are present in the superproject.
> > C. Go back on our previous decision, switching to 3.

[snip]

Show 8 quoted lines

> But it doesn't make sense to mix both A _and_ C, since C would already
> give us the same result as A, so it probably makes sense to go straight
> to C in this series (i.e. only for the initial clone, not subsequent
> checkouts). I'll do that in v3.
> 
> I prefer C in the long run, since both A and B require that the list of
> submodule branches never get out of sync with the superproject, which is
> hard to enforce, e.g.:

 
I discussed this with Glen in-office and Glen pointed out that A is actually
not necessarily redundant with respect to C, since a "git submodule add" may
clone a submodule, but it would not run "git submodule update" (so 3 + C would
mean that no branch is created in the submodule, which is not what we want). So
we still need A.

As for 1 vs. 3, we will still need 3 in the future for the reasons I described in my previous e-mail, but I think that that can be done incrementally. My concern is to avoid doing something in a patch set that we will later need to undo; I think that we are indeed avoiding it here (we're doing A but we will still need it in the future, so there is no undoing of A needed).

So overall, after this discussion, this patch set looks good to me, except for the minor points that I have commented on in my previous emails.