Re: [PATCH v4 0/4] git-submodule add: Add --local-branch option (summary)

On Tue, Nov 27, 2012 at 09:42:05PM -0500, W. Trevor King wrote:

On Wed, Nov 28, 2012 at 12:28:58AM +0100, Heiko Voigt wrote:

quoted

https://github.com/hvoigt/git/commits/hv/floating_submodules_draft

I looked over this before, but maybe not thoroughly enough ;).

Heiko pointed out that I likely hadn't looked at this branch at all,
which is true.  I'd confused it with his earlier:

On Fri, Nov 09, 2012 at 05:29:27PM +0100, Heiko Voigt wrote:

https://github.com/hvoigt/git/commits/hv/floating_submodules

but the new floating_submodules_draft branch adds Heiko's function
reorganization and floating submodule pull (with 'update --branch') on
top of my v4 commits (instead of my branch-checkout with 'update
--branch').

This looks fine, but my current --branch implementation (which doesn't
pull) is only a thin branch-checkout layer on top of the standard
`update` functionality.  I'm still unsure if built-in pulls are worth
the configuration trouble.  I'll sleep on it.  Maybe I'll feel better
about them tomorrow ;).

I do feel better about them today.  To get a better feel for how
people see this going forward, here is a summary of proposals to date:

v1. Record submodule.<name>.branch with 'submodule add --branch …',
    leave interpretation up to the user.

    Feedback (paraphrased):
      Phil Hord: that clobbers a configure option used by Gerritt and
        possibly others, rename to --record-branch.  Maybe we should
        export submodule.<name>.* as $submodule_* in 'foreach'?
      Nahor: what about corner cases (e.g. upstream renames branches)?
      Jens Lehmann: if you record it, people will expect Git to use
        it.  What about automatic pulls & commits?

v2. Add --record instead of using --branch to address Phil's v1
    comment.

    Feedback (paraphrased):
      Jens Lehmann: this is still a tiny optimization over using 'git
        config'.  People still use this option in different ways.
      Shawn Pearce: the Gerrit use is basically the same as Ævar's,
        but Gerrit has special handling for '.'.
      Jeff King: if we set up submodule.<name>.branch, we should
        either use it in Git, or make it very clear that we do not use
        it.  If we use Phil's $submodule_* export, we should clear the
        variables for recursive submodules.

v3. Renamed Added $submodule_* export to v2.  Give an example of
    Ævar's pull workflow when explaining that Git does not use the
    value internally.

    Feedback (paraphrased):
      Heiko Voigt: what about automatic pulls & commits?  You should
        allow .git/config overrides of the .gitmodules settings.  if
        we set up submodule.<name>.branch, we should use it in Git.
        How does the submodule know which remote branch to track?
        Junio's proposed 'diff' changes may hide $sha1 information.
      Junio C Hamano: if we set up submodule.<name>.branch, we should
        use it in Git.  Use something more specific than --record.
        Update 'git diff [$path]' and friends in the superproject
        compares the HEAD of the checkout of the submodule at $path
        with the tip of the branch named by submodule.$name.branch in
        .gitmodules of the superproject, instead of the commit that is
        recorded in the index of the superproject.
      Sascha Cunz: you can use 'git shortlog $OLD_SHA1..$NEW_SHA1' for
        the automatic commit message.
      Trevor King: actually, Ævar's update command only specifies the
        local branch name.  The remote is recorded for that branch in
        .git/modules/<name>/config.

v4. Rename --record to --local-branch.  Remove $submodule_* export.
    Add .git/config overrides for submodule.<name>.branch.  Add
    'submodule update --branch' to check out $sha1 as
    submodule.<name>.branch.

    Feedback (paraphrased):
      Heiko Voigt: who cares about the local branch name?  You should
        be pulling origin/$branch, but still using
        .git/modules/<name>/config to record the tracked branch.  You
        should also use 'submodule add --branch[=branch]' instead of
        '--local-branch'.  You should pull the 'update --branch'
        functionality out into a sub-function like
        'handle_tracked_branch_update'.
      Jens Lehmann: .git/config overrides are nice, but they should be
        optional.
      Trevor King: floating branches are following the submodule's
        remote, while 'update --rebase/--merge' are following the
        superproject's recorded $sha1.  Bundling remote-following and
        superproject-following in the same command may be confusing.

Here's what I think we need in v5:

* Jens' optional .git/config overrides.
* Return --local-branch handling to a side effect of --branch (as in
  v1).
* A new 'submodule pull' for tracking the submodule's remote, which is
  pulling --ff-only origin/$branch into a whatever state the submodule
  is currently in.  If any changes were made to submodule $shas,
  optionally commit them with a shortlog-generated commit message.
* Remove my current 'submodule update --branch' functionality.

