Re: [PATCH] git-submodule add: Record branch name in .gitmodules

On Tue, Oct 23, 2012 at 03:44:36PM -0400, W. Trevor King wrote:

On Tue, Oct 23, 2012 at 12:16:22PM -0700, Nahor wrote:

quoted

On 2012-10-22 09:34, W. Trevor King wrote:
For instance, the module may later be updated to a commit in branch B 
instead of branch A. Unless you remember to also update .gitmodule, you 
have then inconsistent information.

But you're explicitly *using* the configured setting in

  git config --file $toplevel/.gitmodules submodule.$name.branch

That should be a reminder that the configuration is important, and
you'll remember to change it.

To make my case more cleanly, people already handle all the
troublesome cases for branch.$name.remote, so handling similar
upstream volatility for submodule.$name.branch should not be too
difficult or surprising.

On Tue, Oct 23, 2012 at 03:58:48PM -0400, Phil Hord wrote:

On Mon, Oct 22, 2012 at 6:55 PM, W. Trevor King [off-list ref] wrote:

quoted

How about -r/--record, with the recorded name being optional?

  --record-branch[=<recorded_name>]

I like that just fine.

quoted

This would satisfy Gerrit users that wanted to use '.', but also
satisfy me with:

  git submodule add -rb=master foo bar

However, there is a change that people would see that, and then use

  git submodule add -r -b=master foo bar

which would checkout the HEAD from foo and store `-b=master` in
submodule.$name.branch.

I don't think it would.

Ah, right, forcing the =<name> attached case would mean they'd have to
use

  git submodule add -r=-b=master

which doesn't sound like the sort of thing you'd do accidentally.

Though I see in rev-parse--parseopts that the use of
optional-argument options "is discouraged".

I'll work up a v2 patch and we'll see if anyone complains ;).

-- 
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 option allows you to record a submodule.<name>.branch option in
.gitmodules.  Git does not currently use this configuration option for
anything, but users have used it for several things, so it makes sense
to add some syntactic sugar for initializing the value.

Current consumers:

Ævar uses this setting to designate the upstream branch for pulling
submodule updates:

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

as he describes 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 this setting to

  “indicate the branch of a submodule project that when updated will
  trigger automatic update of its registered gitlink.” [1]

I'm not clear on what that means, but they accept special values like
'.', so their usage is not compatible with Ævar's proposal.

By remaining agnostic on the variable usage, this patch makes
submodule setup more convenient for all parties.

[1] https://gerrit.googlesource.com/gerrit/+/master/Documentation/user-submodules.txt

Signed-off-by: W. Trevor King <redacted>
---
 Documentation/git-submodule.txt | 11 ++++++++++-
 git-submodule.sh                | 19 ++++++++++++++++++-
 t/t7400-submodule-basic.sh      | 25 +++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

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

@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 --------
 [verse]
-'git submodule' [--quiet] add [-b branch] [-f|--force]
+'git submodule' [--quiet] add [-b branch] [--record[=<branch>]] [-f|--force]
 	      [--reference <repository>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]

@@ -209,6 +209,15 @@ OPTIONS
 --branch::
 	Branch of repository to add as submodule.
 
+-r::
+--record::
+  Record a branch name used as `submodule.<path>.branch` in
+  `.gitmodules` for future reference.  If you do not list an explicit
+  name here, the name given with `--branch` will be recorded.  If that
+  is not set either, `HEAD` will be recorded.  Because the branch name
+  is optional, you must use the equal-sign form (`-r=<branch>`), not
+  `-r <branch>`.
+
 -f::
 --force::
 	This option is only valid for add and update commands.

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

@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
+USAGE="[--quiet] add [-b branch] [--record[=<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>...]

@@ -20,6 +20,8 @@ require_work_tree
 
 command=
 branch=
+record_branch=
+record_branch_empty=
 force=
 reference=
 cached=

@@ -257,6 +259,12 @@ cmd_add()
 			branch=$2
 			shift
 			;;
+		-r | --record)
+			record_branch_empty=true
+			;;
+		-r=* | --record=*)
+			record_branch="${1#*=}"
+			;;
 		-f | --force)
 			force=$1
 			;;

@@ -328,6 +336,11 @@ cmd_add()
 	git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
 	die "$(eval_gettext "'\$sm_path' already exists in the index")"
 
