{"thread":{"id":"28506","subject":"[PATCH] git-submodule: a small fix","startedAt":"2011-09-26T22:00:21Z","lastAt":"2011-09-27T07:24:13Z","messageCount":3,"participants":["Roy Liu","Andrew Ardill","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"176268","messageId":"CAOOg04z0UjOQCBMTuii37y3ykNU17hDTcD9E81C+r1whFpMaCg@mail.gmail.com","threadId":"28506","inReplyTo":null,"subject":"[PATCH] git-submodule: a small fix","fromName":"Roy Liu","fromEmail":"carsomyr@gmail.com","sentAt":"2011-09-26T22:00:21Z","receivedAt":"2011-09-26T22:00:21Z","isPatch":true,"sender":{"key":"carsomyr@gmail.com","avatar":null},"body":"In git-submodule.sh, the \"url\" variable may contain a stale value from\nthe previous loop iteration, so clear it.\n\n--- git-submodule.sh.orig   2011-09-26 17:50:45.000000000 -0400\n+++ git-submodule.sh    2011-09-26 17:51:18.000000000 -0400\n@@ -370,6 +370,8 @@\n           esac\n           git config submodule.\"$name\".url \"$url\" ||\n           die \"Failed to register url for submodule path '$path'\"\n+        else\n+            url=\"\"\n       fi\n\n       # Copy \"update\" setting when it is not set yet\n"},{"id":"176302","messageId":"CAH5451k-6HHx2xxddJauE8=P1umjG=TnrcOKmQfeh=4GOzpCKQ@mail.gmail.com","threadId":"28506","inReplyTo":"CAOOg04z0UjOQCBMTuii37y3ykNU17hDTcD9E81C+r1whFpMaCg@mail.gmail.com","subject":"Re: [PATCH] git-submodule: a small fix","fromName":"Andrew Ardill","fromEmail":"andrew.ardill@gmail.com","sentAt":"2011-09-27T03:22:32Z","receivedAt":"2011-09-27T03:22:32Z","isPatch":true,"sender":{"key":"andrew.ardill@gmail.com","avatar":"https://gravatar.com/avatar/da14cb7c091dd44dc6c63a4d3361b149acaf25226dc78eb4131a17b93d9b0993?d=mp&s=160"},"body":"On 27 September 2011 08:00, Roy Liu <carsomyr@gmail.com> wrote:\n> In git-submodule.sh, the \"url\" variable may contain a stale value from\n> the previous loop iteration, so clear it.\n>\n> --- git-submodule.sh.orig   2011-09-26 17:50:45.000000000 -0400\n> +++ git-submodule.sh    2011-09-26 17:51:18.000000000 -0400\n> @@ -370,6 +370,8 @@\n>            esac\n>            git config submodule.\"$name\".url \"$url\" ||\n>            die \"Failed to register url for submodule path '$path'\"\n> +        else\n> +            url=\"\"\n>        fi\n>\n>        # Copy \"update\" setting when it is not set yet\n\nPerhaps a better commit description would be:\n\ngit-submodule: clear the url variable when not set to avoid using stale values\n\notherwise looks clean enough\n\nAndrew\n"},{"id":"176310","messageId":"4E817A1D.8050902@web.de","threadId":"28506","inReplyTo":"CAH5451k-6HHx2xxddJauE8=P1umjG=TnrcOKmQfeh=4GOzpCKQ@mail.gmail.com","subject":"Re: [PATCH] git-submodule: a small fix","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2011-09-27T07:24:13Z","receivedAt":"2011-09-27T07:24:13Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 27.09.2011 05:22, schrieb Andrew Ardill:\n> On 27 September 2011 08:00, Roy Liu <carsomyr@gmail.com> wrote:\n>> In git-submodule.sh, the \"url\" variable may contain a stale value from\n>> the previous loop iteration, so clear it.\n>>\n>> --- git-submodule.sh.orig   2011-09-26 17:50:45.000000000 -0400\n>> +++ git-submodule.sh    2011-09-26 17:51:18.000000000 -0400\n>> @@ -370,6 +370,8 @@\n>>            esac\n>>            git config submodule.\"$name\".url \"$url\" ||\n>>            die \"Failed to register url for submodule path '$path'\"\n>> +        else\n>> +            url=\"\"\n>>        fi\n>>\n>>        # Copy \"update\" setting when it is not set yet\n> \n> Perhaps a better commit description would be:\n> \n> git-submodule: clear the url variable when not set to avoid using stale values\n\nYes, the commit description needs to describe what was changed (and you can\nalso drop the \"git-\", to start with \"submodule:\" is sufficient here). Also\nit would be nice if the message would describe under what circumstances this\nhappens (how did you notice this problem?).\n\nBut I wonder if the patch does it the right way. While it fixes the basic\nissue that \"url\" might not be set, I doubt it does what the user expects.\nThe place where the - sometimes uninitialized - variable \"url\" is used is\na few lines down:\n\n\t\tsay \"$(eval_gettext \"Submodule '\\$name' (\\$url) registered for path '\\$path'\")\"\n\nI doesn't make much sense to say \"Submodule 'foo' () registered for path 'foo'\"\nhere. Shouldn't \"url\" be set to \"$(git config \"submodule.$name.url\")\"? And\nwhen looking at the if you added the else to it might make sense to set it\nunconditionally before the if and then test \"$url\" there instead of adding an\nextra else.\n"}]}