Re: [PATCH v4 3/4] git-submodule update: Add --branch option

[Heiko Voigt <hvoigt@hvoigt.net>]

On Mon, Nov 26, 2012 at 04:00:18PM -0500, W. Trevor King wrote:
> From: "W. Trevor King" <wking@tremily.us>
> 
> This allows users to checkout the current
> superproject-recorded-submodule-sha as a branch, avoiding the detached
> head state that the standard submodule update creates.  This may be
> useful for the existing --rebase/--merge workflows which already avoid
> detached heads.
> 
> It is also useful if you want easy tracking of upstream branches.  The
> particular upstream branch to be tracked is configured locally with
> .git/modules/<name>/config.  With the new option Ævar's suggested
> 
>   $ git submodule foreach 'git checkout $(git config --file $toplevel/.gitm
> odules submodule.$name.branch) && git pull'
> 
> reduces to a
> 
>   $ git submodule update --branch
> 
> after each supermodule .gitmodules edit, and a
> 
>   $ git submodule foreach 'git pull'
> 
> whenever you feel like updating the submodules.  Your still on you're
> own to commit (or not) the updated submodule hashes in the
> superproject's .gitmodules.
> 
> Signed-off-by: W. Trevor King <wking@tremily.us>
> ---
>  Documentation/git-submodule.txt | 20 +++++++++++------
>  git-submodule.sh                | 48 +++++++++++++++++++++++++++++----------
>  t/t7406-submodule-update.sh     | 50 ++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 98 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index d0b4436..34392a1 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>  	      [-f|--force] [--reference <repository>] [--] <repository> [<path>]
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] init [--] [<path>...]
> -'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--rebase]
> +'git submodule' [--quiet] update [--init] [-N|--no-fetch] [--branch] [--rebase]
>  	      [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
>  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
>  	      [commit] [--] [<path>...]
> @@ -136,11 +136,11 @@ init::
>  
>  update::
>  	Update the registered submodules, i.e. clone missing submodules and
> -	checkout the commit specified in the index of the containing repository.
> -	This will make the submodules HEAD be detached unless `--rebase` or
> -	`--merge` is specified or the key `submodule.$name.update` is set to
> -	`rebase`, `merge` or `none`. `none` can be overridden by specifying
> -	`--checkout`.
> +	checkout the commit specified in the index of the containing
> +	repository.  This will make the submodules HEAD be detached unless
> +	`--branch`, `--rebase`, `--merge` is specified or the key
> +	`submodule.$name.update` is set to `branch`, `rebase`, `merge` or
> +	`none`. `none` can be overridden by specifying `--checkout`.
>  +
>  If the submodule is not yet initialized, and you just want to use the
>  setting as stored in .gitmodules, you can automatically initialize the
> @@ -207,7 +207,13 @@ OPTIONS
>  
>  -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.

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

>  		then
>  			subforce=$force
>  			# If we don't already have a -f flag and the submodule has never been checked out
> @@ -650,16 +654,21 @@ Maybe you want to use 'update --init'?")"
>  			case ";$cloned_modules;" in
>  			*";$name;"*)
>  				# then there is no local change to integrate
> -				update_module= ;;
> +				case "$update_module" in
> +					rebase|merge)
> +						update_module=
> +						;;
> +				esac
> +				;;
>  			esac
>  
>  			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.

>  				;;
>  			merge)
>  				command="git merge"
> @@ -674,15 +683,30 @@ Maybe you want to use 'update --init'?")"
>  				;;
>  			esac
>  
>  			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

here and the setup of that in the initialization of the submodule.

BTW, I am more and more convinced that an automatically manufactured
commit on update with --branch should be the default. What do other
think? Sascha raised a concern that he would not want this, but as far as
I understood he let the CI-server do that so I see no downside to
natively adding that to git. People who want to manually craft those
commits can still amend the generated commit. Since this is all about
helping people keeping their submodules updated why not go the full way?

Cheers Heiko