Thoughs?  Do we need any more information for an automatic pull, or is

  $ git pull --ff-only origin/$branch

on a headless checkout sufficient?

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

On Wed, Nov 28, 2012 at 08:09:03AM -0500, W. Trevor King wrote:

* A new 'submodule pull' for tracking the submodule's remote, which is
  pulling --ff-only origin/$branch into a whatever state the submodule
  is currently in.  If any changes were made to submodule $shas,
  optionally commit them with a shortlog-generated commit message.

I thought of a better idea on the train.  How about adding `--remote`
to `submodule update` that overrides the gitlinked SHA-1 with the
SHA-1 for origin/$branch?  All of the other checkout/merge/rebase
functionality is untouched.  The only thing that changes is where we
look for the update.  I'm working up the patch now, and will submit v5
shortly.

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

From: "W. Trevor King" <redacted>

This allows you to easily record a submodule.<name>.branch option in
.gitmodules when you add a new submodule.  With this patch,

  $ git submodule add -b <branch> <repository> [<path>]
  $ git config -f .gitmodules submodule.<path>.branch <branch>

reduces to

  $ git submodule add -b <branch> <repository> [<path>]

This means that future calls to

  $ git submodule update --remote ...

will get updates from the same branch that you used to initialize the
submodule, which is usually what you want.

Signed-off-by: W. Trevor King <redacted>
---
 Documentation/git-submodule.txt | 2 ++
 git-submodule.sh                | 4 ++++
 t/t7400-submodule-basic.sh      | 1 +
 3 files changed, 7 insertions(+)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 39aa02d..43e5b4b 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt

@@ -208,6 +208,8 @@ OPTIONS
 -b::
 --branch::
 	Branch of repository to add as submodule.
+	The name of the branch is recorded as `submodule.<path>.branch` in
+	`.gitmodules` for `update --remote`.
 
 -f::
 --force::

diff --git a/git-submodule.sh b/git-submodule.sh
index b63d869..1f76893 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -368,6 +368,10 @@ Use -f if you really want to add it." >&2
 
 	git config -f .gitmodules submodule."$sm_path".path "$sm_path" &&
 	git config -f .gitmodules submodule."$sm_path".url "$repo" &&
+	if test -n "$branch"
+	then
+		git config -f .gitmodules submodule."$sm_path".branch "$branch"
+	fi &&
 	git add --force .gitmodules ||
 	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
 }

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5397037..90e2915 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -133,6 +133,7 @@ test_expect_success 'submodule add --branch' '
 	(
 		cd addtest &&
 		git submodule add -b initial "$submodurl" submod-branch &&
+		test "initial" = "$(git config -f .gitmodules submodule.submod-branch.branch)" &&
 		git submodule init
 	) &&
 

-- 
1.8.0.2.gad10246.dirty

From: "W. Trevor King" <redacted>

On Wed, Nov 28, 2012 at 11:53:34AM -0500, W. Trevor King wrote:

I thought of a better idea on the train.  How about adding `--remote`
to `submodule update` that overrides the gitlinked SHA-1 with the
SHA-1 for origin/$branch?  All of the other checkout/merge/rebase
functionality is untouched.  The only thing that changes is where we
look for the update.  I'm working up the patch now, and will submit v5
shortly.

Here it is.

Changes since v4:

* Remove `update --branch` in favor of the new `update --remote` logic
  as mentioned above.
* Optional config overrides for submodule.<name>.branch (as suggested
  by Jens)
* Save --branch as submodule.<name>.branch instead of requiring
  --local-branch.
* Restructure doc edits.

I'm a lot happier with this two-patch proposal.  The first patch
implements --remote and documents submodule.<name>.branch.  The second
patch just gives you a convenient way to set it.  I haven't heard from
anyone other than Heiko recently about the --branch/--remote-branch
choice, so I returned to the simpler --branch side effect storage from
v1.  I'd be happy to submit v6 with explicit --remote-branch recording
if desired.

W. Trevor King (2):
  submodule update: add --remote for submodule's upstream changes
  submodule add: If --branch is given, record it in .gitmodules

 Documentation/config.txt        |  9 +++++----
 Documentation/git-submodule.txt | 26 +++++++++++++++++++++++++-
 Documentation/gitmodules.txt    |  5 +++++
 git-submodule.sh                | 30 +++++++++++++++++++++++++++++-
 t/t7400-submodule-basic.sh      |  1 +
 t/t7406-submodule-update.sh     | 31 +++++++++++++++++++++++++++++++
 6 files changed, 96 insertions(+), 6 deletions(-)

-- 
1.8.0.2.gad10246.dirty

From: "W. Trevor King" <redacted>

