[PATCH] http.proxy: also mention https_proxy and all_proxy

[PATCH] http.proxy: also mention https_proxy and all_proxy

[Clemens Buchacher]

The current wording of the http.proxy documentation suggests that
http_proxy is somehow equivalent to http.proxy. However, while
http.proxy (by the means of curl's CURLOPT_PROXY option) overrides the
proxy for both HTTP and HTTPS protocols, the http_proxy environment
variable is used only for HTTP. But since the docs mention only
http_proxy, a user might expect it to apply to all HTTP-like protocols.

Avoid any such misunderstanding by explicitly mentioning https_proxy and
all_proxy as well.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

As you can maybe guess from the commit message, I was bitten by this
myself. I also found a couple of questions online where users were
expecting the http_proxy environment variable to apply to HTTPS as well.
Hopefully, this will prevent some confusion in the future.

Clemens

 Documentation/config.txt |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..7d197bb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1258,9 +1258,10 @@ help.autocorrect::
 	This is the default.
 
 http.proxy::
-	Override the HTTP proxy, normally configured using the 'http_proxy'
-	environment variable (see linkgit:curl[1]).  This can be overridden
-	on a per-remote basis; see remote.<name>.proxy
+	Override the HTTP proxy, normally configured using the 'http_proxy',
+	'https_proxy', and 'all_proxy' environment variables (see
+	linkgit:curl[1]).  This can be overridden on a per-remote basis; see
+	remote.<name>.proxy
 
 http.cookiefile::
 	File containing previously stored cookie lines which should be used
-- 
1.7.9.1

Re: [PATCH] http.proxy: also mention https_proxy and all_proxy

[Jeff King]

On Sat, Mar 03, 2012 at 03:50:53PM +0100, Clemens Buchacher wrote:

> The current wording of the http.proxy documentation suggests that
> http_proxy is somehow equivalent to http.proxy. However, while
> http.proxy (by the means of curl's CURLOPT_PROXY option) overrides the
> proxy for both HTTP and HTTPS protocols, the http_proxy environment
> variable is used only for HTTP. But since the docs mention only
> http_proxy, a user might expect it to apply to all HTTP-like protocols.

Hmm, I didn't know that. This certainly adds an interesting twist to the
patch in a nearby thread to start reading http_proxy ourselves.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index abeb82b..7d197bb 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1258,9 +1258,10 @@ help.autocorrect::
>  	This is the default.
>  
>  http.proxy::
> -	Override the HTTP proxy, normally configured using the 'http_proxy'
> -	environment variable (see linkgit:curl[1]).  This can be overridden
> -	on a per-remote basis; see remote.<name>.proxy
> +	Override the HTTP proxy, normally configured using the 'http_proxy',
> +	'https_proxy', and 'all_proxy' environment variables (see
> +	linkgit:curl[1]).  This can be overridden on a per-remote basis; see
> +	remote.<name>.proxy

Text looks OK. I think this linkgit:curl is wrong, though. In the
manpages, it formats as simply curl(1), but in the HTML pages, it
creates a link to curl.html, which does not exist. This is not a problem
introduced by your patch, obviously, but maybe it is worth cleaning up.
I think just using:

  `curl(1)`

might be sufficient.

-Peff

[RFH] 'man:' macro for asciidoc

[Jakub Narebski]

Jeff King <peff@peff.net> writes:
> On Sat, Mar 03, 2012 at 03:50:53PM +0100, Clemens Buchacher wrote:

> >  http.proxy::
> > -	Override the HTTP proxy, normally configured using the 'http_proxy'
> > -	environment variable (see linkgit:curl[1]).  This can be overridden
> > -	on a per-remote basis; see remote.<name>.proxy
> > +	Override the HTTP proxy, normally configured using the 'http_proxy',
> > +	'https_proxy', and 'all_proxy' environment variables (see
> > +	linkgit:curl[1]).  This can be overridden on a per-remote basis; see
> > +	remote.<name>.proxy
> 
> Text looks OK. I think this linkgit:curl is wrong, though. In the
> manpages, it formats as simply curl(1), but in the HTML pages, it
> creates a link to curl.html, which does not exist. This is not a problem
> introduced by your patch, obviously, but maybe it is worth cleaning up.
> I think just using:
> 
>   `curl(1)`
> 
> might be sufficient.

BTW it wouldreally be nice be able to use simply  man:curl[1]  instead
of worrying if it shoudl be `curl(1)`, or *curl*(1), or curl(1)


-- 
Jakub Narebski

Re: [PATCH] http.proxy: also mention https_proxy and all_proxy

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Mar 03, 2012 at 03:50:53PM +0100, Clemens Buchacher wrote:
>
>> The current wording of the http.proxy documentation suggests that
>> http_proxy is somehow equivalent to http.proxy. However, while
>> http.proxy (by the means of curl's CURLOPT_PROXY option) overrides the
>> proxy for both HTTP and HTTPS protocols, the http_proxy environment
>> variable is used only for HTTP. But since the docs mention only
>> http_proxy, a user might expect it to apply to all HTTP-like protocols.
>
> Hmm, I didn't know that. This certainly adds an interesting twist to the
> patch in a nearby thread to start reading http_proxy ourselves.

Yeah, I too had an "Ahhhh" moment.

I recall that I was frustrated more than 10 years ago when I had to point
at our company proxy with many *_proxy variables in my .login and ended up
writing a shell script (and placed it under source control) that generates
dot files from a single source (which of course is under source control).

The manual page for curl seems to say that they have separate HTTPS_PROXY
and friends all in caps, and http_proxy is left lowercase for historical
reasons [*1*].

As to the way forward, I suspect that http.proxy was a mistake to begin
with, considering the structure of namespace our configuration variables
fit in.  Shouldn't they be proxy.http, proxy.https, etc.?

And if we were to map curl's HTTPS_PROXY to proxy.https to fix this
namespace gotha, we could treat http.proxy as a historical mistake, and
introduce proxy.http as what matches curl's http_proxy.

Then the change proposed in the nearby thread can notice http_proxy and
behave as if proxy.http is set, which would only affect http but not
https.  When http.proxy is set, we still let it affect anything curl
handles.


[Footnote]

*1* Come to think of it, the alleged Mac OS X breakage I mentined may
merely be a second-hand rumor of a misdiagnosed user error---the user
might have been trying to reach https:// resource with only http_proxy
set.

Re: [PATCH] http.proxy: also mention https_proxy and all_proxy

[Clemens Buchacher]

On Sat, Mar 03, 2012 at 02:22:39PM -0800, Junio C Hamano wrote:
> 
> As to the way forward, I suspect that http.proxy was a mistake to begin
> with, considering the structure of namespace our configuration variables
> fit in.  Shouldn't they be proxy.http, proxy.https, etc.?

I actually prefer the current behavior, which is to configure only one
proxy for all protocols. I have not seen a setup where HTTP and HTTPS
are routed through different proxies before. But if this is really
needed, one has the option to use the environment variable, or
remote.<name>.proxy.

I suggest instead that we map curl's CURLOPT_PROXY to core.proxy.  That
would also fit well with the remote.<name>.proxy scheme.

[PATCH v2] http.proxy: also mention https_proxy and all_proxy

[Clemens Buchacher]

The current wording of the http.proxy documentation suggests that
http_proxy is somehow equivalent to http.proxy. However, while
http.proxy (by the means of curl's CURLOPT_PROXY option) overrides the
proxy for both HTTP and HTTPS protocols, the http_proxy environment
variable is used only for HTTP. But since the docs mention only
http_proxy, a user might expect it to apply to all HTTP-like protocols.

Avoid any such misunderstanding by explicitly mentioning https_proxy and
all_proxy as well.

Also replace linkgit:curl[1] with a literal 'curl(1)', because the
former gets translated to a dead link in the HTML pages.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sat, Mar 03, 2012 at 12:42:52PM -0500, Jeff King wrote:
>
> I think just using: `curl(1)` might be sufficient.

Done.

 Documentation/config.txt |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..f0ab288 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1258,9 +1258,10 @@ help.autocorrect::
 	This is the default.
 
 http.proxy::
-	Override the HTTP proxy, normally configured using the 'http_proxy'
-	environment variable (see linkgit:curl[1]).  This can be overridden
-	on a per-remote basis; see remote.<name>.proxy
+	Override the HTTP proxy, normally configured using the 'http_proxy',
+	'https_proxy', and 'all_proxy' environment variables (see
+	`curl(1)`).  This can be overridden on a per-remote basis; see
+	remote.<name>.proxy
 
 http.cookiefile::
 	File containing previously stored cookie lines which should be used
-- 
1.7.9.2

Re: [PATCH] http.proxy: also mention https_proxy and all_proxy

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Sat, Mar 03, 2012 at 02:22:39PM -0800, Junio C Hamano wrote:
>> 
>> As to the way forward, I suspect that http.proxy was a mistake to begin
>> with, considering the structure of namespace our configuration variables
>> fit in.  Shouldn't they be proxy.http, proxy.https, etc.?
>
> I actually prefer the current behavior, which is to configure only one
> proxy for all protocols. I have not seen a setup where HTTP and HTTPS
> are routed through different proxies before. But if this is really
> needed, one has the option to use the environment variable, or
> remote.<name>.proxy.
>
> I suggest instead that we map curl's CURLOPT_PROXY to core.proxy.  That
> would also fit well with the remote.<name>.proxy scheme.

Contaminating core.* namespace would not solve anything.

People are already relying on http.proxy to apply to any cURL transport,
so we will keep supporting it anyway.  There is no justification to teach
new people to learn that both exists, and the transport is not core anyway; 
it is still cURL specific.

I forgot about remote.<name>.proxy that already can let you use different
proxies for http/https, and the use of different ones per protocol would
be rare to begin with, so I agree that we would not have to worry about
introducing proxy.{http,https,...}.

Thanks.

Re: [PATCH v2] http.proxy: also mention https_proxy and all_proxy

[Junio C Hamano]

Thanks; will queue.