Re: [RFC v2] submodule: Respect requested branch on all clones

On Thu, Jan 09, 2014 at 10:40:52PM +0100, Jens Lehmann wrote:

Am 09.01.2014 20:55, schrieb W. Trevor King:

quoted

On Thu, Jan 09, 2014 at 08:23:07PM +0100, Jens Lehmann wrote:

quoted

Am 09.01.2014 18:32, schrieb W. Trevor King:

quoted

 However, the local-branch setting needs to be both
per-submodule and per-superproject-branch, so .git/config doesn't work
very well.  I think it's better to use something like my
.git/modules/<submodule-name>/config implementation [1] to set this
override.

Yes, the local branch should be set in the submodule's .git/config
to make operations done inside the submodule work seamlessly.

Once you're inside the submodule my local-branch setting shouldn't
matter, because it just connects superproject branches with submodule
branches. The submodule's config is just a convenient out-of-tree
place to store per-submodule overrides.

Now I get it, you want to be able to override a submodule branch for
every superproject branch. I'm not sure I'd add that in the first
iteration though, as it seems to add quite some complexity and I'm
not convinced yet users really need it (but I won't object when we
find real world use cases for that).

Not much complexity in the code, it's all in the first patch of my v3
series [1].  Adding a new override location doesn't seem that
complicated to me, but I haven't been very successful at getting this
idea across, so maybe it's weirder than I think ;).  Clearer
explanations welcome ;).

quoted

quoted

And it isn't a "per-superproject-branch override" but a
"per-superproject-branch default" which can be overridden in
.git/config (except for 'update', but I intend to fix that).

You're talking about .gitmodules vs. .git/config here, but for
local-branch, I'm talking about a fallback chain like [1]:

1. superproject.<superproject-branch>.local-branch in the submodule's
   config (superproject/.git/modules/≤submodule-name>/config).
2. submodule.<submodule-name>.local-branch in the superproject's
   config (.git/config).
3. submodule.<submodule-name>.local-branch in the superproject's
   .gitmodules file.
4. default to 'master'

Only #1 is a new idea.

Thanks for the explanation, now I understand what you're aiming at.

For additional clarity, my whole v3 series is not super long [2]… ;)

quoted

quoted

quoted

On the other hand, maybe an in-tree .gitmodules is good enough,
and folks who want a local override can just edit .gitmodules in
their local branch?  I've never felt the need to override
.gitmodules myself (for any setting), so feedback from someone
who has would be useful.

That way these changes would propagate to others working on the
same branch when pushing, which I believe is a feature.

Sure.  Unless they don't want to propagate them, at which point
they use an out-of-tree override masking the .gitmodules value.
The question is, would folks want local overrides for local-branch
(like they do for submodule.<name>.update), or not?  Since it's
easy to do [1], I don't see the point of *not* supporting
per-superproject-branch overrides.

Unless actual use cases are shown I'd vote for YAGNI here. A new
config option means considerable maintenance burden, no matter how
easy it is to implement in the first place.

Automatically checking out the preferred submodule branch for a given
superproject branch already requires a new config option.  The
per-superproject-branch out-of-tree override just renames it (from
submodule.<submodule-name>.local-branch to
superproject.<superproject-branch>.local-branch).  So different names
depending on superproject-level or submodule-level config, but still
the same option.  That doesn't sound like it's adding that much of a
maintenance burden.

On the other hand, I, personally, have no need for out-of-tree
overrides for *any* submodule-related config, so I'm fine if we drop
the submodule-level lookup location ;).

quoted

I'm all for rolling my 'git submodule checkout' into 'git checkout
--recurse-submodules' [2].  It was just faster to mock up in shell
while we decide how it should work.

Sure. As I said that's perfectly fine for testing this approach,
but we should do that right in "git checkout" and friends and not
add yet another submodule command.

The current C code looked fairly focused on detached HEAD sha1
checkouts, which was so far away from what I think should happen that
I didn't know where to start ;).  If we like the logic layed out in my
v3 series, I'll take another look at the C series and see if I can
come up with something.

quoted

quoted

quoted

quoted

quoted

If it's not the first clone, you should take no action (and your
original patch was ok about this).

I'm not sure this is the right thing to do, after all you
configured git to follow that branch so I'd expect it to be
updated later too, no? Otherwise you might end up with an old
version of your branch while upstream is a zillion commits
ahead.

