[PATCH 1/2] submodule: preserve all arguments exactly when recursing

[PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

When performing a recursive status or update, any argments with whitespace
would be split along that whitespace when passed to the recursive invocation
of the update or status command.

This is caused by the special handling that sh provides to the $@ variable.
Status and update stored "$@" into a separate variable, and passed that
variable to the recursive invocation. Unfortunately, the special handling
afforded to $@ isn't given to this new variable, and word-breaking occurs
along whitespace boundaries.

We can work around this by taking advantage of an easy technique to quote
any arbitrary string in the shell. Because single-quoted strings don't
support backslash-escapes, any arbitrary string can be quoted by wrapping
it in single-quotes and replacing any single-quotes inside the string with
the sequence '\''.

This commit introduces a new shell function quote_words that uses the quote
trick to produce a string containing the quoted version of all its
arguments. This string can then be used with `set -` to restore the original
value of $@. This shell function is used in cmd_status and in cmd_update
to store the original value of $@, which is then restored before the
recursive invocation takes place.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
I tried to write tests for this, but there are only two ways to get args
with spaces to be accepted and passed to the recursive invocation.
The first is via the --reference flag, but I don't think it really makes
sense to use that flag in connection with --recursive and was not comfortable
using it in a test. The second is as a pathname after the flags, but it
also doesn't make sense to pass these to recursive invocations, and in fact
the subsequent commit fixes it so the pathnames are not passed to recursive
invocations.

That said, despite the lack of tests I still believe this is a worthwhile
change, and it will certainly future-proof the command in case new flags
are added. It's also a reasonable model for how to handle this problem
in other shell commands.
 git-submodule.sh |   43 +++++++++++++++++++++++++++++++++++++++----
 1 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9ebbab7..ec7a5e4 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -64,6 +64,39 @@ module_list()
 }
 
 #
+# Emit a quoted version of the all argument suitable for passing to `eval`
+# $@ = words to quote
+#
+# This is intended to be used like the following:
+#   orig_args="$(quote_words "$@")"
+#   # do some work that includes calling shift
+#   eval "set - $orig_args"
+#   # now $@ has been restored, suitable for passing to another command
+#
+# Note that you cannot simply save off $@ into another variable because
+# the shell gives $@ and $* special handling in parameter expansion
+#
+quote_words ()
+{
+	while test $# -ne 0; do
+		# this can be done using sed like so:
+		#   printf "'%s'" "$(printf "%s" "$1" | sed -e "s/'/'\\\\''/g")"
+		# but in an attempt to avoid spawning a process for every argument, we'll
+		# just use the prefix/suffix pattern matching stuff
+		local word= suffix="$1" prefix=
+		while test -n "$suffix"
+		do
+			prefix="${suffix%%\'*}"
+			test "$prefix" != "$suffix" || break
+			suffix="${suffix#*\'}"
+			word="$word$prefix'\\''"
+		done
+		printf "'%s' " "$word$suffix"
+		shift
+	done
+}
+
+#
 # Map submodule path to submodule name
 #
 # $1 = path
@@ -374,7 +407,7 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args="$@"
+	orig_args="$(quote_words "$@")"
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -500,7 +533,8 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && cmd_update $orig_args) ||
+			eval "set - $orig_args"
+			(clear_local_git_env; cd "$path" && cmd_update "$@") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
 	done
@@ -733,7 +767,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args="$@"
+	orig_args="$(quote_words "$@")"
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -788,9 +822,10 @@ cmd_status()
 		then
 			(
 				prefix="$displaypath/"
+				eval "set - $orig_args"
 				clear_local_git_env
 				cd "$path" &&
-				cmd_status $orig_args
+				cmd_status "$@"
 			) ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
-- 
1.7.3.2.200.ga1bd

[PATCH 2/2] submodule: only preserve flags across recursive status/update invocations

[Kevin Ballard]

Recursive invocations of submodule update/status preserve all arguments
in the recursive invocation. This tends to cause undesired behavior when
those arguments include the name of a submodule, as it tries to recurse
into a nested submodule with the same name rather than recursing into all
child submodules of the named submodule.

