{"thread":{"id":"29934","subject":"[PATCH v5 1/5] http: try http_proxy env var when http.proxy config option is not set","startedAt":"2012-03-13T14:02:58Z","lastAt":"2012-04-14T23:27:37Z","messageCount":4,"participants":["Nelson Benitez Leon","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":5},"messages":[{"id":"186853","messageId":"4F5F5392.5010700@seap.minhap.es","threadId":"29934","inReplyTo":null,"subject":"[PATCH v5 1/5] http: try http_proxy env var when http.proxy config option is not set","fromName":"Nelson Benitez Leon","fromEmail":"nelsonjesus.benitez@seap.minhap.es","sentAt":"2012-03-13T14:02:58Z","receivedAt":"2012-03-13T14:02:58Z","isPatch":true,"sender":{"key":"nelsonjesus.benitez@seap.minhap.es","avatar":null},"body":"cURL already reads it, but if $http_proxy has username but no password\ncURL will not ask you for the password and so failed to authenticate\nreturning a 407 error code. So we read it ourselves to detect that and\nask for the password. Also we read it prior to connection to be able to\nmake a proactive authentication in case the flag http_proactive_auth is\nset.\n\nWe also take care to read env proxy var according to protocol being\nused in the destination url, e.g.  when the url to retrieve is a https\none, then the proxy env var we look at is https_proxy. To make this\npossible we now passed destination url parameter to get_active_slot()\nand get_curl_handle() functions.\n\nWe also read no_proxy env var so to ignore aforementioned proxy env var\nif no_proxy contains an asterisk ('*') or contains the host used in url\ndestination.\n\nSigned-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>\n---\n http-push.c   |   24 ++++++++++++------------\n http-walker.c |    2 +-\n http.c        |   35 ++++++++++++++++++++++++++++-------\n http.h        |    2 +-\n remote-curl.c |    4 ++--\n 5 files changed, 44 insertions(+), 23 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex f22f7e4..e7bb4cf 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -297,7 +297,7 @@ static void start_mkcol(struct transfer_request *request)\n \n \trequest->url = get_remote_object_url(repo->url, hex, 1);\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(request->url);\n \tslot->callback_func = process_response;\n \tslot->callback_data = request;\n \tcurl_setup_http_get(slot->curl, request->url, DAV_MKCOL);\n@@ -417,7 +417,7 @@ static void start_put(struct transfer_request *request)\n \tstrbuf_add(&buf, request->lock->tmpfile_suffix, 41);\n \trequest->url = strbuf_detach(&buf, NULL);\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(request->url);\n \tslot->callback_func = process_response;\n \tslot->callback_data = request;\n \tcurl_setup_http(slot->curl, request->url, DAV_PUT,\n@@ -438,7 +438,7 @@ static void start_move(struct transfer_request *request)\n \tstruct active_request_slot *slot;\n \tstruct curl_slist *dav_headers = NULL;\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(request->url);\n \tslot->callback_func = process_response;\n \tslot->callback_data = request;\n \tcurl_setup_http_get(slot->curl, request->url, DAV_MOVE);\n@@ -467,7 +467,7 @@ static int refresh_lock(struct remote_lock *lock)\n \n \tdav_headers = get_dav_token_headers(lock, DAV_HEADER_IF | DAV_HEADER_TIMEOUT);\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(lock->url);\n \tslot->results = &results;\n \tcurl_setup_http_get(slot->curl, lock->url, DAV_LOCK);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n@@ -882,7 +882,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \twhile (ep) {\n \t\tchar saved_character = ep[1];\n \t\tep[1] = '\\0';\n-\t\tslot = get_active_slot();\n+\t\tslot = get_active_slot(url);\n \t\tslot->results = &results;\n \t\tcurl_setup_http_get(slot->curl, url, DAV_MKCOL);\n \t\tif (start_active_slot(slot)) {\n@@ -912,7 +912,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)\n \tdav_headers = curl_slist_append(dav_headers, timeout_header);\n \tdav_headers = curl_slist_append(dav_headers, \"Content-Type: text/xml\");\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(url);\n \tslot->results = &results;\n \tcurl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n@@ -980,7 +980,7 @@ static int unlock_remote(struct remote_lock *lock)\n \n \tdav_headers = get_dav_token_headers(lock, DAV_HEADER_LOCK);\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(lock->url);\n \tslot->results = &results;\n \tcurl_setup_http_get(slot->curl, lock->url, DAV_UNLOCK);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);\n@@ -1158,7 +1158,7 @@ static void remote_ls(const char *path, int flags,\n \tdav_headers = curl_slist_append(dav_headers, \"Depth: 1\");\n \tdav_headers = curl_slist_append(dav_headers, \"Content-Type: text/xml\");\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(url);\n \tslot->results = &results;\n \tcurl_setup_http(slot->curl, url, DAV_PROPFIND,\n \t\t\t&out_buffer, fwrite_buffer);\n@@ -1232,7 +1232,7 @@ static int locking_available(void)\n \tdav_headers = curl_slist_append(dav_headers, \"Depth: 0\");\n \tdav_headers = curl_slist_append(dav_headers, \"Content-Type: text/xml\");\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(repo->url);\n \tslot->results = &results;\n \tcurl_setup_http(slot->curl, repo->url, DAV_PROPFIND,\n \t\t\t&out_buffer, fwrite_buffer);\n@@ -1409,7 +1409,7 @@ static int update_remote(unsigned char *sha1, struct remote_lock *lock)\n \n \tstrbuf_addf(&out_buffer.buf, \"%s\\n\", sha1_to_hex(sha1));\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(lock->url);\n \tslot->results = &results;\n \tcurl_setup_http(slot->curl, lock->url, DAV_PUT,\n \t\t\t&out_buffer, fwrite_null);\n@@ -1535,7 +1535,7 @@ static void update_remote_info_refs(struct remote_lock *lock)\n \tif (!aborted) {\n \t\tdav_headers = get_dav_token_headers(lock, DAV_HEADER_IF);\n \n-\t\tslot = get_active_slot();\n+\t\tslot = get_active_slot(lock->url);\n \t\tslot->results = &results;\n \t\tcurl_setup_http(slot->curl, lock->url, DAV_PUT,\n \t\t\t\t&buffer, fwrite_null);\n@@ -1695,7 +1695,7 @@ static int delete_remote_branch(const char *pattern, int force)\n \t\treturn 0;\n \turl = xmalloc(strlen(repo->url) + strlen(remote_ref->name) + 1);\n \tsprintf(url, \"%s%s\", repo->url, remote_ref->name);\n-\tslot = get_active_slot();\n+\tslot = get_active_slot(url);\n \tslot->results = &results;\n \tcurl_setup_http_get(slot->curl, url, DAV_DELETE);\n \tif (start_active_slot(slot)) {\ndiff --git a/http-walker.c b/http-walker.c\nindex 51a906e..5d5ae34 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -348,7 +348,7 @@ static void fetch_alternates(struct walker *walker, const char *base)\n \t * Use a callback to process the result, since another request\n \t * may fail and need to have alternates loaded before continuing\n \t */\n-\tslot = get_active_slot();\n+\tslot = get_active_slot(url);\n \tslot->callback_func = process_alternates_response;\n \talt_req.walker = walker;\n \tslot->callback_data = &alt_req;\ndiff --git a/http.c b/http.c\nindex 8ac8eb6..be88acb 100644\n--- a/http.c\n+++ b/http.c\n@@ -42,6 +42,7 @@ static long curl_low_speed_time = -1;\n static int curl_ftp_no_epsv;\n static const char *curl_http_proxy;\n static const char *curl_cookie_file;\n+static struct credential cre_url = CREDENTIAL_INIT;\n static struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n@@ -232,7 +233,7 @@ static int has_cert_password(void)\n \treturn 1;\n }\n \n-static CURL *get_curl_handle(void)\n+static CURL *get_curl_handle(const char *url)\n {\n \tCURL *result = curl_easy_init();\n \n@@ -295,6 +296,26 @@ 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, *no_proxy;\n+\t\tchar *env_proxy_var;\n+\t\tint read_http_proxy;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tcredential_from_url(&cre_url, url);\n+\t\tstrbuf_addf(&buf, \"%s_proxy\", cre_url.protocol);\n+\t\tenv_proxy_var = strbuf_detach(&buf, NULL);\n+\t\tenv_proxy = getenv(env_proxy_var);\n+\t\tif (env_proxy) {\n+\t\t\tread_http_proxy = 1;\n+\t\t\tno_proxy = getenv(\"no_proxy\");\n+\t\t\tif (no_proxy && (!strcmp(\"*\", no_proxy) || strstr(no_proxy, cre_url.host)))\n+\t\t\t\tread_http_proxy = 0;\n+\t\t\t\n+\t\t\tif (read_http_proxy)\n+\t\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n+\t\t}\n+\t\tfree(env_proxy_var);\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@@ -385,7 +406,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t}\n \n #ifndef NO_CURL_EASY_DUPHANDLE\n-\tcurl_default = get_curl_handle();\n+\tcurl_default = get_curl_handle(url);\n #endif\n }\n \n@@ -434,7 +455,7 @@ void http_cleanup(void)\n \tssl_cert_password_required = 0;\n }\n \n-struct active_request_slot *get_active_slot(void)\n+struct active_request_slot *get_active_slot(const char *url)\n {\n \tstruct active_request_slot *slot = active_queue_head;\n \tstruct active_request_slot *newslot;\n@@ -472,7 +493,7 @@ struct active_request_slot *get_active_slot(void)\n \n \tif (slot->curl == NULL) {\n #ifdef NO_CURL_EASY_DUPHANDLE\n-\t\tslot->curl = get_curl_handle();\n+\t\tslot->curl = get_curl_handle(url);\n #else\n \t\tslot->curl = curl_easy_duphandle(curl_default);\n #endif\n@@ -745,7 +766,7 @@ static int http_request(const char *url, void *result, int target, int options)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(url);\n \tslot->results = &results;\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \n@@ -1090,7 +1111,7 @@ struct http_pack_request *new_http_pack_request(\n \t\tgoto abort;\n \t}\n \n-\tpreq->slot = get_active_slot();\n+\tpreq->slot = get_active_slot(preq->url);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_FILE, preq->packfile);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);\n \tcurl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);\n@@ -1250,7 +1271,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \t\t}\n \t}\n \n-\tfreq->slot = get_active_slot();\n+\tfreq->slot = get_active_slot(freq->url);\n \n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\ndiff --git a/http.h b/http.h\nindex 0b61653..303eafb 100644\n--- a/http.h\n+++ b/http.h\n@@ -73,7 +73,7 @@ extern curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);\n #endif\n \n /* Slot lifecycle functions */\n-extern struct active_request_slot *get_active_slot(void);\n+extern struct active_request_slot *get_active_slot(const char *url);\n extern int start_active_slot(struct active_request_slot *slot);\n extern void run_active_slot(struct active_request_slot *slot);\n extern void finish_active_slot(struct active_request_slot *slot);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d159fe7..1b5eaf3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -383,7 +383,7 @@ static int probe_rpc(struct rpc_state *rpc)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint err;\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(rpc->service_url);\n \n \theaders = curl_slist_append(headers, rpc->hdr_content_type);\n \theaders = curl_slist_append(headers, rpc->hdr_accept);\n@@ -440,7 +440,7 @@ static int post_rpc(struct rpc_state *rpc)\n \t\t\treturn err;\n \t}\n \n-\tslot = get_active_slot();\n+\tslot = get_active_slot(rpc->service_url);\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n-- \n1.7.7.6\n"},{"id":"189215","messageId":"20120413204858.GA7919@sigill.intra.peff.net","threadId":"29934","inReplyTo":"4F5F5392.5010700@seap.minhap.es","subject":"Re: [PATCH v5 1/5] http: try http_proxy env var when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T20:48:58Z","receivedAt":"2012-04-13T20:48:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 13, 2012 at 03:02:58PM +0100, Nelson Benitez Leon wrote:\n\n> diff --git a/http.c b/http.c\n> index 8ac8eb6..be88acb 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -42,6 +42,7 @@ static long curl_low_speed_time = -1;\n>  static int curl_ftp_no_epsv;\n>  static const char *curl_http_proxy;\n>  static const char *curl_cookie_file;\n> +static struct credential cre_url = CREDENTIAL_INIT;\n>  static struct credential http_auth = CREDENTIAL_INIT;\n>  static int http_proactive_auth;\n>  static const char *user_agent;\n\nThis is a minor issue, but if you are re-rolling anyway, would you mind\nmaking this cre_url not global? It is used only in one small code block:\n\n> +\tif (!curl_http_proxy) {\n> +\t\tconst char *env_proxy, *no_proxy;\n> +\t\tchar *env_proxy_var;\n> +\t\tint read_http_proxy;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\tcredential_from_url(&cre_url, url);\n> +\t\tstrbuf_addf(&buf, \"%s_proxy\", cre_url.protocol);\n> +\t\tenv_proxy_var = strbuf_detach(&buf, NULL);\n> +\t\tenv_proxy = getenv(env_proxy_var);\n> +\t\tif (env_proxy) {\n> +\t\t\tread_http_proxy = 1;\n> +\t\t\tno_proxy = getenv(\"no_proxy\");\n> +\t\t\tif (no_proxy && (!strcmp(\"*\", no_proxy) || strstr(no_proxy, cre_url.host)))\n> +\t\t\t\tread_http_proxy = 0;\n> +\t\t\t\n> +\t\t\tif (read_http_proxy)\n> +\t\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n> +\t\t}\n> +\t\tfree(env_proxy_var);\n> +\t}\n\nso you could just make it local to this block (also, should you be\nchecking for the case that there is no protocol?).\n\nI also found this code a little hard to follow because of the number of\nvariables. You can drop \"env_proxy_var\" and just access the strbuf's\nbuffer directly. And you can drop read_http_proxy, as it is used in the\nconditional immediately after being set. Plus you can declare a few of\nthe variables closer to use, and move the variable cleanup closer to\nwhen they are finished being used. So maybe this would be much simpler:\n\ndiff --git a/http.c b/http.c\nindex 8ac8eb6..ea32ecb 100644\n--- a/http.c\n+++ b/http.c\n@@ -295,6 +295,31 @@ 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\tstatic struct credential parsed_url = CREDENTIAL_INIT;\n+\t\tcredential_from_url(&parsed_url, url);\n+\n+\t\tif (parsed_url.protocol) {\n+\t\t\tconst char *env_proxy;\n+\t\t\tstruct strbuf var = STRBUF_INIT;\n+\n+\t\t\tstrbuf_addf(&var, \"%s_proxy\", parsed_url.protocol);\n+\t\t\tenv_proxy = getenv(var.buf);\n+\t\t\tstrbuf_release(&var);\n+\n+\t\t\tif (env_proxy) {\n+\t\t\t\tconst char *no_proxy = getenv(\"no_proxy\");\n+\t\t\t\tif (!no_proxy)\n+\t\t\t\t\tno_proxy = getenv(\"NO_PROXY\");\n+\t\t\t\tif (!no_proxy ||\n+\t\t\t\t    (strcmp(\"*\", no_proxy) &&\n+\t\t\t\t     !strstr(no_proxy, parsed_url.host)))\n+\t\t\t\t\tcurl_http_proxy = xstrdup(env_proxy);\n+\t\t\t}\n+\t\t}\n+\n+\t\tcredential_clear(&parsed_url);\n+\t}\n\nI think it would be even easier to read if it were broken out into a few\nfunctions.\n\n-Peff\n"},{"id":"189216","messageId":"20120413205222.GB7919@sigill.intra.peff.net","threadId":"29934","inReplyTo":"20120413204858.GA7919@sigill.intra.peff.net","subject":"Re: [PATCH v5 1/5] http: try http_proxy env var when http.proxy config option is not set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-04-13T20:52:22Z","receivedAt":"2012-04-13T20:52:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 13, 2012 at 04:48:58PM -0400, Jeff King wrote:\n\n> > +\tif (!curl_http_proxy) {\n> > +\t\tconst char *env_proxy, *no_proxy;\n> > +\t\tchar *env_proxy_var;\n> > +\t\tint read_http_proxy;\n> > +\t\tstruct strbuf buf = STRBUF_INIT;\n> > +\t\tcredential_from_url(&cre_url, url);\n> > +\t\tstrbuf_addf(&buf, \"%s_proxy\", cre_url.protocol);\n> > +\t\tenv_proxy_var = strbuf_detach(&buf, NULL);\n> > +\t\tenv_proxy = getenv(env_proxy_var);\n\nAlso, shouldn't we be checking HTTPS_PROXY in addition to https_proxy,\nby curl's rules? Only http_proxy is not allowed to be upper-cased.\n\n-Peff\n"},{"id":"189304","messageId":"7vmx6d28t2.fsf@alter.siamese.dyndns.org","threadId":"29934","inReplyTo":"20120413205222.GB7919@sigill.intra.peff.net","subject":"Re: [PATCH v5 1/5] http: try http_proxy env var when http.proxy config option is not set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-14T23:27:37Z","receivedAt":"2012-04-14T23:27:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Apr 13, 2012 at 04:48:58PM -0400, Jeff King wrote:\n>\n>> > +\tif (!curl_http_proxy) {\n>> > +\t\tconst char *env_proxy, *no_proxy;\n>> > +\t\tchar *env_proxy_var;\n>> > +\t\tint read_http_proxy;\n>> > +\t\tstruct strbuf buf = STRBUF_INIT;\n>> > +\t\tcredential_from_url(&cre_url, url);\n>> > +\t\tstrbuf_addf(&buf, \"%s_proxy\", cre_url.protocol);\n>> > +\t\tenv_proxy_var = strbuf_detach(&buf, NULL);\n>> > +\t\tenv_proxy = getenv(env_proxy_var);\n>\n> Also, shouldn't we be checking HTTPS_PROXY in addition to https_proxy,\n> by curl's rules? Only http_proxy is not allowed to be upper-cased.\n\nGood point.\n"}]}