Re: Re: [PATCH] submodule: Respect reqested branch on all clones

[Heiko Voigt <hvoigt@hvoigt.net>]

On Sat, Jan 04, 2014 at 02:54:01PM -0800, W. Trevor King wrote:
> On Sat, Jan 04, 2014 at 11:09:15PM +0100, Heiko Voigt wrote:
> > On Fri, Jan 03, 2014 at 10:06:11AM -0800, W. Trevor King wrote:
> > > @@ -306,7 +307,14 @@ 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")
> > 
> > Why should this line be removed? Is it not needed for correct
> > worktree <-> repo linking of submodules?
> > 
> > > +	(
> > > +		clear_local_git_env
> > > +		cd "$sm_path" &&
> > > +		GIT_WORK_TREE=. git config core.worktree "$rel/$b" &&
> > > +		if test -n "$branch"; then
> > > +			git checkout -f -q -B "$branch" "origin/$branch" && echo "checked out $branch"
> > > +		fi
> > > +	) || die "$(eval_gettext "Unable to setup cloned submodule '\$sm_path'")"
> > >  }
> 
> It's not removed, just merged with the branch manipulation that also
> happens in a clean local environment in $sm_path.  Spawning two
> sequential subshells with the same setup seemed like overkill.

Ah ok, thanks. For some reason I overlooked that.

> > > @@ -861,7 +860,12 @@ Maybe you want to use 'update --init'?")"
> > >  			case ";$cloned_modules;" in
> > >  			*";$name;"*)
> > >  				# then there is no local change to integrate
> > > -				update_module= ;;
> > > +				if test -n "$branch"; then
> > > +					update_module="!git reset --hard -q"
> > 
> > Does that not put the user in danger of loosing changes?
> 
> No, because this is only happens for just-cloned modules.  The user
> hasn't had time to make local changes yet.

Ah ok I see. But why the reset then? Doesn't the earlier git checkout in your
patch take care of checking out the branch and thus update to the right
revision?

> > If submodule.<name>.branch is configured:
> > 
> > 	git submodule update
> > 
> > will checkout the configured branch instead of the sha1?
> 
> The use case described by Francesco, a submodule maintainer Alice sets
> up the submodule, which downstream developer Bob want to checkout to a
> branch.  I think that matching the exact commit specified by Alice in
> Bob's checkout is important, even if the upstream developer Charlie
> has advanced the referenced branch in the meantime.  Shifting the
> referenced submodule commit should be an explicit decision made by
> Alice, not a clone-time accident for Bob.

But from what I understand of this part of Francesco's use-case description:

> # Developer
> $ git pull
> $ git submodule init
> $ git submodule update --remote
> $ cd <path>
> $ branch="$(git config -f ..\.gitmodules submodule.common.branch)"; git checkout $branch

Is that he wants to allow the developer to switch to following a branch instead
of an exact sha1 while some extension in the common module is still under
development. That makes it easier to develop in parallel in the submodule and
the superproject because you do not need to update the sha1 all the time.

E.g. its likely that changes have to be reviewed and cleaned up in the
submodule first until they can be merged to the master branch there. During
this time the developer follows the branch.
Once the submodule review is finished the superproject branch can switch back
to the exact model again (because the submodule will not change anymore) and
finish its implementation there.

Matching the exact commit and checking out the branch only if it points to that
exact commit does not really help the developer in this use-case. But I am only
guessing from my experience with development of features in submodule.

Cheers Heiko