+	if test -z "$record_branch" && test "$record_branch_empty" = "true"
+	then
+		record_branch="${branch:=HEAD}"
+	fi
+
 	if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
 	then
 		eval_gettextln "The following path is ignored by one of your .gitignore files:

@@ -366,6 +379,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 "$record_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..88ae74c 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 -z "$(git config -f .gitmodules submodule.submod-branch.branch)" &&
 		git submodule init
 	) &&
 

@@ -211,6 +212,30 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	test_cmp empty untracked
 '
 
+test_expect_success 'submodule add --record' '
+	(
+		cd addtest &&
+		git submodule add -r "$submodurl" submod-record-head &&
+		test "$(git config -f .gitmodules submodule.submod-record-head.branch)" = "HEAD"
+	)
+'
+
+test_expect_success 'submodule add --record --branch' '
+	(
+		cd addtest &&
+		git submodule add -r -b initial "$submodurl" submod-auto-record &&
+		test "$(git config -f .gitmodules submodule.submod-auto-record.branch)" = "initial"
+	)
+'
+
+test_expect_success 'submodule add --record=<name> --branch' '
+	(
+		cd addtest &&
+		git submodule add -r=final -b initial "$submodurl" submod-record &&
+		test "$(git config -f .gitmodules submodule.submod-record.branch)" = "final"
+	)
+'
+
 test_expect_success 'setup - add an example entry to .gitmodules' '
 	GIT_CONFIG=.gitmodules \
 	git config submodule.example.url git://example.com/init.git

-- 
1.8.0.1.g61a31f6.dirty

I still fail to see what adding that functionality to the submodule
command buys us (unless we also add code which really uses the branch
setting). What's wrong with doing a simple:

   git config -f .gitmodules submodule.<path>.branch <record_branch>

on the command line when you want to use the branch setting for your
own purposes? You could easily wrap that into a helper script, no?

Am 23.10.2012 23:57, schrieb W. Trevor King:

On Wed, Oct 24, 2012 at 09:15:32PM +0200, Jens Lehmann wrote:

I still fail to see what adding that functionality to the submodule
command buys us (unless we also add code which really uses the branch
setting). What's wrong with doing a simple:

   git config -f .gitmodules submodule.<path>.branch <record_branch>

on the command line when you want to use the branch setting for your
own purposes? You could easily wrap that into a helper script, no?

Sure.  But why maintain my own helper script if I can edit
git-submodules.sh?  It seems like a number of people are using this
config option, and they generally store the same name in it that they
use to create the submodule.  This way I can save them time too.

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

Should I rebase this so it lands cleanly atop 38ae92e4 in next?

  commit 38ae92e4d027063b9b87e51a9bf12809d10066f6
  Author: W. Trevor King [off-list ref]
  Date:   Tue Oct 23 17:00:21 2012 -0400

    git-submodule: wrap branch option with "<>" in usage strings.

-- 
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 Thu, Oct 25, 2012 at 06:14:31PM -0400, W. Trevor King wrote:

Should I rebase this so it lands cleanly atop 38ae92e4 in next?

  commit 38ae92e4d027063b9b87e51a9bf12809d10066f6
  Author: W. Trevor King [off-list ref]
  Date:   Tue Oct 23 17:00:21 2012 -0400

    git-submodule: wrap branch option with "<>" in usage strings.

In general, it is not a good idea to base your patches on things in
next, because it means your topic is held hostage to the one in next,
which may or may not graduate to master. We can always do a merge later
(and in this case, it is really just a one-line conflict).

-Peff

Am 25.10.2012 02:53, schrieb W. Trevor King:

On Wed, Oct 24, 2012 at 09:15:32PM +0200, Jens Lehmann wrote:

quoted

I still fail to see what adding that functionality to the submodule
command buys us (unless we also add code which really uses the branch
setting). What's wrong with doing a simple:

   git config -f .gitmodules submodule.<path>.branch <record_branch>

on the command line when you want to use the branch setting for your
own purposes? You could easily wrap that into a helper script, no?

Sure.  But why maintain my own helper script if I can edit
git-submodules.sh?  It seems like a number of people are using this
config option, and they generally store the same name in it that they
use to create the submodule.  This way I can save them time too.

But people are already using the "branch" setting in *different* ways:

Am 23.10.2012 22:55, schrieb W. Trevor King:

As Phil pointed out, doing anything with this variable is ambiguous:

On Mon, Oct 22, 2012 at 06:03:53PM -0400, Phil Hord wrote:

