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

On Sun, Jan 05, 2014 at 08:17:00AM -0800, W. Trevor King wrote:

From: "W. Trevor King" <redacted>

The previous code only checked out the requested branch in cmd_add.
This commit moves the branch-checkout logic into module_clone, where
it can be shared by cmd_add and cmd_update.  I also update the initial
checkout command to use 'rebase' to preserve branches setup during
module_clone.

Signed-off-by: W. Trevor King <redacted>
---
Changes since v1:
* Fix a 'reqested' -> 'requested' typo in the subject/summary.
* Restore a post-clone 'git checkout -f -q' for the empty-branch case
  in module_clone().
* Distinguish between $branch (which defaults to 'master') and a new
  $config_branch (which defaults to empty) in cmd_update

After these fixes, all the existing submodule tests pass.  If we want
to merge this, we'd still want new tests that demonstrate the new
functionality.

I think this patch is going in the right direction. Making add() and
update() do the same is the right thing to do.

I would still like a complete description of Francesco's use case
though. Francesco: Could you give us a short description about what
exactly is your use-case? And please ignore all technical details how we
are going to implement this. I would like to know how, in an ideal
world, you would expect git to behave. Do you really only care about

	git submodule add

and the *initial*

	git submodule update

?

What happens if a developer already has the submodule and wants to work
on a feature?

On Sun, Jan 05, 2014 at 04:53:12AM +0100, Francesco Pretto wrote:

quoted

If I understand it correctly, looking at your intervention in
module_clone and cmd_update, when "submodule.<module>.branch" is set
during "update" the resulting first clone will always be a branch
checkout (cause $branch is filled with "branch" property).

That's correct.

quoted

I believe this will break a lot of tests,

Thanks for prompting me to run the tests.  This v2 now passes all of
the current submodule tests, and the functionality actually matches my
earlier descriptions of it ;).

quoted

as the the documentation says that in this configuration the HEAD
should be detached.

The current Documentation/git-submodule.txt has:

  update::
    Update the registered submodules, i.e. clone missing submodules
    and checkout the commit specified in the index of the containing
    repository.  This will make the submodules HEAD be detached unless
    `--rebase` or `--merge` is specified or the key
    `submodule.$name.update` is set to `rebase`, `merge` or `none`.

It's not clear if this refers to the initial-clone update, future
post-clone updates, or both.  Ideally, the behavior should be the
same, but in the initial-clone case we don't have an existing
checked-out branch to work with.

I do not think that its actually important to end up with a detached
HEAD. The documentation just states it because in most cases there is no
other option. But I do not think anything will break if a branch points
to the exact sha1 we would checkout and we checkout the branch instead.

quoted

Also it could break some users that rely on the current behavior.

The current code always has a detached HEAD after an initial-clone
update, regardless of submodule.<name>.update, which doesn't match
those docs either.  Adding a check to only checkout
submodule.<name>.branch if submodule.<name>.update was 'rebase',
'merge', or 'none' would be easy, but I don't think that makes much
sense.  I can't see any reason for folks who specify
submodule.<name>.branch to prefer a detached HEAD over a local branch
matching the remote branch's name.  If they prefer checkout updates,
the first such update will return them to a detached HEAD.  If they
prefer merge/rebase updates, future updates will keep them on the same
branch.  All my commit does is setup that initial branch for folks who
get the submodule via 'update', in the same way it's currently setup
for folks who get the submodule via 'add'.

I like your goal of putting this logic into one place. But a few things
still could be improved IMO.

In the simple case (update=checkout, no branch specified) with the new
checkout branch stuff in module_clone() this code here ends up calling
checkout twice.  First for master and then here later with the sha1.
This feels a little bit double. I would prefer if we skip the checkout
in module_clone() if its not necessary.

How about we move the whole "what to checkout"-decision into one place
instead of having it in update() and moving it from add() into
module_clone() ?

Previously there was not much in add() regarding checkout but since it
seems to grow (and now parts need to be shared between add() and
update()). I would like it if we could move that code into one central
location.

If we get here the checkout has already been done. Shouldn't this rather
specify a noop. I.E. like

	update_module="!true"

?

Cheers Heiko