Re: [PATCH] Added recurse command to git submodule

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2008, 05:44 UTC
Message-ID: <7v3at8yiq4.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7bfdc29a0801071914s207be500veae8e507d7d9d29@mail.gmail.com>

"Imran M Yousuf" <imyousuf@gmail.com> writes:

> Following is the diff with git-submdoule version 1.5.3.7. I also
> attached the diff and the modified file in the attachment.

Please do not do this when the submission is for real (I think you did not mean this for inclusion but for comment so it is Ok this time around), as such a message cannot be processed with our automated tools but need to be applied by hand.

Just send a non whitespace-corrupted, plain text patch. A corrupt text patch followed by an attachment is the worst of both worlds.

Show 14 quoted lines

> @@ -251,6 +252,78 @@ modules_list()
>  	done
>  }
>
> +# Simply checks whether the submodule is initialized
> +# or not. If not initialized it does so.
> +initializeSubModule() {
> +	if [ ! -d "$1"/.git ]; then
> +		if [ $recurse_verbose -eq 1 ]; then
> +			echo Initializing and updating "$1"
> +		fi
> +		git-submodule init "$1"; git-submodule update "$1"
> +	fi
> +}

The comment above sounds quite wrong, isn't it? It is not just "simply check" but actively makes sure it is initialized.

Do we use CamelCase in our shell script?

Making -r to always recurse _fully_ feels quite wrong. It is one thing to allow "git submodule init --recursive" to recurse fully and initialize everything, but it is another thing to force full instantiation of submodules that the user _chose_ not to check out, when the user has checkout of some but not all submodules and asks "git submodule update --recursive".

Show 13 quoted lines

> +# This actually traverses the module; checks
> +# whether the module is initialized or not.
> +# if not initialized, then done so and then the
> +# intended command is evaluated. Then it
> +# recursively goes into it modules.
> +traverseModule() {
> +	current_dir=`pwd`
> +	dir_path="$current_dir:$dir_path"
> +	initializeSubModule "$1"
> +        cd "$1"
> +	if [ $recurse_verbose -eq 1 ]; then
> +		echo Working in mod $1 @ `pwd` with $2
> +	fi

Always quote variable references unless you mean you want field splitting. Even when you know $1 and $2 do not have IFS characters, to make it easier to readers.

Show 12 quoted lines

> +        eval "$2"
> +	if [ -f .gitmodules ]; then
> +                for mod_path in `grep "path =" .gitmodules | awk
> '{print $3}'`; do
> +                        traverseModule "$mod_path" "$2"
> +                done
> +        fi
> +	old_dir=$(echo $dir_path | cut -d':' -f1-1)
> +	length_old_dir=`expr "$old_dir" : '.*'`
> +	cd $old_dir
> +	index=$(echo "$length_old_dir+2" | bc)
> +	dir_path=`echo $dir_path $index | awk '{print substr($1, $2)}'`

This dir_path separated with ":" and shuffling the $cwd with chdir'ing around makes me say "Yuck". Is it essential that these operation in the submodule happen in the same process? IOW, wouldn't it be enough to do something like:

	initialize_submodule "$1"
        (
        	cd "$1"
                if test -n "$recursive_verbose"
                then
                	echo "Working in module $1 with $2"
		fi
                eval "$2"
		if test -f .gitmodules
                then
                	for p in `sed -n -e 's/^path = //p' .gitmodules`
			do
                        	traverse_module "$p" "$2"
			done
		fi
	)

Show 15 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
> +propagate() {
> +	project_home=`pwd`
> +	echo Project Home: $project_home
> +	if [ -d $project_home/.git/ ]; then
> +		git_command=$1
> +		shift
> +		command_arguments=""
> +		for arg in "$@"; do

That 'in "$@"' is distracting.

	for arg
        do
		...

is enough.

> +			if [ `expr index "$arg" ' '` -gt 0 ]; then

Do we ever use `expr index` anywhere else? I thought we fixed that non-portability.

Show 9 quoted lines

> +				arg="\"$arg\""
> +			fi
> +			command_arguments="$command_arguments $arg"
> +		done
> +		if [ $recurse_verbose -eq 1 ]; then
> +			echo GIT Command git-$git_command with arguments\($#\) "$command_arguments"
> +		fi
> +		main_command="git-$git_command $command_arguments"
> +		eval $main_command

This feels like a sloppy quoting, although I haven't looked at the code very deeply. Does it work when $arg has funny letters like backslash, double-quote and or backquote?

Show 15 quoted lines

> @@ -286,6 +359,17 @@ do
>  	-*)
>  		usage
>  		;;
> +	recurse)
> +		recurse=1
> +		case "$2" in
> +			-v)
> +				recurse_verbose=1
> +				shift
> +				;;
> +		esac
> +		shift
> +		break
> +		;;

I was ahead of myself earlier but I think it makes more sense to pretend (at the command line syntax level) that --recursive is an option to other commands.

Show 27 quoted lines

> @@ -303,17 +387,21 @@ case "$add,$branch" in
>  	;;
>  esac
>
> -case "$add,$init,$update,$status,$cached" in
> -1,,,,)
> +
> +case "$add,$init,$update,$recurse,$status,$cached" in
> +1,,,,,)
>  	module_add "$@"
>  	;;
> -,1,,,)
> +,1,,,,)
>  	modules_init "$@"
>  	;;
> -,,1,,)
> +,,1,,,)
>  	modules_update "$@"
>  	;;
> -,,,*,*)
> +,,,1,,)
> +	propagate "$@"
> +	;;
> +,,,,*,*)
>  	modules_list "$@"
>  	;;
>  *)

I've always hated this part of the script. How about a bit of clean-up patch first before adding $recurse or anything else, to introduce command variable and do:

	case "$command" in
        add)
        	module_add "$@"
                ;;
	init)
        	module_init "$@"
                ;;
                ...
	*)
        	modules_list "$@"
                ;;
	esac

or even just a single:

	"module_$command" "$@"