quoted

Some projects now use the 'branch' config value to record the tracking
branch for the submodule.  Some ascribe different meaning to the
configuration if the value is given vs. undefined.  For example, see
the Gerrit submodule-subscription mechanism.  This change will cause
those workflows to behave differently than they do now.

I don't have a problem with the amount or complexity of the code being
added, But by adding that option we may be giving the impression that it
is officially sanctioned, or that it will be kept up to date by further
submodule commands. I added Shawn to the CC, maybe he can comment on how
the "branch" setting is used in Gerrit and what he thinks about adding
code to set that with "git submodule add -r <branch> ..." to core git.

On Sun, Oct 28, 2012 at 09:48:18PM +0100, Jens Lehmann wrote:

Am 25.10.2012 02:53, schrieb W. Trevor King:

quoted

On Wed, Oct 24, 2012 at 09:15:32PM +0200, Jens Lehmann wrote:

quoted

I still fail to see what adding that functionality to the submodule
command buys us (unless we also add code which really uses the branch
setting). What's wrong with doing a simple:

   git config -f .gitmodules submodule.<path>.branch <record_branch>

on the command line when you want to use the branch setting for your
own purposes? You could easily wrap that into a helper script, no?

Sure.  But why maintain my own helper script if I can edit
git-submodules.sh?  It seems like a number of people are using this
config option, and they generally store the same name in it that they
use to create the submodule.  This way I can save them time too.

But people are already using the "branch" setting in *different* ways:

And they are usually storing the same string.  Now, more easily.  If
they want a different string, it is also easier.  If they don't want
to use --record, they can do things however they were already doing
them.  I don't see the problem.

Am 23.10.2012 22:55, schrieb W. Trevor King:

quoted

As Phil pointed out, doing anything with this variable is ambiguous:

On Mon, Oct 22, 2012 at 06:03:53PM -0400, Phil Hord wrote:

quoted

Some projects now use the 'branch' config value to record the tracking
branch for the submodule.  Some ascribe different meaning to the
configuration if the value is given vs. undefined.  For example, see
the Gerrit submodule-subscription mechanism.  This change will cause
those workflows to behave differently than they do now.

I don't have a problem with the amount or complexity of the code being
added, But by adding that option we may be giving the impression that it
is officially sanctioned, or that it will be kept up to date by further
submodule commands.

Storing something there will be officially sanctioned.  Using it for
anything in particular will not be officially sanctioned.  Phil's
submodule_<var-name> export in foreach will expose the variable so the
user can do whatever they think is appropriate with it, but it's still
up to the user to give the option some kind of semantic meaning.

I added Shawn to the CC, maybe he can comment on how the "branch"
setting is used in Gerrit and what he thinks about adding code to
set that with "git submodule add -r <branch> ..." to core git.

Good idea.

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 Sun, Oct 28, 2012 at 1:48 PM, Jens Lehmann [off-list ref] wrote:

Am 23.10.2012 22:55, schrieb W. Trevor King:

quoted

As Phil pointed out, doing anything with this variable is ambiguous:

On Mon, Oct 22, 2012 at 06:03:53PM -0400, Phil Hord wrote:

quoted

Some projects now use the 'branch' config value to record the tracking
branch for the submodule.  Some ascribe different meaning to the
configuration if the value is given vs. undefined.  For example, see
the Gerrit submodule-subscription mechanism.  This change will cause
those workflows to behave differently than they do now.

I don't have a problem with the amount or complexity of the code being
added, But by adding that option we may be giving the impression that it
is officially sanctioned, or that it will be kept up to date by further
submodule commands. I added Shawn to the CC, maybe he can comment on how
the "branch" setting is used in Gerrit and what he thinks about adding
code to set that with "git submodule add -r <branch> ..." to core git.

Looks like the Gerrit meaning is basically the same as Ævar's. Gerrit
updates the parent project as if you had done:

  $ git submodule foreach 'git checkout $(git config --file
$toplevel/.gitmodules submodule.$name.branch) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

and it does this automatically each time the submodule's branch is
modified by the Gerrit server.

On Tue, Oct 23, 2012 at 2:57 PM, W. Trevor King [off-list ref] wrote:

I'm not clear on what that means, but they accept special values like
'.', so their usage is not compatible with Ævar's proposal.

"." is a special value to mean use the parent project's branch name.
So its more like this:

  $ git submodule foreach 'git checkout $(git --git-dir $toplevel/.git
