{"thread":{"id":"22413","subject":"[PATCH v2] Fix remote..vcs","startedAt":"2010-01-27T17:53:17Z","lastAt":"2010-01-27T23:54:11Z","messageCount":7,"participants":["Ilari Liusvaara","Sverre Rabbelier","Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"132817","messageId":"1264614797-22394-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22413","inReplyTo":null,"subject":"[PATCH v2] Fix remote..vcs","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-27T17:53:17Z","receivedAt":"2010-01-27T17:53:17Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"remote..vcs causes remote->foreign_vcs to be set on entry to\ntransport_get(). Unfortunately, the code assumed that any such entry\nis stale from previous round.\n\nFix this by making VCS set by URL to be volatile w.r.t. transport_get()\ninstead.\n\nSigned-off-by: Ilari Liusvaara \n---\n transport.c | 11 +++++------\n 1 files changed, 5 insertions(+), 6 deletions(-)\n\nDifferences from first round:\n\nThis makes VCS setting apply to all URLs that don't explicitly override,\ninstead of it applying to just the first one.\n\ndiff --git a/transport.c b/transport.c\nindex 7714fdb..87581b8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -912,20 +912,19 @@ static int external_specification_len(const char *url)\n \n struct transport *transport_get(struct remote *remote, const char *url)\n {\n+\tconst char *helper;\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tif (!remote)\n \t\tdie(\"No remote provided to transport_get()\");\n \n \tret->remote = remote;\n+\thelper = remote->foreign_vcs;\n \n \tif (!url && remote && remote->url)\n \t\turl = remote->url[0];\n \tret->url = url;\n \n-\t/* In case previous URL had helper forced, reset it. */\n-\tremote->foreign_vcs = NULL;\n-\n \t/* maybe it is a foreign URL? */\n \tif (url) {\n \t\tconst char *p = url;\n@@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\twhile (isalnum(*p))\n \t\t\tp++;\n \t\tif (!prefixcmp(p, \"::\"))\n-\t\t\tremote->foreign_vcs = xstrndup(url, p - url);\n+\t\t\thelper = xstrndup(url, p - url);\n \t}\n \n-\tif (remote && remote->foreign_vcs) {\n-\t\ttransport_helper_init(ret, remote->foreign_vcs);\n+\tif (helper) {\n+\t\ttransport_helper_init(ret, helper);\n \t} else if (!prefixcmp(url, \"rsync:\")) {\n \t\tret->get_refs_list = get_refs_via_rsync;\n \t\tret->fetch = fetch_objs_via_rsync;\n-- \n1.7.0.rc0.19.gb557e6\n"},{"id":"132818","messageId":"fabb9a1e1001271009m283bd725v9582b7c0c0acbad2@mail.gmail.com","threadId":"22413","inReplyTo":"1264614797-22394-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-01-27T18:09:10Z","receivedAt":"2010-01-27T18:09:10Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Jan 27, 2010 at 18:53, Ilari Liusvaara\n wrote:\n> Fix this by making VCS set by URL to be volatile w.r.t. transport_get()\n> instead.\n\nPatch looks good (didn't test it though), but I had to read this line\ntwice; I'm afraid I don't have any suggestions on how to improve it\nthough.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"132819","messageId":"alpine.LNX.2.00.1001271335140.14365@iabervon.org","threadId":"22413","inReplyTo":"1264614797-22394-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-01-27T18:39:00Z","receivedAt":"2010-01-27T18:39:00Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 27 Jan 2010, Ilari Liusvaara wrote:\n\n> remote..vcs causes remote->foreign_vcs to be set on entry to\n> transport_get(). Unfortunately, the code assumed that any such entry\n> is stale from previous round.\n> \n> Fix this by making VCS set by URL to be volatile w.r.t. transport_get()\n> instead.\n> \n> Signed-off-by: Ilari Liusvaara \n\nExcept that you missed the \"remote == NULL\" case (noted below), this is \nwhat I was thinking of.\n\nAcked-by: Daniel Barkalow \n\n> ---\n> transport.c | 11 +++++------\n> 1 files changed, 5 insertions(+), 6 deletions(-)\n> \n> Differences from first round:\n> \n> This makes VCS setting apply to all URLs that don't explicitly override,\n> instead of it applying to just the first one.\n> \n> diff --git a/transport.c b/transport.c\n> index 7714fdb..87581b8 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -912,20 +912,19 @@ static int external_specification_len(const char *url)\n> \n> struct transport *transport_get(struct remote *remote, const char *url)\n> {\n> +\tconst char *helper;\n> \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n> \n> \tif (!remote)\n> \t\tdie(\"No remote provided to transport_get()\");\n> \n> \tret->remote = remote;\n> +\thelper = remote->foreign_vcs;\n\nNeeds to be \"helper = remote ? remote->foreign_vcs : NULL\", for the same \nreason that the test below had been \"remote && remote->foreign_vcs\".\n\n> \n> \tif (!url && remote && remote->url)\n> \t\turl = remote->url[0];\n> \tret->url = url;\n> \n> -\t/* In case previous URL had helper forced, reset it. */\n> -\tremote->foreign_vcs = NULL;\n> -\n> \t/* maybe it is a foreign URL? */\n> \tif (url) {\n> \t\tconst char *p = url;\n> @@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> \t\twhile (isalnum(*p))\n> \t\t\tp++;\n> \t\tif (!prefixcmp(p, \"::\"))\n> -\t\t\tremote->foreign_vcs = xstrndup(url, p - url);\n> +\t\t\thelper = xstrndup(url, p - url);\n> \t}\n> \n> -\tif (remote && remote->foreign_vcs) {\n> -\t\ttransport_helper_init(ret, remote->foreign_vcs);\n> +\tif (helper) {\n> +\t\ttransport_helper_init(ret, helper);\n> \t} else if (!prefixcmp(url, \"rsync:\")) {\n> \t\tret->get_refs_list = get_refs_via_rsync;\n> \t\tret->fetch = fetch_objs_via_rsync;\n> -- \n> 1.7.0.rc0.19.gb557e6\n> \n> \n"},{"id":"132823","messageId":"20100127185927.GA22630@Knoppix","threadId":"22413","inReplyTo":"alpine.LNX.2.00.1001271335140.14365@iabervon.org","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-27T18:59:27Z","receivedAt":"2010-01-27T18:59:27Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:\n> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:\n> > \n> > \tif (!remote)\n> > \t\tdie(\"No remote provided to transport_get()\");\n> > \n> > \tret->remote = remote;\n> > +\thelper = remote->foreign_vcs;\n> \n> Needs to be \"helper = remote ? remote->foreign_vcs : NULL\", for the same \n> reason that the test below had been \"remote && remote->foreign_vcs\".\n\nFew lines above that:\n\n if (!remote)\n die(\"No remote provided to transport_get()\");\n\n\n-Ilari\n"},{"id":"132826","messageId":"7vaavzqs2q.fsf@alter.siamese.dyndns.org","threadId":"22413","inReplyTo":"alpine.LNX.2.00.1001271335140.14365@iabervon.org","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T19:13:33Z","receivedAt":"2010-01-27T19:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> Except that you missed the \"remote == NULL\" case (noted below), this is \n> what I was thinking of.\n>\n> Acked-by: Daniel Barkalow \n\n\n>> \tif (!remote)\n>> \t\tdie(\"No remote provided to transport_get()\");\n>> \n>> \tret->remote = remote;\n>> +\thelper = remote->foreign_vcs;\n>\n> Needs to be \"helper = remote ? remote->foreign_vcs : NULL\", for the same \n> reason that the test below had been \"remote && remote->foreign_vcs\".\n\nEven in the presense of \"if remote is NULL then we die\" in the context\nabove?\n\n>> \n>> \tif (!url && remote && remote->url)\n>> \t\turl = remote->url[0];\n>> \tret->url = url;\n>> \n>> -\t/* In case previous URL had helper forced, reset it. */\n>> -\tremote->foreign_vcs = NULL;\n>> -\n>> \t/* maybe it is a foreign URL? */\n>> \tif (url) {\n>> \t\tconst char *p = url;\n>> @@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)\n>> \t\twhile (isalnum(*p))\n>> \t\t\tp++;\n>> \t\tif (!prefixcmp(p, \"::\"))\n>> -\t\t\tremote->foreign_vcs = xstrndup(url, p - url);\n>> +\t\t\thelper = xstrndup(url, p - url);\n>> \t}\n>> \n>> -\tif (remote && remote->foreign_vcs) {\n>> -\t\ttransport_helper_init(ret, remote->foreign_vcs);\n>> +\tif (helper) {\n>> +\t\ttransport_helper_init(ret, helper);\n>> \t} else if (!prefixcmp(url, \"rsync:\")) {\n>> \t\tret->get_refs_list = get_refs_via_rsync;\n>> \t\tret->fetch = fetch_objs_via_rsync;\n>> -- \n>> 1.7.0.rc0.19.gb557e6\n>> \n>> \n"},{"id":"132833","messageId":"7vk4v3pabr.fsf@alter.siamese.dyndns.org","threadId":"22413","inReplyTo":"20100127185927.GA22630@Knoppix","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T20:22:16Z","receivedAt":"2010-01-27T20:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:\n>> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:\n>> > \n>> > \tif (!remote)\n>> > \t\tdie(\"No remote provided to transport_get()\");\n>> > \n>> > \tret->remote = remote;\n>> > +\thelper = remote->foreign_vcs;\n>> \n>> Needs to be \"helper = remote ? remote->foreign_vcs : NULL\", for the same \n>> reason that the test below had been \"remote && remote->foreign_vcs\".\n>\n> Few lines above that:\n>\n> if (!remote)\n> die(\"No remote provided to transport_get()\");\n\nPerhaps we would want this micro-clean-up on top then.\n\n-- >8 --\nSubject: transport_get(): drop unnecessary check for !remote\n\nAt the beginning of the function we make sure remote is not NULL, and\nthe remainder of the funciton already depends on it.\n\nSigned-off-by: Junio C Hamano \n---\ndiff --git a/transport.c b/transport.c\nindex 87581b8..3846aac 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -921,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tret->remote = remote;\n \thelper = remote->foreign_vcs;\n \n-\tif (!url && remote && remote->url)\n+\tif (!url && remote->url)\n \t\turl = remote->url[0];\n \tret->url = url;\n \n"},{"id":"132847","messageId":"alpine.LNX.2.00.1001271853130.14365@iabervon.org","threadId":"22413","inReplyTo":"7vk4v3pabr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Fix remote..vcs","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2010-01-27T23:54:11Z","receivedAt":"2010-01-27T23:54:11Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 27 Jan 2010, Junio C Hamano wrote:\n\n> Ilari Liusvaara writes:\n> \n> > On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:\n> >> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:\n> >> > \n> >> > \tif (!remote)\n> >> > \t\tdie(\"No remote provided to transport_get()\");\n> >> > \n> >> > \tret->remote = remote;\n> >> > +\thelper = remote->foreign_vcs;\n> >> \n> >> Needs to be \"helper = remote ? remote->foreign_vcs : NULL\", for the same \n> >> reason that the test below had been \"remote && remote->foreign_vcs\".\n> >\n> > Few lines above that:\n> >\n> > if (!remote)\n> > die(\"No remote provided to transport_get()\");\n> \n> Perhaps we would want this micro-clean-up on top then.\n> \n> -- >8 --\n> Subject: transport_get(): drop unnecessary check for !remote\n> \n> At the beginning of the function we make sure remote is not NULL, and\n> the remainder of the funciton already depends on it.\n\nI agree with both of these; there used to be code that used a NULL remote \nand just a URL, but that's gone now.\n\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/transport.c b/transport.c\n> index 87581b8..3846aac 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -921,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> \tret->remote = remote;\n> \thelper = remote->foreign_vcs;\n> \n> -\tif (!url && remote && remote->url)\n> +\tif (!url && remote->url)\n> \t\turl = remote->url[0];\n> \tret->url = url;\n> \n> \n"}]}