This commit changes the argument preservation to only preserve flags.
When specifying a submodule name on the command-line, all child submodules
of that named submodule will be recursed into correctly, and it will not
attempt to recurse into a phantom nested submodule with the same name
as its parent.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
This commit was inspired by my coworker, who tried to run `git submodule
update --recursive molecules` (where molecules is the name of a submodule)
and received the rather unexpected error

  error: pathspec 'molecules' did not match any file(s) known to git.
  Did you forget to 'git add'?

 git-submodule.sh             |   21 +++++++++------------
 t/t7407-submodule-foreach.sh |   19 +++++++++++++++++++
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index ec7a5e4..52c693c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -407,41 +407,35 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args="$(quote_words "$@")"
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
 		-q|--quiet)
-			shift
 			GIT_QUIET=1
 			;;
 		-i|--init)
 			init=1
-			shift
 			;;
 		-N|--no-fetch)
-			shift
 			nofetch=1
 			;;
 		-r|--rebase)
-			shift
 			update="rebase"
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
 			reference="--reference=$2"
-			shift 2
+			orig_flags="$orig_flags $(quote_words "$1")"
+			shift
 			;;
 		--reference=*)
 			reference="$1"
-			shift
 			;;
 		-m|--merge)
-			shift
 			update="merge"
 			;;
 		--recursive)
-			shift
 			recursive=1
 			;;
 		--)
@@ -455,6 +449,8 @@ cmd_update()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(quote_words "$1")"
+		shift
 	done
 
 	if test -n "$init"
@@ -533,7 +529,7 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			eval "set - $orig_args"
+			eval "set - $orig_flags"
 			(clear_local_git_env; cd "$path" && cmd_update "$@") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
@@ -767,7 +763,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args="$(quote_words "$@")"
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -791,6 +787,7 @@ cmd_status()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(quote_words "$1")"
 		shift
 	done
 
@@ -822,7 +819,7 @@ cmd_status()
 		then
 			(
 				prefix="$displaypath/"
-				eval "set - $orig_args"
+				eval "set - $orig_flags"
 				clear_local_git_env
 				cd "$path" &&
 				cmd_status "$@"
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 905a8ba..2d5a855 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -238,4 +238,23 @@ test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	test -d clone4/nested1/nested2/nested3/submodule/.git
 '
 
+test_expect_success 'use "update --recursive nested1" to checkout all submodules rooted in nested1' '
+	git clone super clone5 &&
+	(
+		cd clone5 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test ! -d nested1/.git &&
+		git submodule update --init --recursive -- nested1 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test -d nested1/.git &&
+		test -d nested1/nested2/.git &&
+		test -d nested1/nested2/nested3/.git &&
+		test -d nested1/nested2/nested3/submodule/.git
+	)
+'
+
 test_done
-- 
1.7.3.2.200.ga1bd

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Jonathan Nieder]

Hi Kevin,

Kevin Ballard wrote:

>            It's also a reasonable model for how to handle this problem
> in other shell commands.

This caught my attention. :)

> +# Emit a quoted version of the all argument suitable for passing to `eval`
> +# $@ = words to quote
> +#
> +# This is intended to be used like the following:
> +#   orig_args="$(quote_words "$@")"
> +#   # do some work that includes calling shift
> +#   eval "set - $orig_args"
> +#   # now $@ has been restored, suitable for passing to another command
> +#
> +# Note that you cannot simply save off $@ into another variable because
> +# the shell gives $@ and $* special handling in parameter expansion
> +#
> +quote_words ()

Have you looked into "git rev-parse --sq-quote"?

Hope that helps,
Jonathan

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

On Nov 2, 2010, at 9:37 PM, Jonathan Nieder wrote:

