{"thread":{"id":"29771","subject":"[PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","startedAt":"2012-02-28T12:19:00Z","lastAt":"2012-03-15T09:38:26Z","messageCount":24,"participants":["Nelson Benitez Leon","Thomas Rast","Jeff King","Junio C Hamano","Sam Vilain","Matthieu Moy","Daniel Stenberg","James Cloos"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"185616","messageId":"878vjn8823.fsf@thomas.inf.ethz.ch","threadId":"29771","inReplyTo":"4F4CCE8A.4010800@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-28T12:19:00Z","receivedAt":"2012-02-28T12:19:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Nelson Benitez Leon writes:\n\n> +\tif (!curl_http_proxy) {\n> +\t\tconst char *env_proxy;\n> +\t\tenv_proxy = getenv(\"HTTP_PROXY\");\n> +\t\tif (!env_proxy) {\n> +\t\t\tenv_proxy = getenv(\"http_proxy\");\n> +\t\t}\n> +\t\tif (env_proxy) {\n> +\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n> +\t\t}\n> +\t}\n\nAdmittedly I'm mostly clueless about curl, but while investigating the\nNTLM login thing I noticed this bit in curl(1):\n\nENVIRONMENT\n The environment variables can be specified in lower case or upper\n case. The lower case version has precedence. http_proxy is an\n exception as it is only available in lower case.\n\nWhich raises the questions:\n\n* Why is this needed? Does git's use of libcurl ignore http_proxy? [1]\n seems to indicate that libcurl respects _proxy\n automatically.\n\n* Why do you (need to?) support HTTP_PROXY when curl doesn't?\n\n\n[1] http://curl.haxx.se/libcurl/c/libcurl-tutorial.html, \"Environment Variables\"\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185603","messageId":"4F4CCE8A.4010800@seap.minhap.es","threadId":"29771","inReplyTo":null,"subject":"[PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-02-28T12:54:34Z","receivedAt":"2012-02-28T12:54:34Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"Signed-off-by: Nelson Benitez Leon \n---\n http.c | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 8ac8eb6..79cbe50 100644\n--- a/http.c\n+++ b/http.c\n@@ -295,6 +295,16 @@ static CURL *get_curl_handle(void)\n \tif (curl_ftp_no_epsv)\n \t\tcurl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);\n \n+\tif (!curl_http_proxy) {\n+\t\tconst char *env_proxy;\n+\t\tenv_proxy = getenv(\"HTTP_PROXY\");\n+\t\tif (!env_proxy) {\n+\t\t\tenv_proxy = getenv(\"http_proxy\");\n+\t\t}\n+\t\tif (env_proxy) {\n+\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n+\t\t}\n+\t}\n \tif (curl_http_proxy) {\n \t\tcurl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);\n \t\tcurl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n-- \n1.7.7.6\n"},{"id":"185626","messageId":"87mx8358nu.fsf@thomas.inf.ethz.ch","threadId":"29771","inReplyTo":"4F4CEB5D.5020808@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-02-28T14:34:13Z","receivedAt":"2012-02-28T14:34:13Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Nelson Benitez Leon writes:\n\n> On 02/28/2012 01:19 PM, Thomas Rast wrote:\n>> \n>> * Why is this needed? Does git's use of libcurl ignore http_proxy? [1]\n>> seems to indicate that libcurl respects _proxy\n>> automatically.\n>\n> It could not be needed, because, as you noted, curl already reads it, but then we will\n> loose the feature on patch [3/3] because if $http_proxy has username but no password\n> curl will not ask you for the password.. instead if we read it we could detect that,\n> and ask for the password. \n\nOk. An explanation along these lines should definitely go into the\ncommit message!\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185622","messageId":"4F4CEB5D.5020808@seap.minhap.es","threadId":"29771","inReplyTo":"878vjn8823.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-02-28T14:57:33Z","receivedAt":"2012-02-28T14:57:33Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 02/28/2012 01:19 PM, Thomas Rast wrote:\n> Nelson Benitez Leon writes:\n> \n>> +\tif (!curl_http_proxy) {\n>> +\t\tconst char *env_proxy;\n>> +\t\tenv_proxy = getenv(\"HTTP_PROXY\");\n>> +\t\tif (!env_proxy) {\n>> +\t\t\tenv_proxy = getenv(\"http_proxy\");\n>> +\t\t}\n>> +\t\tif (env_proxy) {\n>> +\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n>> +\t\t}\n>> +\t}\n> \n> Admittedly I'm mostly clueless about curl, but while investigating the\n> NTLM login thing I noticed this bit in curl(1):\n> \n> ENVIRONMENT\n> The environment variables can be specified in lower case or upper\n> case. The lower case version has precedence. http_proxy is an\n> exception as it is only available in lower case.\n> \n> Which raises the questions:\n> \n> * Why is this needed? Does git's use of libcurl ignore http_proxy? [1]\n> seems to indicate that libcurl respects _proxy\n> automatically.\n\nIt could not be needed, because, as you noted, curl already reads it, but then we will\nloose the feature on patch [3/3] because if $http_proxy has username but no password\ncurl will not ask you for the password.. instead if we read it we could detect that,\nand ask for the password. \n\nAs a minor note if we let curl to read it then patch [1/1] has\nto be changed to include CURLOPT_PROXYAUTH unconditionally (ie. out of the \n'if (curl_http_proxy)'). I personally like the feature of not writing my password \non $http_proxy at the cost of reading the env vars ourselves.\n\n\n> \n> * Why do you (need to?) support HTTP_PROXY when curl doesn't?\n\nI found somewhere documented HTTP_PROXY as well as http_proxy, but I've just checked \nwget[1] and also only supports http_proxy so I think we can discard it as is not widely \nused..\n\n[1] http://www.gnu.org/software/wget/manual/html_node/Proxies.html\n\n> \n> \n> [1] http://curl.haxx.se/libcurl/c/libcurl-tutorial.html, \"Environment Variables\"\n> \n"},{"id":"185656","messageId":"20120228191514.GD11260@sigill.intra.peff.net","threadId":"29771","inReplyTo":"4F4CCE8A.4010800@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T19:15:14Z","receivedAt":"2012-02-28T19:15:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 01:54:34PM +0100, Nelson Benitez Leon wrote:\n\n> diff --git a/http.c b/http.c\n> index 8ac8eb6..79cbe50 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -295,6 +295,16 @@ static CURL *get_curl_handle(void)\n> \tif (curl_ftp_no_epsv)\n> \t\tcurl_easy_setopt(result, CURLOPT_FTP_USE_EPSV, 0);\n> \n> +\tif (!curl_http_proxy) {\n> +\t\tconst char *env_proxy;\n> +\t\tenv_proxy = getenv(\"HTTP_PROXY\");\n> +\t\tif (!env_proxy) {\n> +\t\t\tenv_proxy = getenv(\"http_proxy\");\n> +\t\t}\n> +\t\tif (env_proxy) {\n> +\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n> +\t\t}\n> +\t}\n\nUsually we would prefer environment variables to config. So that:\n\n $ git config http.proxy foo\n $ HTTP_PROXY=bar git fetch\n\nwould use \"bar\" as the proxy, not \"foo\". But your code above would\nprefer \"foo\", right?\n\n>From reading Thomas's messages, I think there is a slight complication\nin that right now curl is respecting $http_proxy, and it is probably\nletting git's http.proxy overwrite (though I didn't check). If that is\nthe case, then that is IMHO a bug that should be fixed. So the rationale\nfor this patch would be three-fold:\n\n 1. Support HTTP_PROXY, which curl does not accept.\n\n 2. Fix the precedence of environment variables over config.\n\n 3. By handling the proxy variables ourselves, we have more flexibility\n in handling the authentication.\n\n-Peff\n"},{"id":"185659","messageId":"7v62eqzrqm.fsf@alter.siamese.dyndns.org","threadId":"29771","inReplyTo":"878vjn8823.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T19:24:01Z","receivedAt":"2012-02-28T19:24:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Which raises the questions:\n>\n> * Why is this needed? Does git's use of libcurl ignore http_proxy? [1]\n> seems to indicate that libcurl respects _proxy\n> automatically.\n>\n> * Why do you (need to?) support HTTP_PROXY when curl doesn't?\n\nLet me add a third bullet point.\n\nI've heard rumors that libcurl on some versions/installations of Mac OS X\ndeliberately ignores the environment. For those who agree with Apple, it\nwould be a regression if we suddenly start the environment ourselves and\nusing it.\n"},{"id":"185660","messageId":"4F4D2AAD.3040107@vilain.net","threadId":"29771","inReplyTo":"20120228191514.GD11260@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2012-02-28T19:27:41Z","receivedAt":"2012-02-28T19:27:41Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"On 2/28/12 11:15 AM, Jeff King wrote:\n> Usually we would prefer environment variables to config. So that:\n>\n> $ git config http.proxy foo\n> $ HTTP_PROXY=bar git fetch\n>\n> would use \"bar\" as the proxy, not \"foo\". But your code above would\n> prefer \"foo\", right?\n\nApparently I'm the author of the http.proxy feature, though I barely \nremember what problem I was actually solving at the time. At the time I \njustified it on the grounds that a user might want to use a different \nproxy for git and/or a particular remote. The \"http_proxy\" environment \nvariable is likely to be a global system default, or perhaps a desktop \nsetting, and therefore I'd say probably less and not more specific than \na git configuration variable.\n\nAs to this matter of \"HTTP_PROXY\", I'm not sure about whether that helps \nor confuses matters to support. I must admit I'm still confused by the \nmotivation of this patch series.\n\nSam\n"},{"id":"185663","messageId":"20120228193443.GB11725@sigill.intra.peff.net","threadId":"29771","inReplyTo":"4F4D2AAD.3040107@vilain.net","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-28T19:34:43Z","receivedAt":"2012-02-28T19:34:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2012 at 11:27:41AM -0800, Sam Vilain wrote:\n\n> On 2/28/12 11:15 AM, Jeff King wrote:\n> >Usually we would prefer environment variables to config. So that:\n> >\n> > $ git config http.proxy foo\n> > $ HTTP_PROXY=bar git fetch\n> >\n> >would use \"bar\" as the proxy, not \"foo\". But your code above would\n> >prefer \"foo\", right?\n> \n> Apparently I'm the author of the http.proxy feature, though I barely\n> remember what problem I was actually solving at the time. At the\n> time I justified it on the grounds that a user might want to use a\n> different proxy for git and/or a particular remote. The \"http_proxy\"\n> environment variable is likely to be a global system default, or\n> perhaps a desktop setting, and therefore I'd say probably less and\n> not more specific than a git configuration variable.\n\nGood point. We sometimes follow this order:\n\n 1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if\n it existed)\n 2. git config files (i.e., http.proxy)\n 3. generic system environment (i.e., $http_proxy).\n\nSo thinking about it that way, the original patch makes more sense.\n\n-Peff\n"},{"id":"185720","messageId":"vpqlinmq7zd.fsf@bauges.imag.fr","threadId":"29771","inReplyTo":"20120228193443.GB11725@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-02-29T09:55:34Z","receivedAt":"2012-02-29T09:55:34Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> Good point. We sometimes follow this order:\n>\n> 1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if\n> it existed)\n> 2. git config files (i.e., http.proxy)\n> 3. generic system environment (i.e., $http_proxy).\n\nYes, just like $EDITOR << core.editor << $GIT_EDITOR.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"185717","messageId":"4F4E003C.1050301@seap.minhap.es","threadId":"29771","inReplyTo":"7v62eqzrqm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-02-29T10:38:52Z","receivedAt":"2012-02-29T10:38:52Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 02/28/2012 08:24 PM, Junio C Hamano wrote:\n> Thomas Rast writes:\n> \n>> Which raises the questions:\n>>\n>> * Why is this needed? Does git's use of libcurl ignore http_proxy? [1]\n>> seems to indicate that libcurl respects _proxy\n>> automatically.\n>>\n>> * Why do you (need to?) support HTTP_PROXY when curl doesn't?\n> \n> Let me add a third bullet point.\n> \n> I've heard rumors that libcurl on some versions/installations of Mac OS X\n> deliberately ignores the environment. For those who agree with Apple, it\n> would be a regression if we suddenly start the environment ourselves and\n> using it.\n\nHi Junio, what did you mean by \"we start the environment and using it\"?\nI didn't understand what you mean there..\n"},{"id":"185719","messageId":"4F4E01EB.3070707@seap.minhap.es","threadId":"29771","inReplyTo":"20120228193443.GB11725@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-02-29T10:46:03Z","receivedAt":"2012-02-29T10:46:03Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 02/28/2012 08:34 PM, Jeff King wrote:\n> On Tue, Feb 28, 2012 at 11:27:41AM -0800, Sam Vilain wrote:\n> \n>> On 2/28/12 11:15 AM, Jeff King wrote:\n>>> Usually we would prefer environment variables to config. So that:\n>>>\n>>> $ git config http.proxy foo\n>>> $ HTTP_PROXY=bar git fetch\n>>>\n>>> would use \"bar\" as the proxy, not \"foo\". But your code above would\n>>> prefer \"foo\", right?\n>>\n>> Apparently I'm the author of the http.proxy feature, though I barely\n>> [snip]\n> \n> Good point. We sometimes follow this order:\n> \n> 1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if\n> it existed)\n> 2. git config files (i.e., http.proxy)\n> 3. generic system environment (i.e., $http_proxy).\n> \n> So thinking about it that way, the original patch makes more sense.\n\nSo, in PATCH 2/3, apart from expanding the commit message.. do we want\nto support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be\nvery used by existent programs, but support it it's only a gentenv call..\n"},{"id":"185728","messageId":"7vlinltsja.fsf@alter.siamese.dyndns.org","threadId":"29771","inReplyTo":"4F4E003C.1050301@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-29T18:15:37Z","receivedAt":"2012-02-29T18:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> On 02/28/2012 08:24 PM, Junio C Hamano wrote:\n>\n>> I've heard rumors that libcurl on some versions/installations of Mac OS X\n>> deliberately ignores the environment. For those who agree with Apple, it\n>> would be a regression if we suddenly start the environment ourselves and\n>> using it.\n>\n> Hi Junio, what did you mean by \"we start the environment and using it\"?\n> I didn't understand what you mean there..\n\nThe reason you didn't understand is because the statement does not parse\nX-<. Thanks for pointing it out.\n\nWhat I meant was that on these platforms, allegedly (note that I do not\nhave a first-hand experience with them), user's http_proxy environment\nsetting did not affect libcurl based applications and that is a deliberate\nplatform decision to give precedence to proxy settings the platform has\nelsewhere. The users who agree with this platform decision are happily\nusing the proxy settings stored elsewhere in the platform with git, but\nmay have http_proxy environment pointing at a proxy that they do not want\nto use for git.\n\nIf we suddenly start reading from http_proxy environment ourselves and\nexplicitly telling libcurl to use the proxy specified, it will change the\nbehaviour for these users, i.e. a regression.\n"},{"id":"185746","messageId":"20120229210816.GB628@sigill.intra.peff.net","threadId":"29771","inReplyTo":"4F4E01EB.3070707@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-29T21:08:16Z","receivedAt":"2012-02-29T21:08:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 29, 2012 at 11:46:03AM +0100, Nelson Benitez Leon wrote:\n\n> > Good point. We sometimes follow this order:\n> > \n> > 1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if\n> > it existed)\n> > 2. git config files (i.e., http.proxy)\n> > 3. generic system environment (i.e., $http_proxy).\n> > \n> > So thinking about it that way, the original patch makes more sense.\n> \n> So, in PATCH 2/3, apart from expanding the commit message.. do we want\n> to support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be\n> very used by existent programs, but support it it's only a gentenv call..\n\nIf HTTP_PROXY is not in wide use, I don't see a reason to support it.\nAnd I take back what I said about environment precedence, based on the\ndiscussion. Also, I don't think there is a need to strdup the results of\ngetenv here, is there? So I think the code you want is just:\n\n if (!curl_http_proxy)\n curl_http_proxy = getenv(\"http_proxy\");\n\nand the justification for the commit message is that we need to know the\nproxy value outside of curl, because the next patch will do some\nextra processing on the value.\n\n-Peff\n"},{"id":"185798","messageId":"20120301091038.GB16033@sigill.intra.peff.net","threadId":"29771","inReplyTo":"4F4F47EF.40405@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-01T09:10:38Z","receivedAt":"2012-03-01T09:10:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 01, 2012 at 10:57:03AM +0100, Nelson Benitez Leon wrote:\n\n> > And I take back what I said about environment precedence, based on the\n> > discussion. Also, I don't think there is a need to strdup the results of\n> > getenv here, is there? So I think the code you want is just:\n> > \n> > if (!curl_http_proxy)\n> > curl_http_proxy = getenv(\"http_proxy\");\n> \n> but curl_http_proxy gets freed in http_cleanup as follows:\n> \n> free((void *)curl_http_proxy);\n> \n> Is it ok to free strings returned by getenv() ? I thought nope, so I\n> used strdup which existent code was already using..\n\nAh, you're right. I was worried more about lifetime issues (i.e., would\nthe string still be valid) and didn't check to see whether we freed it\n(and we should, because if it comes from config, then it will be\nallocated). So yes, you should duplicate it.\n\n-Peff\n"},{"id":"185795","messageId":"4F4F47EF.40405@seap.minhap.es","threadId":"29771","inReplyTo":"20120229210816.GB628@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-01T09:57:03Z","receivedAt":"2012-03-01T09:57:03Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 02/29/2012 10:08 PM, Jeff King wrote:\n> On Wed, Feb 29, 2012 at 11:46:03AM +0100, Nelson Benitez Leon wrote:\n> \n>>> Good point. We sometimes follow this order:\n>>>\n>>> 1. git-specific environment variables (i.e., $GIT_HTTP_PROXY, if\n>>> it existed)\n>>> 2. git config files (i.e., http.proxy)\n>>> 3. generic system environment (i.e., $http_proxy).\n>>>\n>>> So thinking about it that way, the original patch makes more sense.\n>>\n>> So, in PATCH 2/3, apart from expanding the commit message.. do we want\n>> to support HTTP_PROXY or only http_proxy ? HTTP_PROXY seems to not be\n>> very used by existent programs, but support it it's only a gentenv call..\n> \n> If HTTP_PROXY is not in wide use, I don't see a reason to support it.\n\nOk\n\n> And I take back what I said about environment precedence, based on the\n> discussion. Also, I don't think there is a need to strdup the results of\n> getenv here, is there? So I think the code you want is just:\n> \n> if (!curl_http_proxy)\n> curl_http_proxy = getenv(\"http_proxy\");\n\nbut curl_http_proxy gets freed in http_cleanup as follows:\n\nfree((void *)curl_http_proxy);\n\nIs it ok to free strings returned by getenv() ? I thought nope, so I\nused strdup which existent code was already using..\n> \n> and the justification for the commit message is that we need to know the\n> proxy value outside of curl, because the next patch will do some\n> extra processing on the value.\n> \n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"185799","messageId":"4F4F4D05.8080201@seap.minhap.es","threadId":"29771","inReplyTo":"7vlinltsja.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-01T10:18:45Z","receivedAt":"2012-03-01T10:18:45Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 02/29/2012 07:15 PM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n>> On 02/28/2012 08:24 PM, Junio C Hamano wrote:\n>>\n>>> I've heard rumors that libcurl on some versions/installations of Mac OS X\n>>> deliberately ignores the environment. For those who agree with Apple, it\n>>> would be a regression if we suddenly start the environment ourselves and\n>>> using it.\n>>\n>> Hi Junio, what did you mean by \"we start the environment and using it\"?\n>> I didn't understand what you mean there..\n> \n> The reason you didn't understand is because the statement does not parse\n> X-<. Thanks for pointing it out.\n> \n> What I meant was that on these platforms, allegedly (note that I do not\n> have a first-hand experience with them), user's http_proxy environment\n> setting did not affect libcurl based applications and that is a deliberate\n> platform decision to give precedence to proxy settings the platform has\n> elsewhere. The users who agree with this platform decision are happily\n\nI googled for that but didn't find any info, anyway if that's true did they \ndo this by 1) removing http_proxy from environment or by 2) patching\nlibcurl not to read them ? if it's the first case, we are safe as they will \nalso remove the vars and we won't read them, if it's second case, it's silly\non their part as any other console program will be reading env vars as they\nwould need to patch every program..\n\nThe normal thing os's do (as gnome used to do) is to automatically set the\nhttp_proxy env vars when you set a proxy on the GUI..\n\n> using the proxy settings stored elsewhere in the platform with git, but\n> may have http_proxy environment pointing at a proxy that they do not want\n> to use for git.\n> \n> If we suddenly start reading from http_proxy environment ourselves and\n> explicitly telling libcurl to use the proxy specified, it will change the\n> behaviour for these users, i.e. a regression.\n"},{"id":"186046","messageId":"alpine.DEB.2.00.1203042013410.5351@tvnag.unkk.fr","threadId":"29771","inReplyTo":"7v62eqzrqm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2012-03-04T19:19:45Z","receivedAt":"2012-03-04T19:19:45Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 28 Feb 2012, Junio C Hamano wrote:\n\n>> * Why do you (need to?) support HTTP_PROXY when curl doesn't?\n>\n> Let me add a third bullet point.\n>\n> I've heard rumors that libcurl on some versions/installations of Mac OS X \n> deliberately ignores the environment. For those who agree with Apple, it \n> would be a regression if we suddenly start the environment ourselves and \n> using it.\n\nI can also add that in libcurl we completely deliberately do not support \n\"HTTP_PROXY\" (using all upper case) for a quite simple and boring reason:\n\nThe (very old) CGI interface that web servers use(d) to invoke programs \nserver-side passes on header values in environment variables named \n\"HTTP_[header]\". So if you send a request to a server that starts a CGI script \nand use a request header named Proxy: in that request, that header will be \npassed to the CGI script using the environment variable \"HTTP_PROXY\" ...\n\n-- \n\n / daniel.haxx.se\n"},{"id":"186656","messageId":"m3pqcjt6s2.fsf@carbon.jhcloos.org","threadId":"29771","inReplyTo":"4F4CCE8A.4010800@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"James Cloos","fromEmail":"cloos@jhcloos.com","sentAt":"2012-03-11T16:56:53Z","receivedAt":"2012-03-11T16:56:53Z","isPatch":true,"sender":{"key":"cloos@jhcloos.com","avatar":"https://gravatar.com/avatar/ec9a05787d29afe41e243e4b60bd0e2f69d757688e8f0bfe5e78bc185a3e317f?d=mp&s=160"},"body":"Please include a way, eg via ~/.gitconfig, to ignore any http_proxy in\nthe environment and connect directly.\n\nThe proxy might exist solely to aggregate a cache and not as a necessary\nlink to the outside; forcing git to use it in such cases is more harmful\nthan beneficial.\n\n-JimC\n-- \nJames Cloos OpenPGP: 1024D/ED7DAEA6\n"},{"id":"186662","messageId":"7v4ntvx87v.fsf@alter.siamese.dyndns.org","threadId":"29771","inReplyTo":"m3pqcjt6s2.fsf@carbon.jhcloos.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-11T19:12:36Z","receivedAt":"2012-03-11T19:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"James Cloos writes:\n\n> Please include a way, eg via ~/.gitconfig, to ignore any http_proxy in\n> the environment and connect directly.\n\nHrm.\n\nI think without this patch series, the \"NO_PROXY\" environment\nvariable is honored by the curl library when it uses http_proxy\nto make the decision. If this patch (or a future reroll of it) fails\nto do the same, it would be a regression.\n\nNelson, do you agree?\n"},{"id":"186824","messageId":"4F5F1FEA.8020103@seap.minhap.es","threadId":"29771","inReplyTo":"7v4ntvx87v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-13T10:22:34Z","receivedAt":"2012-03-13T10:22:34Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/11/2012 08:12 PM, Junio C Hamano wrote:\n> James Cloos writes:\n> \n>> Please include a way, eg via ~/.gitconfig, to ignore any http_proxy in\n>> the environment and connect directly.\n> \n> Hrm.\n> \n> I think without this patch series, the \"NO_PROXY\" environment\n> variable is honored by the curl library when it uses http_proxy\n> to make the decision. If this patch (or a future reroll of it) fails\n> to do the same, it would be a regression.\n> \n> Nelson, do you agree?\n\nI agree, so I would need to handle $no_proxy in the patch-set, will look\ninto that.\n"},{"id":"186917","messageId":"7vhaxrsssm.fsf@alter.siamese.dyndns.org","threadId":"29771","inReplyTo":"4F5F1FEA.8020103@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-14T04:36:09Z","receivedAt":"2012-03-14T04:36:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> On 03/11/2012 08:12 PM, Junio C Hamano wrote:\n>> James Cloos writes:\n>> \n>>> Please include a way, eg via ~/.gitconfig, to ignore any http_proxy in\n>>> the environment and connect directly.\n>> \n>> Hrm.\n>> \n>> I think without this patch series, the \"NO_PROXY\" environment\n>> variable is honored by the curl library when it uses http_proxy\n>> to make the decision. If this patch (or a future reroll of it) fails\n>> to do the same, it would be a regression.\n>> \n>> Nelson, do you agree?\n>\n> I agree, so I would need to handle $no_proxy in the patch-set, will look\n> into that.\n\nAre you sure $no_proxy is spelled in lowercase? man curl(1) seems to\nindicate otherwise.\n"},{"id":"186922","messageId":"4F606AE9.70608@seap.minhap.es","threadId":"29771","inReplyTo":"7vhaxrsssm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-14T09:54:49Z","receivedAt":"2012-03-14T09:54:49Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/14/2012 05:36 AM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n>> On 03/11/2012 08:12 PM, Junio C Hamano wrote:\n>>> James Cloos writes:\n>>>\n>>>> Please include a way, eg via ~/.gitconfig, to ignore any http_proxy in\n>>>> the environment and connect directly.\n>>>\n>>> Hrm.\n>>>\n>>> I think without this patch series, the \"NO_PROXY\" environment\n>>> variable is honored by the curl library when it uses http_proxy\n>>> to make the decision. If this patch (or a future reroll of it) fails\n>>> to do the same, it would be a regression.\n>>>\n>>> Nelson, do you agree?\n>>\n>> I agree, so I would need to handle $no_proxy in the patch-set, will look\n>> into that.\n> \n> Are you sure $no_proxy is spelled in lowercase? man curl(1) seems to\n> indicate otherwise.\n\nInstead here[1] in section \"Environment Variables\" it's spelled lowercase,\nand given that cURL reads $http_proxy only in lowercase I think it does\nthe same for $no_proxy.\n\n[1] http://curl.haxx.se/libcurl/c/libcurl-tutorial.html\n"},{"id":"186991","messageId":"7v1uouq5j9.fsf@alter.siamese.dyndns.org","threadId":"29771","inReplyTo":"4F606AE9.70608@seap.minhap.es","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-14T20:41:30Z","receivedAt":"2012-03-14T20:41:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nelson Benitez Leon writes:\n\n> On 03/14/2012 05:36 AM, Junio C Hamano wrote:\n>\n>> Are you sure $no_proxy is spelled in lowercase? man curl(1) seems to\n>> indicate otherwise.\n>\n> Instead here[1] in section \"Environment Variables\" it's spelled lowercase,\n> and given that cURL reads $http_proxy only in lowercase I think it does\n> the same for $no_proxy.\n\nDon't think, but read ;-). Quoting from man curl(1):\n\n\tENVIRONMENT\n\n The environment variables can be specified in lower case or upper\n case. The lower case version has precedence. http_proxy is an\n exception as it is only available in lower case.\n"},{"id":"187032","messageId":"4F61B892.8030604@seap.minhap.es","threadId":"29771","inReplyTo":"7v1uouq5j9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] http: try standard proxy env vars when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-15T09:38:26Z","receivedAt":"2012-03-15T09:38:26Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"On 03/14/2012 09:41 PM, Junio C Hamano wrote:\n> Nelson Benitez Leon writes:\n> \n>> On 03/14/2012 05:36 AM, Junio C Hamano wrote:\n>>\n>>> Are you sure $no_proxy is spelled in lowercase? man curl(1) seems to\n>>> indicate otherwise.\n>>\n>> Instead here[1] in section \"Environment Variables\" it's spelled lowercase,\n>> and given that cURL reads $http_proxy only in lowercase I think it does\n>> the same for $no_proxy.\n> \n> Don't think, but read ;-). Quoting from man curl(1):\n> \n> \tENVIRONMENT\n> \n> The environment variables can be specified in lower case or upper\n> case. The lower case version has precedence. http_proxy is an\n> exception as it is only available in lower case.\n> \n\nOk, good advice nonetheless :-), so will send a new 1/5 patch that also\nreads $NO_PROXY.\n"}]}