Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

[Jeff King <peff@xxxxxxxx>]

On Thu, May 03, 2012 at 06:39:47PM +0200, Nelson Benitez Leon wrote:

> +static const char *read_prot_proxy_env(const char *protocol)
> +{
> +	const char *env_proxy;
> +	struct strbuf var = STRBUF_INIT;
> +
> +	strbuf_addf(&var, "%s_proxy", protocol);
> +	env_proxy = getenv(var.buf);
> +	if (!env_proxy && strcmp("http_proxy", var.buf)) {
> +		char *p;
> +		for (p = var.buf; *p; p++)
> +			*p = toupper(*p);
> +		env_proxy = getenv(var.buf);
> +	}
> +	strbuf_release(&var);
> +	
> +	return env_proxy;
> +}

Thanks, this is way more readable than the previous iteration.

> +static int host_allowed_by_noproxy_env (const char *host)
> +{
> +	const char *no_proxy = getenv("no_proxy");
> +	if (!no_proxy)
> +		no_proxy = getenv("NO_PROXY");
> +	if (!no_proxy ||
> +	    (strcmp("*", no_proxy) &&
> +	     !strstr(no_proxy, host)))
> +		return 1;
> +	
> +	return 0;
> +}

This simplified parsing misses a lot of corner cases. Three I can see
right off the bat:

  1. If your NO_PROXY is "no-proxy.com", and your host is
     "proxy.com", your code will consider that a match, but curl does
     not.

  2. If your NO_PROXY contains "no-proxy.com", but your host is
     "www.no-proxy.com", curl will consider that a match, but your code
     does not.

  3. If your NO_PROXY contains "no-proxy.com", but your host is
     "no-proxy.com:80", curl will consider that a match, but your code
     does not.

I don't see any way around it besides implementing curl's full
tokenizing and matching algorithm, which is about a page of code. I'd
really prefer not to re-implement bits of curl (especially because they
may change later), but AFAIK there is no way to ask curl "is there a
proxy configured, and if so, what is it?".

The rest of this patch looks OK to me, though.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html