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

[Junio C Hamano <gitster@pobox.com>]

David Aguilar <davvid@gmail.com> writes:

> +#
> +# 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.

> +	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.

> +		(
> +			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.