{"thread":{"id":"31615","subject":"[PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","startedAt":"2012-09-20T02:55:53Z","lastAt":"2012-10-02T13:57:29Z","messageCount":36,"participants":["Shawn O. Pearce","Shawn Pearce","Jeff King","Junio C Hamano","Drew Northup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"199562","messageId":"1348109753-32388-1-git-send-email-spearce@spearce.org","threadId":"31615","inReplyTo":null,"subject":"[PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-20T02:55:53Z","receivedAt":"2012-09-20T02:55:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nIf the user doesn't want to use the dumb HTTP protocol, she may\nset GIT_CURL_FALLBACK=0 in the environment before invoking a Git\nprotocol operation. This is mostly useful when testing against\nservers that are known to not support the dumb protocol. If the\nsmart service detection fails the client should not continue with\ndumb behavior, but instead provide accurate HTTP failure data.\n\nSigned-off-by: Shawn O. Pearce \n---\n remote-curl.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4a0927e..2f91128 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -17,7 +17,8 @@ struct options {\n \tunsigned progress : 1,\n \t\tfollowtags : 1,\n \t\tdry_run : 1,\n-\t\tthin : 1;\n+\t\tthin : 1,\n+\t\tfallback : 1;\n };\n static struct options options;\n \n@@ -115,7 +116,8 @@ static struct discovery* discover_refs(const char *service)\n \thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n \n \t/* try again with \"plain\" url (no ? or & appended) */\n-\tif (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {\n+\tif (options.fallback && http_ret != HTTP_OK\n+\t && http_ret != HTTP_NOAUTH) {\n \t\tfree(refs_url);\n \t\tstrbuf_reset(&buffer);\n \n@@ -868,6 +870,12 @@ int main(int argc, const char **argv)\n \toptions.verbosity = 1;\n \toptions.progress = !!isatty(2);\n \toptions.thin = 1;\n+\toptions.fallback = 1;\n+\n+\tif (getenv(\"GIT_CURL_FALLBACK\")) {\n+\t\tchar *fb = getenv(\"GIT_CURL_FALLBACK\");\n+\t\toptions.fallback = *fb != '0';\n+\t}\n \n \tremote = remote_get(argv[1]);\n \n-- \n1.7.12.1.512.g9b230e6\n"},{"id":"199564","messageId":"CAJo=hJtx25=5Lb3sgu_o42=VrcXkRE1DF_noPpjqyjE1zuzKJg@mail.gmail.com","threadId":"31615","inReplyTo":"1348109753-32388-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-20T03:22:58Z","receivedAt":"2012-09-20T03:22:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Sep 19, 2012 at 7:55 PM, Shawn O. Pearce wrote:\n> From: \"Shawn O. Pearce\" \n\nI can't explain why git send-email did this. I obviously didn't need\nthe extra From header here. format-patch didn't write it to the patch\nfile, it was injected by send-email. My .git/config is pretty simple,\nthe name/email are derived from there:\n\n [user]\n\tname = Shawn O. Pearce\n\temail = spearce@spearce.org\n\nIck. I really don't want to debug this right now so I'm just going to\npretend it wasn't written.\n"},{"id":"199565","messageId":"20120920034804.GA32313@sigill.intra.peff.net","threadId":"31615","inReplyTo":"1348109753-32388-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T03:48:05Z","receivedAt":"2012-09-20T03:48:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 07:55:53PM -0700, Shawn O. Pearce wrote:\n\n> From: \"Shawn O. Pearce\" \n> \n> If the user doesn't want to use the dumb HTTP protocol, she may\n> set GIT_CURL_FALLBACK=0 in the environment before invoking a Git\n> protocol operation. This is mostly useful when testing against\n> servers that are known to not support the dumb protocol. If the\n> smart service detection fails the client should not continue with\n> dumb behavior, but instead provide accurate HTTP failure data.\n\nI have been looking into this recently, as well. GitHub does not allow\ndumb http at all these days, so transient errors on the initial smart\ncontact can cause us to fall back to dumb, and end up reporting a\ntotally useless 403 Forbidden error. I guess Google Code has a similar\nissue.\n\nNote that it is not really do not \"fall back to dumb\"; we detect the\ndumb nature from the response. It is really \"fall back to trying the URL\nwithout the query string, because there are some servers that cannot\nhandle it\". With your patch, we might still end up performing a dumb\ntransfer.\n\nI think what you're doing here is sane, because you have to turn it on\nmanually, and thus there are no possible backwards compatibility issues.\nBut it might be nice to make things work better out of the box. Here are\ntwo client-side changes I've been toying with:\n\n 1. If both smart and dumb requests fail, report the error for the\n smart request. Now that smart-http clients are common, I'd expect\n most http servers to be smart these days. Of course I don't have\n any sort of numbers to back this up (nor am I sure how to get them;\n obviously big sites like GitHub and Google Code do a lot of\n traffic, but who knows how many one-off repo-on-a-generic-web-host\n sites still exist?).\n\n An alternative would be to simply be more verbose, and mention that\n we tried to fallback and list both failures (or we could do this\n with just \"fetch -v\").\n\n 2. Be more discerning about which errors will cause a fallback.\n Something like \"504 Gateway Timeout\" should not give a fallback.\n The problem is that you are really guessing at what kinds of http\n errors you are going to get from a dumb server when you try the\n smart URL. I dug back into the list thread that spawned the \"retry\n without query string\" patch (703e6e7).\n\n The thread is here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/137609\n\n If you read the thread, it turns out that the problem in this case\n (which is the only reported case I could find in the archive) is\n that the server was misconfigured to treat _anything_ with a query\n string as a gitweb URL. And then it got fixed pretty much\n immediately.\n\n So as far as we know, there may be zero servers for which this\n fallback is actually doing anything useful.\n\nI'm tempted to just reverse the logic. Try the request with the query\nstring and immediately fail if it doesn't work. For the few (if any)\npeople who are hitting a server that will not serve the dumb file in\nthat case, add a \"remote.*.dumbhttp\" setting that will turn off smart\ncompletely as a workaround.\n\nThat would serve the (presumed) majority who are using smart http,\neveryone using dumb http on a reasonably-configured server, and still\nallow an easy workaround for people with badly configured servers.\n\nWhat do you think?\n\n> ---\n> remote-curl.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n\nIf we do go this route, the patch itself looks fairly obvious, although:\n\n> @@ -868,6 +870,12 @@ int main(int argc, const char **argv)\n> \toptions.verbosity = 1;\n> \toptions.progress = !!isatty(2);\n> \toptions.thin = 1;\n> +\toptions.fallback = 1;\n> +\n> +\tif (getenv(\"GIT_CURL_FALLBACK\")) {\n> +\t\tchar *fb = getenv(\"GIT_CURL_FALLBACK\");\n> +\t\toptions.fallback = *fb != '0';\n> +\t}\n\nThis can just be:\n\n options.fallback = git_env_bool(\"GIT_CURL_FALLBACK\", 1);\n\nFewer lines, and you get all of the true/false parsing for free.\n\n-Peff\n"},{"id":"199566","messageId":"20120920035231.GB32313@sigill.intra.peff.net","threadId":"31615","inReplyTo":"CAJo=hJtx25=5Lb3sgu_o42=VrcXkRE1DF_noPpjqyjE1zuzKJg@mail.gmail.com","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T03:52:31Z","receivedAt":"2012-09-20T03:52:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 08:22:58PM -0700, Shawn O. Pearce wrote:\n\n> On Wed, Sep 19, 2012 at 7:55 PM, Shawn O. Pearce wrote:\n> > From: \"Shawn O. Pearce\" \n> \n> I can't explain why git send-email did this. I obviously didn't need\n> the extra From header here. format-patch didn't write it to the patch\n> file, it was injected by send-email. My .git/config is pretty simple,\n> the name/email are derived from there:\n> \n> [user]\n> \tname = Shawn O. Pearce\n> \temail = spearce@spearce.org\n> \n> Ick. I really don't want to debug this right now so I'm just going to\n> pretend it wasn't written.\n\nI was looking at the send-email author comparison code a month or three\nago, and I remember noticing that it totally fails to canonicalize\nbefore comparing. Without even looking at it again, I'm fairly sure\nthat it thinks '\"Shawn O. Pearce\"' and 'Shawn O. Pearce' (i.e., with and\nwithout the quotes) are different, and therefore author != committer.\n\nIn your case it is particularly egregious because the quotes are\nintroduced (correctly) by format-patch, so it is not even like you have\nconfigured two different versions of your name. \n\nI think the same bug exists for different rfc2047 encodings of a name\nwith non-ascii characters. Fixing both would involve canonicalizing the\nnames before comparing. I wonder if it would be simpler to just compare\nthe email addresses and ignore the names entirely.\n\n-Peff\n"},{"id":"199569","messageId":"1348114499-22811-1-git-send-email-gitster@pobox.com","threadId":"31615","inReplyTo":"1348109753-32388-1-git-send-email-spearce@spearce.org","subject":"Re* [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T04:14:57Z","receivedAt":"2012-09-20T04:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> From: \"Shawn O. Pearce\" \n>\n> If the user doesn't want to use the dumb HTTP protocol, she may\n> set GIT_CURL_FALLBACK=0 in the environment before invoking a Git\n> protocol operation. This is mostly useful when testing against\n> servers that are known to not support the dumb protocol. If the\n> smart service detection fails the client should not continue with\n> dumb behavior, but instead provide accurate HTTP failure data.\n>\n> Signed-off-by: Shawn O. Pearce \n> ---\n> remote-curl.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n\nI can see how this feature may be useful, but I have to say the\nexternal interface triply sucks.\n\n - If it is primarily for debugging smart HTTP, a solution with an\n environment variable without more permanent configuration\n variable would be sufficient, but then the environment variable\n is better named GIT_SMART_HTTP_DEBUG, or something no?\n\n - If it is useful outside the context of debugging, perhaps a per\n remote configuration variable remote.$name.$variable (or\n http.$prefix_of_server_url.$variable) might be necessary?\n\n - I do not see this as \"fallback (to) curl\"; you still talk your\n smart protocol over curl library. \"fallback to dumb http\" is\n more understandable. \n\nIn any case, I think CURL_FALLBACK was named with CURL in its name\nprimarily because the environment applies only to remote-curl, but\nthat means we cannot have any fallback logic other than the current\n\"smart does not work, fall back on dumb\" in the future.\n\nHere is a bit of rewrite. [1/2] is yours but with a bit more\nsensible name. [2/2] is entirely optional.\n\n\nJunio C Hamano (1):\n remote-curl: make dumb-http fallback configurable per URL\n\nShawn O. Pearce (1):\n Disable dumb HTTP fallback with GIT_DUMB_HTTP_FALLBACK=false\n\n remote-curl.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 53 insertions(+), 2 deletions(-)\n\n-- \n1.7.12.1.389.g3dff30b\n"},{"id":"199568","messageId":"1348114499-22811-2-git-send-email-gitster@pobox.com","threadId":"31615","inReplyTo":"1348114499-22811-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] Disable dumb HTTP fallback with GIT_DUMB_HTTP_FALLBACK=false","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T04:14:58Z","receivedAt":"2012-09-20T04:14:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nIf the user doesn't want to use the dumb HTTP protocol, she may set\nGIT_DUMB_HTTP_FALLBACK=false in the environment before invoking a\nGit protocol operation. This is mostly useful when testing against\nservers that are known to not support the dumb protocol. If the\nsmart service detection fails the client should not continue with\ndumb behavior, but instead provide accurate HTTP failure data.\n\nSigned-off-by: Shawn O. Pearce \nSigned-off-by: Junio C Hamano \n---\n remote-curl.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 3ec474f..f25cf3c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -17,7 +17,8 @@ struct options {\n \tunsigned progress : 1,\n \t\tfollowtags : 1,\n \t\tdry_run : 1,\n-\t\tthin : 1;\n+\t\tthin : 1,\n+\t\tfallback : 1;\n };\n static struct options options;\n \n@@ -115,7 +116,8 @@ static struct discovery* discover_refs(const char *service)\n \thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n \n \t/* try again with \"plain\" url (no ? or & appended) */\n-\tif (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {\n+\tif (options.fallback && http_ret != HTTP_OK\n+\t && http_ret != HTTP_NOAUTH) {\n \t\tfree(refs_url);\n \t\tstrbuf_reset(&buffer);\n \n@@ -853,6 +855,8 @@ static void parse_push(struct strbuf *buf)\n \tfree(specs);\n }\n \n+static const char DUMB_HTTP_FALLBACK_ENV[] = \"GIT_DUMB_HTTP_FALLBACK\";\n+\n int main(int argc, const char **argv)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -868,6 +872,12 @@ int main(int argc, const char **argv)\n \toptions.verbosity = 1;\n \toptions.progress = !!isatty(2);\n \toptions.thin = 1;\n+\toptions.fallback = 1;\n+\n+\tif (getenv(DUMB_HTTP_FALLBACK_ENV)) {\n+\t\tchar *fb = getenv(DUMB_HTTP_FALLBACK_ENV);\n+\t\toptions.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);\n+\t}\n \n \tremote = remote_get(argv[1]);\n \n-- \n1.7.12.1.389.g3dff30b\n"},{"id":"199567","messageId":"1348114499-22811-3-git-send-email-gitster@pobox.com","threadId":"31615","inReplyTo":"1348114499-22811-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] remote-curl: make dumb-http fallback configurable per URL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T04:14:59Z","receivedAt":"2012-09-20T04:14:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Introduce http.$url_prefix.dumbhttpfallback configuration variables\nso that users do not have to set GIT_DUMB_HTTP_FALLBACK environment\ndepending on which remote they are talking with.\n\nSigned-off-by: Junio C Hamano \n---\n remote-curl.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 47 insertions(+), 6 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex f25cf3c..44544c7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -855,6 +855,46 @@ static void parse_push(struct strbuf *buf)\n \tfree(specs);\n }\n \n+struct dumb_fallback_cb {\n+\tconst char *url;\n+\tint value;\n+};\n+\n+static int dumb_fallback_cb(const char *key, const char *value, void *cb_)\n+{\n+\tstruct dumb_fallback_cb *cb = cb_;\n+\tint i, len;\n+\n+\t/* Is this \"http.$url.dumbHttpFallback\"? */\n+\tif (prefixcmp(key, \"http.\"))\n+\t\treturn 0;\n+\tlen = strrchr(key, '.') - key;\n+\tif (len <= 5)\n+\t\t/* we found the dot at the end of \"http.\" */\n+\t\treturn 0;\n+\tkey += 5; /* skip over http. part */\n+\tlen -= 5;\n+\tif (strcmp(key + len, \".dumbhttpfallback\"))\n+\t\treturn 0;\n+\n+\t/* Does the $url part match? */\n+\tfor (i = 0; i < len; i++)\n+\t\tif (cb->url[i] != key[i])\n+\t\t\treturn 0;\n+\tcb->value = git_config_bool(key, value);\n+\treturn 0;\n+}\n+\n+static int dumb_fallback_config(const char *url)\n+{\n+\tstruct dumb_fallback_cb cb;\n+\n+\tcb.url = url;\n+\tcb.value = 1; /* defaults to true */\n+\tgit_config(dumb_fallback_cb, &cb);\n+\treturn cb.value;\n+}\n+\n static const char DUMB_HTTP_FALLBACK_ENV[] = \"GIT_DUMB_HTTP_FALLBACK\";\n \n int main(int argc, const char **argv)\n@@ -872,12 +912,6 @@ int main(int argc, const char **argv)\n \toptions.verbosity = 1;\n \toptions.progress = !!isatty(2);\n \toptions.thin = 1;\n-\toptions.fallback = 1;\n-\n-\tif (getenv(DUMB_HTTP_FALLBACK_ENV)) {\n-\t\tchar *fb = getenv(DUMB_HTTP_FALLBACK_ENV);\n-\t\toptions.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);\n-\t}\n \n \tremote = remote_get(argv[1]);\n \n@@ -889,6 +923,13 @@ int main(int argc, const char **argv)\n \n \turl = strbuf_detach(&buf, NULL);\n \n+\tif (getenv(DUMB_HTTP_FALLBACK_ENV)) {\n+\t\tchar *fb = getenv(DUMB_HTTP_FALLBACK_ENV);\n+\t\toptions.fallback = git_config_bool(DUMB_HTTP_FALLBACK_ENV, fb);\n+\t} else {\n+\t\toptions.fallback = dumb_fallback_config(url);\n+\t}\n+\n \thttp_init(remote, url, 0);\n \n \tdo {\n-- \n1.7.12.1.389.g3dff30b\n"},{"id":"199571","messageId":"CAJo=hJvXtSBO3QEzhZCFfhk9OF_e0B10k8tjCUWMHZvGKt599Q@mail.gmail.com","threadId":"31615","inReplyTo":"20120920034804.GA32313@sigill.intra.peff.net","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-20T05:57:35Z","receivedAt":"2012-09-20T05:57:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Sep 19, 2012 at 8:48 PM, Jeff King wrote:\n> On Wed, Sep 19, 2012 at 07:55:53PM -0700, Shawn O. Pearce wrote:\n>\n>> If the user doesn't want to use the dumb HTTP protocol, she may\n>> set GIT_CURL_FALLBACK=0 in the environment before invoking a Git\n>> protocol operation. This is mostly useful when testing against\n>> servers that are known to not support the dumb protocol. If the\n>> smart service detection fails the client should not continue with\n>> dumb behavior, but instead provide accurate HTTP failure data.\n>\n> I have been looking into this recently, as well. GitHub does not allow\n> dumb http at all these days,\n\nInteresting that GitHub doesn't support dumb transfer either.\n\n> so transient errors on the initial smart\n> contact can cause us to fall back to dumb,\n\nTransient errors is actually what is leading me down this path. We see\nabout 0.0455% of our requests to the Android hosting service as these\ndumb fallbacks. This means the client never got a proper smart service\nreply. Server logs suggest we sent a valid response, so I am\nsuspecting transient proxy errors, but its hard to debug because\nclients discard the error.\n\n> and end up reporting a\n> totally useless 403 Forbidden error.\n\nToday I posted a change to JGit [1] to make this 406 Not Acceptable as\nI think it better matches the description of the failure. It also no\nlonger reuses the common 403 Forbidden error that a server might\nchoose to return if a request was given with invalid credentials.\n\n[1] https://git.eclipse.org/r/7847\n\n> I guess Google Code has a similar\n> issue.\n\nYes, and {android,kernel,gerrit}.googlesource.com also only support\nthe smart protocol.\n\n> Note that it is not really do not \"fall back to dumb\"; we detect the\n> dumb nature from the response. It is really \"fall back to trying the URL\n> without the query string, because there are some servers that cannot\n> handle it\". With your patch, we might still end up performing a dumb\n> transfer.\n\nYes, the server could ignore the parameter and return a dumb info/refs\nresponse on the initial request, forcing the client to a dumb\ntransfer. I forgot this case being common on a dumb server. I have\nonly been working with or worrying about smart servers, whose\ntransient failures have this info/refs request that they always\nfail... giving a poor user experience.\n\n> I think what you're doing here is sane, because you have to turn it on\n> manually, and thus there are no possible backwards compatibility issues.\n> But it might be nice to make things work better out of the box. Here are\n> two client-side changes I've been toying with:\n>\n> 1. If both smart and dumb requests fail, report the error for the\n> smart request. Now that smart-http clients are common, I'd expect\n> most http servers to be smart these days. Of course I don't have\n> any sort of numbers to back this up (nor am I sure how to get them;\n> obviously big sites like GitHub and Google Code do a lot of\n> traffic, but who knows how many one-off repo-on-a-generic-web-host\n> sites still exist?).\n\nI suspect there are still a number of servers that rely on dumb\nprotocol to host repositories because its very simple to setup.\nBreaking support for this wouldn't be a good idea.\n\nkernel.org and eclipse.org both support the smart protocol, but I\nthink this is a common trend. Sites that host a lot of Git\nrepositories take the time to setup the smart protocol. Everyone else,\nits hit-or-miss, mostly miss.\n\n> An alternative would be to simply be more verbose, and mention that\n> we tried to fallback and list both failures (or we could do this\n> with just \"fetch -v\").\n\nI would bet the big hosting providers would appreciate having the\nfailure listed in non-verbose mode. Many Git users are going to use a\nhosting service like GitHub or Google Code, or work with their\norganization like kernel.org for central Git hosting. Consumers of\nhosted projects that are less familiar with Git will be accessing\nthese sites often enough.\n\nI considered logging this failure to stderr, but didn't.\n\n> 2. Be more discerning about which errors will cause a fallback.\n> Something like \"504 Gateway Timeout\" should not give a fallback.\n> The problem is that you are really guessing at what kinds of http\n> errors you are going to get from a dumb server when you try the\n> smart URL.\n>\n> I dug back into the list thread that spawned the \"retry\n> without query string\" patch (703e6e7).\n>\n> The thread is here:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/137609\n>\n> If you read the thread, it turns out that the problem in this case\n> (which is the only reported case I could find in the archive) is\n> that the server was misconfigured to treat _anything_ with a query\n> string as a gitweb URL. And then it got fixed pretty much\n> immediately.\n>\n> So as far as we know, there may be zero servers for which this\n> fallback is actually doing anything useful.\n>\n> I'm tempted to just reverse the logic.\n\nAfter re-reading that thread, it was a mistake to apply 703e6e76. We\nshould just revert it.\n\n> Try the request with the query\n> string and immediately fail if it doesn't work. For the few (if any)\n> people who are hitting a server that will not serve the dumb file in\n> that case, add a \"remote.*.dumbhttp\" setting that will turn off smart\n> completely as a workaround.\n>\n> That would serve the (presumed) majority who are using smart http,\n> everyone using dumb http on a reasonably-configured server, and still\n> allow an easy workaround for people with badly configured servers.\n>\n> What do you think?\n\nYes, this is the better idea. Revert 703e6e76 and add a new feature to\ndisable the smart protocol entirely as an escape hatch.\n"},{"id":"199572","messageId":"1348120680-24788-1-git-send-email-spearce@spearce.org","threadId":"31615","inReplyTo":"CAJo=hJvXtSBO3QEzhZCFfhk9OF_e0B10k8tjCUWMHZvGKt599Q@mail.gmail.com","subject":"[PATCH] Revert \"retry request without query when info/refs?query fails\"","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-20T05:58:00Z","receivedAt":"2012-09-20T05:58:00Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nThis reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.\n\nRetrying without the query parameter was added as a workaround\nfor a single broken HTTP server at git.debian.org[1]. The server\nwas misconfigured to route every request with a query parameter\ninto gitweb.cgi. Admins fixed the server's configuration within\n16 hours of the bug report to the Git mailing list, but we still\npatched Git with this fallback and have been paying for it since.\n\nMost Git hosting services configure the smart HTTP protocol and the\nretry logic confuses users when there is a transient HTTP error as\nGit dropped the real error from the smart HTTP request. Removing the\nretry makes root causes easier to identify.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/137609\nSigned-off-by: Shawn O. Pearce \n---\n remote-curl.c | 18 ++----------------\n 1 file changed, 2 insertions(+), 16 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 3ec474f..2359f59 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -95,7 +95,7 @@ static struct discovery* discover_refs(const char *service)\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n \tchar *refs_url;\n-\tint http_ret, is_http = 0, proto_git_candidate = 1;\n+\tint http_ret, is_http = 0;\n \n \tif (last && !strcmp(service, last->service))\n \t\treturn last;\n@@ -113,19 +113,6 @@ static struct discovery* discover_refs(const char *service)\n \trefs_url = strbuf_detach(&buffer, NULL);\n \n \thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n-\n-\t/* try again with \"plain\" url (no ? or & appended) */\n-\tif (http_ret != HTTP_OK && http_ret != HTTP_NOAUTH) {\n-\t\tfree(refs_url);\n-\t\tstrbuf_reset(&buffer);\n-\n-\t\tproto_git_candidate = 0;\n-\t\tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n-\t\trefs_url = strbuf_detach(&buffer, NULL);\n-\n-\t\thttp_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE);\n-\t}\n-\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n@@ -144,8 +131,7 @@ static struct discovery* discover_refs(const char *service)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tif (is_http && proto_git_candidate\n-\t\t&& 5 <= last->len && last->buf[4] == '#') {\n+\tif (is_http && 5 <= last->len && last->buf[4] == '#') {\n \t\t/* smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-- \n1.7.12.1.512.g9b230e6\n"},{"id":"199573","messageId":"7vlig5cilt.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"1348120680-24788-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Revert \"retry request without query when info/refs?query fails\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T06:29:34Z","receivedAt":"2012-09-20T06:29:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.\n>\n> Retrying without the query parameter was added as a workaround\n> for a single broken HTTP server at git.debian.org[1]. The server\n> was misconfigured to route every request with a query parameter\n> into gitweb.cgi. Admins fixed the server's configuration within\n> 16 hours of the bug report to the Git mailing list, but we still\n> patched Git with this fallback and have been paying for it since.\n\nAs the consequence of the above, the only two things we know about\nthe servers in the wild are (1) a misconfiguration that requires\nthis retry was once made, so it is not very unlikely others did the\nsame misconfiguration, and (2) those unknown number of servers have\nbeen happily serving the current clients because the workaround\npatch have been hiding the misconfiguration ever since.\n\nBut as long as the failure diagnosis from updated clients that\nrevert this workaround is sufficient to allow such misconfigured\nservers, I think it is OK. We might see a large number of small\npeople having to run around and fix the configuration as a fallout,\nthough.\n\n> Most Git hosting services configure the smart HTTP protocol and the\n> retry logic confuses users when there is a transient HTTP error as\n> Git dropped the real error from the smart HTTP request. Removing the\n> retry makes root causes easier to identify.\n\nDoes that hold true also for dumb only small people installations?\nThey are the ones that need more help than the large installations\nstaffed sufficiently and run smart http gateway.\n"},{"id":"199574","messageId":"7vhaqtciij.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"7vlig5cilt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"retry request without query when info/refs?query fails\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T06:31:32Z","receivedAt":"2012-09-20T06:31:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Shawn O. Pearce\" writes:\n>\n>> This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.\n>>\n>> Retrying without the query parameter was added as a workaround\n>> for a single broken HTTP server at git.debian.org[1]. The server\n>> was misconfigured to route every request with a query parameter\n>> into gitweb.cgi. Admins fixed the server's configuration within\n>> 16 hours of the bug report to the Git mailing list, but we still\n>> patched Git with this fallback and have been paying for it since.\n>\n> As the consequence of the above, the only two things we know about\n> the servers in the wild are (1) a misconfiguration that requires\n> this retry was once made, so it is not very unlikely others did the\n> same misconfiguration, and (2) those unknown number of servers have\n> been happily serving the current clients because the workaround\n> patch have been hiding the misconfiguration ever since.\n>\n> But as long as the failure diagnosis from updated clients that\n> revert this workaround is sufficient to allow such misconfigured\n> servers, I think it is OK. We might see a large number of small\n\ns/servers,/servers diagnosed,/;\n\n> people having to run around and fix the configuration as a fallout,\n> though.\n>\n>> Most Git hosting services configure the smart HTTP protocol and the\n>> retry logic confuses users when there is a transient HTTP error as\n>> Git dropped the real error from the smart HTTP request. Removing the\n>> retry makes root causes easier to identify.\n>\n> Does that hold true also for dumb only small people installations?\n> They are the ones that need more help than the large installations\n> staffed sufficiently and run smart http gateway.\n\nIn any case, will queue.\n"},{"id":"199586","messageId":"20120920162456.GA25418@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7vlig5cilt.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"retry request without query when info/refs?query fails\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T16:24:56Z","receivedAt":"2012-09-20T16:24:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 11:29:34PM -0700, Junio C Hamano wrote:\n\n> \"Shawn O. Pearce\" writes:\n> \n> > This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.\n> >\n> > Retrying without the query parameter was added as a workaround\n> > for a single broken HTTP server at git.debian.org[1]. The server\n> > was misconfigured to route every request with a query parameter\n> > into gitweb.cgi. Admins fixed the server's configuration within\n> > 16 hours of the bug report to the Git mailing list, but we still\n> > patched Git with this fallback and have been paying for it since.\n> \n> As the consequence of the above, the only two things we know about\n> the servers in the wild are (1) a misconfiguration that requires\n> this retry was once made, so it is not very unlikely others did the\n> same misconfiguration, and (2) those unknown number of servers have\n> been happily serving the current clients because the workaround\n> patch have been hiding the misconfiguration ever since.\n\nThe misconfiguration was pretty wild in this case. I'd be much more\nworried about stupidly non-compliant servers that will not serve\n\"foo/bar\" when asked for \"foo/bar?key=value\".\n\n> But as long as the failure diagnosis from updated clients that\n> revert this workaround is sufficient to allow such misconfigured\n> servers, I think it is OK. We might see a large number of small\n> people having to run around and fix the configuration as a fallout,\n> though.\n\nI think Shawn's revert is the right thing to do. But it is not complete\nwithout the manual workaround. I'm putting that patch together now and\nshould have it out in a few minutes.\n\n> > Most Git hosting services configure the smart HTTP protocol and the\n> > retry logic confuses users when there is a transient HTTP error as\n> > Git dropped the real error from the smart HTTP request. Removing the\n> > retry makes root causes easier to identify.\n> \n> Does that hold true also for dumb only small people installations?\n> They are the ones that need more help than the large installations\n> staffed sufficiently and run smart http gateway.\n\nFor the most part, yes. They will get a useful error out of the smart\nrequest if there is a transient error, the repo does not exist, etc.\nThe real fallout is the people who are hitting a broken or misconfigured\nserver and may get a confusing error code (in the one case we know\nabout, it was a 404, but it really could be anything, depending on the\nexact nature of the misconfiguration).\n\n-Peff\n"},{"id":"199592","messageId":"20120920165938.GB18655@sigill.intra.peff.net","threadId":"31615","inReplyTo":"20120920162456.GA25418@sigill.intra.peff.net","subject":"[PATCH 0/2] smart http toggle switch fails\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T16:59:38Z","receivedAt":"2012-09-20T16:59:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 12:24:56PM -0400, Jeff King wrote:\n\n> I think Shawn's revert is the right thing to do. But it is not complete\n> without the manual workaround. I'm putting that patch together now and\n> should have it out in a few minutes.\n\nAnd here it is. This goes on top of Shawn's revert patch (it might be\nnice to mention the commit id of that in the commit message of the\nsecond patch. I couldn't do so because it is not yet in your repo).\n\n [1/2]: remote-curl: rename is_http variable\n [2/2]: remote-curl: let users turn off smart http\n\n-Peff\n"},{"id":"199593","messageId":"20120920170022.GA18981@sigill.intra.peff.net","threadId":"31615","inReplyTo":"20120920165938.GB18655@sigill.intra.peff.net","subject":"[PATCH 1/2] remote-curl: rename is_http variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T17:00:22Z","receivedAt":"2012-09-20T17:00:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually care whether the connection is http or\nnot; what we care about is whether it might be smart http.\nRename the variable to be more accurate, which will make it\neasier to later make smart-http optional.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 2359f59..c0b98cc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -95,7 +95,7 @@ static struct discovery* discover_refs(const char *service)\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct discovery *last = last_discovery;\n \tchar *refs_url;\n-\tint http_ret, is_http = 0;\n+\tint http_ret, maybe_smart = 0;\n \n \tif (last && !strcmp(service, last->service))\n \t\treturn last;\n@@ -103,7 +103,7 @@ static struct discovery* discover_refs(const char *service)\n \n \tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n \tif (!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) {\n-\t\tis_http = 1;\n+\t\tmaybe_smart = 1;\n \t\tif (!strchr(url, '?'))\n \t\t\tstrbuf_addch(&buffer, '?');\n \t\telse\n@@ -131,7 +131,7 @@ static struct discovery* discover_refs(const char *service)\n \tlast->buf_alloc = strbuf_detach(&buffer, &last->len);\n \tlast->buf = last->buf_alloc;\n \n-\tif (is_http && 5 <= last->len && last->buf[4] == '#') {\n+\tif (maybe_smart && 5 <= last->len && last->buf[4] == '#') {\n \t\t/* smart HTTP response; validate that the service\n \t\t * pkt-line matches our request.\n \t\t */\n-- \n1.7.11.7.15.g085c6bd\n"},{"id":"199594","messageId":"20120920170517.GB18981@sigill.intra.peff.net","threadId":"31615","inReplyTo":"20120920165938.GB18655@sigill.intra.peff.net","subject":"[PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T17:05:17Z","receivedAt":"2012-09-20T17:05:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Usually there is no need for users to specify whether an\nhttp remote is smart or dumb; the protocol is designed so\nthat a single initial request is made, and the client can\ndetermine the server's capability from the response.\n\nHowever, some misconfigured dumb-only servers may not like\nthe initial request by a smart client, as it contains a\nquery string. Until recently, commit 703e6e7 worked around\nthis by making a second request. However, that commit was\nrecently reverted due to its side effect of masking the\ninitial request's error code.\n\nThis patch takes a different approach to the workaround. We\nassume that the common case is that the server is either\nsmart-http or a reasonably configured dumb-http. If that is\nnot the case, we provide both a per-remote config option and\nan environment variable with which the user can manually\nwork around the issue.\n\nSigned-off-by: Jeff King \n---\nI added the config item as remote.foo.smarthttp. You could also allow\n\"http.$url.smart\" (and just \"http.smart\", for that matter), which could\nbe more flexible if you have multiple remotes pointing to the same\nbroken server. However, it is also more complex to use, and is a lot\nmore code. Since we don't know if any such servers even exist, I tried\nto give the minimal escape hatch, and we can easily build more features\non it later if people complain.\n\n Documentation/config.txt | 11 +++++++++++\n remote-curl.c | 3 ++-\n remote.c | 3 +++\n remote.h | 1 +\n t/t5551-http-fetch.sh | 17 +++++++++++++++++\n 5 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6416cae..651b23c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1871,6 +1871,17 @@ remote..uploadpack::\n \tThe default program to execute on the remote side when fetching. See\n \toption \\--upload-pack of linkgit:git-fetch-pack[1].\n \n+remote..smartHttp::\n+\tIf true, this remote will attempt to use git's smart http\n+\tprotocol when making remote http requests. Normally git sends an\n+\tinitial smart-http request, and falls back to the older \"dumb\"\n+\tprotocol if the server does not claim to support the smart\n+\tprotocol. However, some misconfigured dumb-only servers may\n+\tproduce confusing results for the initial request. Setting this\n+\toption to false disables the initial smart request, which can\n+\tworkaround problems with such servers. You should not generally\n+\tneed to set this. Defaults to `true`.\n+\n remote..tagopt::\n \tSetting this value to \\--no-tags disables automatic tag following when\n \tfetching from remote . Setting it to \\--tags will fetch every\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c0b98cc..8829bfb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)\n \tfree_discovery(last);\n \n \tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n-\tif (!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) {\n+\tif ((!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) &&\n+\t git_env_bool(\"GIT_SMART_HTTP\", remote->smart_http)) {\n \t\tmaybe_smart = 1;\n \t\tif (!strchr(url, '?'))\n \t\t\tstrbuf_addch(&buffer, '?');\ndiff --git a/remote.c b/remote.c\nindex 04fd9ea..a334390 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -152,6 +152,7 @@ static struct remote *make_remote(const char *name, int len)\n \t\tret->name = xstrndup(name, len);\n \telse\n \t\tret->name = xstrdup(name);\n+\tret->smart_http = 1;\n \treturn ret;\n }\n \n@@ -453,6 +454,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \".vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n+\t} else if (!strcmp(subkey, \".smarthttp\")) {\n+\t\tremote->smart_http = git_config_bool(key, value);\n \t}\n \treturn 0;\n }\ndiff --git a/remote.h b/remote.h\nindex 251d8fd..9031d18 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -40,6 +40,7 @@ struct remote {\n \tint fetch_tags;\n \tint skip_default_update;\n \tint mirror;\n+\tint smart_http;\n \n \tconst char *receivepack;\n \tconst char *uploadpack;\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex 2db5c35..48173ed 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -129,6 +129,23 @@ test_expect_success 'clone from auth-only-for-push repository' '\n \ttest_cmp expect actual\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+'\n+\n+test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n+\t(GIT_SMART_HTTP=0 &&\n+\t export GIT_SMART_HTTP &&\n+\t cd clone &&\n+\t test_must_fail git fetch)\n+'\n+\n+test_expect_success 'remote.*.smartHTTP can disable smart http' '\n+\t(cd clone &&\n+\t test_must_fail git -c remote.origin.smartHTTP=false fetch)\n+'\n+\n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n \n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n-- \n1.7.11.7.15.g085c6bd\n"},{"id":"199595","messageId":"20120920172408.GC18655@sigill.intra.peff.net","threadId":"31615","inReplyTo":"CAJo=hJvXtSBO3QEzhZCFfhk9OF_e0B10k8tjCUWMHZvGKt599Q@mail.gmail.com","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T17:24:08Z","receivedAt":"2012-09-20T17:24:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote:\n\n> > I have been looking into this recently, as well. GitHub does not allow\n> > dumb http at all these days,\n> \n> Interesting that GitHub doesn't support dumb transfer either.\n\nOur objects are still in regular repos, and is served by fairly stock\ngit. The main show-stopper is that we share objects via alternates, and\nwe aggressively repack the alternates repos. So a dumb client would end\nup being quite inefficient.\n\nWe also toyed with having mixed public/private fork networks at one\npoint, which would obviously necessitate disabling dumb access. But we\ngave it up as too risky, as you can do all sorts of weird tricks to\nconvince git to disclose information about what's in the private repos\n(e.g., you can reliably find out which sha1s exist, and you can lie\nabout which sha1s you have to ask git to create deltas against them).\n\nDoing a shared-object store right would require marking which repo\n\"knows\" which objects (I assume that's what Google Code does, but I\ndon't remember if Dave talked about that aspect at last year's\nGitTogether).\n\n> > so transient errors on the initial smart\n> > contact can cause us to fall back to dumb,\n> \n> Transient errors is actually what is leading me down this path. We see\n> about 0.0455% of our requests to the Android hosting service as these\n> dumb fallbacks. This means the client never got a proper smart service\n> reply. Server logs suggest we sent a valid response, so I am\n> suspecting transient proxy errors, but its hard to debug because\n> clients discard the error.\n\nYup, we see the same thing. I've tracked a few down manually to actual\nthings like gateway timeouts on our reverse proxy.\n\n> > and end up reporting a\n> > totally useless 403 Forbidden error.\n> \n> Today I posted a change to JGit [1] to make this 406 Not Acceptable as\n> I think it better matches the description of the failure. It also no\n> longer reuses the common 403 Forbidden error that a server might\n> choose to return if a request was given with invalid credentials.\n\nThat might be worth doing for git-http-backend, too. It might even make\nsense for the git client to recognize the 406 (and possibly the 403) and\nprint a more useful message.\n\n> > 1. If both smart and dumb requests fail, report the error for the\n> > smart request. Now that smart-http clients are common, I'd expect\n> > most http servers to be smart these days. Of course I don't have\n> > any sort of numbers to back this up (nor am I sure how to get them;\n> > obviously big sites like GitHub and Google Code do a lot of\n> > traffic, but who knows how many one-off repo-on-a-generic-web-host\n> > sites still exist?).\n> \n> I suspect there are still a number of servers that rely on dumb\n> protocol to host repositories because its very simple to setup.\n> Breaking support for this wouldn't be a good idea.\n\nI don't think it would break on most servers, though. Even for a dumb\nserver, the initial error will be a useful one. It's only the\nweirdly-configured ones where you get wildly different results depending\non whether the query string is there.\n\nIn other words, it is really no worse than reverting 703e6e76, and it\nmight be better. In the common case, you get a better error message. In\nthe broken-server case, we still try the fallback. So it will keep\nworking on a broken server without any manual intervention, whereas\nreverting and adding a manual escape hatch means the user has to do\nsomething.\n\nBUT.\n\nI still think it's better to revert 703e6e76, because I really do think\nthe broken-server case is an extreme enough minority that it is not even\nworth wasting the time of clients to make a second request (especially\nbecause the first request may very well have failed because of a network\nerror that causes a long timeout, and the user then has to wait double\nto find out what the error is).\n\nOf course, I have no actual data aside from reading the original thread\nthat led to 703e6e76, and the fact that nobody else mentioned it (not\nduring the time when it was broken, and not even after, when people\noften still complain because they haven't upgraded yet). But who knows?\nMaybe I will eat my words, and we will end up getting that data in the\nform of complaints. :)\n\nWe can always switch to fallback-but-prefer-the-initial-error then. And\nwe'll have more data on exactly how the misconfigured servers behave.\n\n-Peff\n"},{"id":"199598","messageId":"7va9wkbmyc.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"20120920170517.GB18981@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T17:53:15Z","receivedAt":"2012-09-20T17:53:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I added the config item as remote.foo.smarthttp. You could also allow\n> \"http.$url.smart\" (and just \"http.smart\", for that matter), which could\n> be more flexible if you have multiple remotes pointing to the same\n> broken server.\n\nWhat would the user experience be when we introduce \"even smarter\"\nhttp server protocol extension? Will we add remote.foo.starterhttp?\n\nPerhaps\n\n remote.$name.httpvariants = [smart] [dumb]\n\nto allow users to say \"smart only\", \"dumb only\", or \"smart and/or\ndumb\" might be more code but less burden on the users.\n\nThe code obviously looks correct, and the documentation reads fine.\n"},{"id":"199601","messageId":"20120920181231.GA19204@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7va9wkbmyc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T18:12:31Z","receivedAt":"2012-09-20T18:12:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 10:53:15AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I added the config item as remote.foo.smarthttp. You could also allow\n> > \"http.$url.smart\" (and just \"http.smart\", for that matter), which could\n> > be more flexible if you have multiple remotes pointing to the same\n> > broken server.\n> \n> What would the user experience be when we introduce \"even smarter\"\n> http server protocol extension? Will we add remote.foo.starterhttp?\n\nI would hope that it would actually be negotiated reliably at the\nprotocol level so we do not have to deal with this mess again.\n\n> Perhaps\n> \n> remote.$name.httpvariants = [smart] [dumb]\n> \n> to allow users to say \"smart only\", \"dumb only\", or \"smart and/or\n> dumb\" might be more code but less burden on the users.\n\nI don't mind that format if we are going that direction, but is there\nanybody who actually wants to say \"smart only?\"\n\n-Peff\n"},{"id":"199604","messageId":"7vzk4ka6dp.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"20120920181231.GA19204@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T18:36:34Z","receivedAt":"2012-09-20T18:36:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 20, 2012 at 10:53:15AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > I added the config item as remote.foo.smarthttp. You could also allow\n>> > \"http.$url.smart\" (and just \"http.smart\", for that matter), which could\n>> > be more flexible if you have multiple remotes pointing to the same\n>> > broken server.\n>> \n>> What would the user experience be when we introduce \"even smarter\"\n>> http server protocol extension? Will we add remote.foo.starterhttp?\n>\n> I would hope that it would actually be negotiated reliably at the\n> protocol level so we do not have to deal with this mess again.\n\nThe original dumb vs smart was supposed to be \"negotiated reliably\nat the protocol level\", no? Yet we need this band-aid, so...\n\n>> Perhaps\n>> \n>> remote.$name.httpvariants = [smart] [dumb]\n>> \n>> to allow users to say \"smart only\", \"dumb only\", or \"smart and/or\n>> dumb\" might be more code but less burden on the users.\n>\n> I don't mind that format if we are going that direction, but is there\n> anybody who actually wants to say \"smart only?\"\n\nWith 703e6e7 reverted, we take a failure from the initial smart\nrequest to mean the server is simply not serving, so \"smart only\" to\nfail quickly without trying dumb fallback is not needed. \"smart\nonly\" to say \"I wouldn't want to talk to dumb-only server---I do not\nhave infinite amount of time, and I'd rather try another server\" is\nstill a possibility, but likely not worth supporting.\n"},{"id":"199625","messageId":"20120920205107.GB22284@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7vzk4ka6dp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T20:51:07Z","receivedAt":"2012-09-20T20:51:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 11:36:34AM -0700, Junio C Hamano wrote:\n\n> >> What would the user experience be when we introduce \"even smarter\"\n> >> http server protocol extension? Will we add remote.foo.starterhttp?\n> >\n> > I would hope that it would actually be negotiated reliably at the\n> > protocol level so we do not have to deal with this mess again.\n> \n> The original dumb vs smart was supposed to be \"negotiated reliably\n> at the protocol level\", no? Yet we need this band-aid, so...\n\nI started to write much more in my original response, but deleted it as\nbeing too wordy. I guess I will have to rewrite it now. :)\n\nThe difference is that jumping from dumb to smart had to give context\nclues at the HTTP layer. That is, by sending a query string, the client\nsends a single bit that tells the server \"I understand smart http\", and\nthe server responds with output that indicates it also understands. We\nhad to embed this in the HTTP layer, because the previous iteration\nwasn't running any custom git code at all.\n\nWhereas if we were to enhance the protocol again, it would probably\n_still_ begin with the same type of initial query, but we would\nnegotiate more at the git-protocol level. And there we are in charge of\nhow the implementation responds and handles backwards compatibility.\n\nThis has already happened to some degree. We have added new capabilities\nat the git-protocol level, and it worked without these problems. It's\nnot a \"new protocol\", but it is a backwards-compatible enhancement. And\nit's the likely mode for new enhancements in the future.\n\nIt's possible we could have something drastically different in the\nfuture that does not even start with the same initial git conversation.\nBut even then, I think we'd do it with a new \"git-upload-pack2\" service\ntag, or git:// and ssh access would be left behind.\n\n> >> Perhaps\n> >> \n> >> remote.$name.httpvariants = [smart] [dumb]\n> >> \n> >> to allow users to say \"smart only\", \"dumb only\", or \"smart and/or\n> >> dumb\" might be more code but less burden on the users.\n> >\n> > I don't mind that format if we are going that direction, but is there\n> > anybody who actually wants to say \"smart only?\"\n> \n> With 703e6e7 reverted, we take a failure from the initial smart\n> request to mean the server is simply not serving, so \"smart only\" to\n> fail quickly without trying dumb fallback is not needed. \"smart\n> only\" to say \"I wouldn't want to talk to dumb-only server---I do not\n> have infinite amount of time, and I'd rather try another server\" is\n> still a possibility, but likely not worth supporting.\n\nYes. I do still need to resurrect my fetch-a-bundle-by-http code, which\ncould also be covered by such a switch. But I guess I am just not sure\nif there is any point in spending effort to implement toggles that\nnobody has actually asked for.\n\nI'm also a little iffy on it because we would be inventing new config\nsyntax. I don't think we want to split the list across multiple config\nitems (which makes our usual later-config-overwrites-earlier rules\nbehave badly). So what is the value format? Is it a whitespace-delimited\ncase-insensitive list completely specifying the transports allowed? What\nhappens if a new value is added. Do people who have said \"smart\" not get\nthe new value, even though all they really wanted to say was \"not dumb\"?\nWhat about people who write \"bundle smart\" because their new\nversion of git understands it, but then have old versions of git barf on\nit?\n\nMost of our current config is very toggle-oriented, and I'm not sure\nthere is precedent for an option exactly like this. We can try to come\nup with answers to those questions, but I don't think doing it is as\nsimple as just changing a few lines of code to support !dumb and !smart\nmodes.\n\nI'm half-tempted to just drop the config entirely, leave\nGIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.\nAt least then we're not promising support for a config option that we\nmay want to change later.\n\nWhat do you want to do?\n\n-Peff\n"},{"id":"199628","messageId":"7vd31g9z13.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"20120920205107.GB22284@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-20T21:15:20Z","receivedAt":"2012-09-20T21:15:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm half-tempted to just drop the config entirely, leave\n> GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.\n\nSounds like a very attractive minimalistic way to go forward. We\ncan always add per-remote configuration when we find it necessary,\nbut once we add support, we cannot easily yank it out.\n"},{"id":"199631","messageId":"20120920213058.GA23904@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7vd31g9z13.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-20T21:30:58Z","receivedAt":"2012-09-20T21:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 02:15:20PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm half-tempted to just drop the config entirely, leave\n> > GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.\n> \n> Sounds like a very attractive minimalistic way to go forward. We\n> can always add per-remote configuration when we find it necessary,\n> but once we add support, we cannot easily yank it out.\n\nLike this?\n\n-- >8 --\nSubject: [PATCH] remote-curl: let users turn off smart http\n\nUsually there is no need for users to specify whether an\nhttp remote is smart or dumb; the protocol is designed so\nthat a single initial request is made, and the client can\ndetermine the server's capability from the response.\n\nHowever, some misconfigured dumb-only servers may not like\nthe initial request by a smart client, as it contains a\nquery string. Until recently, commit 703e6e7 worked around\nthis by making a second request. However, that commit was\nrecently reverted due to its side effect of masking the\ninitial request's error code.\n\nSince git has had that workaround for several years, we\ndon't know exactly how many such misconfigured servers are\nout there. The reversion of 703e6e7 assumes they are rare\nenough not to worry about. Still, that reversion leaves\nsomebody who does run into such a server with no escape\nhatch at all. Let's give them an environment variable they\ncan tweak to perform the \"dumb\" request.\n\nThis is intentionally not a documented interface. It's\noverly simple and is really there for debugging in case\nsomebody does complain about git not working with their\nserver. A real user-facing interface would entail a\nper-remote or per-URL config variable.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 3 ++-\n t/t5551-http-fetch.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex c0b98cc..7b19ebb 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)\n \tfree_discovery(last);\n \n \tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n-\tif (!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) {\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, '?');\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex 2db5c35..8427943 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -129,6 +129,18 @@ test_expect_success 'clone from auth-only-for-push repository' '\n \ttest_cmp expect actual\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+'\n+\n+test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n+\t(GIT_SMART_HTTP=0 &&\n+\t export GIT_SMART_HTTP &&\n+\t cd clone &&\n+\t test_must_fail git fetch)\n+'\n+\n test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n \n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n-- \n1.7.11.7.15.g085c6bd\n"},{"id":"199635","messageId":"CAJo=hJvyEtTDVJ6+Gv1kgqs1=UQEVbLaSFMEmUmCX-JWRCrDxA@mail.gmail.com","threadId":"31615","inReplyTo":"20120920172408.GC18655@sigill.intra.peff.net","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-20T23:05:03Z","receivedAt":"2012-09-20T23:05:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Sep 20, 2012 at 10:24 AM, Jeff King wrote:\n> On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote:\n>\n>> > so transient errors on the initial smart\n>> > contact can cause us to fall back to dumb,\n>>\n>> Transient errors is actually what is leading me down this path. We see\n>> about 0.0455% of our requests to the Android hosting service as these\n>> dumb fallbacks. This means the client never got a proper smart service\n>> reply. Server logs suggest we sent a valid response, so I am\n>> suspecting transient proxy errors, but its hard to debug because\n>> clients discard the error.\n>\n> Yup, we see the same thing. I've tracked a few down manually to actual\n> things like gateway timeouts on our reverse proxy.\n\nOur reverse proxies also fail sometimes. :-)\n\nBut right now I am seeing failures in libcurl's SSL connection that\nmay also be causing the smart connection failures. For example this\ntrace, where libcurl was just not able to connect to respond to the\n401 with a password. I suspect what is happening is the SSL session\ndropped out of cache on our servers, and libcurl couldn't reuse the\nexisting SSL session. Instead of discarding the bad session and\nretrying, Git aborts. I'm willing to bet modern browsers just discard\nthe bad session and start a new one, because clients can't assume the\nremote server will be able to remember their session forever.\n\n\n* Couldn't find host android.googlesource.com in the .netrc file; using defaults\n* About to connect() to android.googlesource.com port 443 (#0)\n* Trying 2607:f8b0:400e:c02::52... * Connected to\nandroid.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0)\n* successfully set certificate verify locations:\n* CAfile: none\n CApath: /etc/ssl/certs\n* SSL connection using RC4-SHA\n* Server certificate:\n* subject: C=US; ST=California; L=Mountain View; O=Google Inc;\nCN=*.googlecode.com\n* start date: 2012-08-16 12:25:39 GMT\n* expire date: 2013-06-07 19:43:27 GMT\n* subjectAltName: android.googlesource.com matched\n* issuer: C=US; O=Google Inc; CN=Google Internet Authority\n* SSL certificate verify ok.\n> GET /a/platform/tools/build/info/refs?service=git-upload-pack HTTP/1.1\nUser-Agent: git/1.7.12.1.1.g9b7ccb3\nHost: android.googlesource.com\nAccept: */*\nPragma: no-cache\n\n* The requested URL returned error: 401\n* Closing connection #0\n* Couldn't find host android.googlesource.com in the .netrc file; using defaults\n* About to connect() to android.googlesource.com port 443 (#0)\n* Trying 2607:f8b0:400e:c02::52... * Connected to\nandroid.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0)\n* successfully set certificate verify locations:\n* CAfile: none\n CApath: /etc/ssl/certs\n* SSL re-using session ID\n* Unknown SSL protocol error in connection to android.googlesource.com:443\n* Expire cleared\n* Closing connection #0\nerror: Unknown SSL protocol error in connection to\nandroid.googlesource.com:443 while accessing\nhttps://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack\nfatal: HTTP request failed\n"},{"id":"199639","messageId":"20120921052606.GA9659@sigill.intra.peff.net","threadId":"31615","inReplyTo":"CAJo=hJvyEtTDVJ6+Gv1kgqs1=UQEVbLaSFMEmUmCX-JWRCrDxA@mail.gmail.com","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T05:26:06Z","receivedAt":"2012-09-21T05:26:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 20, 2012 at 04:05:03PM -0700, Shawn O. Pearce wrote:\n\n> But right now I am seeing failures in libcurl's SSL connection that\n> may also be causing the smart connection failures. For example this\n> trace, where libcurl was just not able to connect to respond to the\n> 401 with a password. I suspect what is happening is the SSL session\n> dropped out of cache on our servers, and libcurl couldn't reuse the\n> existing SSL session. Instead of discarding the bad session and\n> retrying, Git aborts. I'm willing to bet modern browsers just discard\n> the bad session and start a new one, because clients can't assume the\n> remote server will be able to remember their session forever.\n\nThat's something I haven't seen. But then, I don't usually see the\nclient side; I just see the fallback dumb fetch in our logs, and\nhave occasionally followed up.\n\nIs there a long pause while the user is typing their password?\n\n> * SSL re-using session ID\n> * Unknown SSL protocol error in connection to android.googlesource.com:443\n> * Expire cleared\n> * Closing connection #0\n> error: Unknown SSL protocol error in connection to\n> android.googlesource.com:443 while accessing\n> https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack\n> fatal: HTTP request failed\n\nYou could try turning off CURLOPT_SSL_SESSIONID_CACHE and seeing if that\nimproves it. Of course, it is probably hard to reproduce, so it would be\ntough to know if that helped or not. It would also be nice if you could\ndump more information on the error from the ssl library (I typically\nbuild curl against openssl; I wonder if it could be related to using\ngnutls or something).\n\n-Peff\n"},{"id":"199658","messageId":"CAJo=hJs=Zm4BPm94-sNWDUNkg2vAReSsTmKnDVw+xOU9NWcfUQ@mail.gmail.com","threadId":"31615","inReplyTo":"20120921052606.GA9659@sigill.intra.peff.net","subject":"Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-09-21T14:19:50Z","receivedAt":"2012-09-21T14:19:50Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Thu, Sep 20, 2012 at 10:26 PM, Jeff King wrote:\n> On Thu, Sep 20, 2012 at 04:05:03PM -0700, Shawn O. Pearce wrote:\n>\n>> But right now I am seeing failures in libcurl's SSL connection that\n>> may also be causing the smart connection failures. For example this\n>> trace, where libcurl was just not able to connect to respond to the\n>> 401 with a password. I suspect what is happening is the SSL session\n>> dropped out of cache on our servers, and libcurl couldn't reuse the\n>> existing SSL session. Instead of discarding the bad session and\n>> retrying, Git aborts. I'm willing to bet modern browsers just discard\n>> the bad session and start a new one, because clients can't assume the\n>> remote server will be able to remember their session forever.\n>\n> That's something I haven't seen. But then, I don't usually see the\n> client side; I just see the fallback dumb fetch in our logs, and\n> have occasionally followed up.\n\nI hadn't seen this either until I deleted the fallback code from\nremote-curl.c and ran git ls-remote in a while true loop for 6 hours.\nIts obviously happening though.\n\n> Is there a long pause while the user is typing their password?\n\nNo. The password comes off a credential helper that has access to it\nfrom a credential store. There is very little lag here, under 100 ms.\n\n>> * SSL re-using session ID\n>> * Unknown SSL protocol error in connection to android.googlesource.com:443\n>> * Expire cleared\n>> * Closing connection #0\n>> error: Unknown SSL protocol error in connection to\n>> android.googlesource.com:443 while accessing\n>> https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack\n>> fatal: HTTP request failed\n>\n> You could try turning off CURLOPT_SSL_SESSIONID_CACHE and seeing if that\n> improves it. Of course, it is probably hard to reproduce, so it would be\n> tough to know if that helped or not. It would also be nice if you could\n> dump more information on the error from the ssl library (I typically\n> build curl against openssl; I wonder if it could be related to using\n> gnutls or something).\n\nThis is OpenSSL, because I also always build against OpenSSL. :-)\n\nI'll try the CURLOPT_SSL_SESSIONID_CACHE today. It is hard to\nreproduce, so not producing it doesn't necessarily mean it isn't still\nthere.\n"},{"id":"199676","messageId":"7vmx0j700x.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"20120920213058.GA23904@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-21T17:34:22Z","receivedAt":"2012-09-21T17:34:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Sep 20, 2012 at 02:15:20PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > I'm half-tempted to just drop the config entirely, leave\n>> > GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.\n>> \n>> Sounds like a very attractive minimalistic way to go forward. We\n>> can always add per-remote configuration when we find it necessary,\n>> but once we add support, we cannot easily yank it out.\n>\n> Like this?\n\nYeah. Will queue this one instead. The simpler, the better ;-)\n\n>\n> -- >8 --\n> Subject: [PATCH] remote-curl: let users turn off smart http\n>\n> Usually there is no need for users to specify whether an\n> http remote is smart or dumb; the protocol is designed so\n> that a single initial request is made, and the client can\n> determine the server's capability from the response.\n>\n> However, some misconfigured dumb-only servers may not like\n> the initial request by a smart client, as it contains a\n> query string. Until recently, commit 703e6e7 worked around\n> this by making a second request. However, that commit was\n> recently reverted due to its side effect of masking the\n> initial request's error code.\n>\n> Since git has had that workaround for several years, we\n> don't know exactly how many such misconfigured servers are\n> out there. The reversion of 703e6e7 assumes they are rare\n> enough not to worry about. Still, that reversion leaves\n> somebody who does run into such a server with no escape\n> hatch at all. Let's give them an environment variable they\n> can tweak to perform the \"dumb\" request.\n>\n> This is intentionally not a documented interface. It's\n> overly simple and is really there for debugging in case\n> somebody does complain about git not working with their\n> server. A real user-facing interface would entail a\n> per-remote or per-URL config variable.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 3 ++-\n> t/t5551-http-fetch.sh | 12 ++++++++++++\n> 2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index c0b98cc..7b19ebb 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)\n> \tfree_discovery(last);\n> \n> \tstrbuf_addf(&buffer, \"%sinfo/refs\", url);\n> -\tif (!prefixcmp(url, \"http://\") || !prefixcmp(url, \"https://\")) {\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> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\n> index 2db5c35..8427943 100755\n> --- a/t/t5551-http-fetch.sh\n> +++ b/t/t5551-http-fetch.sh\n> @@ -129,6 +129,18 @@ test_expect_success 'clone from auth-only-for-push repository' '\n> \ttest_cmp expect actual\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> +'\n> +\n> +test_expect_success 'GIT_SMART_HTTP can disable smart http' '\n> +\t(GIT_SMART_HTTP=0 &&\n> +\t export GIT_SMART_HTTP &&\n> +\t cd clone &&\n> +\t test_must_fail git fetch)\n> +'\n> +\n> test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n> \n> test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n"},{"id":"199678","messageId":"20120921174129.GB20896@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7vmx0j700x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] remote-curl: let users turn off smart http","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-09-21T17:41:29Z","receivedAt":"2012-09-21T17:41:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 21, 2012 at 10:34:22AM -0700, Junio C Hamano wrote:\n\n> >> > I'm half-tempted to just drop the config entirely, leave\n> >> > GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.\n> >> \n> >> Sounds like a very attractive minimalistic way to go forward. We\n> >> can always add per-remote configuration when we find it necessary,\n> >> but once we add support, we cannot easily yank it out.\n> >\n> > Like this?\n> \n> Yeah. Will queue this one instead. The simpler, the better ;-)\n\nThanks. I almost followed up with a rebased version of my config patch,\nshould we want to apply it later separately. But I think I would really\nrather gather data from even a single bug report before we move any\nfurther (and with any luck, there will be zero bug reports :) ).\n\n-Peff\n"},{"id":"200295","messageId":"1349126586-755-1-git-send-email-spearce@spearce.org","threadId":"31615","inReplyTo":"CAJo=hJs=Zm4BPm94-sNWDUNkg2vAReSsTmKnDVw+xOU9NWcfUQ@mail.gmail.com","subject":"[PATCH] Retry HTTP requests on SSL connect failures","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-10-01T21:23:06Z","receivedAt":"2012-10-01T21:23:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"From: \"Shawn O. Pearce\" \n\nWhen libcurl fails to connect to an SSL server always retry the\nrequest once. Since the connection failed before the HTTP headers\ncan be sent, no data has exchanged hands, so the remote side has\nnot learned of the request and will not perform it twice.\n\nIn the wild we have seen git-remote-https fail to connect to\nsome load-balanced SSL servers sporadically, while modern popular\nbrowsers (e.g. Firefox and Chromium) have no trouble with the same\nserver pool.\n\nLets assume the site operators (Hi Google!) have a clue and are\ndoing everything they already can to ensure secure, successful\nSSL connections from a wide range of HTTP clients. Implementing a\nsingle level of retry in the client can make it more robust against\ntransient failure modes.\n---\n http.c | 19 ++++++++++++-------\n remote-curl.c | 2 ++\n 2 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 345c171..953f2e6 100644\n--- a/http.c\n+++ b/http.c\n@@ -784,7 +784,7 @@ static int http_request(const char *url, void *result, int target, int options)\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint ret;\n+\tint ret, attempts;\n \n \tslot = get_active_slot();\n \tslot->results = &results;\n@@ -820,12 +820,17 @@ static int http_request(const char *url, void *result, int target, int options)\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n \n-\tif (start_active_slot(slot)) {\n-\t\trun_active_slot(slot);\n-\t\tret = handle_curl_result(slot);\n-\t} else {\n-\t\terror(\"Unable to start HTTP request for %s\", url);\n-\t\tret = HTTP_START_FAILED;\n+\tfor (attempts = 0; attempts < 2; attempts++) {\n+\t\tif (start_active_slot(slot)) {\n+\t\t\trun_active_slot(slot);\n+\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n+\t\t\t\tcontinue;\n+\t\t\tret = handle_curl_result(slot);\n+\t\t} else {\n+\t\t\terror(\"Unable to start HTTP request for %s\", url);\n+\t\t\tret = HTTP_START_FAILED;\n+\t\t}\n+\t\tbreak;\n \t}\n \n \tcurl_slist_free_all(headers);\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a269608..04a379c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)\n \n \tslot->results = &results;\n \tslot->curl_result = curl_easy_perform(slot->curl);\n+\tif (slot->curl_result == CURLE_SSL_CONNECT_ERROR)\n+\t\tslot->curl_result = curl_easy_perform(slot->curl);\n \tfinish_active_slot(slot);\n \n \terr = handle_curl_result(slot);\n-- \n1.7.12.1.590.g4bb1bc4\n"},{"id":"200296","messageId":"7v626tc19t.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"1349126586-755-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T21:47:58Z","receivedAt":"2012-10-01T21:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Lets assume the site operators (Hi Google!) have a clue and are\n> doing everything they already can to ensure secure, successful\n> SSL connections from a wide range of HTTP clients. Implementing a\n> single level of retry in the client can make it more robust against\n> transient failure modes.\n> ---\n\nSign off?\n\n> http.c | 19 ++++++++++++-------\n> remote-curl.c | 2 ++\n> 2 files changed, 14 insertions(+), 7 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 345c171..953f2e6 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -784,7 +784,7 @@ static int http_request(const char *url, void *result, int target, int options)\n> \tstruct slot_results results;\n> \tstruct curl_slist *headers = NULL;\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tint ret;\n> +\tint ret, attempts;\n> \n> \tslot = get_active_slot();\n> \tslot->results = &results;\n> @@ -820,12 +820,17 @@ static int http_request(const char *url, void *result, int target, int options)\n> \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> \n> -\tif (start_active_slot(slot)) {\n> -\t\trun_active_slot(slot);\n> -\t\tret = handle_curl_result(slot);\n> -\t} else {\n> -\t\terror(\"Unable to start HTTP request for %s\", url);\n> -\t\tret = HTTP_START_FAILED;\n> +\tfor (attempts = 0; attempts < 2; attempts++) {\n> +\t\tif (start_active_slot(slot)) {\n> +\t\t\trun_active_slot(slot);\n> +\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\t\t\tcontinue;\n> +\t\t\tret = handle_curl_result(slot);\n> +\t\t} else {\n> +\t\t\terror(\"Unable to start HTTP request for %s\", url);\n> +\t\t\tret = HTTP_START_FAILED;\n> +\t\t}\n> +\t\tbreak;\n> \t}\n\nTwo naïve questions, that applies to this and the one in remote-curl.c::run_slot().\n\n (1) why only twice?\n (2) no need for \"wait a bit and then retry\"?\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index a269608..04a379c 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)\n> \n> \tslot->results = &results;\n> \tslot->curl_result = curl_easy_perform(slot->curl);\n> +\tif (slot->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\tslot->curl_result = curl_easy_perform(slot->curl);\n> \tfinish_active_slot(slot);\n> \n> \terr = handle_curl_result(slot);\n"},{"id":"200297","messageId":"7v1uhhc10y.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"1349126586-755-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T21:53:17Z","receivedAt":"2012-10-01T21:53:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> +\tfor (attempts = 0; attempts < 2; attempts++) {\n> +\t\tif (start_active_slot(slot)) {\n> +\t\t\trun_active_slot(slot);\n> +\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\t\t\tcontinue;\n\nIs it safe to continue and let start_active_slot() to add the same\ncurl handle again when USE_CURL_MULTI is in effect?\n\n> +\t\t\tret = handle_curl_result(slot);\n> +\t\t} else {\n> +\t\t\terror(\"Unable to start HTTP request for %s\", url);\n> +\t\t\tret = HTTP_START_FAILED;\n> +\t\t}\n> +\t\tbreak;\n> \t}\n> \n> \tcurl_slist_free_all(headers);\n> diff --git a/remote-curl.c b/remote-curl.c\n> index a269608..04a379c 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)\n> \n> \tslot->results = &results;\n> \tslot->curl_result = curl_easy_perform(slot->curl);\n> +\tif (slot->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\tslot->curl_result = curl_easy_perform(slot->curl);\n> \tfinish_active_slot(slot);\n> \n> \terr = handle_curl_result(slot);\n"},{"id":"200298","messageId":"20121001221817.GA12496@sigill.intra.peff.net","threadId":"31615","inReplyTo":"1349126586-755-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-01T22:18:17Z","receivedAt":"2012-10-01T22:18:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:\n\n> From: \"Shawn O. Pearce\" \n> \n> When libcurl fails to connect to an SSL server always retry the\n> request once. Since the connection failed before the HTTP headers\n> can be sent, no data has exchanged hands, so the remote side has\n> not learned of the request and will not perform it twice.\n> \n> In the wild we have seen git-remote-https fail to connect to\n> some load-balanced SSL servers sporadically, while modern popular\n> browsers (e.g. Firefox and Chromium) have no trouble with the same\n> server pool.\n> \n> Lets assume the site operators (Hi Google!) have a clue and are\n> doing everything they already can to ensure secure, successful\n> SSL connections from a wide range of HTTP clients. Implementing a\n> single level of retry in the client can make it more robust against\n> transient failure modes.\n\nI find this a little distasteful just because we haven't figured out the\nactual _reason_ for the failure. That is, I'm not convinced this isn't\nsomething that curl or the ssl library can't handle internally if we\nwould only configure them correctly. Did you ever follow up on tweaking\nthe session caching options for curl?\n\nHave you tried running your fails-after-a-few-hours request with other\nclients that don't have the problem and seeing what they do (I'm\nthinking a small webkit harness or something would be the most\nfeasible)?\n\nThat being said, you did make it so that it only kicks in during ssl\nconnect errors:\n\n> +\tfor (attempts = 0; attempts < 2; attempts++) {\n> +\t\tif (start_active_slot(slot)) {\n> +\t\t\trun_active_slot(slot);\n> +\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\t\t\tcontinue;\n> +\t\t\tret = handle_curl_result(slot);\n> +\t\t} else {\n> +\t\t\terror(\"Unable to start HTTP request for %s\", url);\n> +\t\t\tret = HTTP_START_FAILED;\n> +\t\t}\n> +\t\tbreak;\n\nwhich means it shouldn't really be affecting the general populace. So\neven though it feels like a dirty hack, at least it is self-contained,\nand it does fix a real-world problem. If your answer to the above\nquestions is \"hunting this further is just not worth the effort\", I can\nlive with that.\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index a269608..04a379c 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)\n> \n> \tslot->results = &results;\n> \tslot->curl_result = curl_easy_perform(slot->curl);\n> +\tif (slot->curl_result == CURLE_SSL_CONNECT_ERROR)\n> +\t\tslot->curl_result = curl_easy_perform(slot->curl);\n> \tfinish_active_slot(slot);\n\nHow come the first hunk gets a nice for-loop and this one doesn't?\n\nAlso, are these hunks the only two spots where this error can come up?\nThe first one does http_request, which handles smart-http GET requests.\nthe second does run_slot, which handles smart-http POST requests.\n\nSome of the dumb http fetches will go through http_request. But some\nwill not. And I think almost none of dumb http push will.\n\n-Peff\n"},{"id":"200299","messageId":"20121001222306.GB12496@sigill.intra.peff.net","threadId":"31615","inReplyTo":"7v1uhhc10y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-01T22:23:06Z","receivedAt":"2012-10-01T22:23:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 01, 2012 at 02:53:17PM -0700, Junio C Hamano wrote:\n\n> \"Shawn O. Pearce\" writes:\n> \n> > +\tfor (attempts = 0; attempts < 2; attempts++) {\n> > +\t\tif (start_active_slot(slot)) {\n> > +\t\t\trun_active_slot(slot);\n> > +\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n> > +\t\t\t\tcontinue;\n> \n> Is it safe to continue and let start_active_slot() to add the same\n> curl handle again when USE_CURL_MULTI is in effect?\n\nI _think_ so. We reuse the slots anyway. So the usual workflow would be\nget_active_slot, then start_active_slot, then run_active_slot. This loop\nomits get_active_slot, which is responsible for (re-)initializing a\nbunch of aspects of the slot. But we wouldn't want that here, since it\nwould mean we'd have to set up our URL, callbacks, etc, again.\n\nMy only worry would be that the failed curl request actually ended up\nwriting some data or made some other state change. But since we are\nexplicitly catching only ssl connection failures, presumably that would\nnot have happened.\n\n-Peff\n"},{"id":"200303","messageId":"7vobklaiek.fsf@alter.siamese.dyndns.org","threadId":"31615","inReplyTo":"20121001222306.GB12496@sigill.intra.peff.net","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T23:20:51Z","receivedAt":"2012-10-01T23:20:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Oct 01, 2012 at 02:53:17PM -0700, Junio C Hamano wrote:\n>\n>> \"Shawn O. Pearce\" writes:\n>> \n>> > +\tfor (attempts = 0; attempts < 2; attempts++) {\n>> > +\t\tif (start_active_slot(slot)) {\n>> > +\t\t\trun_active_slot(slot);\n>> > +\t\t\tif (slot->results->curl_result == CURLE_SSL_CONNECT_ERROR)\n>> > +\t\t\t\tcontinue;\n>> \n>> Is it safe to continue and let start_active_slot() to add the same\n>> curl handle again when USE_CURL_MULTI is in effect?\n>\n> I _think_ so. \n\nIt seems that at the beginning of curl_multi_add_handle() there is a\ncheck to see if the incoming slot->curl has already been added to\nsome curl-multi-handle and the function would return an error code\nCURLM_BAD_EASY_HANDLE without doing anything useful. Doesn't the\nsecond attempt to call start_active_slot() set the slot->in_use to\nzero and return false, skipping the call to run_active_slot() in\nthat case?\n"},{"id":"200304","messageId":"CAM9Z-nkSio-fXPAw_qaZsPhT-DHjn+AOOfZMXQYFCmeQAs+cJA@mail.gmail.com","threadId":"31615","inReplyTo":"1349126586-755-1-git-send-email-spearce@spearce.org","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-10-02T00:14:35Z","receivedAt":"2012-10-02T00:14:35Z","isPatch":true,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"On Mon, Oct 1, 2012 at 5:23 PM, Shawn O. Pearce wrote:\n> From: \"Shawn O. Pearce\" \n>\n> When libcurl fails to connect to an SSL server always retry the\n> request once. Since the connection failed before the HTTP headers\n> can be sent, no data has exchanged hands, so the remote side has\n> not learned of the request and will not perform it twice.\n>\n> In the wild we have seen git-remote-https fail to connect to\n> some load-balanced SSL servers sporadically, while modern popular\n> browsers (e.g. Firefox and Chromium) have no trouble with the same\n> server pool.\n>\n> Lets assume the site operators (Hi Google!) have a clue and are\n> doing everything they already can to ensure secure, successful\n> SSL connections from a wide range of HTTP clients. Implementing a\n> single level of retry in the client can make it more robust against\n> transient failure modes.\n\nOk, this begs for some background info...\n@Dayjob one of the many things I do is mange our load balancers\n(redundant pair in our case). If the attempted SSL connections in one\n\"bin\" (time-slot) exceeds the licensed size of that \"bin\" then the\nexcess attempts are just \"dropped on the floor.\" Normal web browsers\ndetect this initial failure and try again. This may be implemented\ninternally—I haven't checked.\n\nGoogle, as I am sure you are well aware, doesn't rely upon a\ntraditional L2/L3 network level load balancing architecture.\nTherefore, I would not attempt to argue that the results that apply to\ntheir systems would apply much of anywhere else. (They have done\npresentations publicly, which are archived on the 'net, about how they\ndo things.)\n\n-- \n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"200307","messageId":"CAJo=hJtxNq_DYNkok7D29G1takRCp6mw85zsa49XetZNVNEm8g@mail.gmail.com","threadId":"31615","inReplyTo":"20121001221817.GA12496@sigill.intra.peff.net","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2012-10-02T02:38:41Z","receivedAt":"2012-10-02T02:38:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Mon, Oct 1, 2012 at 3:18 PM, Jeff King wrote:\n> On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:\n>>\n>> When libcurl fails to connect to an SSL server always retry the\n>> request once. Since the connection failed before the HTTP headers\n>> can be sent, no data has exchanged hands, so the remote side has\n>> not learned of the request and will not perform it twice.\n>\n> I find this a little distasteful just because we haven't figured out the\n> actual _reason_ for the failure. That is, I'm not convinced this isn't\n> something that curl or the ssl library can't handle internally if we\n> would only configure them correctly. Did you ever follow up on tweaking\n> the session caching options for curl?\n\nNo. I didn't try because I reproduced the issue on the initial \"GET\n/.../info/refs?service=git-upload-pack\" request with no authentication\nrequired. So the very first thing the remote-https process did was\nfail on an SSL error. During this run I was using a patched Git that\nhad a different version of the retry logic, but it immediately retried\nand the retry was successful. At that point I decided the SSL session\ncache wasn't possibly relevant since the first request failed and the\nimmediate retry was OK.\n\n> Have you tried running your fails-after-a-few-hours request with other\n> clients that don't have the problem and seeing what they do\n\nThis is harder to reproduce than you think. It took me about 5 days of\ncontinuous polling to reproduce the error. And I have thus far only\nreproduced it against our production servers. This makes it very hard\nto test anything. Or to prove that any given patch is better than a\ndifferent version.\n\n> (I'm\n> thinking a small webkit harness or something would be the most\n> feasible)?\n\nSo I suspect the contrib/persistent-https proxy thing in Go actually\npapers over this problem by having the Go SSL client handle the\nconnection. But this is only based on a test I ran for several days\nthrough that proxy that did not reproduce the bug. This doesn't mean\nit doesn't reproduce with the proxy, it just means _I_ didn't get\nlucky with an error in a ~48 hour run.\n\n> which means it shouldn't really be affecting the general populace. So\n> even though it feels like a dirty hack, at least it is self-contained,\n> and it does fix a real-world problem. If your answer to the above\n> questions is \"hunting this further is just not worth the effort\", I can\n> live with that.\n\nI am sort of at that point, but the hack is so ugly... yea, we\nshouldn't have to do this. Or pollute our code with it. I'm willing to\ngo back and iterate on this further, but its going to be a while\nbefore I can provide any more information.\n\n>> diff --git a/remote-curl.c b/remote-curl.c\n>> index a269608..04a379c 100644\n>> --- a/remote-curl.c\n>> +++ b/remote-curl.c\n>> @@ -353,6 +353,8 @@ static int run_slot(struct active_request_slot *slot)\n>>\n>> slot->results = &results;\n>> slot->curl_result = curl_easy_perform(slot->curl);\n>> + if (slot->curl_result == CURLE_SSL_CONNECT_ERROR)\n>> + slot->curl_result = curl_easy_perform(slot->curl);\n>> finish_active_slot(slot);\n>\n> How come the first hunk gets a nice for-loop and this one doesn't?\n\nBoth hunks retry exactly once after an SSL connect error. I just tried\nto pick something reasonably clean to implement. This hunk seemed\nsimple with the if, the other was uglier and a loop seemed the most\nsimple way to get a retry in there.\n\n> Also, are these hunks the only two spots where this error can come up?\n> The first one does http_request, which handles smart-http GET requests.\n> the second does run_slot, which handles smart-http POST requests.\n\nGrrr. I thought I caught all of the curl perform calls but I guess I\nmissed the dumb transport.\n\n> Some of the dumb http fetches will go through http_request. But some\n> will not. And I think almost none of dumb http push will.\n\nWell, don't use those? :-)\n"},{"id":"200319","messageId":"CAM9Z-nmWdM1g5B+M7sSV3jkAJsUEkHUGPxbUi-r4x7zAwD29qA@mail.gmail.com","threadId":"31615","inReplyTo":"CAJo=hJtxNq_DYNkok7D29G1takRCp6mw85zsa49XetZNVNEm8g@mail.gmail.com","subject":"Re: [PATCH] Retry HTTP requests on SSL connect failures","fromName":"Drew Northup","fromEmail":"n1xim.email@gmail.com","sentAt":"2012-10-02T13:57:29Z","receivedAt":"2012-10-02T13:57:29Z","isPatch":true,"sender":{"key":"n1xim.email@gmail.com","avatar":null},"body":"On Mon, Oct 1, 2012 at 10:38 PM, Shawn Pearce wrote:\n> On Mon, Oct 1, 2012 at 3:18 PM, Jeff King wrote:\n>> On Mon, Oct 01, 2012 at 02:23:06PM -0700, Shawn O. Pearce wrote:\n>>>\n>>> When libcurl fails to connect to an SSL server always retry the\n>>> request once. Since the connection failed before the HTTP headers\n>>> can be sent, no data has exchanged hands, so the remote side has\n>>> not learned of the request and will not perform it twice.\n>>\n>> I find this a little distasteful just because we haven't figured out the\n>> actual _reason_ for the failure.\n>\n> No. I didn't try because I reproduced the issue on the initial \"GET\n> /.../info/refs?service=git-upload-pack\" request with no authentication\n> required. So the very first thing the remote-https process did was\n> fail on an SSL error. During this run I was using a patched Git that\n> had a different version of the retry logic, but it immediately retried\n> and the retry was successful. At that point I decided the SSL session\n> cache wasn't possibly relevant since the first request failed and the\n> immediate retry was OK.\n>\n>> Have you tried running your fails-after-a-few-hours request with other\n>> clients that don't have the problem and seeing what they do\n>\n> This is harder to reproduce than you think. It took me about 5 days of\n> continuous polling to reproduce the error. And I have thus far only\n> reproduced it against our production servers. This makes it very hard\n> to test anything. Or to prove that any given patch is better than a\n> different version.\n\nThe only sure way to make sure your patch works is to get your load\nbalancers Slashdotted first (reason noted in my previous mail on this\nsubject). For the sake of your relationship with your networking crew\nI'd not advise doing that intentionally.\n\n\n>> which means it shouldn't really be affecting the general populace. So\n>> even though it feels like a dirty hack, at least it is self-contained,\n>> and it does fix a real-world problem. If your answer to the above\n>> questions is \"hunting this further is just not worth the effort\", I can\n>> live with that.\n>\n> I am sort of at that point, but the hack is so ugly... yea, we\n> shouldn't have to do this. Or pollute our code with it. I'm willing to\n> go back and iterate on this further, but its going to be a while\n> before I can provide any more information.\n\n>> How come the first hunk gets a nice for-loop and this one doesn't?\n>\n> Both hunks retry exactly once after an SSL connect error. I just tried\n> to pick something reasonably clean to implement. This hunk seemed\n> simple with the if, the other was uglier and a loop seemed the most\n> simple way to get a retry in there.\n\nIf indeed the problem you are having is with a load balanced setup\nthen applying TCP/IP like back-off semantics is the right way to go.\nThe only reason the network stack isn't doing it for you is because\nthe load balancers wait for the SSL/TLS start before dumping the\n\"excess\" (exceeding of license) SSL connections.\n\n-- \n-Drew Northup\n--------------------------------------------------------------\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"}]}