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:
> > 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.
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.
>
> > > - 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.
I agree, that would be good.
> > > - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
