[PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Jay Soffian]

Teach "git submodule foreach" a --revision <tree-ish> option. This
is useful in combination with $sha1 to perform git commands that
take a revision argument. For example:

  $ git submodule foreach --revision v1.0 'git tag v1.0 $sha1'

Previously, this would have required multiple steps:

  $ git checkout v1.0
  $ git submodule update
  $ git submodule foreach 'git tag v1.0'

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 Documentation/git-submodule.txt |  7 ++++++-
 git-submodule.sh                | 27 ++++++++++++++++++++++++---
 t/t7407-submodule-foreach.sh    | 15 +++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index b4683bba1b..6c889f5fd6 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -17,7 +17,8 @@ SYNOPSIS
 	      [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) <n>]
 	      [commit] [--] [<path>...]
-'git submodule' [--quiet] foreach [--recursive] <command>
+'git submodule' [--quiet] foreach [--recursive] [--revision <tree-ish>]
+	      <command>
 'git submodule' [--quiet] sync [--] [<path>...]
 
 
@@ -180,6 +181,10 @@ foreach::
 	of each submodule before evaluating the command.
 	If `--recursive` is given, submodules are traversed recursively (i.e.
 	the given shell command is evaluated in nested submodules as well).
+	If `--revision <tree-ish>` is given, submodules are traversed starting
+	at the given <tree-ish>. Though this does not alter the submodule check
+	outs, it may be combined with $sha1 to perform git commands that can
+	operate	on a particular commit, such as linkgit:git-tag[1].
 	A non-zero return from the command in any submodule causes
 	the processing to terminate. This can be overridden by adding '|| :'
 	to the end of the command.
diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b1107b6..5e7458e155 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -10,7 +10,7 @@ USAGE="[--quiet] add [-b branch] [-f|--force] [--reference <repository>] [--] <r
    or: $dashless [--quiet] init [--] [<path>...]
    or: $dashless [--quiet] update [--init] [-N|--no-fetch] [-f|--force] [--rebase] [--reference <repository>] [--merge] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
-   or: $dashless [--quiet] foreach [--recursive] <command>
+   or: $dashless [--quiet] foreach [--recursive] [--revision <tree-ish>] <command>
    or: $dashless [--quiet] sync [--] [<path>...]"
 OPTIONS_SPEC=
 . git-sh-setup
@@ -379,6 +379,7 @@ Use -f if you really want to add it." >&2
 cmd_foreach()
 {
 	# parse $args after "submodule ... foreach".
+	revision=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -388,6 +389,11 @@ cmd_foreach()
 		--recursive)
 			recursive=1
 			;;
+		--revision)
+			git rev-parse --quiet --verify "$2" >/dev/null || usage
+			revision=$2
+			shift
+			;;
 		-*)
 			usage
 			;;
@@ -404,7 +410,17 @@ cmd_foreach()
 	# command in the subshell (and a recursive call to this function)
 	exec 3<&0
 
-	module_list |
+	if test -n "$revision"
+	then
+		# make ls-tree output look like ls-files output
+		git ls-tree -r $revision | grep '^160000 ' |
+		while read mode unused sha1 sm_path
+		do
+			echo "$mode $sha1 0 $sm_path"
+		done
+	else
+		module_list
+	fi |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"
@@ -421,7 +437,12 @@ cmd_foreach()
 				eval "$@" &&
 				if test -n "$recursive"
 				then
-					cmd_foreach "--recursive" "$@"
+					if test -n "$revision"
+					then
+						cmd_foreach "--recursive" "--revision" "$sha1" "$@"
+					else
+						cmd_foreach "--recursive" "$@"
+					fi
 				fi
 			) <&3 3<&- ||
 			die "$(eval_gettext "Stopping at '\$sm_path'; script returned non-zero status.")"
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2e14..5c798b901b 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -179,6 +179,21 @@ test_expect_success 'test "foreach --quiet --recursive"' '
 	test_cmp expect actual
 '
 