read-ref HEAD | sed s,^refs/heads/,,) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

We use "." in Gerrit to make branching an entire forest of projects
easier. Setting up dev-fix-yy in the parent project will automatically
track dev-fix-yy in each submodule.

On Sun, Oct 28, 2012 at 02:59:33PM -0700, Shawn Pearce wrote:

Looks like the Gerrit meaning is basically the same as Ævar's. Gerrit
updates the parent project as if you had done:

  $ git submodule foreach 'git checkout $(git config --file
$toplevel/.gitmodules submodule.$name.branch) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

Ah, good, then we *are* all using the option for the same thing.

On Tue, Oct 23, 2012 at 2:57 PM, W. Trevor King [off-list ref] wrote:

quoted

I'm not clear on what that means, but they accept special values like
'.', so their usage is not compatible with Ævar's proposal.

"." is a special value to mean use the parent project's branch name.
So its more like this:

  $ git submodule foreach 'git checkout $(git --git-dir $toplevel/.git
read-ref HEAD | sed s,^refs/heads/,,) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

We use "." in Gerrit to make branching an entire forest of projects
easier. Setting up dev-fix-yy in the parent project will automatically
track dev-fix-yy in each submodule.

Ok.  If we wanted "." expansion to be a general submodule thing, it
would add a special case to Phil's submodule_<var-name> export.  I
don't think such a special case would be worth the mental overhead,
but obviously the Gerrit folks think it is.  I don't care either way
on this one.

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 Sun, Oct 28, 2012 at 06:34:31PM -0400, W. Trevor King wrote:

On Sun, Oct 28, 2012 at 02:59:33PM -0700, Shawn Pearce wrote:

quoted

Looks like the Gerrit meaning is basically the same as Ævar's. Gerrit
updates the parent project as if you had done:

  $ git submodule foreach 'git checkout $(git config --file
$toplevel/.gitmodules submodule.$name.branch) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

Ah, good, then we *are* all using the option for the same thing.

That makes me more comfortable. Your patch adds support for setting the
variable initially. Does it need any special magic for maintenance, or
is it something that would always be updated by hand?

Right now, the variable is not an official git-submodule thing and it is
OK to say "you are on your own by setting and using it from external
tools". But as soon as we support setting it in the first place, it is
reasonable to claim it as a bug if we do not keep it up to date for
certain operations.

I'm not familiar enough with the workflows around branching submodules
to know whether any such operations actually exist.

-Peff

On Mon, Oct 29, 2012 at 01:34:01AM -0400, Jeff King wrote:

On Sun, Oct 28, 2012 at 06:34:31PM -0400, W. Trevor King wrote:

quoted

On Sun, Oct 28, 2012 at 02:59:33PM -0700, Shawn Pearce wrote:

quoted

Looks like the Gerrit meaning is basically the same as Ævar's. Gerrit
updates the parent project as if you had done:

  $ git submodule foreach 'git checkout $(git config --file
$toplevel/.gitmodules submodule.$name.branch) && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

Ah, good, then we *are* all using the option for the same thing.

That makes me more comfortable. Your patch adds support for setting the
variable initially. Does it need any special magic for maintenance, or
is it something that would always be updated by hand?

Everyone we've heard from so far interprets the setting as “pull from
$branch in the remote repository $url to update the submodule”.  With
Phil's export, that would become

  $ git submodule foreach 'git checkout "$submodule_branch" && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

As Nahor mentioned on the 23rd, there are a number of ways that the
upstream branch could disappear, but Git has no way to know what the
new branch setting should be.  This means that even if we make “pull
from $branch” interpretation official, we still couldn't do anything
slick about updating it.  So, yes, it will be updated by hand.

-- 
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 Mon, Oct 29, 2012 at 06:45:44AM -0400, W. Trevor King wrote:

quoted

quoted

Ah, good, then we *are* all using the option for the same thing.

That makes me more comfortable. Your patch adds support for setting the
variable initially. Does it need any special magic for maintenance, or
is it something that would always be updated by hand?

Everyone we've heard from so far interprets the setting as “pull from
$branch in the remote repository $url to update the submodule”.  With
Phil's export, that would become

  $ git submodule foreach 'git checkout "$submodule_branch" && git pull'
  $ git commit -a -m "Updated submodules"
  $ git push

