Re: [PATCH] - Added 'recurse' subcommand to git submodule

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 6, 2008, 10:42 UTC
Message-ID: <7vr6eo9lrt.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1204788817-22720-2-git-send-email-imyousuf@gmail.com>

imyousuf@gmail.com writes:

> The purpose of the recurse command in the git submodule is to recurse

s/command/sub&/;

> a command in its submodule. For example if one wants to do a diff on its
> project with submodules at once, one can simply do
> 	git-submodule recurse diff HEAD
> and would see the diff for all the modules it contains.

Can we please have a blank line around the example command line to make it visually stand out more?

Show 12 quoted lines

> The recurse commands behavior can be customized with several arguments
> that it accepts. The synopsis for the recurse command is:
>
> 	git-submodule [-q|--quiet] recurse [-i|--initialize]
> 	[-e|--exit-after-error] [-d|--depth <recursion depth>]
> 	[-df|--depth-first] [-ca|--customized-argument] [-p|--pre-command]
> 	<command> [<arguments> ...]
>
> When traversing modules, a module could be uninitialized that is git
> submodule init and update has not been called for it; if [-i|--initialize]
> option is specified, it will initialize any module that is not initialized;
> else if the module is not initialized it will simply skip it.

I really do not think the -i option should exist. "init" is a conscious action and should not be a side effect of something else. (Why doesn't "git submodule status -i" exist? ;-)

I do not mind "git submodule recurse init", though. "git submodule recurse update" might also be a natural thing to do.

Show 11 quoted lines

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 257be4c..ee3c928 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -8,7 +8,8 @@
>  # git-submodule [-q|--quiet] add [-b|--branch branch] <repository> [<path>]
>  # git-submodule [-q|--quiet] [status] [-c|--cached] [--] [<path>...]
>  # git-submodule [-q|--quiet] init|update [--] [<path>...]
> -USAGE='[-q|--quiet] [--cached] [add <repo> [-b branch]|status|init|update] [--] [<path>...]'
> +# git-submodule [-q|--quiet] recurse [-i|--initialize] [-e|--exit-after-error] [-d|--depth <recursion depth>] [-df|--depth-first] <command> [<arguments> ...]
> +USAGE='[-q|--quiet] [[[add [-b|--branch branch] <repo>]|[[[status [-c|--cached]]|init|update] [--]]]  [<path>...]]|[recurse [-i|--initialize] [-e|--exit-after-error] [-d|--depth <recursion depth>] [-df|--depth-first] <command> [<arguments> ...]]'

I commented on the overlong USAGE and also not-so-useful comments that would only hurt maintainability in the previous message. The comment you have above would look reasonably good in LONG_USAGE without leading "#", don't you think?

> +# This module simply checks whether the depth is traverseable

s/module/function/; I wouldn't nitpick like this, but in the context of "git-submodule", saying "module" when you do not mean the _module_ you are working on is misleading.

Show 8 quoted lines

> +# This actually traverses a module; checks
> +# whether the module is initialized or not.
> +# if not initialized, then tries to do so 
> +# based on the user preference and then the
> +# intended command is evaluated in the 
> +# traversal manner requested - breadth first 
> +# or depth first. Then it# recursively goes 
> +# into it modules.

Overnarrow lines are harder to read as well as overlong ones...

Show 14 quoted lines

> +traverse_module()
> +{
> +	# Will work in the module if and only if the module is initialized
> +	initialize_sub_module "$1" &&
> +	(
> +		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 module
> +		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 "Working in mod $submod_path" @ `pwd` "with $@ ($#)"

This feels more like a debug message than progress report useful for the end users. Perhaps:

	say "git submodule recurse $submod_path $*"

(which is modeled after how "diff -r" repeats itself) would be enough.

By the way, please make it the habit of using $@ only when you are asking for the magic field splitting with "$@"; interpolating all params inside a single string should be done with "$*". say() happens to take multiple parameters, so your code happens to work Ok, but it is error prone; I do not think you deliberately tried to send multiple parameters to the above "say" by using "$@", knowing what this piece of code would do:

	frotz () {
        	echo $#; for i; do echo $i; done
	}
	set b c d
        frotz "a $@ e"

Show 6 quoted lines

> +		cmd_status=
> +		git "$@" || cmd_status=1
> +		# 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
> +		# TODO - If possible facilitate transaction

You can test $? here without $cmd_status.

> +		if  test -n "$cmd_status" && test -n "$on_error"

Excess SP between "if test".

> +		then
> +			die "git $@ failed in module $submod_path @ $(pwd)"

Same issue with $@ vs $*, and excess $submod_path vs the remainder cruft.

If the issue you wanted to solve with $(pwd) was that $submod_path is a local path within the current submodule, a better way to solve it would be to pass another "full path from the top" around when recursing into a new sublevel.

Show 10 quoted lines

> +# 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
> +cmd_recurse() {
> +	while :
> +	do
> ...
> +			elif test "$(expr $1 : '[1-9][0-9]*')" -eq "$(expr $1 : '.*')"

What is this doing?  $1 is underquoted here, by the way.

> +		-df|--depth-first)
> +			depth_first=1
> +			;;

Single dash followed by two letters is a somewhat unconventional option flag.

> +		-ca|--customized-argument)
> +			use_custom_args=1
> +			;;

Who uses this and other options? The series seems to be split incorrectly.

Show 9 quoted lines

> +	project_home="$(pwd)"
> +	if test "$depth" -gt 0
> +	then
> +		say Command will recurse upto "$depth" depth
> +	fi
> +	if test -d "$project_home"/.git/
> +	then
> +		say "Command to recurse: git $@"
> +		traverse_module . "$@"

These "say" are too noisy, compared to other existing uses. It feels as if the command is being run with --debug option.

Show 5 quoted lines

> @@ -441,3 +598,4 @@ then
>  fi
>  
>  "cmd_$command" "$@"
> +

Adds trailing blank line.