Re: [PATCH v2 2/5] git-submodule.sh: Add recurse subcommand with basic options

imyousuf@gmail.com writes:

The recurse commands behavior can be customized with several arguments
that it accepts. The synopsis for the recurse command is:

	git-submodule recurse [-q|--quiet] [-e|--exit-after-error]
	[-d|--depth <recursion depth>] [-b|--breadth-first]
	<git command> [<arguments> ...]

Is there a reason to limit the command that can be run per submodule to
only "git" commands?  To me, this "recurse" looks like a glorified "find"
command that can trigger its action only to submodule directories, but
limits what can be given to its -exec option to "git" commands.  While it
would not make sense to give certain git command to recurse (e.g. neither
"git show 65ea3b8" nor "git clone $there" would make any sense), it would
be handy if we can give certain non-git commands to it (e.g. "du -sh").

Everybody else seems to spell "<do-something>_submodule"; should this be
any different?

+{
+	if test ! -d "$1"/.git
+	then
+		say "Submodule $1 is not initialized and skipped"
+		return 1
+	# Returns true if submodule is already initialized

Micronit; s/Returns/Return/.  A sentence that begins with a capitalized
verb in comments is almost always in imperative mood, not third-person
singular present.

+	elif test -d "$1"/.git
+	then
+		return 0
+	fi
+}

Otherwise, what does it return?  Do you need elif there, or just "else"?

+# This function simply checks whether the depth is traverseable in terms of
+# depth and if so then it sequentially traverses its submodules
+traverse_submodules()
+{
+	# If current depth is the range specified than it will continue
+	# else return with success
+	if test "$depth" -gt 0 &&
+		test "$current_depth" -ge "$depth"
+	then
+		return 0;
+	fi
+	# If submodules exists than it will traverse over them
+	if test -f .gitmodules
+	then
+		# Incrementing the depth for the next level of submodules
+		current_depth=$(($current_depth + 1))
+                for mod_path in `sed -n -e 's/path = //p' .gitmodules`; do
+                        traverse_module "$mod_path" "$@"
+                done
+		# Decremented the depth to bring it back to the depth of
+		# the current submodule
+		current_depth=$(($current_depth - 1))
+	fi
+}

This makes me wonder if you should be iterating over .gitmodules, or
perhaps you may want to iterate over output of git-ls-files (picking
entries of gitlink type).  How should a local change that adds a new
submodule or removes an existing submodule, or moves an existing submodule
interact with "submodule recurse"? 

Also the same micronits (s/Incrementing/Increment/; s/Decremented/Decrement/).

Even if iterating over .gitmodules entries is a good idea, I suspect that
sed script is too fragile.  Doesn't .gitmodules use the same format as git
configuration files, allowing spaces around values, value quoting and
trailing comments on the same line?

+# This actually traverses a submodule; checks whether the its initialized
+# or not, does nothing if not initialized.

s/the //;?

+traverse_module()
+{
+	# Will work in the submodule if and only if its initialized
+	initialize_sub_module "$1" &&

"initialize_sub_module" does not sound like a function that checks if it
is initialized, but more like a function to, eh, initialize the submodule.
Perhaps the function should be renamed to make it clearer that it is a
predicate?

+	(
+		submod_path="$1"
+		shift
+		cd "$submod_path"
+		# If depth-first is specified in that case submodules are
+		# are traversed before executing the command on this submodule
+		test -n "$depth_first" && traverse_submodules "$@"
+		# pwd is mentioned in order to enable the ser to distinguish
+		# between same name modules, e.g. a/lib and b/lib.
+		say "git submodule recurse $submod_path $*"
+		git "$@"
+		# if exit on error is specifed than script will exit if any
+		# command fails. As there is no transaction there will be
+		# no rollback either

s/than/then/;?

+		# TODO - If possible facilitate transaction
+		if test "$?" -ne 0 && test -n "$on_error"
+		then
+			die "FAILED: git submodule $submod_path $*"

Dying before doing further damage to the repository tree may be a good
idea, but I did not see the calling loop in traverse_submodules pay
attention to the exit code from here.

+		fi
+		# If depth-first is not specified in that case submodules are
+		# are traversed after executing the command on this submodule
+		test -z "$depth_first" && traverse_submodules "$@"
+	)
+}
+
+# Propagates or recurses over all the submodules at any depth with any
+# git command, e.g. git-clone, git-status, git-commit etc., with the
+# arguments supplied exactly as it would have been supplied to the command
+# otherwise. This actually starts the recursive propagation.

Is "git-clone" a good example to give here?  What would that mean to
recurse into each submodule directories in a superproject to run "clone"?

+cmd_recurse() {
+	while :
+	do
+		case "$1" in
+		-q|--quiet)
+			quiet=1
+			;;
+		-d|--depth)
+			shift
+			if test -z "$1"
+			then
+				echo "No <recursion depth> specified"
+				usage
+			# Arithmatic operation will give an error if depth is not number
+			# thus chose to check intergerness with regular expression.
+			# $1 is underquoted becuase the expr is in quotation
+			elif test "$(expr $1 : '[1-9][0-9]*')" -eq "$(expr $1 : '.*')"

Huh?

	$ a='1 2 3'
        $ expr $a : '[1-9]'
	expr: syntax error
	$ expr "$a" : '[1-9]'
        1
        $ z=$(expr $a : '[1-9]')
	expr: syntax error
	$ z=$(expr "$a" : '[1-9]')
        $ echo $z
        1
        $ echo "$(expr $a : '[1-9]')"
	expr: syntax error
        $ echo "$(expr "$a" : '[1-9]')"
        1

If you want to make sure that $(( ... )) would not choke with given "$1",
you can check by attempting to do a simple $(( ... )) to see if it errors
out, which would be simpler.

	if test -z "$1"
        then
        	...
	elif ! echo $(( "$1" + 0 )) >/dev/null
        then
        	die "$1 is not an integer"
	...