As Nahor mentioned on the 23rd, there are a number of ways that the
upstream branch could disappear, but Git has no way to know what the
new branch setting should be.  This means that even if we make “pull
from $branch” interpretation official, we still couldn't do anything
slick about updating it.  So, yes, it will be updated by hand.

OK.

Can you send an updated version of the patch that summarizes the
situation in the commit message?

I also think it is probably worth saying something in the documentation
for the feature like "Note that this value is not actually used by git;
however, some external tools and workflows may make use of it."

-Peff

On Mon, Oct 29, 2012 at 06:58:55AM -0400, Jeff King wrote:

Can you send an updated version of the patch that summarizes the
situation in the commit message?

Sure.  Should I include Phil's $submodule_<var-name> export, or would
you rather have that be a separate series?

Phil, were you planning on rolling your patch into something more
formal, or was your preliminary patch a suggestion for me to build
from?

-- 
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 Mon, Oct 29, 2012 at 07:29:45AM -0400, W. Trevor King wrote:

On Mon, Oct 29, 2012 at 06:58:55AM -0400, Jeff King wrote:

quoted

Can you send an updated version of the patch that summarizes the
situation in the commit message?

Sure.  Should I include Phil's $submodule_<var-name> export, or would
you rather have that be a separate series?

I think it probably makes sense as a separate patch in the same series,
since it is meant to support the same workflows.

I am not sure it is sufficient as-is, though. It does not seem to ever
clear variables, only set them, which means that values could leak
across iterations of the loop, or down to recursive calls. E.g., when
the first submodule has submodule.*.foo set but the second one does not,
you will still end up with $submodule_foo set when you process the
second one.

-Peff

On Mon, Oct 29, 2012 at 7:43 AM, Jeff King [off-list ref] wrote:

On Mon, Oct 29, 2012 at 07:29:45AM -0400, W. Trevor King wrote:

quoted

On Mon, Oct 29, 2012 at 06:58:55AM -0400, Jeff King wrote:

quoted

Can you send an updated version of the patch that summarizes the
situation in the commit message?

Sure.  Should I include Phil's $submodule_<var-name> export, or would
you rather have that be a separate series?

I think it probably makes sense as a separate patch in the same series,
since it is meant to support the same workflows.

I agree.  I did expect to clean it up some, but also to suffer some
review.  Feel free to clean it up as you see fit and submit it with
your series.

I am not sure it is sufficient as-is, though. It does not seem to ever
clear variables, only set them, which means that values could leak
across iterations of the loop,  [...] E.g., when
the first submodule has submodule.*.foo set but the second one does not,
you will still end up with $submodule_foo set when you process the
second one.

Good point.  That should not happen.

or down to recursive calls.

Frankly, I consider that to be a feature.  However, I can see how it
would be considered inconsistent in many ways, so it's probably best
to squash it.  :-\

Phil

On Mon, Oct 29, 2012 at 01:38:28PM -0400, Phil Hord wrote:

quoted

I am not sure it is sufficient as-is, though. It does not seem to ever
clear variables, only set them, which means that values could leak
across iterations of the loop,  [...] E.g., when
the first submodule has submodule.*.foo set but the second one does not,
you will still end up with $submodule_foo set when you process the
second one.

Good point.  That should not happen.

quoted

or down to recursive calls.

Frankly, I consider that to be a feature.  However, I can see how it
would be considered inconsistent in many ways, so it's probably best
to squash it.  :-\

I think it would depend on the semantics of the option. Some options
would probably make sense to apply recursively, and some not.

Maybe instead of blindly converting config into the environment, it
should forward or clear specific known-meaning config.

-Peff

On Mon, Oct 29, 2012 at 5:36 PM, Jeff King [off-list ref] wrote:

On Mon, Oct 29, 2012 at 01:38:28PM -0400, Phil Hord wrote:

quoted

quoted

I am not sure it is sufficient as-is, though. It does not seem to ever
clear variables, only set them, which means that values could leak
across iterations of the loop,  [...] E.g., when
the first submodule has submodule.*.foo set but the second one does not,
you will still end up with $submodule_foo set when you process the
second one.

Good point.  That should not happen.

quoted

or down to recursive calls.

Frankly, I consider that to be a feature.  However, I can see how it
would be considered inconsistent in many ways, so it's probably best
to squash it.  :-\

I think it would depend on the semantics of the option. Some options
would probably make sense to apply recursively, and some not.

Maybe instead of blindly converting config into the environment, it
should forward or clear specific known-meaning config.