The current `update` command incorporates the superproject's gitlinked
SHA-1 ($sha1) into the submodule HEAD ($subsha1).  Depending on the
options you use, it may checkout $sha1, rebase the $subsha1 onto
$sha1, or merge $sha1 into $subsha1.  This helps you keep up with
changes in the upstream superproject.

However, it's also useful to stay up to date with changes in the
upstream subproject.  Previous workflows for incorporating such
changes include the ungainly:

  $ git submodule foreach 'git checkout $(git config --file $toplevel/.gitmodules submodule.$name.branch) && git pull'

With this patch, all of the useful functionality for incorporating
superproject changes can be reused to incorporate upstream subproject
updates.  When you specify --remote, the target $sha1 is replaced with
a $sha1 of the submodule's origin/master tracking branch.  If you want
to merge a different tracking branch, you can configure the
`submodule.<name>.branch` option in `.gitmodules`.  You can override
the `.gitmodules` configuration setting for a particular superproject
by configuring the option in that superproject's default configuration
(using the usual configuration hierarchy, e.g. `.git/config`,
`~/.gitconfig`, etc.).

Previous use of submodule.<name>.branch
=======================================

Because we're adding a new configuration option, it's a good idea to
check if anyone else is already using the option.  The foreach-pull
example above was described by Ævar in

  commit f030c96d8643fa0a1a9b2bd9c2f36a77721fb61f
  Author: Ævar Arnfjörð Bjarmason [off-list ref]
  Date:   Fri May 21 16:10:10 2010 +0000

    git-submodule foreach: Add $toplevel variable

Gerrit uses the same interpretation for the setting, but because
Gerrit has direct access to the subproject repositories, it updates
the superproject repositories automatically when a subproject changes.
Gerrit also accepts the special value '.', which it expands into the
superproject's branch name.

Although the --remote functionality is using `submodule.<name>.branch`
slightly differently, the effect is the same.  The foreach-pull
example uses the option to record the name of the local branch to
checkout before pulls.  The tracking branch to be pulled is recorded
in `.git/modules/<name>/config`, which was initialized by the module
clone during `submodule add` or `submodule init`.  Because the branch
name stored in `submodule.<name>.branch` was likely the same as the
branch name used during the initial `submodule add`, the same branch
will be pulled in each workflow.

Implementation details
======================

In order to ensure a current tracking branch state, `update --remote`
fetches the submodule's remote repository before calculating the
SHA-1.  However, I didn't change the logic guarding the existing fetch:

  if test -z "$nofetch"
  then
    # Run fetch only if $sha1 isn't present or it
    # is not reachable from a ref.
    (clear_local_git_env; cd "$path" &&
      ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
       test -z "$rev") || git-fetch)) ||
    die "$(eval_gettext "Unable to fetch in submodule path '\$path'")"
  fi

There will not be a double-fetch, because the new $sha1 determined
after the `--remote` triggered fetch should always exist in the
repository.  If it doesn't, it's because some racy process removed it
from the submodule's repository and we *should* be re-fetching.

Signed-off-by: W. Trevor King <redacted>
---
 Documentation/config.txt        |  9 +++++----
 Documentation/git-submodule.txt | 24 +++++++++++++++++++++++-
 Documentation/gitmodules.txt    |  5 +++++
 git-submodule.sh                | 26 +++++++++++++++++++++++++-
 t/t7406-submodule-update.sh     | 31 +++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 11f320b..de39b1c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1994,10 +1994,11 @@ status.submodulesummary::
 submodule.<name>.path::
 submodule.<name>.url::
 submodule.<name>.update::
-	The path within this project, URL, and the updating strategy
-	for a submodule.  These variables are initially populated
-	by 'git submodule init'; edit them to override the
-	URL and other values found in the `.gitmodules` file.  See
+submodule.<name>.branch::
+	The path within this project, URL, the updating strategy, and the
+	remote branch name for a submodule.  These variables are initially
+	populated by 'git submodule init'; edit them to override the URL and
+	other values found in the `.gitmodules` file.  See
 	linkgit:git-submodule[1] and linkgit:gitmodules[5] for details.
 
 submodule.<name>.fetchRecurseSubmodules::

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index b4683bb..39aa02d 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt

@@ -13,7 +13,7 @@ SYNOPSIS
 	      [--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] [--remote] [-N|--no-fetch] [--rebase]
 	      [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]

@@ -236,6 +236,28 @@ OPTIONS
 	(the default). This limit only applies to modified submodules. The
 	size is always limited to 1 for added/deleted/typechanged submodules.
 
