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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Jan 5, 2014, 00:39 UTC
Message-ID: <20140105003922.GA21772@sandbox-ub>
In-Reply-To: <20140104225401.GA3156@odin.tremily.us>

On Sat, Jan 04, 2014 at 02:54:01PM -0800, W. Trevor King wrote:

Show 24 quoted lines

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

Show 12 quoted lines

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

Show 13 quoted lines

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

Show 6 quoted lines

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