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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 9, 2012, 05:55 UTC
Message-ID: <7v8vbgi3yz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1349743810-10753-1-git-send-email-jaysoffian@gmail.com>

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:

Show 7 quoted lines

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

Show 15 quoted lines

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

Show 21 quoted lines

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