+sha1=$(cd submodule && git rev-parse HEAD~1)
+cat > expect <<EOF
+sub1 $sha1
+sub2 $sha1
+sub3 $sha1
+EOF
+
+test_expect_success 'test "foreach --quiet --revision"' '
+	(
+		cd clone2 &&
+		git submodule foreach -q --revision HEAD~2 "echo \$path \$sha1" > ../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'use "update --recursive" to checkout all submodules' '
 	git clone super clone3 &&
 	(
-- 
1.7.12.2

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> Teach "git submodule foreach" a --revision <tree-ish> option. This
> is useful in combination with $sha1 to perform git commands that
> take a revision argument.

The above says:

 - "--revision T" is added.

   OK.  There is no information whatsoever what it does to convince
   us why it is useful.

 - This is useful.

   Huh?  How can anybody supposed to agree or disagree with that
   claim, when nothing is said about what it does in the first
   place?

> For example:
>
>   $ git submodule foreach --revision v1.0 'git tag v1.0 $sha1'

Whose "v1.0" does this example refer to?

The first line of the proposed log message says it is <tree-ish>,
which means that you can safely substitute "--revision T" with
"--revision $(git rev-parse T^{tree}), so it must name a concrete
single object that is a tree (not a tree-ish).  In which repository
is that object found?  The top-level superproject?  All submodule
repositories share the same object store with the superproject?

The description doesn't make _any_ sense to me. The feature might be
something worth considering about with a better description, but
with the above, I can't tell if it is.

> +	If `--revision <tree-ish>` is given, submodules are traversed starting
> +	at the given <tree-ish>.

What does "are traversed starting at the given <tree-ish>"?  The
desired or expected state of each submodule is recorded as a commit
object name (not even commit-ish) in its superproject.  Did you mean
"commit-ish"?

> + Though this does not alter the submodule check
> +	outs, it may be combined with $sha1 to perform git commands that can
> +	operate	on a particular commit, such as linkgit:git-tag[1].

Here is what I am guessing, partially with help from the horrible example:

>   $ git submodule foreach --revision v1.0 'git tag v1.0 $sha1'
>
> Previously, this would have required multiple steps:
>
>   $ git checkout v1.0
>   $ git submodule update
>   $ git submodule foreach 'git tag v1.0'

where there appears two v1.0 that are used for totally different
purposes which does not help guessing.  Perhaps "--revision" names a
tree-ish taken from the top-level superproject, and for each
submodule that appear in the tree in the superproject, the command
specified by foreach is run with the usual $sha1, $name, $path set
to the state in the submodules that top-level tree wants to have,
and this is done without actually checking anything out.  So the
first v1.0 in that confusing example is about specifying a tree in
the superproject repository, and the second v1.0 does not have any
relationship with that first v1.0 (the first one could have been HEAD~2
when you have committed twice in the superproject since you tagged v1.0
and remembered that you forgot to tag its submodules).

Assuming that the above guess is correct (which is a huge
assumption, given the lack of clarity in the description), I think
the feature might make sense.  The example would have been a lot
easier to follow if it were something like this:

    $ git submodule foreach --revision v1.0 'git grep -e frotz $sha1'

> @@ -379,6 +379,7 @@ Use -f if you really want to add it." >&2
>  cmd_foreach()
>  {
>  	# parse $args after "submodule ... foreach".
> +	revision=
>  	while test $# -ne 0
>  	do
>  		case "$1" in
> @@ -388,6 +389,11 @@ cmd_foreach()
>  		--recursive)
>  			recursive=1
>  			;;
> +		--revision)
> +			git rev-parse --quiet --verify "$2" >/dev/null || usage
> +			revision=$2

Shouldn't this part of the code verify $2^{tree} instead to ensure
that "$2" is a tree-ish?

> +			shift
> +			;;
>  		-*)
>  			usage
>  			;;
> @@ -404,7 +410,17 @@ cmd_foreach()
>  	# command in the subshell (and a recursive call to this function)
>  	exec 3<&0
>  
> -	module_list |
> +	if test -n "$revision"
> +	then
> +		# make ls-tree output look like ls-files output
> +		git ls-tree -r $revision | grep '^160000 ' |
> +		while read mode unused sha1 sm_path
> +		do
> +			echo "$mode $sha1 0 $sm_path"
> +		done
> +	else
> +		module_list
> +	fi |

