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

On Sun, Jan 05, 2014 at 01:24:58PM -0800, W. Trevor King wrote:

On Sun, Jan 05, 2014 at 08:48:50PM +0100, Heiko Voigt wrote:

quoted

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

quoted

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.

There's no "if the remote-tracking branch points to the exact sha1"
logic in my patch.

I know I was more referring to the discussion whether detached HEAD for
submodules is important or not.

If submodule.<name>.branch is set, it *always*
creates a new local branch of that name pointing to the exact sha1.
If submodule.<name>.branch is not set, we still create a detached-HEAD
checkout of the exact sha1.

Thanks for this clarification. Since the usual usage with --remote is
with a remote-tracking branch, I confused this here. I am not sure
whether blindly creating a local branch from the recorded sha1 is the
right thing to do. In what situations would that be helpful?

At $dayjob we usually use feature branches for our work. So if someone
wants to work in a submodule you simply create a branch at the current
sha1 which you then send out for review.

The reason why one would set a branch option here is to share the
superproject branch with colleagues. He can make sure they can always
fetch and checkout the submodule even though the branch there is still
under cleanup and thus will be rebased often. The commit referenced by
sha1 would not be available to a developer fetching after a rebase.

Thinking through this more, perhaps the
logic should be:

* If submodule.<name>.update (defaulting to checkout) is checkout,
  create a detached HEAD.
* Otherwise, create a new branch submodule.<name>.branch (defaulting
  to master).

Why not trigger the attached state with the submodule.<name>.branch
configuration option? If there is a local branch available use that, if
not the tracking branch (as it is currently). Then a developer can start
working on the branch with:

	cd submodule; git checkout -t origin/<branchname>

assuming that submodule update learns some more support for this.

The motivation is that if submodule.<name>.update is checkout, the
user is unlikely to be developing locally in the submodule, as
subsequent updates would clobber their local commits.  Having a
detached HEAD is a helpful "don't develop here" reminder ;).  If
submodule.<name>.update is set, the user is likely to be developing
locally, and will probably want a local branch already checked out to
facilitate that.

quoted

quoted

-			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" || exit
+			module_clone "$sm_path" "$name" "$url" "$reference" "$depth" "$config_branch" || exit

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.

There is no guarantee that the remote master and the exact sha1 point
at the same commit.  Ideally we'd just clone the exact sha1 in this
case.

quoted

I would prefer if we skip the checkout in module_clone() if its not
necessary.

When I tried to drop the '' case here:

quoted

quoted

@@ -306,7 +307,15 @@ module_clone()
 	echo "gitdir: $rel/$a" >"$sm_path/.git"
 
 	rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
-	(clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config core.worktree "$rel/$b")
+	(
+		clear_local_git_env
+		cd "$sm_path" &&
+		GIT_WORK_TREE=. git config core.worktree "$rel/$b" &&
+		case "$branch" in
+		'') git checkout -f -q ;;
+		?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
+		esac
+	) || die "$(eval_gettext "Unable to setup cloned submodule '\$sm_path'")"
 }

I got test-suite errors that I didn't get to the bottom of.  However…

quoted

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() ?

…this sounds like a good idea to me.  However, it would be a more
intrusive change, and there may be conflicts with Francesco's proposed
attach/detach functionality.  I'll wait until we have a clearer idea
of where that is headed before I attempt a more complete
consolidation.

I agree, that would be good.

quoted

quoted

-				update_module= ;;
+				if test -n "$config_branch"; then
+					update_module="!git reset --hard -q"

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

	update_module="!true"

We are on a local branch at this point, but not neccessarily pointing
at the gitlinked sha1.  The reset here ensures that the new local
branch does indeed point at the gitlinked sha1.

But isn't this a fresh clone? Why should the branch point at anything
else?

Cheers Heiko