Well, that's where we started.  I was aiming for the more generic
"never needs updating" direction.

P

On Mon, Oct 29, 2012 at 06:21:08PM -0400, Phil Hord wrote:

quoted

Maybe instead of blindly converting config into the environment, it
should forward or clear specific known-meaning config.

Well, that's where we started.  I was aiming for the more generic
"never needs updating" direction.

Then I think you are probably stuck taking the conservative approach of
not propagating recursively.

-Peff

From: "W. Trevor King" <redacted>

This option allows you to record a submodule.<name>.branch option in
.gitmodules.  Git does not currently use this configuration option for
anything, but users have used it for several things, so it makes sense
to add some syntactic sugar for initializing the value.

Current consumers:

Ævar uses this setting to designate the upstream branch for pulling
submodule updates:

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

as he describes 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.

By remaining agnostic on the variable usage, this patch makes
submodule setup more convenient for all parties.

[1] https://gerrit.googlesource.com/gerrit/+/master/Documentation/user-submodules.txt

Signed-off-by: W. Trevor King <redacted>
---
 Documentation/git-submodule.txt | 11 ++++++++++-
 git-submodule.sh                | 19 ++++++++++++++++++-
 t/t7400-submodule-basic.sh      | 25 +++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

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

@@ -9,7 +9,7 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 --------
 [verse]
-'git submodule' [--quiet] add [-b branch] [-f|--force]
+'git submodule' [--quiet] add [-b branch] [--record[=<branch>]] [-f|--force]
 	      [--reference <repository>] [--] <repository> [<path>]
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] init [--] [<path>...]

@@ -209,6 +209,15 @@ OPTIONS
 --branch::
 	Branch of repository to add as submodule.
 
+-r::
+--record::
+	Record a branch name used as `submodule.<path>.branch` in
+	`.gitmodules` for future reference.  If you do not list an explicit
+	name here, the name given with `--branch` will be recorded.  If that
+	is not set either, `HEAD` will be recorded.  Because the branch name
+	is optional, you must use the equal-sign form (`-r=<branch>`), not
+	`-r <branch>`.
+
 -f::
 --force::
 	This option is only valid for add and update commands.

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

@@ -5,7 +5,7 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <repository> [<path>]
+USAGE="[--quiet] add [-b branch] [--record[=<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>...]

@@ -20,6 +20,8 @@ require_work_tree
 
 command=
 branch=
+record_branch=
+record_branch_empty=
 force=
 reference=
 cached=

@@ -257,6 +259,12 @@ cmd_add()
 			branch=$2
 			shift
 			;;
+		-r | --record)
+			record_branch_empty=true
+			;;
+		-r=* | --record=*)
+			record_branch="${1#*=}"
+			;;
 		-f | --force)
 			force=$1
 			;;

@@ -328,6 +336,11 @@ cmd_add()
 	git ls-files --error-unmatch "$sm_path" > /dev/null 2>&1 &&
 	die "$(eval_gettext "'\$sm_path' already exists in the index")"
 
+	if test -z "$record_branch" && test "$record_branch_empty" = "true"
+	then
+		record_branch="${branch:=HEAD}"
+	fi
+
 	if test -z "$force" && ! git add --dry-run --ignore-missing "$sm_path" > /dev/null 2>&1
 	then
 		eval_gettextln "The following path is ignored by one of your .gitignore files:

@@ -366,6 +379,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 "$record_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..88ae74c 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 -z "$(git config -f .gitmodules submodule.submod-branch.branch)" &&
 		git submodule init
 	) &&
 

@@ -211,6 +212,30 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	test_cmp empty untracked
 '
 
+test_expect_success 'submodule add --record' '
+	(
+		cd addtest &&
+		git submodule add -r "$submodurl" submod-record-head &&
+		test "$(git config -f .gitmodules submodule.submod-record-head.branch)" = "HEAD"
+	)
+'
+
+test_expect_success 'submodule add --record --branch' '
+	(
+		cd addtest &&
+		git submodule add -r -b initial "$submodurl" submod-auto-record &&
+		test "$(git config -f .gitmodules submodule.submod-auto-record.branch)" = "initial"
+	)
+'
+
+test_expect_success 'submodule add --record=<name> --branch' '
+	(
+		cd addtest &&
+		git submodule add -r=final -b initial "$submodurl" submod-record &&
+		test "$(git config -f .gitmodules submodule.submod-record.branch)" = "final"
+	)
+'
+
 test_expect_success 'setup - add an example entry to .gitmodules' '
 	GIT_CONFIG=.gitmodules \
 	git config submodule.example.url git://example.com/init.git

