Re: [PATCH] http: honnor empty http.proxy option to bypass proxy

From: Jeff King <peff@peff.net>
Date: Apr 10, 2017, 16:33 UTC
Message-ID: <20170410163350.3zqm33tgqafsp76u@sigill.intra.peff.net>
In-Reply-To: <20170410151556.10054-1-ryazanov.s.a@gmail.com>

On Mon, Apr 10, 2017 at 06:15:56PM +0300, Sergey Ryazanov wrote:

Show 27 quoted lines

> Curl distinguish between empty proxy address and NULL proxy address. In
> the first case it completly disable proxy usage, but if proxy address
> option is NULL then curl attempt to determine proxy address from
> http_proxy environment variable.
> 
> According to documentation, if http.proxy configured to empty string
> then git should bypass proxy and connects to the server directly:
> 
>     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:
> 
>     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.

That makes sense. And if I understand correctly, this was a regression in 372370f; before that we fed curl_http_proxy directly, and it was either NULL or not, depending on whether we had seen the config option.

It looks like we _still_ set CURLOPT_PROXY to curl_http_proxy, and then immediately afterward set it to proxy_auth.host. That should make the first one always a noop, I would think, and it should be removed.

But...

Show 15 quoted lines

> diff --git a/http.c b/http.c
> index 96d84bb..bf0e709 100644
> --- a/http.c
> +++ b/http.c
> @@ -861,7 +861,12 @@ static CURL *get_curl_handle(void)
>  			strbuf_release(&url);
>  		}
>  
> -		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
> +		/*
> +		 * Avoid setting CURLOPT_PROXY to NULL if empty http.proxy
> +		 * option configured.
> +		 */
> +		if (proxy_auth.host)
> +			curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);

Here that second one becomes conditional, and we rely on the earlier setting (but only sometimes). I would think this whole thing would be more clear if we dropped the first CURLOPT_PROXY call entirely, and just did:

  /*
   * If we parsed a null host from the URL, we must convert that
   * back into an empty string so that curl knows we want no proxy at
   * all (not to find the default).
   */
  curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host ?
                                          proxy_auth.host : "");

-Peff