[PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Nelson Benitez Leon]

CuRL already reads it, but if $http_proxy has username but no password
curl will not ask you for the password.. so we read it ourselves to
detect that and ask for the password.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
 http.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index 8ac8eb6..8932da5 100644
--- a/http.c
+++ b/http.c
@@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);

+	if (!curl_http_proxy) {
+		const char *env_proxy;
+		env_proxy = getenv("http_proxy");
+		if (env_proxy) {
+			curl_http_proxy = xstrdup(env_proxy);
+		}
+	}
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-- 
1.7.7.6

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Sam Vilain]

On 3/1/12 10:21 AM, Nelson Benitez Leon wrote:
> CuRL already reads it, but if $http_proxy has username but no password
> curl will not ask you for the password.. so we read it ourselves to
> detect that and ask for the password.

That's not what this change does.  This change explicitly loads from the 
environment the 'http_proxy' variable and sets up curl to use it.  As 
Junio said, this is (on its own) a regression.

Sam


> Signed-off-by: Nelson Benitez Leon<nbenitezl@gmail.com>
> ---
>   http.c |    7 +++++++
>   1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8ac8eb6..8932da5 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
>   	if (curl_ftp_no_epsv)
>   		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>
> +	if (!curl_http_proxy) {
> +		const char *env_proxy;
> +		env_proxy = getenv("http_proxy");
> +		if (env_proxy) {
> +			curl_http_proxy = xstrdup(env_proxy);
> +		}
> +	}
>   	if (curl_http_proxy) {
>   		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>   		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Sam Vilain <sam@vilain.net> writes:

> On 3/1/12 10:21 AM, Nelson Benitez Leon wrote:
>> CuRL already reads it, but if $http_proxy has username but no password
>> curl will not ask you for the password.. so we read it ourselves to
>> detect that and ask for the password.
>
> That's not what this change does.  This change explicitly loads from
> the environment the 'http_proxy' variable and sets up curl to use it.
> As Junio said, this is (on its own) a regression.

Just to make sure there is no understanding down the road, I only
expressed a concern that this _might_ be a regression. That Mac OS X
behaviour is not something I observed first-hand.

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

> CuRL already reads it, but if $http_proxy has username but no password
> curl will not ask you for the password.. so we read it ourselves to
> detect that and ask for the password.

Please stop the double-dot.  Also your capitalization for cURL is screwed
up.

More importantly, please describe what happens after "will not ask".
"will not ask you for the password and the connection fails"?
"will not ask you for the password and the gives an error message saying
'authentication failure'"?

The logic in the patch, needless to say, seems OK, though.

Thanks.

>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
>  http.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8ac8eb6..8932da5 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,6 +295,13 @@ static CURL *get_curl_handle(void)
>  	if (curl_ftp_no_epsv)
>  		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>
> +	if (!curl_http_proxy) {
> +		const char *env_proxy;
> +		env_proxy = getenv("http_proxy");
> +		if (env_proxy) {
> +			curl_http_proxy = xstrdup(env_proxy);
> +		}
> +	}
>  	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>  		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Jeff King]

On Thu, Mar 01, 2012 at 11:10:38AM -0800, Junio C Hamano wrote:

> Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:
> 
> > CuRL already reads it, but if $http_proxy has username but no password
> > curl will not ask you for the password.. so we read it ourselves to
> > detect that and ask for the password.
> 
> Please stop the double-dot.  Also your capitalization for cURL is screwed
> up.
> 
> More importantly, please describe what happens after "will not ask".
> "will not ask you for the password and the connection fails"?
> "will not ask you for the password and the gives an error message saying
> 'authentication failure'"?

When we need to authenticate for the destination webserver, we detect an
HTTP 401, _then_ ask for the credentials, and retry the request. I'm
curious what the error condition is for the authentication failure, and
if we can do the same here (from a brief skim of rfc2616, it looks like
it should be a 407, but I do not even have a proxy set up to try).

-Peff

Re: [PATCH v2 2/3] http: try http_proxy env var when http.proxy config option is not set

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When we need to authenticate for the destination webserver, we detect an
> HTTP 401, _then_ ask for the credentials, and retry the request. I'm
> curious what the error condition is for the authentication failure, and
> if we can do the same here.

Yeah, that would be ideal.