[PATCH v2] Fix remote.<remote>.vcs

[PATCH v2] Fix remote.<remote>.vcs

[Ilari Liusvaara]

remote.<remote>.vcs causes remote->foreign_vcs to be set on entry to
transport_get(). Unfortunately, the code assumed that any such entry
is stale from previous round.

Fix this by making VCS set by URL to be volatile w.r.t. transport_get()
instead.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

Differences from first round:

This makes VCS setting apply to all URLs that don't explicitly override,
instead of it applying to just the first one.

diff --git a/transport.c b/transport.c
index 7714fdb..87581b8 100644
--- a/transport.c
+++ b/transport.c
@@ -912,20 +912,19 @@ static int external_specification_len(const char *url)
 
 struct transport *transport_get(struct remote *remote, const char *url)
 {
+	const char *helper;
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
 	if (!remote)
 		die("No remote provided to transport_get()");
 
 	ret->remote = remote;
+	helper = remote->foreign_vcs;
 
 	if (!url && remote && remote->url)
 		url = remote->url[0];
 	ret->url = url;
 
-	/* In case previous URL had helper forced, reset it. */
-	remote->foreign_vcs = NULL;
-
 	/* maybe it is a foreign URL? */
 	if (url) {
 		const char *p = url;
@@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		while (isalnum(*p))
 			p++;
 		if (!prefixcmp(p, "::"))
-			remote->foreign_vcs = xstrndup(url, p - url);
+			helper = xstrndup(url, p - url);
 	}
 
-	if (remote && remote->foreign_vcs) {
-		transport_helper_init(ret, remote->foreign_vcs);
+	if (helper) {
+		transport_helper_init(ret, helper);
 	} else if (!prefixcmp(url, "rsync:")) {
 		ret->get_refs_list = get_refs_via_rsync;
 		ret->fetch = fetch_objs_via_rsync;
-- 
1.7.0.rc0.19.gb557e6

Re: [PATCH v2] Fix remote.<remote>.vcs

[Sverre Rabbelier]

Heya,

On Wed, Jan 27, 2010 at 18:53, Ilari Liusvaara
<ilari.liusvaara@elisanet.fi> wrote:
> Fix this by making VCS set by URL to be volatile w.r.t. transport_get()
> instead.

Patch looks good (didn't test it though), but I had to read this line
twice; I'm afraid I don't have any suggestions on how to improve it
though.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2] Fix remote.<remote>.vcs

[Daniel Barkalow]

On Wed, 27 Jan 2010, Ilari Liusvaara wrote:

> remote.<remote>.vcs causes remote->foreign_vcs to be set on entry to
> transport_get(). Unfortunately, the code assumed that any such entry
> is stale from previous round.
> 
> Fix this by making VCS set by URL to be volatile w.r.t. transport_get()
> instead.
> 
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>

Except that you missed the "remote == NULL" case (noted below), this is 
what I was thinking of.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

> ---
>  transport.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
> 
> Differences from first round:
> 
> This makes VCS setting apply to all URLs that don't explicitly override,
> instead of it applying to just the first one.
> 
> diff --git a/transport.c b/transport.c
> index 7714fdb..87581b8 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -912,20 +912,19 @@ static int external_specification_len(const char *url)
>  
>  struct transport *transport_get(struct remote *remote, const char *url)
>  {
> +	const char *helper;
>  	struct transport *ret = xcalloc(1, sizeof(*ret));
>  
>  	if (!remote)
>  		die("No remote provided to transport_get()");
>  
>  	ret->remote = remote;
> +	helper = remote->foreign_vcs;

Needs to be "helper = remote ? remote->foreign_vcs : NULL", for the same 
reason that the test below had been "remote && remote->foreign_vcs".

>  
>  	if (!url && remote && remote->url)
>  		url = remote->url[0];
>  	ret->url = url;
>  
> -	/* In case previous URL had helper forced, reset it. */
> -	remote->foreign_vcs = NULL;
> -
>  	/* maybe it is a foreign URL? */
>  	if (url) {
>  		const char *p = url;
> @@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  		while (isalnum(*p))
>  			p++;
>  		if (!prefixcmp(p, "::"))
> -			remote->foreign_vcs = xstrndup(url, p - url);
> +			helper = xstrndup(url, p - url);
>  	}
>  
> -	if (remote && remote->foreign_vcs) {
> -		transport_helper_init(ret, remote->foreign_vcs);
> +	if (helper) {
> +		transport_helper_init(ret, helper);
>  	} else if (!prefixcmp(url, "rsync:")) {
>  		ret->get_refs_list = get_refs_via_rsync;
>  		ret->fetch = fetch_objs_via_rsync;
> -- 
> 1.7.0.rc0.19.gb557e6
> 
> 

Re: [PATCH v2] Fix remote.<remote>.vcs

[Ilari Liusvaara]

On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:
> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:
> >  
> >  	if (!remote)
> >  		die("No remote provided to transport_get()");
> >  
> >  	ret->remote = remote;
> > +	helper = remote->foreign_vcs;
> 
> Needs to be "helper = remote ? remote->foreign_vcs : NULL", for the same 
> reason that the test below had been "remote && remote->foreign_vcs".

Few lines above that:

     if (!remote)
             die("No remote provided to transport_get()");


-Ilari

Re: [PATCH v2] Fix remote.<remote>.vcs

[Junio C Hamano]

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:
>> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:
>> >  
>> >  	if (!remote)
>> >  		die("No remote provided to transport_get()");
>> >  
>> >  	ret->remote = remote;
>> > +	helper = remote->foreign_vcs;
>> 
>> Needs to be "helper = remote ? remote->foreign_vcs : NULL", for the same 
>> reason that the test below had been "remote && remote->foreign_vcs".
>
> Few lines above that:
>
>      if (!remote)
>              die("No remote provided to transport_get()");

Perhaps we would want this micro-clean-up on top then.

-- >8 --
Subject: transport_get(): drop unnecessary check for !remote

At the beginning of the function we make sure remote is not NULL, and
the remainder of the funciton already depends on it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
diff --git a/transport.c b/transport.c
index 87581b8..3846aac 100644
--- a/transport.c
+++ b/transport.c
@@ -921,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	ret->remote = remote;
 	helper = remote->foreign_vcs;
 
-	if (!url && remote && remote->url)
+	if (!url && remote->url)
 		url = remote->url[0];
 	ret->url = url;
 

Re: [PATCH v2] Fix remote.<remote>.vcs

[Daniel Barkalow]

On Wed, 27 Jan 2010, Junio C Hamano wrote:

> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> > On Wed, Jan 27, 2010 at 01:39:00PM -0500, Daniel Barkalow wrote:
> >> On Wed, 27 Jan 2010, Ilari Liusvaara wrote:
> >> >  
> >> >  	if (!remote)
> >> >  		die("No remote provided to transport_get()");
> >> >  
> >> >  	ret->remote = remote;
> >> > +	helper = remote->foreign_vcs;
> >> 
> >> Needs to be "helper = remote ? remote->foreign_vcs : NULL", for the same 
> >> reason that the test below had been "remote && remote->foreign_vcs".
> >
> > Few lines above that:
> >
> >      if (!remote)
> >              die("No remote provided to transport_get()");
> 
> Perhaps we would want this micro-clean-up on top then.
> 
> -- >8 --
> Subject: transport_get(): drop unnecessary check for !remote
> 
> At the beginning of the function we make sure remote is not NULL, and
> the remainder of the funciton already depends on it.

I agree with both of these; there used to be code that used a NULL remote 
and just a URL, but that's gone now.

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/transport.c b/transport.c
> index 87581b8..3846aac 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -921,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
>  	ret->remote = remote;
>  	helper = remote->foreign_vcs;
>  
> -	if (!url && remote && remote->url)
> +	if (!url && remote->url)
>  		url = remote->url[0];
>  	ret->url = url;
>  
> 

Re: [PATCH v2] Fix remote.<remote>.vcs

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Except that you missed the "remote == NULL" case (noted below), this is 
> what I was thinking of.
>
> Acked-by: Daniel Barkalow <barkalow@iabervon.org>


>>  	if (!remote)
>>  		die("No remote provided to transport_get()");
>>  
>>  	ret->remote = remote;
>> +	helper = remote->foreign_vcs;
>
> Needs to be "helper = remote ? remote->foreign_vcs : NULL", for the same 
> reason that the test below had been "remote && remote->foreign_vcs".

Even in the presense of "if remote is NULL then we die" in the context
above?

>>  
>>  	if (!url && remote && remote->url)
>>  		url = remote->url[0];
>>  	ret->url = url;
>>  
>> -	/* In case previous URL had helper forced, reset it. */
>> -	remote->foreign_vcs = NULL;
>> -
>>  	/* maybe it is a foreign URL? */
>>  	if (url) {
>>  		const char *p = url;
>> @@ -933,11 +932,11 @@ struct transport *transport_get(struct remote *remote, const char *url)
>>  		while (isalnum(*p))
>>  			p++;
>>  		if (!prefixcmp(p, "::"))
>> -			remote->foreign_vcs = xstrndup(url, p - url);
>> +			helper = xstrndup(url, p - url);
>>  	}
>>  
>> -	if (remote && remote->foreign_vcs) {
>> -		transport_helper_init(ret, remote->foreign_vcs);
>> +	if (helper) {
>> +		transport_helper_init(ret, helper);
>>  	} else if (!prefixcmp(url, "rsync:")) {
>>  		ret->get_refs_list = get_refs_via_rsync;
>>  		ret->fetch = fetch_objs_via_rsync;
>> -- 
>> 1.7.0.rc0.19.gb557e6
>> 
>>