Re: [PATCH v2 1/3]http: authenticate on NTLM proxies and others suppported by CuRL

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2012, 18:50 UTC
Message-ID: <7v4nu6et1x.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4F50D16D.3090800@seap.minhap.es>

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

Is there anything specific about NTLM in this patch? Let's retitle it to something like

	http: support proxies that needs authentication

> ...
> So as CURLAUTH_ANY provide us out-of-the-box proxy support, we don't
> want it activated manually from a config option, instead we added it
> automatically when a proxy is being used.

I can sort of parse this paragraph but cannot follow the logic.

 - Because we use CURLAUTH_ANY, a proxy that requires authentication is
   supported out-of-the-box.

 - We do not want having to manually enable it.

 - Instead of adding a manual configuration, we do so automatically.

It sounds like it is saying the same thing three times. Do we even need that paragraph? Shouldn't we drop it instead?

Otherwise it all looks good, so you can either

 (1) explain why the above suggestions are wrong and why I should apply
     the message I am responding to as-is;

 (2) send another re-roll; or

 (3) say "yeah, your suggestions all sound good to me", in which case I'll
     just locally amend and apply.

Thanks.

Show 22 quoted lines

>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
>  http.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/http.c b/http.c
> index 0ffd79c..8ac8eb6 100644
> --- a/http.c
> +++ b/http.c
> @@ -295,8 +295,10 @@ static CURL *get_curl_handle(void)
>  	if (curl_ftp_no_epsv)
>  		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
>  
> -	if (curl_http_proxy)
> +	if (curl_http_proxy) {
>  		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +	}
>  
>  	return result;
>  }