> Hi Kevin,
> 
> Kevin Ballard wrote:
> 
>>           It's also a reasonable model for how to handle this problem
>> in other shell commands.
> 
> This caught my attention. :)
> 
>> +# Emit a quoted version of the all argument suitable for passing to `eval`
>> +# $@ = words to quote
>> +#
>> +# This is intended to be used like the following:
>> +#   orig_args="$(quote_words "$@")"
>> +#   # do some work that includes calling shift
>> +#   eval "set - $orig_args"
>> +#   # now $@ has been restored, suitable for passing to another command
>> +#
>> +# Note that you cannot simply save off $@ into another variable because
>> +# the shell gives $@ and $* special handling in parameter expansion
>> +#
>> +quote_words ()
> 
> Have you looked into "git rev-parse --sq-quote"?

Well crud, I wish I'd seen that before. Looks like it does pretty much the
exact same thing as my quote_words function. I'll send out another patch
that uses this instead of quote_words. Thanks for the info!

-Kevin Ballard

[PATCHv2 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

When performing a recursive status or update, any argments with whitespace
would be split along that whitespace when passed to the recursive invocation
of the update or status command.

This is caused by the special handling that sh provides to the $@ variable.
Status and update stored "$@" into a separate variable, and passed that
variable to the recursive invocation. Unfortunately, the special handling
afforded to $@ isn't given to this new variable, and word-breaking occurs
along whitespace boundaries.

We can use $(git rev-parse --sq-quote "$@") to produce a string containing
a quoted version of all given args, suitable for passing to eval. We then
recurse using something like `eval cmd_status "$orig_args"` instead of the
former `cmd_status $orig_args`. This preserves all arguments exactly as
given to the initial invocation of the command.

Helped-By: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Kevin Ballard <kevin@sb.org>
---
This patch ditches the quote_words function and uses
`git rev-parse --sq-quote "$@"` instead, as suggested by Jonathan Nieder.
 git-submodule.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9ebbab7..543554b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -374,7 +374,7 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args="$@"
+	orig_args="$(git rev-parse --sq-quote "$@")"
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -500,7 +500,7 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && cmd_update $orig_args) ||
+			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_args") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
 	done
@@ -733,7 +733,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args="$@"
+	orig_args="$(git rev-parse --sq-quote "$@")"
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -790,7 +790,7 @@ cmd_status()
 				prefix="$displaypath/"
 				clear_local_git_env
 				cd "$path" &&
-				cmd_status $orig_args
+				eval cmd_status "$orig_args"
 			) ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
-- 
1.7.3.2.200.g479de

Re: [PATCHv2 1/2] submodule: preserve all arguments exactly when recursing

[Jonathan Nieder]

Kevin Ballard wrote:

> When performing a recursive status or update, any argments with whitespace
> would be split along that whitespace when passed to the recursive invocation
> of the update or status command.
> 
> This is caused by the special handling that sh provides to the $@ variable.
> Status and update stored "$@" into a separate variable, and passed that
> variable to the recursive invocation. Unfortunately, the special handling
> afforded to $@ isn't given to this new variable, and word-breaking occurs
> along whitespace boundaries.
> 
> We can use $(git rev-parse --sq-quote "$@") to produce a string containing
> a quoted version of all given args, suitable for passing to eval. We then
> recurse using something like `eval cmd_status "$orig_args"` instead of the
> former `cmd_status $orig_args`. This preserves all arguments exactly as
> given to the initial invocation of the command.

Probably it is because it is late hear, but I find myself intimidated
by the block of explanatory text.  Maybe an example like

	Environment variables only hold strings, not lists of parameters,
	so $orig_args after

		orig_args="$@"

	fails to remember where each parameter starts and ends, if
	some include whitespace.  So

		git submodule update \
			--reference='/var/lib/common objects.git' \
			--recursive

	becomes

		git submodule update --reference=/var/lib/common \
			objects.git --recursive

	in the inner repositories.  Use "git rev-parse --sq-quote" to
	save parameters in quoted form ready for evaluation by the
	shell, avoiding this problem.

would be simpler?

> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -374,7 +374,7 @@ cmd_init()
>  cmd_update()
>  {
>  	# parse $args after "submodule ... update".
> -	orig_args="$@"
> +	orig_args="$(git rev-parse --sq-quote "$@")"

No quotes are needed around the RHS to an assignment like this.

Anyway,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCHv2 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

On Nov 2, 2010, at 10:28 PM, Jonathan Nieder wrote:

> Kevin Ballard wrote:
> 
>> When performing a recursive status or update, any argments with whitespace
>> would be split along that whitespace when passed to the recursive invocation
>> of the update or status command.
>> 
>> This is caused by the special handling that sh provides to the $@ variable.
>> Status and update stored "$@" into a separate variable, and passed that
>> variable to the recursive invocation. Unfortunately, the special handling
>> afforded to $@ isn't given to this new variable, and word-breaking occurs
>> along whitespace boundaries.
>> 
>> We can use $(git rev-parse --sq-quote "$@") to produce a string containing
>> a quoted version of all given args, suitable for passing to eval. We then
>> recurse using something like `eval cmd_status "$orig_args"` instead of the
>> former `cmd_status $orig_args`. This preserves all arguments exactly as
>> given to the initial invocation of the command.
> 
> Probably it is because it is late hear, but I find myself intimidated
> by the block of explanatory text.  Maybe an example like
> 
> 	Environment variables only hold strings, not lists of parameters,
> 	so $orig_args after
> 
> 		orig_args="$@"
> 
> 	fails to remember where each parameter starts and ends, if
> 	some include whitespace.  So
> 
> 		git submodule update \
> 			--reference='/var/lib/common objects.git' \
> 			--recursive
> 
> 	becomes
> 
> 		git submodule update --reference=/var/lib/common \
> 			objects.git --recursive
> 
> 	in the inner repositories.  Use "git rev-parse --sq-quote" to
> 	save parameters in quoted form ready for evaluation by the
> 	shell, avoiding this problem.
> 
> would be simpler?

I agree, yours is simpler. I'll send one final patch with the updated description.

>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -374,7 +374,7 @@ cmd_init()
>> cmd_update()
>> {
>> 	# parse $args after "submodule ... update".
>> -	orig_args="$@"
>> +	orig_args="$(git rev-parse --sq-quote "$@")"
> 
> No quotes are needed around the RHS to an assignment like this.

Hrm, looks like you're right. I was under the impression it was necessary in some
cases when the output of the command included newlines, but a quick test shows
that belief to be wrong.

> Anyway,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-Kevin Ballard

[PATCHv2 2/2] submodule: only preserve flags across recursive status/update invocations

[Kevin Ballard]

Recursive invocations of submodule update/status preserve all arguments
in the recursive invocation. This tends to cause undesired behavior when
those arguments include the name of a submodule, as it tries to recurse
into a nested submodule with the same name rather than recursing into all
child submodules of the named submodule.

This commit changes the argument preservation to only preserve flags.
When specifying a submodule name on the command-line, all child submodules
of that named submodule will be recursed into correctly, and it will not
attempt to recurse into a phantom nested submodule with the same name
as its parent.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
I'm tempted to make this commit also omit the --reference flag in recursive
calls to cmd_update, as I don't believe it makes sense to use --reference
in conjunction with --recursive, but it may be a better idea to simply
produce an error if both flags are used together.
 git-submodule.sh             |   19 ++++++++-----------
 t/t7407-submodule-foreach.sh |   19 +++++++++++++++++++
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 543554b..4fd8982 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -374,41 +374,35 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args="$(git rev-parse --sq-quote "$@")"
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
 		-q|--quiet)
-			shift
 			GIT_QUIET=1
 			;;
 		-i|--init)
 			init=1
-			shift
 			;;
 		-N|--no-fetch)
-			shift
 			nofetch=1
 			;;
 		-r|--rebase)
-			shift
 			update="rebase"
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
 			reference="--reference=$2"
-			shift 2
+			orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
+			shift
 			;;
 		--reference=*)
 			reference="$1"
-			shift
 			;;
 		-m|--merge)
-			shift
 			update="merge"
 			;;
 		--recursive)
-			shift
 			recursive=1
 			;;
 		--)
@@ -422,6 +416,8 @@ cmd_update()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
+		shift
 	done
 
 	if test -n "$init"
