On Tue, Nov 27, 2012 at 07:51:42PM +0100, Heiko Voigt wrote:
> On Mon, Nov 26, 2012 at 04:00:18PM -0500, W. Trevor King wrote:
> >  -b::
> >  --branch::
> > -	Branch of repository to add as submodule.
> > +	When used with the add command, gives the branch of repository to
> > +	add as submodule.
> > ++
> > +When used with the update command, checks out a branch named
> > +`submodule.<name>.branch` (as set by `--local-branch`) pointing at the
> > +current HEAD SHA-1.  This is useful for commands like `update
> > +--rebase` that do not work on detached heads.
> 
> Since you are reusing this option for update it further convinces me
> that reusing it for add makes sense and simplifies the logic for users.
> 
> I think an optional argument for --branch would be nice in the update
> case:
> 
> 	$ git submodule update --branch=master
> 
> would then allow a user that has not configured anything (except the
> branch tracking info in the submodule of course) to pull all submodules
> master branches.

Sounds good to me.  Remember that this is checking the branch and
pointing it at $sha1 (preparing for the pull), not pulling remote
branches.  The pull happens in a later

  $ git submodules foreach 'git pull'

> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index c51b6ae..28eb4b1 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -627,7 +631,7 @@ Maybe you want to use 'update --init'?")"
> >  			die "$(eval_gettext "Unable to find current revision in submodule path '\$sm_path'")"
> >  		fi
> >  
> > -		if test "$subsha1" != "$sha1" -o -n "$force"
> > +		if test "$subsha1" != "$sha1" -o -n "$force" -o "$update_module" = "branch"
> 
> As said before I think separating your code from the current update
> logic will simplify the handling below.

This felt less invasive (it avoids duplicating the recursion logic),
but I don't mind breaking it into a separate function/block.

> >  			must_die_on_failure=
> >  			case "$update_module" in
> >  			rebase)
> >  				command="git rebase"
> > -				die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$sm_path'")"
> > +				die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$sm_path'")"	
> >  				say_msg="$(eval_gettext "Submodule path '\$sm_path': rebased into '\$sha1'")"
> > -				must_die_on_failure=yes
> > +			must_die_on_failure=yes
> 
> Please always cleanup whitespace changes.

Oops, sloppy me.  Will fix.

> >  			then
> > -				die_with_status 2 "$die_msg"
> > -			else
> > -				err="${err};$die_msg"
> > -				continue
> > +				if (clear_local_git_env; cd "$sm_path" &&
> > +					git branch -f "$branch" "$sha1" &&
> > +					git checkout "$branch")
> 
> You wrote in earlier emails that you wanted to protect the user from
> non-fastforward changes. So I would expect a
> 
> 	$ git pull --ff-only

I'm not pulling here, I'm doing a regular `submodule update`, and
after that's done I checkout the branch pointing at the $sha1 to which
the branch was just updated.  All the submodule-state-clobbering
caveats of a usual `submodule update` still apply to this new
`submodule update --branch`, and I'm fine with that.

> BTW, I am more and more convinced that an automatically manufactured
> commit on update with --branch should be the default.

Again, there's nothing to update.  The pull happens in a separate
step.

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