On Mon, Apr 10, 2017 at 06:15:56PM +0300, Sergey Ryazanov wrote:
> 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...
> 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