+--remote::
+	This option is only valid for the update command.
+	Instead of using the superproject's recorded SHA-1 to update the
+	submodule, use the status of the submodule's remote tracking branch.
+	The remote tracking branch defaults to origin/master, but the branch
+	name may be overriden by setting the `submodule.<name>.branch`
+	option in either `.gitmodules` or `.git/config` (with `.git/config`
+	taking precedence).
++
+This works for any of the supported update procedures (`--checkout`,
+`--rebase`, etc.).  The only change is the source of the target SHA-1.
+For example, `submodule update --remote --merge` will merge upstream
+submodule changes into the submodules, while `submodule update
+--merge` will merge superproject gitlink changes into the submodules.
++
+In order to ensure a current tracking branch state, `update --remote`
+fetches the submodule's remote repository before calculating the
+SHA-1.  This makes `submodule update --remote --merge` similar to
+running `git pull` in the submodule.  If you don't want to fetch (for
+something closer to `git merge`), you should use `submodule update
+--remote --no-fetch --merge`.
+
 -N::
 --no-fetch::
 	This option is only valid for the update command.

diff --git a/Documentation/gitmodules.txt b/Documentation/gitmodules.txt
index 4effd78..4004fa6 100644
--- a/Documentation/gitmodules.txt
+++ b/Documentation/gitmodules.txt

@@ -47,6 +47,11 @@ submodule.<name>.update::
 	This config option is overridden if 'git submodule update' is given
 	the '--merge', '--rebase' or '--checkout' options.
 
+submodule.<name>.branch::
+	A remote branch name for tracking updates in the upstream submodule.
+	If the option is not specified, it defaults to 'master'.  See the
+	`--remote` documentation in linkgit:git-submodule[1] for details.
+
 submodule.<name>.fetchRecurseSubmodules::
 	This option can be used to control recursive fetching of this
 	submodule. If this option is also present in the submodules entry in

diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..b63d869 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -8,7 +8,8 @@ dashless=$(basename "$0" | sed -e 's/-/ /')
 USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
    or: $dashless [--quiet] status [--cached] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] init [--] [<path>...]
-   or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
+ges
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--] [<path>...]"

@@ -26,6 +27,7 @@ cached=
 recursive=
 init=
 files=
+remote=
 nofetch=
 update=
 prefix=

@@ -509,6 +511,9 @@ cmd_update()
 		-i|--init)
 			init=1
 			;;
+		--remote)
+			remote=1
+			;;
 		-N|--no-fetch)
 			nofetch=1
 			;;

@@ -569,6 +574,12 @@ cmd_update()
 		fi
 		name=$(module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
+		branch=$(git config submodule."$name".branch)
+		if test -z "$branch"
+		then  # fall back on .gitmodules
+			branch=$(git config -f .gitmodules submodule."$name".branch)
+		fi
+		branch="${branch:-master}"
 		if ! test -z "$update"
 		then
 			update_module=$update

@@ -603,6 +614,19 @@ Maybe you want to use 'update --init'?")"
 			die "$(eval_gettext "Unable to find current revision in submodule path '\$sm_path'")"
 		fi
 
+		if test -n "$remote"
+		then
+			if test -z "$nofetch"
+			then
+				# Fetch remote before determining tracking $sha1
+				(clear_local_git_env; cd "$sm_path" && git-fetch) ||
+				die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
+			fi
+			sha1=$(clear_local_git_env; cd "$sm_path" &&
+				git rev-parse --verify origin/"$branch") ||
+			die "$(eval_gettext "Unable to find current origin/$branch revision in submodule path '\$sm_path'")"
+		fi
+
 		if test "$subsha1" != "$sha1" -o -n "$force"
 		then
 			subforce=$force

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1542653..a567834 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh

@@ -135,6 +135,37 @@ test_expect_success 'submodule update --force forcibly checks out submodules' '
 	)
 '
 
+test_expect_success 'submodule update --remote should fetch upstream changes' '
+	(cd submodule &&
+	 echo line4 >> file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m "upstream line4"
+	) &&
+	(cd super &&
+	 git submodule update --remote --force submodule &&
+	 cd submodule &&
+	 test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline)"
+	)
+'
+
+test_expect_success 'local config should override .gitmodules branch' '
+	(cd submodule &&
+	 git checkout -b test-branch &&
+	 echo line5 >> file &&
+	 git add file &&
+	 test_tick &&
+	 git commit -m "upstream line5" &&
+	 git checkout master
+	) &&
+	(cd super &&
+	 git config submodule.submodule.branch test-branch &&
+	 git submodule update --remote --force submodule &&
+	 cd submodule &&
+	 test "$(git log -1 --oneline)" = "$(GIT_DIR=../../submodule/.git git log -1 --oneline test-branch)"
+	)
+'
+
 test_expect_success 'submodule update --rebase staying on master' '
 	(cd super/submodule &&
 	  git checkout master

-- 
1.8.0.2.gad10246.dirty