@@ -500,7 +496,7 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_args") ||
+			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_flags") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
 	done
@@ -733,7 +729,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args="$(git rev-parse --sq-quote "$@")"
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -757,6 +753,7 @@ cmd_status()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
 		shift
 	done
 
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 905a8ba..2d5a855 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -238,4 +238,23 @@ test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	test -d clone4/nested1/nested2/nested3/submodule/.git
 '
 
+test_expect_success 'use "update --recursive nested1" to checkout all submodules rooted in nested1' '
+	git clone super clone5 &&
+	(
+		cd clone5 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test ! -d nested1/.git &&
+		git submodule update --init --recursive -- nested1 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test -d nested1/.git &&
+		test -d nested1/nested2/.git &&
+		test -d nested1/nested2/nested3/.git &&
+		test -d nested1/nested2/nested3/submodule/.git
+	)
+'
+
 test_done
-- 
1.7.3.2.200.g479de

Re: [PATCHv2 2/2] submodule: only preserve flags across recursive status/update invocations

[Jonathan Nieder]

Kevin Ballard wrote:

> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -374,41 +374,35 @@ cmd_init()
[...]
>  		--reference)
>  			case "$2" in '') usage ;; esac
>  			reference="--reference=$2"
> -			shift 2
> +			orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
> +			shift

Hmm.  Maybe a helper would make it clearer.

	save_arg () {
		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
	}

making this

 +			save_arg --reference
 +			shift

> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -238,4 +238,23 @@ test_expect_success 'use "git clone --recursive" to checkout all submodules' '
>  	test -d clone4/nested1/nested2/nested3/submodule/.git
>  '
>  
> +test_expect_success 'use "update --recursive nested1" to checkout all submodules rooted in nested1' '

Maybe a submodule status --cached --recursive -- <files> test, too?

Sleepily,
Jonathan

Re: [PATCHv2 2/2] submodule: only preserve flags across recursive status/update invocations

[Kevin Ballard]

On Nov 2, 2010, at 10:38 PM, Jonathan Nieder wrote:

> Kevin Ballard wrote:
> 
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -374,41 +374,35 @@ cmd_init()
> [...]
>> 		--reference)
>> 			case "$2" in '') usage ;; esac
>> 			reference="--reference=$2"
>> -			shift 2
>> +			orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
>> +			shift
> 
> Hmm.  Maybe a helper would make it clearer.
> 
> 	save_arg () {
> 		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
> 	}
> 
> making this
> 
> +			save_arg --reference
> +			shift

I considered that, but I already dislike the fact that orig_flags is a global.
I left it as such for the sake of not fixing what ain't broke, but the use of
a helper function would imply that the global nature of the variable is
intentional.

>> --- a/t/t7407-submodule-foreach.sh
>> +++ b/t/t7407-submodule-foreach.sh
>> @@ -238,4 +238,23 @@ test_expect_success 'use "git clone --recursive" to checkout all submodules' '
>> 	test -d clone4/nested1/nested2/nested3/submodule/.git
>> '
>> 
>> +test_expect_success 'use "update --recursive nested1" to checkout all submodules rooted in nested1' '
> 
> Maybe a submodule status --cached --recursive -- <files> test, too?

Good idea.

-Kevin Ballard

[PATCHv3 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

Shell variables only hold strings, not lists of parameters,
so $orig_args after

        orig_args="$@"

fails to remember where each parameter starts and ends, if
some include whitespace.  So

        git submodule update \
                --reference='/var/lib/common objects.git' \
                --recursive --init

becomes

        git submodule update --reference=/var/lib/common \
                objects.git --recursive --init

in the inner repositories.  Use "git rev-parse --sq-quote" to
save parameters in quoted form ready for evaluation by the
shell, avoiding this problem.

Helped-By: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Kevin Ballard <kevin@sb.org>
---
This version introduces a test using --reference.
It also uses the nice description written by Jonathan
and it stops unnecessarily quoting the results of
the call to $(git rev-parse --sq-quote "$@")
 git-submodule.sh             |    8 ++++----
 t/t7407-submodule-foreach.sh |   16 ++++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9ebbab7..4d2bb37 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -374,7 +374,7 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args="$@"
+	orig_args=$(git rev-parse --sq-quote "$@")
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -500,7 +500,7 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && cmd_update $orig_args) ||
+			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_args") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
 	done