Non-clone updates should not change the submodule's *local* branch
*name*.  They should change the commit that that branch references,
otherwise 'git submodule update' would be a no-op ;).

Okay, I seem to have misunderstood that. But what happens when the
branch setting in .gitmodules changes, shouldn't that be updated?

Not by 'git submodule update'.  If there are no out-of-tree overrides
and the user calls 'git submodule checkout' with a new local-branch in
.gitmodules, *that* should checkout a new submodule branch.

Hmm, but isn't "submodule sync" the command that copies changed
upstream config values (currently only the url) into the local config?
Then a subsequent "submodule update" could do the actual checkout.

'submodule update' currently only checks out detached HEADs with the
'checkout' update mode.  I got rid of that in my v3 series, so now all
'submodule update' does is integrate some branch (the gitlinked sha1
or the upstream --remote).  It has nothing to do with changing the
locally-checked-out branch.

quoted

quoted

quoted

quoted

later updates,

The same thing that currently happens, with the exception that
checkout-style updates should use reset to update the
currently-checked out branch (or detached-HEAD), instead of
always detaching the HEAD.

Won't the user loose any modifications to his local branch here?

They just called for a checkout-style update, so yes.  If they
want to keep local modifications, chose an integration mode that
preserves local changes.

Hmm, as current "submodule updates" already makes it too easy to
loose commits, this does not look right to me. I'd prefer to stop at
that point and tell the user what he can do to solve the conflict.

Users who are worried about loosing local updates should not be using
a checkout-style updates.  If they are using a checkout-style update,
and they ask for an update, they're specifically requesting that we
blow away their local work and checkout/reset to the new sha1.
Solving update conflicts is the whole point of the non-checkout update
modes.

quoted

Maybe you meant "for checkout I can easily overwrite the local
changes with the upstream branch", which is what I understand
checkout to do.

But which I find really unfriendly and would not like to see in a
new feature. We should protect the user from loosing any local
changes, not simply throw them away. Recursive update makes sure it
won't overwrite any local modification before it checks out anything
and will abort before doing so (unless forced of course).

If you want to get rid of checkout-mode updates, I'm fine with that.
However, I don't think it supports use-cases like Heiko's (implied) “I
don't care what's happening upstream, I never touch that submodule,
just checkout what the superproject maintainer says should be checked
out for this branch.  Even if they have been rebasing or whatever”
[3].

quoted

quoted

quoted

quoted

when superproject branches are merged (with and without conflicts),

I don't think this currently does anything to the submodule itself,
and that makes sense to me (use 'submodule update' or my 'submodule
checkout' if you want such effects).  We should keep the current logic
for updating the gitlinked $sha.  In the case that the
.gitmodule-configured local-branches disagree, we should give the
usual conflict warning (and <<<===>>> markup) and let the user resolve
the conflict in the usual way.

For me it makes lots of sense that in recursive checkout mode the
merged submodules are already checked out (if possible) right after
a superproject merge, making another "submodule update" unnecessary
(the whole point of recursive update is to make "submodule update"
obsolete, except for "--remote").

If you force the user to have the configured local-branch checked out
before a non-checkout operations with checkout side-effects (as we
currently do for other kinds of dirty trees), I think you'll avoid
most (all?) of the branch-clobbering problems.

I'm thinking that a local branch works in two directions: It should
make it easy to follow an upstream branch and also make changes to it
(and publish those) if necessary.

Those both sound like “integration with the remote submodule” issues
to me.  I'm more worried about what happens purely locally as the
developer checks out different superproject branches and wants the
submodules to follow along automatically (without detaching HEADs).
Once we have something that works there, I expect it will be easier to
add recursive superproject branch integration cleanly.  For example,
all of the usual branch-level config options (branch.<name>.*) come
along for free.

But neither local nor upstream changes take precedence, so the user
should either use "merge" or "rebase" as update strategy

That sounds good to me.

or be asked to resolve the conflict manually when "checkout" is
configured and the branches diverged.

I still think that checkout-mode updates should be destructive.  See
my paraphrased-version of Heiko's use case above.  How are they going
to resolve this manually?  Merge or rebase?  Why weren't they using
that update mode in the first place?

Cheers,
Trevor

[1]: http://article.gmane.org/gmane.comp.version-control.git/240251
[2]: http://article.gmane.org/gmane.comp.version-control.git/240248
[3]: http://article.gmane.org/gmane.comp.version-control.git/240013

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy