Re: [PATCH 3/3] git-submodule: add "sync" command

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2008, 18:57 UTC
Message-ID: <7vwsi6meas.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <23327a679798d19dc52a27c55a58c5b8c9ebe945.1219598198.git.davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 10 quoted lines

> +#
> +# Sync remote urls for submodules
> +# This makes the value for remote.$remote.url match the value
> +# specified in .gitmodules.
> +#
> +cmd_sync()
> +{
> +...
> +	cd_to_toplevel
> +	toplevel="$PWD"

I do not think you need $toplevel, as you cd around inside a subshell.

Show 6 quoted lines

> +	module_list "$@" |
> +	while read mode sha1 stage path
> +	do
> +		name=$(module_name "$path")
> +		url=$(git config -f .gitmodules --get submodule."$name".url)
> +		if test -d "$path"; then

I think this test is wrong.

Compare it with how cmd_foreach does this. The difference is that you force "sync" to every submodule that could be cloned and checked out, while "foreach" skips submodules the user has not expressed any interest in touching.

Show 6 quoted lines

> +		(
> +			unset GIT_DIR
> +			cd "$path"
> +			remote=$(get_remote)
> +			say "Synchronizing submodule url for '$name'"
> +			git config remote."$remote".url "$url"

I am not sure about the way you determine $remote. When the HEAD in the submodule repository is detached by prior "git submodule update", this will fall back to the default "origin" --- is it a good behaviour?

This is not an objection; I am merely wondering if that fallback is sensible, or if people who are interested in submodules can suggest better alternatives.

> +			cd "$toplevel"

Unneeded (in a subshell).

> +		)
> +		fi
> +	done
> +}

Other than the above comments, the patch looks sensible to me.