Re: [PATCH v2 3/3] http: when proxy url has username but no password, ask for password

[Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es>]

On 03/01/2012 10:58 PM, Jeff King wrote:
> On Thu, Mar 01, 2012 at 09:49:16AM -0800, Sam Vilain wrote:
> 
>>>  	if (curl_http_proxy) {
>>> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>>> +		credential_from_url(&proxy_auth, curl_http_proxy);
>>> +		if (proxy_auth.username != NULL&&  proxy_auth.password == NULL) {
>>> +			/* proxy string has username but no password, ask for password */
>>> +			struct strbuf pbuf = STRBUF_INIT;
>>> +			credential_fill(&proxy_auth);
>>
>> Wouldn't it be better to wait until the proxy returns a 403 before
>> assuming that the proxy setting is incorrect/missing a password?
>> What if the administrator expects the user to fill in both the
>> username and password?  That is the behaviour of a web browser.
>>
>> Also, I think you should wait until that 403 to detect whether the
>> proxy setting came from the environment, and only load it explicitly
>> then.
> 
> It's worth looking at what the http auth code does here.
> 
[snip]
> overhaul it.
> 
> Complicating all of this is the fact that I think Nelson's original
> patch was based on an older, pre-986bbc0 version of git, which is why he
> followed the pre-prompt route, copying the style of regular http auth.
> 
> So there's the history lesson. What should proxy auth do?
> 
>   1. Definitely respond to HTTP 407 by prompting on the fly; this code
>      should go along-side the HTTP 401 code in http.c.
> 
>   2. Definitely do the pre-prompt thing when http_proactive_auth is set
>      (which is used only by http-push). Unless somebody really feels
>      like re-writing http-push to handle retries for authentication.
> 
>   3. Consider doing the pre-prompt thing when http_proactive_auth is not
>      set. This can save a round-trip, but we should not do it if there
>      is a good reason not to. The two possible reasons I can think of
>      are:
> 
>        a. Like http auth, if curl will read the proxy credentials from
>           .netrc, then we should not do it for the same reasons
>           mentioned in 986bbc0.
> 
>        b. If people realistically have proxy URLs with usernames but do
>           _not_ want to ask for a password, then the prompt will be
>           annoying. I'm not sure that anybody expects that.

So, trying to sum up, I will try to redo patch-set as follows:
- Ignore PATCH 2/3 , that is, we won't read any env var.
- Let cURL try to connect and if that fails with 407 , then do a credential_fill
and try to reconnect.

Is that ok? or do I need to do something more?