{"thread":{"id":"34776","subject":"[PATCH] remote: filter out invalid remote configurations","startedAt":"2013-08-27T13:06:36Z","lastAt":"2013-08-30T16:58:24Z","messageCount":4,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"225988","messageId":"1377608796-13732-1-git-send-email-cmn@elego.de","threadId":"34776","inReplyTo":null,"subject":"[PATCH] remote: filter out invalid remote configurations","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-08-27T13:06:36Z","receivedAt":"2013-08-27T13:06:36Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"In remote's configuration callback, anything that looks like\n'remote..*' creates a remote ''. This remote may not end\nup having any configuration for a remote, but it's still in the list,\nso 'git remote' shows it, which means something like\n\n [remote \"bogus\"]\n hocus = pocus\n\nwill show a remote 'bogus' in the listing, even though it won't work\nas a remote name for either git-fetch or git-push.\n\nFilter out the remotes that we created which have no urls in order to\nwork around such configuration entries.\n\nSigned-off-by: Carlos Martín Nieto \n\n---\n\nDue to git's callback-based config, it seemed a lot simpler to let it\ndo it wrong and then filter out what won't be usable, rather than\ndelaying the creation of a remote until we're sure we do want it.\n\nThe tests that made use of a remote 'existing' with just .fetch seem\nto be written that way because they can get away with it, rather than\nany assertion that it should be allowed in day-to-day git usage, but\ncorrect me if I'm wrong.\n\n remote.c | 17 +++++++++++++++++\n t/t5505-remote.sh | 2 ++\n t/t7201-co.sh | 2 +-\n 3 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 68eb99b..00a1d7a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -141,6 +141,9 @@ static struct remote *make_remote(const char *name, int len)\n \tint i;\n \n \tfor (i = 0; i < remotes_nr; i++) {\n+\t\tif (!remotes[i])\n+\t\t\tcontinue;\n+\n \t\tif (len ? (!strncmp(name, remotes[i]->name, len) &&\n \t\t\t !remotes[i]->name[len]) :\n \t\t !strcmp(name, remotes[i]->name))\n@@ -469,6 +472,19 @@ static int handle_config(const char *key, const char *value, void *cb)\n \treturn 0;\n }\n \n+static void filter_valid_remotes(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < remotes_nr; i++) {\n+\t\tif (!remotes[i])\n+\t\t\tcontinue;\n+\n+\t\t/* It's not a remote unless it has at least one url */\n+\t\tif (remotes[i]->url_nr == 0 && remotes[i]->pushurl_nr == 0)\n+\t\t\tremotes[i] = NULL;\n+\t}\n+}\n+\n static void alias_all_urls(void)\n {\n \tint i, j;\n@@ -504,6 +520,7 @@ static void read_config(void)\n \t\t\tmake_branch(head_ref + strlen(\"refs/heads/\"), 0);\n \t}\n \tgit_config(handle_config, NULL);\n+\tfilter_valid_remotes();\n \talias_all_urls();\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex dd10ff0..848e7b7 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -130,9 +130,11 @@ to delete them, use:\n EOF\n \t} &&\n \tgit tag footag &&\n+\tgit remote add oops .\n \tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n \tgit remote remove oops 2>actual1 &&\n \tgit branch foobranch &&\n+\tgit remote add oops .\n \tgit config --add remote.oops.fetch \"+refs/*:refs/*\" &&\n \tgit remote rm oops 2>actual2 &&\n \tgit branch -d foobranch &&\ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 0c9ec0a..4647f1c 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -431,7 +431,7 @@ test_expect_success 'detach a symbolic link HEAD' '\n \n test_expect_success \\\n 'checkout with --track fakes a sensible -b ' '\n- git config remote.origin.fetch \"+refs/heads/*:refs/remotes/origin/*\" &&\n+ git remote add origin . &&\n git update-ref refs/remotes/origin/koala/bear renamer &&\n \n git checkout --track origin/koala/bear &&\n-- \n1.8.4.561.g1c3d45d\n"},{"id":"225997","messageId":"xmqqr4dffarq.fsf@gitster.dls.corp.google.com","threadId":"34776","inReplyTo":"1377608796-13732-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] remote: filter out invalid remote configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-27T14:50:33Z","receivedAt":"2013-08-27T14:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> In remote's configuration callback, anything that looks like\n> 'remote..*' creates a remote ''. This remote may not end\n> up having any configuration for a remote, but it's still in the list,\n> so 'git remote' shows it, which means something like\n>\n> [remote \"bogus\"]\n> hocus = pocus\n>\n> will show a remote 'bogus' in the listing, even though it won't work\n> as a remote name for either git-fetch or git-push.\n\nIsn't this something the user may want to be aware of, though?\nHiding these would rob a chance for such an entry to be noticed from\nthe user---is it a good change?\n\n> Filter out the remotes that we created which have no urls in order to\n> work around such configuration entries.\n"},{"id":"226339","messageId":"1377866509.1714.0.camel@centaur.cmartin.tk","threadId":"34776","inReplyTo":"xmqqr4dffarq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] remote: filter out invalid remote configurations","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2013-08-30T12:41:49Z","receivedAt":"2013-08-30T12:41:49Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto writes:\n> \n> > In remote's configuration callback, anything that looks like\n> > 'remote..*' creates a remote ''. This remote may not end\n> > up having any configuration for a remote, but it's still in the list,\n> > so 'git remote' shows it, which means something like\n> >\n> > [remote \"bogus\"]\n> > hocus = pocus\n> >\n> > will show a remote 'bogus' in the listing, even though it won't work\n> > as a remote name for either git-fetch or git-push.\n> \n> Isn't this something the user may want to be aware of, though?\n> Hiding these would rob a chance for such an entry to be noticed from\n> the user---is it a good change?\n\nIf we want to help the user know that there's something a bit odd in\ntheir configuration, shouldn't we tell them instead of hoping they\nstumble upon it? Otherwise IMO it's more confusing if git-remote does\nshow the remote when git-fetch is interpreting the argument as a path.\n\n cmn\n"},{"id":"226345","messageId":"xmqqwqn3yv2n.fsf@gitster.dls.corp.google.com","threadId":"34776","inReplyTo":"1377866509.1714.0.camel@centaur.cmartin.tk","subject":"Re: [PATCH] remote: filter out invalid remote configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-30T16:58:24Z","receivedAt":"2013-08-30T16:58:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:\n>> Carlos Martín Nieto writes:\n>> \n>> > In remote's configuration callback, anything that looks like\n>> > 'remote..*' creates a remote ''. This remote may not end\n>> > up having any configuration for a remote, but it's still in the list,\n>> > so 'git remote' shows it, which means something like\n>> >\n>> > [remote \"bogus\"]\n>> > hocus = pocus\n>> >\n>> > will show a remote 'bogus' in the listing, even though it won't work\n>> > as a remote name for either git-fetch or git-push.\n>> \n>> Isn't this something the user may want to be aware of, though?\n>> Hiding these would rob a chance for such an entry to be noticed from\n>> the user---is it a good change?\n>\n> If we want to help the user know that there's something a bit odd in\n> their configuration, shouldn't we tell them instead of hoping they\n> stumble upon it?\n\nYeah, I agree that \"git remote\" that tells the above \"bogus\" is\nfishy is better than just hides it.\n"}]}