{"thread":{"id":"35039","subject":"[PATCH 0/9] following http redirects","startedAt":"2013-09-28T08:29:57Z","lastAt":"2013-10-18T19:20:50Z","messageCount":17,"participants":["Jeff King","brian m. carlson","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"228346","messageId":"20130928082956.GA22610@sigill.intra.peff.net","threadId":"35039","inReplyTo":null,"subject":"[PATCH 0/9] following http redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:29:57Z","receivedAt":"2013-09-28T08:29:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Git's http handling takes care of redirects by setting\nCURLOPT_FOLLOWLOCATION, and just assuming curl will handle the details.\nFor the most part, this works, but there are a few downsides:\n\n  1. We do extra round-trips for subsequent requests, as we\n     always feed curl the original URL, and it then has to go hit the\n     redirect.\n\n  2. Some redirects are to upgrade http->https. In this case, we end up\n     sending the credential over unencrypted http. We do advertise the\n     proper unencrypted URL to the credential helpers (and to the user\n     when we prompt them), so to some degree this is the user's fault.\n     Still, there's no reason not to be friendly and avoid exposing the\n     password in plaintext if we know that the server is just going to\n     redirect us to an encrypted connection anyway.\n\n  3. Cross-server redirects do not work at all with authentication. If\n     we tell curl to go to host X, and it redirects to host Y, it will\n     drop the auth credentials to avoid revealing them to Y. This is a\n     good thing, because we prompted the user for host X, and curl is\n     being safe. However, it means that we have no way to feed curl the\n     right password (and the user does not even get a useful error\n     message; simply a 401 from Y, since we didn't give it any\n     credential).\n\nNone of these is fixable at the curl layer; they depend on information\nwe have outside of curl (e.g., the notion that if info/refs is\nredirected, so should the rest of the requests be).\n\nThis series gets the redirect information from curl (after the fact),\nand uses it update the base URL for the repository for the remainder of\nthe program run.\n\nKyle, I didn't call into the urlmatch code at all; we might want another\npatch on top to re-check the http.$url.* config after following such a\nredirect.\n\nThe patches are:\n\n  [1/9]: http_get_file: style fixes\n  [2/9]: http_request: factor out curlinfo_strbuf\n  [3/9]: http: refactor options to http_get_*\n  [4/9]: http: hoist credential request out of handle_curl_result\n  [5/9]: http: provide effective url to callers\n  [6/9]: http: update base URLs when we see redirects\n  [7/9]: remote-curl: make refs_url a strbuf\n  [8/9]: remote-curl: store url as a strbuf\n  [9/9]: remote-curl: rewrite base url from info/refs redirects\n\nHere is sample output from running a clone against a private repo on\ngithub, which redirects from http to https. Generated with:\n\n  GIT_CURL_VERBOSE=1 git clone http://github.com/github/github 2>&1 |\n  egrep '^(< HTTP|< Location|> GET|> POST|^Authorization|\\* Connected to)' |\n  sed -e 's/\\(Authorization\\): .*/\\1: **redacted**/' \n\nHere is the output with stock git (I've added numbering for each\nconnection):\n\n1 * Connected to github.com (192.30.252.129) port 80 (#0)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 301 Moved Permanently\n    < Location: https://github.com/github/github/info/refs?service=git-upload-pack\n\n2 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 401 Authorization Required\n\n3 * Connected to github.com (192.30.252.129) port 80 (#2)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 301 Moved Permanently\n    < Location: https://github.com/github/github/info/refs?service=git-upload-pack\n\n4 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 401 Authorization Required\n\n5 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    Authorization: **redacted**\n    < HTTP/1.1 200 OK\n\n6 * Connected to github.com (192.30.252.129) port 80 (#3)\n    > POST /github/github/git-upload-pack HTTP/1.1\n    Authorization: **redacted**\n    < HTTP/1.1 301 Moved Permanently\n    < Location: https://github.com/github/github/git-upload-pack\n\n7 * Connected to github.com (192.30.252.129) port 443 (#4)\n    [... POST actually happens, and I hit ^C before it finishes ...]\n\nYou can see that connections 3 and 6 are extraneous round-trips, and\nthat we reveal the credential over cleartext in step 6. Curiously, even\nthough we have fed the credential to curl after step 2, curl does not\nshare it until the server sends another 401 in step 4. I'm not sure why\nthat is the case.\n\nHere's the output with these patches applied:\n\n1 * Connected to github.com (192.30.252.129) port 80 (#0)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 301 Moved Permanently\n    < Location: https://github.com/github/github/info/refs?service=git-upload-pack\n\n2 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 401 Authorization Required\n\n3 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    < HTTP/1.1 401 Authorization Required\n\n4 * Connected to github.com (192.30.252.129) port 443 (#1)\n    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1\n    Authorization: **redacted**\n    < HTTP/1.1 200 OK\n\n5 * Connected to github.com (192.30.252.129) port 443 (#2)\n    [... POST actually happens ...]\n\nWe do 2 fewer round-trips. That isn't all that exciting, but keep in\nmind that it saves one per request after the initial contact. So if we\nhad more ref negotiation, we'd see more savings. Similarly, dumb http\nwould see many more savings (since it makes individual requests for each\nloose object and pack).\n\nWe also go straight to https in steps 3 and 4, meaning the credential\nnever gets passed in plaintext to the old URL.\n\nI also tested cross-server redirects using a hacky \"nc -l\" setup as the\nserver. I confirmed that it does not work at all with stock git (you are\nprompted for the password for \"localhost\", which curl then drops after\nfollowing the redirect), and that it does work after the patches.\n\nThe one downside I can think of is that we are making the assumption\nthat if info/refs redirects, the rest of the repo will, too. So you\ncould not have \"info/refs\" redirect to another server to get the ref\nadvertisement, but then expect to get the actual pack from the original\nlocation. I'm OK with making that assumption; I can't imagine the\ninsanity of the setup that would be required to break it.\n\n-Peff\n"},{"id":"228347","messageId":"20130928083059.GA2131@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 1/9] http_get_file: style fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:31:00Z","receivedAt":"2013-09-28T08:31:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Besides being ugly, the extra parentheses are idiomatic for\nsuppressing compiler warnings when we are assigning within a\nconditional. We aren't doing that here, and they just\nconfuse the reader.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex f3e1439..a985c40 100644\n--- a/http.c\n+++ b/http.c\n@@ -958,7 +958,7 @@ static int http_get_file(const char *url, const char *filename, int options)\n \n \tstrbuf_addf(&tmpfile, \"%s.temp\", filename);\n \tresult = fopen(tmpfile.buf, \"a\");\n-\tif (! result) {\n+\tif (!result) {\n \t\terror(\"Unable to open local file %s\", tmpfile.buf);\n \t\tret = HTTP_ERROR;\n \t\tgoto cleanup;\n@@ -967,7 +967,7 @@ static int http_get_file(const char *url, const char *filename, int options)\n \tret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);\n \tfclose(result);\n \n-\tif ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))\n+\tif (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))\n \t\tret = HTTP_ERROR;\n cleanup:\n \tstrbuf_release(&tmpfile);\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228348","messageId":"20130928083111.GB2131@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 2/9] http_request: factor out curlinfo_strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:31:11Z","receivedAt":"2013-09-28T08:31:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we retrieve the content-type of an http response, curl\ngives us a pointer to internal storage, which we then copy\ninto a strbuf. Let's factor out the get-and-copy routine,\nwhich can be used for getting other curl info.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a985c40..d325669 100644\n--- a/http.c\n+++ b/http.c\n@@ -837,6 +837,18 @@ int handle_curl_result(struct slot_results *results)\n \t}\n }\n \n+static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n+{\n+\tchar *ptr;\n+\tCURLcode ret;\n+\n+\tstrbuf_reset(buf);\n+\tret = curl_easy_getinfo(curl, info, &ptr);\n+\tif (!ret && ptr)\n+\t\tstrbuf_addstr(buf, ptr);\n+\treturn ret;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -895,13 +907,8 @@ static int http_request(const char *url, struct strbuf *type,\n \t\tret = HTTP_START_FAILED;\n \t}\n \n-\tif (type) {\n-\t\tchar *t;\n-\t\tstrbuf_reset(type);\n-\t\tcurl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);\n-\t\tif (t)\n-\t\t\tstrbuf_addstr(type, t);\n-\t}\n+\tif (type)\n+\t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);\n \n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228349","messageId":"20130928083122.GC2131@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 3/9] http: refactor options to http_get_*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:31:23Z","receivedAt":"2013-09-28T08:31:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Over time, the http_get_strbuf function has grown several\noptional parameters. We now have a bitfield with multiple\nboolean options, as well as an optional strbuf for returning\nthe content-type of the response. And a future patch in this\nseries is going to add another strbuf option.\n\nTreating these as separate arguments has a few downsides:\n\n  1. Most call sites need to add extra NULLs and 0s for the\n     options they aren't interested in.\n\n  2. The http_get_* functions are actually wrappers around\n     2 layers of low-level implementation functions. We have\n     to pass these options through individually.\n\n  3. The http_get_strbuf wrapper learned these options, but\n     nobody bothered to do so for http_get_file, even though\n     it is backed by the same function that does understand\n     the options.\n\nLet's consolidate the options into a single struct. For the\ncommon case of the default options, we'll allow callers to\nsimply pass a NULL for the options struct.\n\nThe resulting code is often a few lines longer, but it ends\nup being easier to read (and to change as we add new\noptions, since we do not need to update each call site).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-push.c   |  4 ++--\n http.c        | 44 +++++++++++++++++++++++++-------------------\n http.h        | 15 ++++++++++-----\n remote-curl.c |  9 +++++++--\n 4 files changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 69200ba..34cb70f 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1542,7 +1542,7 @@ static int remote_exists(const char *path)\n \n \tsprintf(url, \"%s%s\", repo->url, path);\n \n-\tswitch (http_get_strbuf(url, NULL, NULL, 0)) {\n+\tswitch (http_get_strbuf(url, NULL, NULL)) {\n \tcase HTTP_OK:\n \t\tret = 1;\n \t\tbreak;\n@@ -1566,7 +1566,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)\n \turl = xmalloc(strlen(repo->url) + strlen(path) + 1);\n \tsprintf(url, \"%s%s\", repo->url, path);\n \n-\tif (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)\n+\tif (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)\n \t\tdie(\"Couldn't get %s for remote symref\\n%s\", url,\n \t\t    curl_errorstr);\n \tfree(url);\ndiff --git a/http.c b/http.c\nindex d325669..41ee7aa 100644\n--- a/http.c\n+++ b/http.c\n@@ -853,8 +853,9 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n \n-static int http_request(const char *url, struct strbuf *type,\n-\t\t\tvoid *result, int target, int options)\n+static int http_request(const char *url,\n+\t\t\tvoid *result, int target,\n+\t\t\tconst struct http_get_options *options)\n {\n \tstruct active_request_slot *slot;\n \tstruct slot_results results;\n@@ -887,9 +888,9 @@ static int http_request(const char *url, struct strbuf *type,\n \t}\n \n \tstrbuf_addstr(&buf, \"Pragma:\");\n-\tif (options & HTTP_NO_CACHE)\n+\tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n-\tif (options & HTTP_KEEP_ERROR)\n+\tif (options && options->keep_error)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \theaders = curl_slist_append(headers, buf.buf);\n@@ -907,8 +908,9 @@ static int http_request(const char *url, struct strbuf *type,\n \t\tret = HTTP_START_FAILED;\n \t}\n \n-\tif (type)\n-\t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);\n+\tif (options && options->content_type)\n+\t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,\n+\t\t\t\toptions->content_type);\n \n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n@@ -917,11 +919,10 @@ static int http_request_reauth(const char *url,\n }\n \n static int http_request_reauth(const char *url,\n-\t\t\t       struct strbuf *type,\n \t\t\t       void *result, int target,\n-\t\t\t       int options)\n+\t\t\t       struct http_get_options *options)\n {\n-\tint ret = http_request(url, type, result, target, options);\n+\tint ret = http_request(url, result, target, options);\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n \n@@ -931,7 +932,7 @@ static int http_request_reauth(const char *url,\n \t * making our next request. We only know how to do this for\n \t * the strbuf case, but that is enough to satisfy current callers.\n \t */\n-\tif (options & HTTP_KEEP_ERROR) {\n+\tif (options && options->keep_error) {\n \t\tswitch (target) {\n \t\tcase HTTP_REQUEST_STRBUF:\n \t\t\tstrbuf_reset(result);\n@@ -940,15 +941,14 @@ int http_get_strbuf(const char *url,\n \t\t\tdie(\"BUG: HTTP_KEEP_ERROR is only supported with strbufs\");\n \t\t}\n \t}\n-\treturn http_request(url, type, result, target, options);\n+\treturn http_request(url, result, target, options);\n }\n \n int http_get_strbuf(const char *url,\n-\t\t    struct strbuf *type,\n-\t\t    struct strbuf *result, int options)\n+\t\t    struct strbuf *result,\n+\t\t    struct http_get_options *options)\n {\n-\treturn http_request_reauth(url, type, result,\n-\t\t\t\t   HTTP_REQUEST_STRBUF, options);\n+\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n }\n \n /*\n@@ -957,7 +957,8 @@ int http_get_strbuf(const char *url,\n  * If a previous interrupted download is detected (i.e. a previous temporary\n  * file is still around) the download is resumed.\n  */\n-static int http_get_file(const char *url, const char *filename, int options)\n+static int http_get_file(const char *url, const char *filename,\n+\t\t\t struct http_get_options *options)\n {\n \tint ret;\n \tstruct strbuf tmpfile = STRBUF_INIT;\n@@ -971,7 +972,7 @@ static int http_get_file(const char *url, const char *filename, int options)\n \t\tgoto cleanup;\n \t}\n \n-\tret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);\n+\tret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);\n \tfclose(result);\n \n \tif (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))\n@@ -983,12 +984,15 @@ int http_fetch_ref(const char *base, struct ref *ref)\n \n int http_fetch_ref(const char *base, struct ref *ref)\n {\n+\tstruct http_get_options options = {0};\n \tchar *url;\n \tstruct strbuf buffer = STRBUF_INIT;\n \tint ret = -1;\n \n+\toptions.no_cache = 1;\n+\n \turl = quote_ref_url(base, ref->name);\n-\tif (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {\n+\tif (http_get_strbuf(url, &buffer, &options) == HTTP_OK) {\n \t\tstrbuf_rtrim(&buffer);\n \t\tif (buffer.len == 40)\n \t\t\tret = get_sha1_hex(buffer.buf, ref->old_sha1);\n@@ -1072,6 +1076,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \n int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n {\n+\tstruct http_get_options options = {0};\n \tint ret = 0, i = 0;\n \tchar *url, *data;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1081,7 +1086,8 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)\n \tstrbuf_addstr(&buf, \"objects/info/packs\");\n \turl = strbuf_detach(&buf, NULL);\n \n-\tret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);\n+\toptions.no_cache = 1;\n+\tret = http_get_strbuf(url, &buf, &options);\n \tif (ret != HTTP_OK)\n \t\tgoto cleanup;\n \ndiff --git a/http.h b/http.h\nindex d77c1b5..40404b4 100644\n--- a/http.h\n+++ b/http.h\n@@ -125,11 +125,16 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n extern char *get_remote_object_url(const char *url, const char *hex,\n \t\t\t\t   int only_two_digit_prefix);\n \n-/* Options for http_request_*() */\n-#define HTTP_NO_CACHE\t\t1\n-#define HTTP_KEEP_ERROR\t\t2\n+/* Options for http_get_*() */\n+struct http_get_options {\n+\tunsigned no_cache:1,\n+\t\t keep_error:1;\n \n-/* Return values for http_request_*() */\n+\t/* If non-NULL, returns the content-type of the response. */\n+\tstruct strbuf *content_type;\n+};\n+\n+/* Return values for http_get_*() */\n #define HTTP_OK\t\t\t0\n #define HTTP_MISSING_TARGET\t1\n #define HTTP_ERROR\t\t2\n@@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n  *\n  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.\n  */\n-int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);\n+int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options);\n \n extern int http_fetch_ref(const char *base, struct ref *ref);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex b5ebe01..66705bb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -206,6 +206,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tstruct discovery *last = last_discovery;\n \tchar *refs_url;\n \tint http_ret, maybe_smart = 0;\n+\tstruct http_get_options options;\n \n \tif (last && !strcmp(service, last->service))\n \t\treturn last;\n@@ -223,8 +224,12 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t}\n \trefs_url = strbuf_detach(&buffer, NULL);\n \n-\thttp_ret = http_get_strbuf(refs_url, &type, &buffer,\n-\t\t\t\t   HTTP_NO_CACHE | HTTP_KEEP_ERROR);\n+\tmemset(&options, 0, sizeof(options));\n+\toptions.content_type = &type;\n+\toptions.no_cache = 1;\n+\toptions.keep_error = 1;\n+\n+\thttp_ret = http_get_strbuf(refs_url, &buffer, &options);\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228350","messageId":"20130928083145.GD2131@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 4/9] http: hoist credential request out of handle_curl_result","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:31:45Z","receivedAt":"2013-09-28T08:31:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are handling a curl response code in http_request or\nin the remote-curl RPC code, we use the handle_curl_result\nhelper to translate curl's response into an easy-to-use\ncode. When we see an HTTP 401, we do one of two things:\n\n  1. If we already had a filled-in credential, we mark it as\n     rejected, and then return HTTP_NOAUTH to indicate to\n     the caller that we failed.\n\n  2. If we didn't, then we ask for a new credential and tell\n     the caller HTTP_REAUTH to indicate that they may want\n     to try again.\n\nRejecting in the first case makes sense; it is the natural\nresult of the request we just made. However, prompting for\nmore credentials in the second step does not always make\nsense. We do not know for sure that the caller is going to\nmake a second request, and nor are we sure that it will be\nto the same URL. Logically, the prompt belongs not to the\nrequest we just finished, but to the request we are (maybe)\nabout to make.\n\nIn practice, it is very hard to trigger any bad behavior.\nCurrently, if we make a second request, it will always be to\nthe same URL (even in the face of redirects, because curl\nhandles the redirects internally). And we almost always\nretry on HTTP_REAUTH these days. The one exception is if we\nare streaming a large RPC request to the server (e.g., a\npushed packfile), in which case we cannot restart. It's\nextremely unlikely to see a 401 response at this stage,\nthough, as we would typically have seen it when we sent a\nprobe request, before streaming the data.\n\nThis patch drops the automatic prompt out of case 2, and\ninstead requires the caller to do it. This is a few extra\nlines of code, and the bug it fixes is unlikely to come up\nin practice. But it is conceptually cleaner, and paves the\nway for better handling of credentials across redirects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c        | 6 ++++--\n http.h        | 1 +\n remote-curl.c | 7 ++++++-\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 41ee7aa..5504b2c 100644\n--- a/http.c\n+++ b/http.c\n@@ -47,7 +47,7 @@ static int curl_save_cookies;\n static const char *curl_http_proxy;\n static const char *curl_cookie_file;\n static int curl_save_cookies;\n-static struct credential http_auth = CREDENTIAL_INIT;\n+struct credential http_auth = CREDENTIAL_INIT;\n static int http_proactive_auth;\n static const char *user_agent;\n \n@@ -823,7 +823,6 @@ int handle_curl_result(struct slot_results *results)\n \t\t\tcredential_reject(&http_auth);\n \t\t\treturn HTTP_NOAUTH;\n \t\t} else {\n-\t\t\tcredential_fill(&http_auth);\n \t\t\treturn HTTP_REAUTH;\n \t\t}\n \t} else {\n@@ -941,6 +940,9 @@ static int http_request_reauth(const char *url,\n \t\t\tdie(\"BUG: HTTP_KEEP_ERROR is only supported with strbufs\");\n \t\t}\n \t}\n+\n+\tcredential_fill(&http_auth);\n+\n \treturn http_request(url, result, target, options);\n }\n \ndiff --git a/http.h b/http.h\nindex 40404b4..17116ab 100644\n--- a/http.h\n+++ b/http.h\n@@ -102,6 +102,7 @@ extern size_t http_post_buffer;\n extern int active_requests;\n extern int http_is_verbose;\n extern size_t http_post_buffer;\n+extern struct credential http_auth;\n \n extern char curl_errorstr[CURL_ERROR_SIZE];\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 66705bb..d524462 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -9,6 +9,7 @@\n #include \"string-list.h\"\n #include \"sideband.h\"\n #include \"argv-array.h\"\n+#include \"credential.h\"\n \n static struct remote *remote;\n static const char *url; /* always ends with a trailing slash */\n@@ -468,6 +469,8 @@ static int post_rpc(struct rpc_state *rpc)\n \tif (large_request) {\n \t\tdo {\n \t\t\terr = probe_rpc(rpc);\n+\t\t\tif (err == HTTP_REAUTH)\n+\t\t\t\tcredential_fill(&http_auth);\n \t\t} while (err == HTTP_REAUTH);\n \t\tif (err != HTTP_OK)\n \t\t\treturn -1;\n@@ -567,8 +570,10 @@ retry:\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n \n \terr = run_slot(slot);\n-\tif (err == HTTP_REAUTH && !large_request)\n+\tif (err == HTTP_REAUTH && !large_request) {\n+\t\tcredential_fill(&http_auth);\n \t\tgoto retry;\n+\t}\n \tif (err != HTTP_OK)\n \t\terr = -1;\n \n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228351","messageId":"20130928083202.GE2131@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 5/9] http: provide effective url to callers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:32:02Z","receivedAt":"2013-09-28T08:32:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we ask curl to access a URL, it may follow one or more\nredirects to reach the final location. We have no idea\nthis has happened, as curl takes care of the details and\nsimply returns the final content to us.\n\nThe final URL that we ended up with can be accessed via\nCURLINFO_EFFECTIVE_URL. Let's make that optionally available\nto callers of http_get_*, so that they can make further\ndecisions based on the redirection.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 4 ++++\n http.h | 6 ++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 5504b2c..65a0048 100644\n--- a/http.c\n+++ b/http.c\n@@ -911,6 +911,10 @@ static int http_request(const char *url,\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,\n \t\t\t\toptions->content_type);\n \n+\tif (options && options->effective_url)\n+\t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\n+\t\t\t\toptions->effective_url);\n+\n \tcurl_slist_free_all(headers);\n \tstrbuf_release(&buf);\n \ndiff --git a/http.h b/http.h\nindex 17116ab..974ede7 100644\n--- a/http.h\n+++ b/http.h\n@@ -133,6 +133,12 @@ struct http_get_options {\n \n \t/* If non-NULL, returns the content-type of the response. */\n \tstruct strbuf *content_type;\n+\n+\t/*\n+\t * If non-NULL, returns the URL we ended up at, including any\n+\t * redirects we followed.\n+\t */\n+\tstruct strbuf *effective_url;\n };\n \n /* Return values for http_get_*() */\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228352","messageId":"20130928083405.GA2782@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 6/9] http: update base URLs when we see redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:34:05Z","receivedAt":"2013-09-28T08:34:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a caller asks the http_get_* functions to go to a\nparticular URL and we end up elsewhere due to a redirect,\nthe effective_url field can tell us where we went.\n\nIt would be nice to remember this redirect and short-cut\nfurther requests for two reasons:\n\n  1. It's more efficient. Otherwise we spend an extra http\n     round-trip to the server for each subsequent request,\n     just to get redirected.\n\n  2. If we end up with an http 401 and are going to ask for\n     credentials, it is to feed them to the redirect target.\n     If the redirect is an http->https upgrade, this means\n     our credentials may be provided on the http leg, just\n     to end up redirected to https. And if the redirect\n     crosses server boundaries, then curl will drop the\n     credentials entirely as it follows the redirect.\n\nHowever, it, it is not enough to simply record the effective\nURL we saw and use that for subsequent requests. We were\noriginally fed a \"base\" url like:\n\n   http://example.com/foo.git\n\nand we want to figure out what the new base is, even though\nthe URLs we see may be:\n\n     original: http://example.com/foo.git/info/refs\n    effective: http://example.com/bar.git/info/refs\n\nSubsequent requests will not be for \"info/refs\", but for\nother paths relative to the base. We must ask the caller to\npass in the original base, and we must pass the redirected\nbase back to the caller (so that it can generte more URLs\nfrom it). Furthermore, we need to feed the new base to the\ncredential code, so that requests to credential helpers (or\nto the user) match the URL we will be requesting.\n\nThis patch teaches http_request_reauth to do this munging.\nSince it is the caller who cares about making more URLs, it\nseems at first glance that callers could simply check\neffective_url themselves and handle it. However, since we\nneed to update the credential struct before the second\nre-auth request, we have to do it inside http_request_reauth.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n http.h |  8 ++++++++\n 2 files changed, 68 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 65a0048..8775b5c 100644\n--- a/http.c\n+++ b/http.c\n@@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,\n \treturn ret;\n }\n \n+/*\n+ * Update the \"base\" url to a more appropriate value, as deduced by\n+ * redirects seen when requesting a URL starting with \"url\".\n+ *\n+ * The \"asked\" parameter is a URL that we asked curl to access, and must begin\n+ * with \"base\".\n+ *\n+ * The \"got\" parameter is the URL that curl reported to us as where we ended\n+ * up.\n+ *\n+ * Returns 1 if we updated the base url, 0 otherwise.\n+ *\n+ * Our basic strategy is to compare \"base\" and \"asked\" to find the bits\n+ * specific to our request. We then strip those bits off of \"got\" to yield the\n+ * new base. So for example, if our base is \"http://example.com/foo.git\",\n+ * and we ask for \"http://example.com/foo.git/info/refs\", we might end up\n+ * with \"https://other.example.com/foo.git/info/refs\". We would want the\n+ * new URL to become \"https://other.example.com/foo.git\".\n+ *\n+ * Note that this assumes a sane redirect scheme. It's entirely possible\n+ * in the example above to end up at a URL that does not even end in\n+ * \"info/refs\".  In such a case we simply punt, as there is not much we can\n+ * do (and such a scheme is unlikely to represent a real git repository,\n+ * which means we are likely about to abort anyway).\n+ */\n+static int update_url_from_redirect(struct strbuf *base,\n+\t\t\t\t    const char *asked,\n+\t\t\t\t    const struct strbuf *got)\n+{\n+\tconst char *tail;\n+\tsize_t tail_len;\n+\n+\tif (!strcmp(asked, got->buf))\n+\t\treturn 0;\n+\n+\tif (strncmp(asked, base->buf, base->len))\n+\t\tdie(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n+\t\t    asked, base->buf);\n+\n+\ttail = asked + base->len;\n+\ttail_len = strlen(tail);\n+\n+\tif (got->len < tail_len ||\n+\t    strcmp(tail, got->buf + got->len - tail_len))\n+\t\treturn 0; /* insane redirect scheme */\n+\n+\tstrbuf_reset(base);\n+\tstrbuf_add(base, got->buf, got->len - tail_len);\n+\treturn 1;\n+}\n+\n static int http_request_reauth(const char *url,\n \t\t\t       void *result, int target,\n \t\t\t       struct http_get_options *options)\n {\n \tint ret = http_request(url, result, target, options);\n+\n+\tif (options && options->effective_url && options->base_url) {\n+\t\tif (update_url_from_redirect(options->base_url,\n+\t\t\t\t\t     url, options->effective_url)) {\n+\t\t\tcredential_from_url(&http_auth, options->base_url->buf);\n+\t\t\turl = options->effective_url->buf;\n+\t\t}\n+\t}\n+\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n \ndiff --git a/http.h b/http.h\nindex 974ede7..12ca5c8 100644\n--- a/http.h\n+++ b/http.h\n@@ -139,6 +139,14 @@ struct http_get_options {\n \t * redirects we followed.\n \t */\n \tstruct strbuf *effective_url;\n+\n+\t/*\n+\t * If both base_url and effective_url are non-NULL, the base URL will\n+\t * be munged to reflect any redirections going from the requested url\n+\t * to effective_url. See the definition of update_url_from_redirect\n+\t * for details.\n+\t */\n+\tstruct strbuf *base_url;\n };\n \n /* Return values for http_get_*() */\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228353","messageId":"20130928083510.GB2782@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 7/9] remote-curl: make refs_url a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:35:10Z","receivedAt":"2013-09-28T08:35:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the discover_refs function, we use a strbuf named\n\"buffer\" for multiple purposes. First we build the info/refs\nURL in it, and then detach that to a bare pointer. Then, we\nuse the same strbuf to store the result of fetching the\nrefs.\n\nLet's instead keep a separate refs_url strbuf. This is less\nconfusing, as the \"buffer\" strbuf is now used for only one\nthing.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one is not technically required.  In my original draft, having it\nas a strbuf was helpful, but that didn't make it to the final version.\nHowever, I think it's a worthwhile cleanup on its own, so I left it in.\n\n remote-curl.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d524462..7fb092f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -204,8 +204,8 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n+\tstruct strbuf refs_url = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n-\tchar *refs_url;\n \tint http_ret, maybe_smart = 0;\n \tstruct http_get_options options;\n \n@@ -213,24 +213,23 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\treturn last;\n \tfree_discovery(last);\n \n-\tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n+\tstrbuf_addf(&refs_url, \"%sinfo/refs\", url);\n \tif ((!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) &&\n \t     git_env_bool(\"GIT_SMART_HTTP\", 1)) {\n \t\tmaybe_smart = 1;\n \t\tif (!strchr(url, '?'))\n-\t\t\tstrbuf_addch(&buffer, '?');\n+\t\t\tstrbuf_addch(&refs_url, '?');\n \t\telse\n-\t\t\tstrbuf_addch(&buffer, '&');\n-\t\tstrbuf_addf(&buffer, \"service=%s\", service);\n+\t\t\tstrbuf_addch(&refs_url, '&');\n+\t\tstrbuf_addf(&refs_url, \"service=%s\", service);\n \t}\n-\trefs_url = strbuf_detach(&buffer, NULL);\n \n \tmemset(&options, 0, sizeof(options));\n \toptions.content_type = &type;\n \toptions.no_cache = 1;\n \toptions.keep_error = 1;\n \n-\thttp_ret = http_get_strbuf(refs_url, &buffer, &options);\n+\thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &options);\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n@@ -283,7 +282,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \telse\n \t\tlast->refs = parse_info_refs(last);\n \n-\tfree(refs_url);\n+\tstrbuf_release(&refs_url);\n \tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n \tstrbuf_release(&buffer);\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228354","messageId":"20130928083525.GC2782@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 8/9] remote-curl: store url as a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:35:25Z","receivedAt":"2013-09-28T08:35:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use a strbuf to generate the string containing the remote\nURL, but then detach it to a bare pointer. This makes it\nharder to later manipulate the URL, as we have forgotten the\nlength (and the allocation semantics are not as clear).\n\nLet's instead keep the strbuf around. As a bonus, this\neliminates a confusing double-use of the \"buf\" strbuf in\nmain(). Prior to this, it was used both for constructing the\nurl, and for reading commands from stdin.\n\nThe downside is that we have to update each call site to\nrefer to \"url.buf\" rather than just \"url\" when they want the\nC string.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c | 38 +++++++++++++++++++-------------------\n 1 file changed, 19 insertions(+), 19 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 7fb092f..5add2cb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -12,7 +12,8 @@ static struct remote *remote;\n #include \"credential.h\"\n \n static struct remote *remote;\n-static const char *url; /* always ends with a trailing slash */\n+/* always ends with a trailing slash */\n+static struct strbuf url = STRBUF_INIT;\n \n struct options {\n \tint verbosity;\n@@ -131,7 +132,8 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t\t\tmid = &data[i];\n \t\tif (data[i] == '\\n') {\n \t\t\tif (mid - start != 40)\n-\t\t\t\tdie(\"%sinfo/refs not valid: is this a git repository?\", url);\n+\t\t\t\tdie(\"%sinfo/refs not valid: is this a git repository?\",\n+\t\t\t\t    url.buf);\n \t\t\tdata[i] = 0;\n \t\t\tref_name = mid + 1;\n \t\t\tref = xmalloc(sizeof(struct ref) +\n@@ -150,7 +152,7 @@ static struct ref *parse_info_refs(struct discovery *heads)\n \t}\n \n \tref = alloc_ref(\"HEAD\");\n-\tif (!http_fetch_ref(url, ref) &&\n+\tif (!http_fetch_ref(url.buf, ref) &&\n \t    !resolve_remote_symref(ref, refs)) {\n \t\tref->next = refs;\n \t\trefs = ref;\n@@ -213,11 +215,11 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\treturn last;\n \tfree_discovery(last);\n \n-\tstrbuf_addf(&refs_url, \"%sinfo/refs\", url);\n-\tif ((!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) &&\n+\tstrbuf_addf(&refs_url, \"%sinfo/refs\", url.buf);\n+\tif ((!prefixcmp(url.buf, \"http://\") || !prefixcmp(url.buf, \"https://\")) &&\n \t     git_env_bool(\"GIT_SMART_HTTP\", 1)) {\n \t\tmaybe_smart = 1;\n-\t\tif (!strchr(url, '?'))\n+\t\tif (!strchr(url.buf, '?'))\n \t\t\tstrbuf_addch(&refs_url, '?');\n \t\telse\n \t\t\tstrbuf_addch(&refs_url, '&');\n@@ -235,13 +237,13 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\tbreak;\n \tcase HTTP_MISSING_TARGET:\n \t\tshow_http_message(&type, &buffer);\n-\t\tdie(\"repository '%s' not found\", url);\n+\t\tdie(\"repository '%s' not found\", url.buf);\n \tcase HTTP_NOAUTH:\n \t\tshow_http_message(&type, &buffer);\n-\t\tdie(\"Authentication failed for '%s'\", url);\n+\t\tdie(\"Authentication failed for '%s'\", url.buf);\n \tdefault:\n \t\tshow_http_message(&type, &buffer);\n-\t\tdie(\"unable to access '%s': %s\", url, curl_errorstr);\n+\t\tdie(\"unable to access '%s': %s\", url.buf, curl_errorstr);\n \t}\n \n \tlast= xcalloc(1, sizeof(*last_discovery));\n@@ -607,7 +609,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \trpc->out = client.out;\n \tstrbuf_init(&rpc->result, 0);\n \n-\tstrbuf_addf(&buf, \"%s%s\", url, svc);\n+\tstrbuf_addf(&buf, \"%s%s\", url.buf, svc);\n \trpc->service_url = strbuf_detach(&buf, NULL);\n \n \tstrbuf_addf(&buf, \"Content-Type: application/x-%s-request\", svc);\n@@ -659,7 +661,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)\n \tfor (i = 0; i < nr_heads; i++)\n \t\ttargets[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));\n \n-\twalker = get_http_walker(url);\n+\twalker = get_http_walker(url.buf);\n \twalker->get_all = 1;\n \twalker->get_tree = 1;\n \twalker->get_history = 1;\n@@ -706,7 +708,7 @@ static int fetch_git(struct discovery *heads,\n \t\tdepth_arg = strbuf_detach(&buf, NULL);\n \t\targv[argc++] = depth_arg;\n \t}\n-\targv[argc++] = url;\n+\targv[argc++] = url.buf;\n \targv[argc++] = NULL;\n \n \tfor (i = 0; i < nr_heads; i++) {\n@@ -804,7 +806,7 @@ static int push_dav(int nr_spec, char **specs)\n \t\targv[argc++] = \"--dry-run\";\n \tif (options.verbosity > 1)\n \t\targv[argc++] = \"--verbose\";\n-\targv[argc++] = url;\n+\targv[argc++] = url.buf;\n \tfor (i = 0; i < nr_spec; i++)\n \t\targv[argc++] = specs[i];\n \targv[argc++] = NULL;\n@@ -837,7 +839,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \targv_array_push(&args, options.progress ? \"--progress\" : \"--no-progress\");\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\targv_array_push(&args, cas_option->string);\n-\targv_array_push(&args, url);\n+\targv_array_push(&args, url.buf);\n \tfor (i = 0; i < nr_spec; i++)\n \t\targv_array_push(&args, specs[i]);\n \n@@ -918,14 +920,12 @@ int main(int argc, const char **argv)\n \tremote = remote_get(argv[1]);\n \n \tif (argc > 2) {\n-\t\tend_url_with_slash(&buf, argv[2]);\n+\t\tend_url_with_slash(&url, argv[2]);\n \t} else {\n-\t\tend_url_with_slash(&buf, remote->url[0]);\n+\t\tend_url_with_slash(&url, remote->url[0]);\n \t}\n \n-\turl = strbuf_detach(&buf, NULL);\n-\n-\thttp_init(remote, url, 0);\n+\thttp_init(remote, url.buf, 0);\n \n \tdo {\n \t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228355","messageId":"20130928083535.GD2782@sigill.intra.peff.net","threadId":"35039","inReplyTo":"20130928082956.GA22610@sigill.intra.peff.net","subject":"[PATCH 9/9] remote-curl: rewrite base url from info/refs redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-28T08:35:35Z","receivedAt":"2013-09-28T08:35:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For efficiency and security reasons, an earlier commit in\nthis series taught http_get_* to re-write the base url based\non redirections we saw while making a specific request.\n\nThis commit wires that option into the info/refs request,\nmeaning that a redirect from\n\n    http://example.com/foo.git/info/refs\n\nto\n\n    https://example.com/bar.git/info/refs\n\nwill behave as if \"https://example.com/bar.git\" had been\nprovided to git in the first place.\n\nThe tests bear some explanation. We introduce two new\nhierearchies into the httpd test config:\n\n  1. Requests to /smart-redir-limited will work only for the\n     initial info/refs request, but not any subsequent\n     requests. As a result, we can confirm whether the\n     client is re-rooting its requests after the initial\n     contact, since otherwise it will fail (it will ask for\n     \"repo.git/git-upload-pack\", which is not redirected).\n\n  2. Requests to smart-redir-auth will redirect, and require\n     auth after the redirection. Since we are using the\n     redirected base for further requests, we also update\n     the credential struct, in order not to mislead the user\n     (or credential helpers) about which credential is\n     needed. We can therefore check the GIT_ASKPASS prompts\n     to make sure we are prompting for the new location.\n     Because we have neither multiple servers nor https\n     support in our test setup, we can only redirect between\n     paths, meaning we need to turn on\n     credential.useHttpPath to see the difference.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c           |  4 ++++\n t/lib-httpd.sh          |  3 ++-\n t/lib-httpd/apache.conf |  2 ++\n t/t5551-http-fetch.sh   | 11 +++++++++++\n 4 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 5add2cb..c9b891a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -207,6 +207,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tstruct strbuf type = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct strbuf refs_url = STRBUF_INIT;\n+\tstruct strbuf effective_url = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n \tint http_ret, maybe_smart = 0;\n \tstruct http_get_options options;\n@@ -228,6 +229,8 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \n \tmemset(&options, 0, sizeof(options));\n \toptions.content_type = &type;\n+\toptions.effective_url = &effective_url;\n+\toptions.base_url = &url;\n \toptions.no_cache = 1;\n \toptions.keep_error = 1;\n \n@@ -287,6 +290,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tstrbuf_release(&refs_url);\n \tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n+\tstrbuf_release(&effective_url);\n \tstrbuf_release(&buffer);\n \tlast_discovery = last;\n \treturn last;\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 54dbbfe..ad8f1ef 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -204,7 +204,8 @@ expect_askpass() {\n }\n \n expect_askpass() {\n-\tdest=$HTTPD_DEST\n+\tdest=$HTTPD_DEST${3+/$3}\n+\n \t{\n \t\tcase \"$1\" in\n \t\tnone)\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 397c480..3a03e82 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -110,6 +110,8 @@ RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n RewriteEngine on\n RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]\n RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]\n+RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]\n+RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301]\n \n <IfDefine SSL>\n LoadModule ssl_module modules/mod_ssl.so\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex 8196af1..fb16833 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -113,6 +113,10 @@ test_expect_success 'follow redirects (302)' '\n \tgit clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t\n '\n \n+test_expect_success 'redirects re-root further requests' '\n+\tgit clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited\n+'\n+\n test_expect_success 'clone from password-protected repository' '\n \techo two >expect &&\n \tset_askpass user@host &&\n@@ -146,6 +150,13 @@ test_expect_success 'no-op half-auth fetch does not require a password' '\n \texpect_askpass none\n '\n \n+test_expect_success 'redirects send auth to new location' '\n+\tset_askpass user@host &&\n+\tgit -c credential.useHttpPath=true \\\n+\t  clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&\n+\texpect_askpass both user@host auth/smart/repo.git\n+'\n+\n test_expect_success 'disable dumb http on server' '\n \tgit --git-dir=\"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n \t\tconfig http.getanyfile false\n-- \n1.8.4.rc3.19.g9da5bf6\n"},{"id":"228457","messageId":"20130929185408.GB18384@vauxhall.crustytoothpaste.net","threadId":"35039","inReplyTo":"20130928083405.GA2782@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-09-29T18:54:08Z","receivedAt":"2013-09-29T18:54:08Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Sep 28, 2013 at 04:34:05AM -0400, Jeff King wrote:\n> Subsequent requests will not be for \"info/refs\", but for\n> other paths relative to the base. We must ask the caller to\n> pass in the original base, and we must pass the redirected\n> base back to the caller (so that it can generte more URLs\n\ns/generte/generate/\n\n> from it). Furthermore, we need to feed the new base to the\n> credential code, so that requests to credential helpers (or\n> to the user) match the URL we will be requesting.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"228459","messageId":"CAPig+cQVQ6iY8onvb--88W0XHofC815NZHTb=L744hANtT8mVQ@mail.gmail.com","threadId":"35039","inReplyTo":"20130928083405.GA2782@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-29T19:26:45Z","receivedAt":"2013-09-29T19:26:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:\n> diff --git a/http.c b/http.c\n> index 65a0048..8775b5c 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,\n> +static int update_url_from_redirect(struct strbuf *base,\n> +                                   const char *asked,\n> +                                   const struct strbuf *got)\n> +{\n> +       const char *tail;\n> +       size_t tail_len;\n> +\n> +       if (!strcmp(asked, got->buf))\n> +               return 0;\n> +\n> +       if (strncmp(asked, base->buf, base->len))\n> +               die(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n> +                   asked, base->buf);\n\nIs there something non-obvious going on here? die(...,base->buf) takes\nadvantage of the terminating NUL promised by strbuf, but then\nstrncmp(...,base->buf,base->len) is used rather than the simpler\nstrcmp(...,base->buf).\n\n> +       tail = asked + base->len;\n> +       tail_len = strlen(tail);\n> +\n> +       if (got->len < tail_len ||\n> +           strcmp(tail, got->buf + got->len - tail_len))\n> +               return 0; /* insane redirect scheme */\n> +\n> +       strbuf_reset(base);\n> +       strbuf_add(base, got->buf, got->len - tail_len);\n> +       return 1;\n> +}\n"},{"id":"228460","messageId":"20130929193245.GB17644@sigill.intra.peff.net","threadId":"35039","inReplyTo":"CAPig+cQVQ6iY8onvb--88W0XHofC815NZHTb=L744hANtT8mVQ@mail.gmail.com","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-29T19:32:45Z","receivedAt":"2013-09-29T19:32:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:\n\n> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:\n> > diff --git a/http.c b/http.c\n> > index 65a0048..8775b5c 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,\n> > +static int update_url_from_redirect(struct strbuf *base,\n> > +                                   const char *asked,\n> > +                                   const struct strbuf *got)\n> > +{\n> > +       const char *tail;\n> > +       size_t tail_len;\n> > +\n> > +       if (!strcmp(asked, got->buf))\n> > +               return 0;\n> > +\n> > +       if (strncmp(asked, base->buf, base->len))\n> > +               die(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n> > +                   asked, base->buf);\n> \n> Is there something non-obvious going on here? die(...,base->buf) takes\n> advantage of the terminating NUL promised by strbuf, but then\n> strncmp(...,base->buf,base->len) is used rather than the simpler\n> strcmp(...,base->buf).\n\nYes, we are not checking for equality, but rather making sure that\n\"asked\" begins with \"base->buf\". It might be more clearly written as:\n\n  if (prefixcmp(asked, base->buf))\n\nI was trying to take advantage of the fact that we know base->len\nalready, but this it not a particularly performance-critical code path.\nWe can afford the extra strlen that prefixcmp will do.\n\n-Peff\n"},{"id":"228461","messageId":"CAPig+cQzStMzjONbxm3bhSYZ0AG+67ViQR1UBdqAYnvEnLyWGA@mail.gmail.com","threadId":"35039","inReplyTo":"20130929193245.GB17644@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-29T19:50:29Z","receivedAt":"2013-09-29T19:50:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 29, 2013 at 3:32 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:\n>\n>> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:\n>> > diff --git a/http.c b/http.c\n>> > index 65a0048..8775b5c 100644\n>> > --- a/http.c\n>> > +++ b/http.c\n>> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,\n>> > +static int update_url_from_redirect(struct strbuf *base,\n>> > +                                   const char *asked,\n>> > +                                   const struct strbuf *got)\n>> > +{\n>> > +       const char *tail;\n>> > +       size_t tail_len;\n>> > +\n>> > +       if (!strcmp(asked, got->buf))\n>> > +               return 0;\n>> > +\n>> > +       if (strncmp(asked, base->buf, base->len))\n>> > +               die(\"BUG: update_url_from_redirect: %s is not a superset of %s\",\n>> > +                   asked, base->buf);\n>>\n>> Is there something non-obvious going on here? die(...,base->buf) takes\n>> advantage of the terminating NUL promised by strbuf, but then\n>> strncmp(...,base->buf,base->len) is used rather than the simpler\n>> strcmp(...,base->buf).\n>\n> Yes, we are not checking for equality, but rather making sure that\n> \"asked\" begins with \"base->buf\". It might be more clearly written as:\n\nAh right, I knew that that was the intention but had a synapse\nmisfire. Sorry for the noise.\n\n>\n>   if (prefixcmp(asked, base->buf))\n>\n> I was trying to take advantage of the fact that we know base->len\n> already, but this it not a particularly performance-critical code path.\n> We can afford the extra strlen that prefixcmp will do.\n>\n> -Peff\n"},{"id":"229181","messageId":"xmqqk3hawi5y.fsf@gitster.dls.corp.google.com","threadId":"35039","inReplyTo":"20130928083405.GA2782@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-18T18:25:13Z","receivedAt":"2013-10-18T18:25:13Z","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> + * Our basic strategy is to compare \"base\" and \"asked\" to find the bits\n> + * specific to our request. We then strip those bits off of \"got\" to yield the\n> + * new base. So for example, if our base is \"http://example.com/foo.git\",\n> + * and we ask for \"http://example.com/foo.git/info/refs\", we might end up\n> + * with \"https://other.example.com/foo.git/info/refs\". We would want the\n> + * new URL to become \"https://other.example.com/foo.git\".\n\nNot \"https://other.example.com/foo.git/info/refs\"?\n"},{"id":"229182","messageId":"20131018184250.GA11040@sigill.intra.peff.net","threadId":"35039","inReplyTo":"xmqqk3hawi5y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-10-18T18:42:50Z","receivedAt":"2013-10-18T18:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > + * Our basic strategy is to compare \"base\" and \"asked\" to find the bits\n> > + * specific to our request. We then strip those bits off of \"got\" to yield the\n> > + * new base. So for example, if our base is \"http://example.com/foo.git\",\n> > + * and we ask for \"http://example.com/foo.git/info/refs\", we might end up\n> > + * with \"https://other.example.com/foo.git/info/refs\". We would want the\n> > + * new URL to become \"https://other.example.com/foo.git\".\n> \n> Not \"https://other.example.com/foo.git/info/refs\"?\n\nI think my use of \"the new URL\" is ambiguous. I meant \"the new base\",\nfrom which one could then construct the new refs URL as you suggest.\n\n-Peff\n"},{"id":"229185","messageId":"xmqqbo2mwfl9.fsf@gitster.dls.corp.google.com","threadId":"35039","inReplyTo":"20131018184250.GA11040@sigill.intra.peff.net","subject":"Re: [PATCH 6/9] http: update base URLs when we see redirects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-10-18T19:20:50Z","receivedAt":"2013-10-18T19:20:50Z","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, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > + * Our basic strategy is to compare \"base\" and \"asked\" to find the bits\n>> > + * specific to our request. We then strip those bits off of \"got\" to yield the\n>> > + * new base. So for example, if our base is \"http://example.com/foo.git\",\n>> > + * and we ask for \"http://example.com/foo.git/info/refs\", we might end up\n>> > + * with \"https://other.example.com/foo.git/info/refs\". We would want the\n>> > + * new URL to become \"https://other.example.com/foo.git\".\n>> \n>> Not \"https://other.example.com/foo.git/info/refs\"?\n>\n> I think my use of \"the new URL\" is ambiguous. I meant \"the new base\",\n> from which one could then construct the new refs URL as you suggest.\n\nAhh, there is nothing we need to do to make the new URL to\n\"https://.../info/refs\"; we already have it in \"got\".  And the\nfunction resets \"base\" and then adds \"got\" excluding its tail part\nto compute the new base.  So \"s/new URL/new base/\" would be all we\nneed to do, I think.\n\nThanks.\n"}]}