@@ -733,7 +733,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args="$@"
+	orig_args=$(git rev-parse --sq-quote "$@")
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -790,7 +790,7 @@ cmd_status()
 				prefix="$displaypath/"
 				clear_local_git_env
 				cd "$path" &&
-				cmd_status $orig_args
+				eval cmd_status "$orig_args"
 			) ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 905a8ba..15d420f 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -238,4 +238,20 @@ test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	test -d clone4/nested1/nested2/nested3/submodule/.git
 '
 
+test_expect_success 'test "update --recursive" with a flag with spaces' '
+	git clone super "common objects" &&
+	git clone super clone5 &&
+	(
+		cd clone5 &&
+		test ! -d nested1/.git &&
+		git submodule update --init --recursive --reference="$(dirname "$PWD")/common objects" &&
+		test -d nested1/.git &&
+		test -d nested1/nested2/.git &&
+		test -d nested1/nested2/nested3/.git &&
+		test -f nested1/.git/objects/info/alternates &&
+		test -f nested1/nested2/.git/objects/info/alternates &&
+		test -f nested1/nested2/nested3/.git/objects/info/alternates
+	)
+'
+
 test_done
-- 
1.7.3.2.200.g862e8

[PATCHv3 2/2] submodule: only preserve flags across recursive status/update invocations

[Kevin Ballard]

Recursive invocations of submodule update/status preserve all arguments,
so executing

        git submodule update --recursive -- foo

attempts to recursively update a submodule named "foo".

Naturally, this fails as one cannot have an infinitely-deep stack of
submodules each containing a submodule named "foo". The desired behavior
is instead to update foo and then recursively update all submodules
inside of foo.

This commit accomplishes that by only saving the flags for use in the
recursive invocation.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
As suggested by Jonathan Nieder, I've included a test for
`git submodule status --cached --recursive -- nested` to verify
that the flags are preserved but the paths aren't.
 git-submodule.sh             |   19 ++++++++-----------
 t/t7407-submodule-foreach.sh |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 4d2bb37..4fd8982 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -374,41 +374,35 @@ cmd_init()
 cmd_update()
 {
 	# parse $args after "submodule ... update".
-	orig_args=$(git rev-parse --sq-quote "$@")
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
 		-q|--quiet)
-			shift
 			GIT_QUIET=1
 			;;
 		-i|--init)
 			init=1
-			shift
 			;;
 		-N|--no-fetch)
-			shift
 			nofetch=1
 			;;
 		-r|--rebase)
-			shift
 			update="rebase"
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
 			reference="--reference=$2"
-			shift 2
+			orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
+			shift
 			;;
 		--reference=*)
 			reference="$1"
-			shift
 			;;
 		-m|--merge)
-			shift
 			update="merge"
 			;;
 		--recursive)
-			shift
 			recursive=1
 			;;
 		--)
@@ -422,6 +416,8 @@ cmd_update()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
+		shift
 	done
 
 	if test -n "$init"
@@ -500,7 +496,7 @@ cmd_update()
 
 		if test -n "$recursive"
 		then
-			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_args") ||
+			(clear_local_git_env; cd "$path" && eval cmd_update "$orig_flags") ||
 			die "Failed to recurse into submodule path '$path'"
 		fi
 	done
