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: > > 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. > > - 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: > > @@ -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. > > - 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
Attachment:
signature.asc
Description: OpenPGP digital signature