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

[Heiko Voigt]

Hi,

On Fri, Jan 03, 2014 at 10:06:11AM -0800, W. Trevor King wrote:
 From: W. Trevor King wk...@tremily.us
 
 The previous code only checked out the requested branch in cmd_add.
 This commit moves the branch-checkout logic into module_clone, where
 it can be shared by cmd_add and cmd_update.  I also update the initial
 checkout command to use 'rebase' to preserve branches setup during
 module_clone.
 
 Signed-off-by: W. Trevor King wk...@tremily.us
 ---
 
 On Fri, Jan 03, 2014 at 09:49:01AM +0100, Francesco Pretto wrote:
  - there's a developer update user. He will clone the submodule
  repository with an *attached* HEAD. Subsequent merge or rebase
  update operations will keep the HEAD attached.
 
 'merge' and 'rebase' updates don't change the HEAD attachment.
 Branches stay branches and detached HEADs stay detached.  If you've
 moved away from the 'checkout' update mechanism, the only thing you
 still need is a way to get an initial checkout on a branch.  This
 should do it (I can add tests if folks like the general approach).
 
  git-submodule.sh | 30 +-
  1 file changed, 17 insertions(+), 13 deletions(-)
 
 diff --git a/git-submodule.sh b/git-submodule.sh
 index 2979197..e2e5a6c 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 @@ -253,6 +253,7 @@ module_clone()
   url=$3
   reference=$4
   depth=$5
 + branch=$6
   quiet=
   if test -n $GIT_QUIET
   then
 @@ -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')
  }
  
  isnumber()
 @@ -469,16 +477,7 @@ Use -f if you really want to add it. 2
   echo $(eval_gettext Reactivating local git 
 directory for submodule '\$sm_name'.)
   fi
   fi
 - module_clone $sm_path $sm_name $realrepo $reference 
 $depth || exit
 - (
 - clear_local_git_env
 - cd $sm_path 
 - # ash fails to wordsplit ${branch:+-b $branch...}
 - case $branch in
 - '') git checkout -f -q ;;
 - ?*) git checkout -f -q -B $branch origin/$branch ;;
 - esac
 - ) || die $(eval_gettext Unable to checkout submodule 
 '\$sm_path')
 + module_clone $sm_path $sm_name $realrepo $reference 
 $depth $branch || exit
   fi
   git config submodule.$sm_name.url $realrepo
  
 @@ -815,7 +814,7 @@ Maybe you want to use 'update --init'?)
  
   if ! test -d $sm_path/.git -o -f $sm_path/.git
   then
 - module_clone $sm_path $name $url $reference 
 $depth || exit
 + module_clone $sm_path $name $url $reference 
 $depth $branch || exit
   cloned_modules=$cloned_modules;$name
   subsha1=
   else
 @@ -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?

I am wondering if we should maybe take a little different approach:

If submodule.name.branch is configured:

git submodule update

will checkout the configured branch instead of the sha1? Maybe something like
this (untested):

diff --git a/git-submodule.sh b/git-submodule.sh
index 2677f2e..eca519a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -903,7 +903,13 @@ Maybe you want to use 'update --init'?)
;;
esac
 
-   if (clear_local_git_env; cd $sm_path  $command 
$sha1)
+   revision=$sha1
+   if test -n $branch
+   then
+   revision=$branch
+   fi
+
+   if (clear_local_git_env; cd $sm_path  $command 
$revision)
then
say $say_msg
elif test -n $must_die_on_failure


Then we do not need to write a command configuration into the local 

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

[W. Trevor King]

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.

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

 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.

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


signature.asc
Description: OpenPGP digital signature

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

[Heiko Voigt]

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[W. Trevor King]

On Sun, Jan 05, 2014 at 01:39:22AM +0100, Heiko Voigt wrote:
 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:
@@ -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?

The reset avoids a detached HEAD.  With an empty $update_module, the
following case block will setup:

  command=git checkout $subforce -q

which is later called with:

  $command $sha1

That's going to give you a detached HEAD.  The new code sets up:

  command=git reset --hard -q

which will keep you on the branch checked out in module_clone().

   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.

I'll wait for Francesco to clarify his use case.  All my patch does is
replace the manual:

  $ cd path
  $ branch=$(git config -f ..\.gitmodules submodule.common.branch); git 
checkout $branch