Hrm, it is somewhat unfortunate that you cannot limit the set of
submodules to apply foreach to, like other commands like init,
update, status, etc.  (not a new problem).

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Assuming that the above guess is correct (which is a huge
> assumption, given the lack of clarity in the description), I think
> the feature might make sense.  The example would have been a lot
> easier to follow if it were something like this:
>
>     $ git submodule foreach --revision v1.0 'git grep -e frotz $sha1'

Imagine you have a checkout of v2.0 of the superproject in your
working tree, and you run "git submodule foreach --revision v1.0".
Further imagine a submodule S that used to exist back when the
superproject was at v1.0 no longer exists in the current codebase
(hence there is no such submodule in the working tree).

Shouldn't the above "foreach ... grep" still try to find 'frotz' in
the submodule S that was bound to v1.0 of the superproject?

Given that your patch does not touch the part of cmd_foreach where
it decides which submodule to descend into, it still will base its
decision solely on the set of submodules that are bound to and have
been "git submodule init"ed in the version of the superproject that
is _currently_ checked out, no?

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Jay Soffian]

On Tue, Oct 9, 2012 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Assuming that the above guess is correct (which is a huge
>> assumption, given the lack of clarity in the description), I think
>> the feature might make sense.  The example would have been a lot
>> easier to follow if it were something like this:
>>
>>     $ git submodule foreach --revision v1.0 'git grep -e frotz $sha1'
>
> Imagine you have a checkout of v2.0 of the superproject in your
> working tree, and you run "git submodule foreach --revision v1.0".
> Further imagine a submodule S that used to exist back when the
> superproject was at v1.0 no longer exists in the current codebase
> (hence there is no such submodule in the working tree).
>
> Shouldn't the above "foreach ... grep" still try to find 'frotz' in
> the submodule S that was bound to v1.0 of the superproject?
>
> Given that your patch does not touch the part of cmd_foreach where
> it decides which submodule to descend into, it still will base its
> decision solely on the set of submodules that are bound to and have
> been "git submodule init"ed in the version of the superproject that
> is _currently_ checked out, no?

That's a good observation. My use-case for this (poorly explained in
the commit message) is as part of a release process, where I wish to
apply corresponding tags to the superproject and its submodules like
so:

$ cd /path/to/superproject
$ git tag -m "1.0" v1.0 deadbeef
$ git submodule foreach --revision deadbeef \
  'git tag -m "superproject 1.0" superproject-1.0 $sha1'

Typically deadbeef may be a day or two behind HEAD and it's nice to be
able to tag it and the submodules w/o having to switch everything to a
detached HEAD. In my case, tagging and updating submodule revisions
are somewhat common, while adding/removing submodules is a rare event.

I didn't mention this issue explicitly because I thought it was
covered by the existing documentation: "Any submodules defined in the
superproject but not checked out are ignored by this command."

As you previously stated, I need to improve the documentation that
goes along with this patch, so I'll call-out this limitation. I'm not
sure what else can be done. You can't descend into a submodule that
isn't there.

j.

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> On Tue, Oct 9, 2012 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Assuming that the above guess is correct (which is a huge
>>> assumption, given the lack of clarity in the description), I think
>>> the feature might make sense.  The example would have been a lot
>>> easier to follow if it were something like this:
>>>
>>>     $ git submodule foreach --revision v1.0 'git grep -e frotz $sha1'
>>
>> Imagine you have a checkout of v2.0 of the superproject in your
>> working tree, and you run "git submodule foreach --revision v1.0".
>> Further imagine a submodule S that used to exist back when the
>> superproject was at v1.0 no longer exists in the current codebase
>> (hence there is no such submodule in the working tree).
>>
>> Shouldn't the above "foreach ... grep" still try to find 'frotz' in
>> the submodule S that was bound to v1.0 of the superproject?
>>
>> Given that your patch does not touch the part of cmd_foreach where
>> it decides which submodule to descend into, it still will base its
>> decision solely on the set of submodules that are bound to and have
>> been "git submodule init"ed in the version of the superproject that
>> is _currently_ checked out, no?
>
> That's a good observation. My use-case for this (poorly explained in
> ...
> As you previously stated, I need to improve the documentation that
> goes along with this patch, so I'll call-out this limitation. I'm not
> sure what else can be done. You can't descend into a submodule that
> isn't there.

