[PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set

Signed-off-by: Nelson Benitez Leon <redacted>
---
 http.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index 8ac8eb6..79cbe50 100644
--- a/http.c
+++ b/http.c

@@ -295,6 +295,16 @@ static CURL *get_curl_handle(void)
 	if (curl_ftp_no_epsv)
 		curl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);
 
+	if (!curl_http_proxy) {
+		const char *env_proxy;
+		env_proxy = getenv("HTTP_PROXY");
+		if (!env_proxy) {
+			env_proxy = getenv("http_proxy");
+		}
+		if (env_proxy) {
+			curl_http_proxy = xstrdup(env_proxy);
+		}
+	}
 	if (curl_http_proxy) {
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);

-- 
1.7.7.6

On Tue, Feb 28, 2012 at 01:54:34PM +0100, Nelson Benitez Leon wrote:

Usually we would prefer environment variables to config. So that:

  $ git config http.proxy foo
  $ HTTP_PROXY=bar git fetch

would use "bar" as the proxy, not "foo". But your code above would
prefer "foo", right?

From reading Thomas's messages, I think there is a slight complication

in that right now curl is respecting $http_proxy, and it is probably
letting git's http.proxy overwrite (though I didn't check). If that is
the case, then that is IMHO a bug that should be fixed. So the rationale
for this patch would be three-fold:

  1. Support HTTP_PROXY, which curl does not accept.

  2. Fix the precedence of environment variables over config.

  3. By handling the proxy variables ourselves, we have more flexibility
     in handling the authentication.

-Peff

On 2/28/12 11:15 AM, Jeff King wrote:

Usually we would prefer environment variables to config. So that:

   $ git config http.proxy foo
   $ HTTP_PROXY=bar git fetch

would use "bar" as the proxy, not "foo". But your code above would
prefer "foo", right?

Apparently I'm the author of the http.proxy feature, though I barely 
remember what problem I was actually solving at the time.  At the time I 
justified it on the grounds that a user might want to use a different 
proxy for git and/or a particular remote.  The "http_proxy" environment 
variable is likely to be a global system default, or perhaps a desktop 
setting, and therefore I'd say probably less and not more specific than 
a git configuration variable.

As to this matter of "HTTP_PROXY", I'm not sure about whether that helps 
or confuses matters to support.  I must admit I'm still confused by the 
motivation of this patch series.

Sam

On Tue, Feb 28, 2012 at 11:27:41AM -0800, Sam Vilain wrote:

On 2/28/12 11:15 AM, Jeff King wrote:

quoted

Usually we would prefer environment variables to config. So that:

  $ git config http.proxy foo
  $ HTTP_PROXY=bar git fetch

would use "bar" as the proxy, not "foo". But your code above would
prefer "foo", right?

Apparently I'm the author of the http.proxy feature, though I barely
remember what problem I was actually solving at the time.  At the
time I justified it on the grounds that a user might want to use a
different proxy for git and/or a particular remote.  The "http_proxy"
environment variable is likely to be a global system default, or
perhaps a desktop setting, and therefore I'd say probably less and
not more specific than a git configuration variable.

Good point. We sometimes follow this order:

  1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if
     it existed)
  2. git config files (i.e., http.proxy)
  3. generic system environment (i.e., $http_proxy).

So thinking about it that way, the original patch makes more sense.

-Peff

On 02/28/2012 08:34 PM, Jeff King wrote:

On Tue, Feb 28, 2012 at 11:27:41AM -0800, Sam Vilain wrote:

quoted

On 2/28/12 11:15 AM, Jeff King wrote:

quoted

Usually we would prefer environment variables to config. So that:

  $ git config http.proxy foo
  $ HTTP_PROXY=bar git fetch

would use "bar" as the proxy, not "foo". But your code above would
prefer "foo", right?

Apparently I'm the author of the http.proxy feature, though I barely
[snip]

Good point. We sometimes follow this order:

  1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if
     it existed)
  2. git config files (i.e., http.proxy)
  3. generic system environment (i.e., $http_proxy).

So thinking about it that way, the original patch makes more sense.

So, in PATCH 2/3, apart from expanding the commit message.. do we want
to support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be
very used by existent programs, but support it it's only a gentenv call..

On Wed, Feb 29, 2012 at 11:46:03AM +0100, Nelson Benitez Leon wrote:

quoted

Good point. We sometimes follow this order:

  1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if
     it existed)
  2. git config files (i.e., http.proxy)
  3. generic system environment (i.e., $http_proxy).

So thinking about it that way, the original patch makes more sense.

So, in PATCH 2/3, apart from expanding the commit message.. do we want
to support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be
very used by existent programs, but support it it's only a gentenv call..

If HTTP_PROXY is not in wide use, I don't see a reason to support it.
And I take back what I said about environment precedence, based on the
discussion. Also, I don't think there is a need to strdup the results of
getenv here, is there? So I think the code you want is just:

  if (!curl_http_proxy)
          curl_http_proxy = getenv("http_proxy");

and the justification for the commit message is that we need to know the
proxy value outside of curl, because the next patch will do some
extra processing on the value.

-Peff

On 02/29/2012 10:08 PM, Jeff King wrote:

On Wed, Feb 29, 2012 at 11:46:03AM +0100, Nelson Benitez Leon wrote:

quoted

quoted

Good point. We sometimes follow this order:

  1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if
     it existed)
  2. git config files (i.e., http.proxy)
  3. generic system environment (i.e., $http_proxy).

So thinking about it that way, the original patch makes more sense.

So, in PATCH 2/3, apart from expanding the commit message.. do we want
to support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be
very used by existent programs, but support it it's only a gentenv call..

If HTTP_PROXY is not in wide use, I don't see a reason to support it.

Ok

And I take back what I said about environment precedence, based on the
discussion. Also, I don't think there is a need to strdup the results of
getenv here, is there? So I think the code you want is just:

  if (!curl_http_proxy)
          curl_http_proxy = getenv("http_proxy");

but curl_http_proxy gets freed in http_cleanup as follows:

free((void *)curl_http_proxy);

Is it ok to free strings returned by getenv() ? I thought nope, so I
used strdup which existent code was already using..

and the justification for the commit message is that we need to know the
proxy value outside of curl, because the next patch will do some
extra processing on the value.

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

On Thu, Mar 01, 2012 at 10:57:03AM +0100, Nelson Benitez Leon wrote:

quoted

And I take back what I said about environment precedence, based on the
discussion. Also, I don't think there is a need to strdup the results of
getenv here, is there? So I think the code you want is just:

  if (!curl_http_proxy)
          curl_http_proxy = getenv("http_proxy");

but curl_http_proxy gets freed in http_cleanup as follows:

free((void *)curl_http_proxy);

Is it ok to free strings returned by getenv() ? I thought nope, so I
used strdup which existent code was already using..

Ah, you're right. I was worried more about lifetime issues (i.e., would
the string still be valid) and didn't check to see whether we freed it
(and we should, because if it comes from config, then it will be
allocated). So yes, you should duplicate it.

-Peff