-- 
1.8.0.3.gc2eb43a

From: "W. Trevor King" <redacted>

Signed-off-by: W. Trevor King <redacted>
---
 Documentation/git-submodule.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9a99826..d4e993f 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt

@@ -220,6 +220,14 @@ OPTIONS
 	is not set either, `HEAD` will be recorded.  Because the branch name
 	is optional, you must use the equal-sign form (`-r=<branch>`), not
 	`-r <branch>`.
++
+The recorded setting is not actually used by git; however, some
+external tools and workflows may make use of it.  For example, if the
+upstream branches still exist and you have a recorded branch setting
+for each of your submodules, you can update all of the submodules to
+the current branch tips with:
++
+	git submodule foreach 'git checkout $submodule_branch && git pull'
 
 -f::
 --force::

-- 
1.8.0.3.gc2eb43a

From: "W. Trevor King" <redacted>

Here's my revised patch.  Changes from v2:

* Revised Ævar-vs-Gerrit usage to show agreement, following Shawn's
  comments.
* Added a cleaned up version of Phil's $submodule_* export patch, with
  docs and tests.
* Added a caveat to the -r/--record documentation to make it explicit
  that submodule.<name>.branch is not used internally by Git.  Give an
  example of how the user may use it explicitly for Ævar-style
  updates.

W. Trevor King (3):
  git-submodule add: Add -r/--record option
  git-submodule foreach: export .gitmodules settings as variables
  git-submodule: Motivate --record with an example use case

 Documentation/git-submodule.txt | 22 +++++++++++++++++++++-
 git-sh-setup.sh                 | 20 ++++++++++++++++++++
 git-submodule.sh                | 35 ++++++++++++++++++++++++++++++++++-
 t/t7400-submodule-basic.sh      | 25 +++++++++++++++++++++++++
 t/t7407-submodule-foreach.sh    | 29 +++++++++++++++++++++++++++++
 5 files changed, 129 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 git-sh-setup.sh

-- 
1.8.0.3.gc2eb43a

From: "W. Trevor King" <redacted>

This makes it easy to access per-submodule variables.  For example,

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

can now be reduced to

  git submodule foreach 'git checkout $submodule_branch && git pull'

Every submodule.<name>.<opt> setting from .gitmodules is available as
a $submodule_<sanitized-opt> variable.  These variables are not
propagated recursively into nested submodules.

Signed-off-by: W. Trevor King <redacted>
Based-on-patch-by: Phil Hord [off-list ref]
---
 Documentation/git-submodule.txt |  3 +++
 git-sh-setup.sh                 | 20 ++++++++++++++++++++
 git-submodule.sh                | 16 ++++++++++++++++
 t/t7407-submodule-foreach.sh    | 29 +++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)
 mode change 100644 => 100755 git-sh-setup.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index cbec363..9a99826 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt

@@ -175,6 +175,9 @@ foreach::
 	$path is the name of the submodule directory relative to the
 	superproject, $sha1 is the commit as recorded in the superproject,
 	and $toplevel is the absolute path to the top-level of the superproject.
+	In addition, every submodule.<name>.<opt> setting from .gitmodules
+	is available as the variable $submodule_<sanitized_opt>.  These
+	variables are not propagated recursively into nested submodules.
 	Any submodules defined in the superproject but not checked out are
 	ignored by this command. Unless given `--quiet`, foreach prints the name
 	of each submodule before evaluating the command.

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
old mode 100644
new mode 100755
index ee0e0bc..179a920
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh

@@ -222,6 +222,26 @@ clear_local_git_env() {
 	unset $(git rev-parse --local-env-vars)
 }
 
+# Remove any suspect characters from a user-generated variable name.
+sanitize_variable_name() {
+	VAR_NAME="$1"
+	printf '%s' "$VAR_NAME" |
+	sed -e 's/^[^a-zA-Z]/_/' -e 's/[^a-zA-Z0-9]/_/g'
+}
+
+# Return a command for setting a new variable.
+# Neither the variable name nor the variable value passed to this
+# function need to be sanitized.  You need to eval the returned
+# string, because new variables set by the function itself don't
+# effect the calling process.
+set_user_variable() {
+	VAR_NAME="$1"
+	VAR_VALUE="$2"
+	VAR_NAME=$(sanitize_variable_name "$VAR_NAME")
+	VAR_VALUE=$(printf '%s' "$VAR_VALUE" |
+		sed -e 's/\\/\\\\/g' -e 's/"/\\"/g')
+	printf '%s=%s;\n' "$VAR_NAME" "\"$VAR_VALUE\""
+}
 
 # Platform specific tweaks to work around some commands
 case $(uname -s) in