@@ -733,7 +729,7 @@ cmd_summary() {
 cmd_status()
 {
 	# parse $args after "submodule ... status".
-	orig_args=$(git rev-parse --sq-quote "$@")
+	orig_flags=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -757,6 +753,7 @@ cmd_status()
 			break
 			;;
 		esac
+		orig_flags="$orig_flags $(git rev-parse --sq-quote "$1")"
 		shift
 	done
 
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 15d420f..d8ad250 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -226,6 +226,21 @@ test_expect_success 'test "status --recursive"' '
 	test_cmp expect actual
 '
 
+sed -e "/nested1 /s/.*/+$nested1sha1 nested1 (file2~1)/;/sub[1-3]/d" < expect > expect2
+mv -f expect2 expect
+
+test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
+	(
+		cd clone3 &&
+		(
+			cd nested1 &&
+			test_commit file2
+		) &&
+		git submodule status --cached --recursive -- nested1 > ../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
 	git clone --recursive super clone4 &&
 	test -d clone4/.git &&
@@ -254,4 +269,23 @@ test_expect_success 'test "update --recursive" with a flag with spaces' '
 	)
 '
 
+test_expect_success 'use "update --recursive nested1" to checkout all submodules rooted in nested1' '
+	git clone super clone6 &&
+	(
+		cd clone6 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test ! -d nested1/.git &&
+		git submodule update --init --recursive -- nested1 &&
+		test ! -d sub1/.git &&
+		test ! -d sub2/.git &&
+		test ! -d sub3/.git &&
+		test -d nested1/.git &&
+		test -d nested1/nested2/.git &&
+		test -d nested1/nested2/nested3/.git &&
+		test -d nested1/nested2/nested3/submodule/.git
+	)
+'
+
 test_done
-- 
1.7.3.2.200.g862e8

Re: [PATCHv3 2/2] submodule: only preserve flags across recursive status/update invocations

[Junio C Hamano]

Well done; thanks both.  Will queue.

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Jonathan Nieder]

Kevin Ballard wrote:
> On Nov 2, 2010, at 9:37 PM, Jonathan Nieder wrote:

>> Have you looked into "git rev-parse --sq-quote"?
>
> Well crud, I wish I'd seen that before. Looks like it does pretty much the
> exact same thing as my quote_words function. I'll send out another patch
> that uses this instead of quote_words. Thanks for the info!

Incidentally, if you still remember where it might have been useful
to find this advertised (git-sh-setup?), maybe we can find a way to document
it better.

Thanks for the submodule fix.  Well spotted.

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

On Nov 2, 2010, at 10:47 PM, Jonathan Nieder wrote:

> Kevin Ballard wrote:
>> On Nov 2, 2010, at 9:37 PM, Jonathan Nieder wrote:
> 
>>> Have you looked into "git rev-parse --sq-quote"?
>> 
>> Well crud, I wish I'd seen that before. Looks like it does pretty much the
>> exact same thing as my quote_words function. I'll send out another patch
>> that uses this instead of quote_words. Thanks for the info!
> 
> Incidentally, if you still remember where it might have been useful
> to find this advertised (git-sh-setup?), maybe we can find a way to document
> it better.

It might make sense to mention it in CodingGuidelines. That's where I always
look when writing shell code in order to make sure I'm not using any bashisms.

-Kevin Ballard

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Jonathan Nieder]

Kevin Ballard wrote:

> I tried to write tests for this, but there are only two ways to get args
> with spaces to be accepted and passed to the recursive invocation.
> The first is via the --reference flag, but I don't think it really makes
> sense to use that flag in connection with --recursive and was not comfortable
> using it in a test.

Could you explain this further?  I think --reference pointing to a
repository with objects shared by multiple (sub)projects makes a lot
of sense.

Re: [PATCH 1/2] submodule: preserve all arguments exactly when recursing

[Kevin Ballard]

On Nov 2, 2010, at 10:30 PM, Jonathan Nieder wrote:

> Kevin Ballard wrote:
> 
>> I tried to write tests for this, but there are only two ways to get args
>> with spaces to be accepted and passed to the recursive invocation.
>> The first is via the --reference flag, but I don't think it really makes
>> sense to use that flag in connection with --recursive and was not comfortable
>> using it in a test.
> 
> Could you explain this further?  I think --reference pointing to a
> repository with objects shared by multiple (sub)projects makes a lot
> of sense.

I hadn't given it a lot of thought, but I suppose you're right. I'm used to
using submodules to pull in disparate projects that don't share objects, but
I can imagine having repos that do share objects. I will try to write a test
for this using --reference.

-Kevin Ballard