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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]