Am 27.09.2011 05:22, schrieb Andrew Ardill: > On 27 September 2011 08:00, Roy Liu <carsomyr@xxxxxxxxx> wrote: >> In git-submodule.sh, the "url" variable may contain a stale value from >> the previous loop iteration, so clear it. >> >> --- git-submodule.sh.orig 2011-09-26 17:50:45.000000000 -0400 >> +++ git-submodule.sh 2011-09-26 17:51:18.000000000 -0400 >> @@ -370,6 +370,8 @@ >> esac >> git config submodule."$name".url "$url" || >> die "Failed to register url for submodule path '$path'" >> + else >> + url="" >> fi >> >> # Copy "update" setting when it is not set yet > > Perhaps a better commit description would be: > > git-submodule: clear the url variable when not set to avoid using stale values Yes, the commit description needs to describe what was changed (and you can also drop the "git-", to start with "submodule:" is sufficient here). Also it would be nice if the message would describe under what circumstances this happens (how did you notice this problem?). But I wonder if the patch does it the right way. While it fixes the basic issue that "url" might not be set, I doubt it does what the user expects. The place where the - sometimes uninitialized - variable "url" is used is a few lines down: say "$(eval_gettext "Submodule '\$name' (\$url) registered for path '\$path'")" I doesn't make much sense to say "Submodule 'foo' () registered for path 'foo'" here. Shouldn't "url" be set to "$(git config "submodule.$name.url")"? And when looking at the if you added the else to it might make sense to set it unconditionally before the if and then test "$url" there instead of adding an extra else. -- 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