David Aguilar <davvid@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html