As recent "submodule rm" work by Jens indicates, since 501770e (Move
git-dir for submodules, 2011-08-15), you should be able to peek into
submodules that have been "git submodule init"ed but do not exist in
the current checkout of the superproject.

I think the right approach to implement this "recurse foreach in the
superproject tree that is not checkout out to the working tree"
feature should be:

 - Advertise it so that it is crystal clear that the command run by
   "foreach" may have to run in a bare repository of submodule to
   look at submodule's commit bound to the historical tree of the
   superproject;

 - Anytime you look at .gitmodules in the superproject, read from
   the historical tree's .gitmodules instead of from the working
   tree (I think this is necessary in order to get the $sm_name vs
   $sm_path mapping right); and

 - Locate submodule's $GIT_DIR in $GIT_DIR/modules/$sm_name of the
   superproject that corresponds to the submodule found in the
   historical tree in the superproject (or if it is the same
   repository as that is currently checked out, use $sm_path/.git),
   and error out when it is not available.

An implementation that works only when all the submodules necessary
in the historical tree in the superproject are still in the current
checkout of the superproject may be fine as a quick throw-away hack,
and it may even be acceptable as a good first step towards the real
feature, but at least it needs to be protected by an error checking
upfront (perhaps running "diff-tree -r" between the index and the
historical tree to make sure there are no removed submodules that
existed in the historical tree), if it does not bother to check with
$GIT_DIR/modules/$sm_name in the superproject.

Jens, anything I missed?

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Jens Lehmann]

Am 09.10.2012 20:24, schrieb Junio C Hamano:
> Jay Soffian <jaysoffian@gmail.com> writes:
> 
>> On Tue, Oct 9, 2012 at 2:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Assuming that the above guess is correct (which is a huge
>>>> assumption, given the lack of clarity in the description), I think
>>>> the feature might make sense.  The example would have been a lot
>>>> easier to follow if it were something like this:
>>>>
>>>>     $ git submodule foreach --revision v1.0 'git grep -e frotz $sha1'
>>>
>>> Imagine you have a checkout of v2.0 of the superproject in your
>>> working tree, and you run "git submodule foreach --revision v1.0".
>>> Further imagine a submodule S that used to exist back when the
>>> superproject was at v1.0 no longer exists in the current codebase
>>> (hence there is no such submodule in the working tree).
>>>
>>> Shouldn't the above "foreach ... grep" still try to find 'frotz' in
>>> the submodule S that was bound to v1.0 of the superproject?
>>>
>>> Given that your patch does not touch the part of cmd_foreach where
>>> it decides which submodule to descend into, it still will base its
>>> decision solely on the set of submodules that are bound to and have
>>> been "git submodule init"ed in the version of the superproject that
>>> is _currently_ checked out, no?
>>
>> That's a good observation. My use-case for this (poorly explained in
>> ...
>> As you previously stated, I need to improve the documentation that
>> goes along with this patch, so I'll call-out this limitation. I'm not
>> sure what else can be done. You can't descend into a submodule that
>> isn't there.
> 
> As recent "submodule rm" work by Jens indicates, since 501770e (Move
> git-dir for submodules, 2011-08-15), you should be able to peek into
> submodules that have been "git submodule init"ed but do not exist in
> the current checkout of the superproject.
> 
> I think the right approach to implement this "recurse foreach in the
> superproject tree that is not checkout out to the working tree"
> feature should be:
> 
>  - Advertise it so that it is crystal clear that the command run by
>    "foreach" may have to run in a bare repository of submodule to
>    look at submodule's commit bound to the historical tree of the
>    superproject;

I think we should even try to enforce that the user shouldn't use
the work tree at all (although at the moment I can't come up with
an idea how we could do that), as the work tree *will* be out of
sync almost always when you need this option. Otherwise strange
things would happen when using "git submodule foreach --revision
..." with a command which examines the work tree, as that won't be
updated to the given revision.

>  - Anytime you look at .gitmodules in the superproject, read from
>    the historical tree's .gitmodules instead of from the working
>    tree (I think this is necessary in order to get the $sm_name vs
>    $sm_path mapping right); and

Yes, that is definitely necessary in case submodules are added,
removed or moved.

