v2http: honnor empty http.proxy option to bypass proxy

>
>     export http_proxy=http://network-proxy/
>     cd ~/foobar-project
>     git config remote.origin.proxy ""
>     git fetch
>
> Previously, proxy host was configured by one line:
>
>     curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>
> Commit 372370f (http: use credential API to handle proxy auth...) parses
> proxy option, extracts proxy host address and additionaly updates curl
> configuration, which makes previous call a noop:

>
>     credential_from_url(&proxy_auth, curl_http_proxy);
>     curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
>
> But if proxy option is empty then proxy host field become NULL this
> force curl to fallback to proxy configuration detection from
> environment. This caused empty http.proxy option not working any more.

>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> ---
>
> Changes since v1:
>  - explicitly handle this case instead of mangling the common code
>
>  http.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 96d84bb..8be75b2 100644
> --- a/http.c
> +++ b/http.c
> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>                 }
>         }
>
> -       if (curl_http_proxy) {
> -               curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +       if (curl_http_proxy && curl_http_proxy[0] == '\0') {
> +               /*
> +                * Handle case with the empty http.proxy value here to keep
> +                * common code clean.
> +                * NB: empty option disables proxying at all.
> +                */
> +               curl_easy_setopt(result, CURLOPT_PROXY, "");
> +       } else if (curl_http_proxy) {
>  #if LIBCURL_VERSION_NUM >= 0x071800
>                 if (starts_with(curl_http_proxy, "socks5h"))
>                         curl_easy_setopt(result,
> --
> 2.10.2
>

> diff --git a/http.c b/http.c
> index 96d84bb..8be75b2 100644
> --- a/http.c
> +++ b/http.c
> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>  		}
>  	}
>  
> -	if (curl_http_proxy) {
> -		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +	if (curl_http_proxy && curl_http_proxy[0] == '\0') {
> +		/*
> +		 * Handle case with the empty http.proxy value here to keep
> +		 * common code clean.
> +		 * NB: empty option disables proxying at all.
> +		 */
> +		curl_easy_setopt(result, CURLOPT_PROXY, "");
> +	} else if (curl_http_proxy) {

> On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:
>> diff --git a/http.c b/http.c
>> index 96d84bb..8be75b2 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>>               }
>>       }
>>
>> -     if (curl_http_proxy) {
>> -             curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>> +     if (curl_http_proxy && curl_http_proxy[0] == '\0') {
>> +             /*
>> +              * Handle case with the empty http.proxy value here to keep
>> +              * common code clean.
>> +              * NB: empty option disables proxying at all.
>> +              */
>> +             curl_easy_setopt(result, CURLOPT_PROXY, "");
>> +     } else if (curl_http_proxy) {
>
> This seems pretty reasonable. You could bump this under the existing "if
> (curl_http_proxy)" condition, but that would add an extra level of
> indentation to the rest of the parsing.
>

> One thing I do wonder, though: can credential_from_url() ever return a
> NULL host field for other inputs, and what should we do on those inputs?
>
> For example, if I set the value to just "https://" with no hostname,
> then I think we'll end up with a NULL proxy_auth.host field. And we'll
> feed that NULL to CURLOPT_PROXY, which will cause it to use the
> defaults.
>
> I don't know _what_ "https://" should do. It's clearly bogus. But
> telling curl to use the defaults seems funny. In that sense, your
> original was much better (we'd feed it to curl, which would be free to
> complain).
>

> > I don't know _what_ "https://" should do. It's clearly bogus. But
> > telling curl to use the defaults seems funny. In that sense, your
> > original was much better (we'd feed it to curl, which would be free to
> > complain).
> >
> 
> I thought about this situation too. IMHO the best solution here is to
> check host after credential_from_url() call. If host is NULL then we
> should show warning message and exit. Then user could fix its
> configuration.
> 
> Since I in any case will send v3 with grammar fixes then I could add
> new patch to the series. This new patch will check host for NULL and
> print warning message. Are you Ok with such solution?