diff --git a/git-submodule.sh b/git-submodule.sh
index bc33112..e4d26f9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -434,8 +434,24 @@ cmd_foreach()
 				clear_local_git_env
 				# we make $path available to scripts ...
 				path=$sm_path
+
+				# make all submodule variables available to scripts
+				eval $(
+					git config -f .gitmodules --get-regexp "^submodule\.${name}\..*" |
+					sed -e "s|^submodule\.${name}\.||" |
+					while read VAR_NAME VAR_VALUE ; do
+						VAR_NAME=$(printf '%s' "$VAR_NAME" | tr A-Z a-z)
+						set_user_variable "submodule_${VAR_NAME}" "$VAR_VALUE"
+					done)
+				UNSET_CMD=$(set |
+					sed -n -e 's|^\(submodule_[a-z_]*\)=.*$|\1|p' |
+					while read VAR_NAME ; do
+						printf 'unset %s;\n' "$VAR_NAME"
+					done)
+
 				cd "$sm_path" &&
 				eval "$@" &&
+				eval "$UNSET_CMD" &&
 				if test -n "$recursive"
 				then
 					cmd_foreach "--recursive" "$@"

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..46ac746 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh

@@ -313,4 +313,33 @@ test_expect_success 'command passed to foreach --recursive retains notion of std
 	test_cmp expected actual
 '
 
+cat > expect <<EOF
+Entering 'nested1'
+nested1 nested1 wonky"value
+Entering 'nested1/nested2'
+nested2 nested2 another wonky"value
+Entering 'nested1/nested2/nested3'
+nested3 nested3
+Entering 'nested1/nested2/nested3/submodule'
+submodule submodule
+Entering 'sub1'
+sub1 sub1
+Entering 'sub2'
+sub2 sub2
+Entering 'sub3'
+sub3 sub3
+EOF
+
+test_expect_success 'test foreach environment variables' '
+	(
+		cd clone2 &&
+		git config -f .gitmodules submodule.nested1.wonky-var "wonky\"value" &&
+		git config -f nested1/.gitmodules submodule.nested2.wonky-var "another wonky\"value" &&
+		git submodule foreach --recursive "echo \$path \$submodule_path \$submodule_wonky_var" > ../actual
+	) &&
+	test_i18ncmp expect actual
+'
+#
+#"echo \$toplevel-\$name-\$submodule_path-\$submodule_url"
+
 test_done

-- 
1.8.0.3.gc2eb43a

Hi,

On Thu, Nov 08, 2012 at 10:35:13PM -0500, W. Trevor King wrote:

From: "W. Trevor King" <redacted>

This makes it easy to access per-submodule variables.  For example,

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

can now be reduced to

  git submodule foreach 'git checkout $submodule_branch && git pull'

What other use cases are there? Would the need for this maybe go away
once you had floating submodules following branches?

The whole thing looks like its adding some complex code which is not so
easy to read. I would like to make sure its worth it.

For completeness you should make the variables possible to override by
repository from the local repository configuration like all other
submodule options that are read directly from .gitmodules.

Cheers Heiko

On Fri, Nov 09, 2012 at 05:45:22PM +0100, Heiko Voigt wrote:

quoted

can now be reduced to

  git submodule foreach 'git checkout $submodule_branch && git pull'

What other use cases are there? Would the need for this maybe go away
once you had floating submodules following branches?

None that I can think of, but I don't use submodules very much.  The
idea of easily-accessible per-submodule configuration variables
strikes me as pretty useful, but I agree the code is a bit ugly.
Actually, I think exporting environment variables and calling the
foreach command in a subshell would be better than the current local
variables and eval.  The subshell would also make variable cleanup
irrelevant, which would make for a cleaner patch.

For completeness you should make the variables possible to override by
repository from the local repository configuration like all other
submodule options that are read directly from .gitmodules.

Good idea (I wasn't aware of the override before).  Will do in v4.

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