with an automatic (on update):

  $ branch=$(git config -f .gitmodules submodule.${name}.branch);
  $ cd $path
  $ git checkout -f -q -B $branch origin/$branch
  $ git reset --hard -q $sha

when submodule.name.branch is configured.  Whether that last bit is
desirable or not is debatable.  If you *do* want to float the
submodule past the commit blessed by Alice, it's easy to add a manual:

  $ git submodule update --remote --rebase $path

If we drop the trailing reset (to float the checkout), it's harder to
rewind to the commit blessed by Alice, because distinguising between:

a) locally added stuff that we want to merge/rebase onto Alice's $sha,
   and
b) advancements from the automatic float that we *don't* want to
   merge/rebase onto Alice's $sha.

is difficult/impossible.  If you use the --checkout strategy (there
are no local commits), you can use:

  $ git submodule update --checkout $path

but you'd still need to update the branch references to point to the
that particular commit.

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


signature.asc
Description: OpenPGP digital signature

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

[Francesco Pretto]

Thanks for adding your contribute. My comments below:

2014/1/3 W. Trevor King wk...@tremily.us:

 The previous code only checked out the requested branch in cmd_add.
 This commit moves the branch-checkout logic into module_clone, where
 it can be shared by cmd_add and cmd_update.  I also update the initial
 checkout command to use 'rebase' to preserve branches setup during
 module_clone.
 [...]
 @@ -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)
 +   (
 +   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')
  }

If I understand it correctly, looking at your intervention in
module_clone and cmd_update, when submodule.module.branch is set
during update the resulting first clone will always be a branch
checkout (cause $branch is filled with branch property). I believe
this will break a lot of tests, as the the documentation says that in
this configuration the HEAD should be detached. Also it could break
some users that rely on the current behavior.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] submodule: Respect reqested branch on all clones

[W. Trevor King]

From: W. Trevor King wk...@tremily.us

The previous code only checked out the requested branch in cmd_add.
This commit moves the branch-checkout logic into module_clone, where
it can be shared by cmd_add and cmd_update.  I also update the initial
checkout command to use 'rebase' to preserve branches setup during
module_clone.

Signed-off-by: W. Trevor King wk...@tremily.us
---

On Fri, Jan 03, 2014 at 09:49:01AM +0100, Francesco Pretto wrote:
 - there's a developer update user. He will clone the submodule
 repository with an *attached* HEAD. Subsequent merge or rebase
 update operations will keep the HEAD attached.

'merge' and 'rebase' updates don't change the HEAD attachment.
Branches stay branches and detached HEADs stay detached.  If you've
moved away from the 'checkout' update mechanism, the only thing you
still need is a way to get an initial checkout on a branch.  This
should do it (I can add tests if folks like the general approach).

 git-submodule.sh | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2979197..e2e5a6c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -253,6 +253,7 @@ module_clone()
url=$3
reference=$4
depth=$5
+   branch=$6
quiet=
if test -n $GIT_QUIET
then
@@ -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)
+   (
+   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')
 }
 
 isnumber()
@@ -469,16 +477,7 @@ Use -f if you really want to add it. 2
echo $(eval_gettext Reactivating local git 
directory for submodule '\$sm_name'.)
fi
fi
-   module_clone $sm_path $sm_name $realrepo $reference 
$depth || exit
-   (
-   clear_local_git_env
-   cd $sm_path 
-   # ash fails to wordsplit ${branch:+-b $branch...}
-   case $branch in
-   '') git checkout -f -q ;;
-   ?*) git checkout -f -q -B $branch origin/$branch ;;
-   esac
-   ) || die $(eval_gettext Unable to checkout submodule 
'\$sm_path')
+   module_clone $sm_path $sm_name $realrepo $reference 
$depth $branch || exit
fi
git config submodule.$sm_name.url $realrepo
 
@@ -815,7 +814,7 @@ Maybe you want to use 'update --init'?)
 
if ! test -d $sm_path/.git -o -f $sm_path/.git
then
-   module_clone $sm_path $name $url $reference 
$depth || exit
+   module_clone $sm_path $name $url $reference 
$depth $branch || exit
cloned_modules=$cloned_modules;$name
subsha1=
else
@@ -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
+   else
+   update_module=
+   fi
+   ;;
esac
 
must_die_on_failure=
-- 
1.8.5.2.gaa5d535.dirty

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html