>  - Locate submodule's $GIT_DIR in $GIT_DIR/modules/$sm_name of the
>    superproject that corresponds to the submodule found in the
>    historical tree in the superproject (or if it is the same
>    repository as that is currently checked out, use $sm_path/.git),
>    and error out when it is not available.

Looking in $GIT_DIR/modules/$sm_name could make sense to tag even
those submodules which aren't currently populated. But IIRC the
tags in such repositories could not be pushed using current git
even when you use the "--recurse-submodules" option because that
only honors populated submodules. So for now it would suffice to
only recurse into populated submodules.

> An implementation that works only when all the submodules necessary
> in the historical tree in the superproject are still in the current
> checkout of the superproject may be fine as a quick throw-away hack,
> and it may even be acceptable as a good first step towards the real
> feature, but at least it needs to be protected by an error checking
> upfront (perhaps running "diff-tree -r" between the index and the
> historical tree to make sure there are no removed submodules that
> existed in the historical tree), if it does not bother to check with
> $GIT_DIR/modules/$sm_name in the superproject.
> 
> Jens, anything I missed?

Nothing I can think of right now, the above is a pretty good summary.
My gut feeling is that having "git submodule foreach --revision ..."
recurse through submodules whose work trees are out of sync is pretty
fragile and could easily lead to inconsistencies. So I tend to think
adding a custom script to the release process Jay uses which does the
tagging itself might be a better solution here. Opinions?

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Jay Soffian]

On Tue, Oct 9, 2012 at 5:21 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
> Nothing I can think of right now, the above is a pretty good summary.
> My gut feeling is that having "git submodule foreach --revision ..."
> recurse through submodules whose work trees are out of sync is pretty
> fragile and could easily lead to inconsistencies. So I tend to think
> adding a custom script to the release process Jay uses which does the
> tagging itself might be a better solution here. Opinions?

I agree now that this is a perilous option, and that its use case may
be so narrow that it may not worth adding. I am indeed already using a
custom script, and maybe I should leave it at that.

j.

Re: [PATCH] submodule: teach "foreach" command a --revision <tree-ish> option

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 09.10.2012 20:24, schrieb Junio C Hamano:
> ...
>> I think the right approach to implement this "recurse foreach in the
>> superproject tree that is not checkout out to the working tree"
>> feature should be:
>> 
>>  - Advertise it so that it is crystal clear that the command run by
>>    "foreach" may have to run in a bare repository of submodule to
>>    look at submodule's commit bound to the historical tree of the
>>    superproject;
>
> I think we should even try to enforce that the user shouldn't use
> the work tree at all (although at the moment I can't come up with
> an idea how we could do that), as the work tree *will* be out of
> sync almost always when you need this option.

Very good point.

>>  - Locate submodule's $GIT_DIR in $GIT_DIR/modules/$sm_name of the
>>    superproject that corresponds to the submodule found in the
>>    historical tree in the superproject (or if it is the same
>>    repository as that is currently checked out, use $sm_path/.git),
>>    and error out when it is not available.
>
> Looking in $GIT_DIR/modules/$sm_name could make sense to tag even
> those submodules which aren't currently populated. But IIRC the
> tags in such repositories could not be pushed using current git
> even when you use the "--recurse-submodules" option because that
> only honors populated submodules. So for now it would suffice to
> only recurse into populated submodules.

There are million reasons why we shouldn't lightly think "recurse
submodules is a good idea", and I think this may be one of them.

But you can always go to $GIT_DIR/modules/$sm_name and push from
there, so I do not see it as a huge problem.

>> Jens, anything I missed?
>
> Nothing I can think of right now, the above is a pretty good summary.
> My gut feeling is that having "git submodule foreach --revision ..."
> recurse through submodules whose work trees are out of sync is pretty
> fragile and could easily lead to inconsistencies. So I tend to think
> adding a custom script to the release process Jay uses which does the
> tagging itself might be a better solution here. Opinions?

Well, I am not a good judge for that, as I've never been a big fan
of "submodule recurse" myself anyway.  But I think an addition that
works only when the user never uses commands that use the working
tree or the index is still a good thing to have.

We could export a magic environment while running foreach script and
make NEED_WORK_TREE check fail when it is set, or something, but we
need to be careful about performance implications.  "foreach" is not
something that is worth sacrificing the general performance over.