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

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

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

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

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

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

When I tried to drop the '' case here:

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…

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.

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.

Cheers,
Trevor

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