{"thread":{"id":"33397","subject":"[PATCH 8/9] remote-curl: die directly with http error messages","startedAt":"2013-04-05T22:13:31Z","lastAt":"2013-04-05T22:22:31Z","messageCount":10,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"213305","messageId":"20130405221331.GA21209@sigill.intra.peff.net","threadId":"33397","inReplyTo":null,"subject":"[RFC/PATCH 0/9] friendlier http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:13:31Z","receivedAt":"2013-04-05T22:13:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The error messages that git generates for routine http problems can\nsometimes be a bit verbose or confusing. They also provide no\nopportunity for the server to communicate any free-form text, even\nthough the server knows much better than the git client the reason for\nthe error or what the next step to suggest to the user might be.\n\nThis series provides a channel for those messages, and does some general\ncleanup and reformatting of the error messages that git itself produces.\n\nHere are some before-and-after examples with this series (apologies for\nthe long lines, but they are part of the ugliness I want to address, so\nI am leaving them in).\n\nThe \"remote:\" bits are simulated in the output below, as I haven't yet\nupdated GitHub's server side to produce more useful messages. So you can\nrepeat the tests, but note that the text you get from the server will\nnot be the same (e.g., our 404 currently just says \"Repository not\nfound\" which is not all that helpful).\n\n  [before]\n  $ git ls-remote https://github.com/non/existent\n  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not found: did you run git update-server-info on the server?\n\n  [after]\n  $ git ls-remote https://github.com/non/existent\n  remote: The remote repository was not found, or you do not have\n  remote: permission to access it.\n  fatal: repository 'https://github.com/non/existent/' not found\n\n  [before]\n  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git\n  error: The requested URL returned error: 403 Forbidden while accessing https://github.com/git/git.git/info/refs\n  fatal: HTTP request failed\n\n  [after]\n  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git\n  remote: Sorry, fetching via dumb http is forbidden.\n  remote: Please upgrade your git client to v1.6.6 or greater\n  remote: and make sure that smart-http is enabled.\n  fatal: unable to access 'https://github.com/git/git.git/': The requested URL returned error: 403\n\nI still really hate the length of the generic http message (which you\ncan see in the final 403 example). The text \"The requested URL returned\nerror:\" comes from curl, though there is actually an opportunity to\nmunge it, as you will see in the patches. However, I was unable to come\nup with a shorter text that sounded any better.\n\nAnother option would be to just split it across lines with some\nindentation, like:\n\n  fatal: The requested URL returned error: 403\n    while accessing https://github.com/git/git.git\n\nI'm open to suggestions.\n\nThere are a lot of little patches, as I tried to explain the rationale\nfor each individual change (and it makes it easy to take or reject\nindividual patches). If we do take them all, it may make sense to just\nsquash patches 3-5.\n\n  [1/9]: http: add HTTP_KEEP_ERROR option\n  [2/9]: remote-curl: show server content on http errors\n  [3/9]: remote-curl: let servers override http 404 advice\n  [4/9]: remote-curl: always show friendlier 404 message\n  [5/9]: remote-curl: consistently report repo url for http errors\n  [6/9]: http: simplify http_error helper function\n  [7/9]: http: re-word http error message\n  [8/9]: remote-curl: die directly with http error messages\n  [9/9]: http: drop http_error function\n\n-Peff\n"},{"id":"213303","messageId":"20130405221406.GA22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 1/9] http: add HTTP_KEEP_ERROR option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:14:06Z","receivedAt":"2013-04-05T22:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently set curl's FAILONERROR option, which means that\nany http failures are reported as curl errors, and the\nhttp body content from the server is thrown away.\n\nThis patch introduces a new option to http_get_strbuf which\nspecifies that the body content from a failed http response\nshould be placed in the destination strbuf, where it can be\naccessed by the caller.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http.c | 37 +++++++++++++++++++++++++++++++++++++\n http.h |  1 +\n 2 files changed, 38 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 8803c70..45cc7c7 100644\n--- a/http.c\n+++ b/http.c\n@@ -761,6 +761,25 @@ int handle_curl_result(struct slot_results *results)\n \n int handle_curl_result(struct slot_results *results)\n {\n+\t/*\n+\t * If we see a failing http code with CURLE_OK, we have turned off\n+\t * FAILONERROR (to keep the server's custom error response), and should\n+\t * translate the code into failure here.\n+\t */\n+\tif (results->curl_result == CURLE_OK &&\n+\t    results->http_code >= 400) {\n+\t\tresults->curl_result = CURLE_HTTP_RETURNED_ERROR;\n+\t\t/*\n+\t\t * Normally curl will already have put the \"reason phrase\"\n+\t\t * from the server into curl_errorstr; unfortunately without\n+\t\t * FAILONERROR it is lost, so we can give only the numeric\n+\t\t * status code.\n+\t\t */\n+\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n+\t\t\t \"The requested URL returned error: %ld\",\n+\t\t\t results->http_code);\n+\t}\n+\n \tif (results->curl_result == CURLE_OK) {\n \t\tcredential_approve(&http_auth);\n \t\treturn HTTP_OK;\n@@ -825,6 +844,8 @@ static int http_request(const char *url, struct strbuf *type,\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options & HTTP_NO_CACHE)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n+\tif (options & HTTP_KEEP_ERROR)\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \theaders = curl_slist_append(headers, buf.buf);\n \n@@ -862,6 +883,22 @@ static int http_request_reauth(const char *url,\n \tint ret = http_request(url, type, result, target, options);\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n+\n+\t/*\n+\t * If we are using KEEP_ERROR, the previous request may have\n+\t * put cruft into our output stream; we should clear it out before\n+\t * making our next request. We only know how to do this for\n+\t * the strbuf case, but that is enough to satisfy current callers.\n+\t */\n+\tif (options & HTTP_KEEP_ERROR) {\n+\t\tswitch (target) {\n+\t\tcase HTTP_REQUEST_STRBUF:\n+\t\t\tstrbuf_reset(result);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"BUG: HTTP_KEEP_ERROR is only supported with strbufs\");\n+\t\t}\n+\t}\n \treturn http_request(url, type, result, target, options);\n }\n \ndiff --git a/http.h b/http.h\nindex 25d1931..0fe54f4 100644\n--- a/http.h\n+++ b/http.h\n@@ -118,6 +118,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n \n /* Options for http_request_*() */\n #define HTTP_NO_CACHE\t\t1\n+#define HTTP_KEEP_ERROR\t\t2\n \n /* Return values for http_request_*() */\n #define HTTP_OK\t\t\t0\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213344","messageId":"20130405221723.GB22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 2/9] remote-curl: show server content on http errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:17:23Z","receivedAt":"2013-04-05T22:17:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If an http request to a remote git server fails, we show\nonly the http response code, or sometimes a custom message\nfor particular codes. This gives the server no opportunity\nto offer a more detailed explanation of the reason for the\nfailure, or to give extra advice.\n\nThis patch teaches remote-curl to record and display the\nbody content of a failed http response. We only display such\nresponses when the content-type is advertised as text/plain,\nas it is the most likely to look presentable on the user's\nterminal (and it is hoped to be a good indication that the\nmessage is intended for git clients, and not for a web\nbrowser).\n\nEach line of the new output is prepended with \"remote:\".\nExample output may look like this (assuming the server is\nconfigured to display such a helpful message):\n\n  $ GIT_SMART_HTTP=0 git clone https://example.com/some/repo.git\n  Cloning into 'repo'...\n  remote: Sorry, fetching via dumb http is forbidden.\n  remote: Please upgrade your git client to v1.6.6 or greater\n  remote: and make sure that smart-http is enabled.\n  error: The requested URL returned error: 403 while accessing http://localhost:5001/some/repo.git/info/refs\n  fatal: HTTP request failed\n\nFor the sake of simplicity, we only record and display these\nerrors during the initial fetch of the ref list, as that is\nthe initial contact with the server and where the most\ncommon, interesting errors happen (and there is already\nprecedent, as that is the only place we currently massage\nhttp error codes into more helpful messages).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI took Jonathan's suggestion of just respecting text/plain to signify a\nmessage that git can show on the terminal. It is the most sensible\ncontent-type to use, I think, but I'm a little worried that\nrandom servers may produce totally uninteresting text/plain (e.g., many\nservers ship with 404 pages that just say \"404\" again with more useless\ntext). But such pages tend to be text/html, so we may be able to get\naway with assuming text/plain will be useful.\n\n remote-curl.c | 33 ++++++++++++++++++++++++++++++++-\n 1 file changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 93a09a6..4fea94f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -151,6 +151,33 @@ static void free_discovery(struct discovery *d)\n \t}\n }\n \n+static int show_http_message(struct strbuf *type, struct strbuf *msg)\n+{\n+\tconst char *p, *eol;\n+\n+\t/*\n+\t * We only show text/plain parts, as other types are likely\n+\t * to be ugly to look at on the user's terminal.\n+\t *\n+\t * TODO should handle \"; charset=XXX\", and re-encode into\n+\t * logoutputencoding\n+\t */\n+\tif (strcasecmp(type->buf, \"text/plain\"))\n+\t\treturn -1;\n+\n+\tstrbuf_trim(msg);\n+\tif (!msg->len)\n+\t\treturn -1;\n+\n+\tp = msg->buf;\n+\tdo {\n+\t\teol = strchrnul(p, '\\n');\n+\t\tfprintf(stderr, \"remote: %.*s\\n\", (int)(eol - p), p);\n+\t\tp = eol + 1;\n+\t} while(*eol);\n+\treturn 0;\n+}\n+\n static struct discovery* discover_refs(const char *service, int for_push)\n {\n \tstruct strbuf exp = STRBUF_INIT;\n@@ -176,16 +203,20 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t}\n \trefs_url = strbuf_detach(&buffer, NULL);\n \n-\thttp_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE);\n+\thttp_ret = http_get_strbuf(refs_url, &type, &buffer,\n+\t\t\t\t   HTTP_NO_CACHE | HTTP_KEEP_ERROR);\n \tswitch (http_ret) {\n \tcase HTTP_OK:\n \t\tbreak;\n \tcase HTTP_MISSING_TARGET:\n+\t\tshow_http_message(&type, &buffer);\n \t\tdie(\"%s not found: did you run git update-server-info on the\"\n \t\t    \" server?\", refs_url);\n \tcase HTTP_NOAUTH:\n+\t\tshow_http_message(&type, &buffer);\n \t\tdie(\"Authentication failed\");\n \tdefault:\n+\t\tshow_http_message(&type, &buffer);\n \t\thttp_error(refs_url, http_ret);\n \t\tdie(\"HTTP request failed\");\n \t}\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213312","messageId":"20130405221740.GC22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 3/9] remote-curl: let servers override http 404 advice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:17:40Z","receivedAt":"2013-04-05T22:17:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we get an http 404 trying to get the initial list of\nrefs from the server, we try to be helpful and remind the\nuser that update-server-info may need to be run. This looks\nlike:\n\n  $ git clone https://github.com/non/existent\n  Cloning into 'existent'...\n  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not found: did you run git update-server-info on the server?\n\nSuggesting update-server-info may be a good suggestion for\nusers who are in control of the server repo and who are\nplanning to set up dumb http. But for users of smart http,\nand especially users who are not in control of the server\nrepo, the advice is useless and confusing.\n\nThe previous patch taught remote-curl to show custom advice\nfrom the server when it is available. When we have shown\nmessages from the server, we can also drop our custom\nadvice; what the server has to say is likely to be more\naccurate and helpful.\n\nWe not only drop the mention of update-server-info, but also\nshow only the main repo URL, not the full \"info/refs\" and\nservice parameter. These elements may be useful for\ndebugging a broken server configuration, but again, anything\nthe server has provided is likely to be more useful (and one\ncan still use GIT_CURL_VERBOSE to get much more complete\ndebugging information).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4fea94f..083efdf 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -209,7 +209,8 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tcase HTTP_OK:\n \t\tbreak;\n \tcase HTTP_MISSING_TARGET:\n-\t\tshow_http_message(&type, &buffer);\n+\t\tif (!show_http_message(&type, &buffer))\n+\t\t\tdie(\"repository '%s' not found\", url);\n \t\tdie(\"%s not found: did you run git update-server-info on the\"\n \t\t    \" server?\", refs_url);\n \tcase HTTP_NOAUTH:\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213315","messageId":"20130405222043.GD22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 4/9] remote-curl: always show friendlier 404 message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:20:43Z","receivedAt":"2013-04-05T22:20:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we get an http 404 trying to get the initial list of\nrefs from the server, we try to be helpful and remind the\nuser that update-server-info may need to be run. This looks\nlike:\n\n  $ git clone https://github.com/non/existent\n  Cloning into 'existent'...\n  fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not found: did you run git update-server-info on the server?\n\nSuggesting update-server-info may be a good suggestion for\nusers who are in control of the server repo and who are\nplanning to set up dumb http. But for users of smart http,\nand especially users who are not in control of the server\nrepo, the advice is useless and confusing.\n\nSince most people are expected to use smart http these days,\nit does not make sense to keep the update-server-info hint.\n\nWe not only drop the mention of update-server-info, but also\nshow only the main repo URL, not the full \"info/refs\" and\nservice parameter. These elements may be useful for\ndebugging a broken server configuration, but in the majority\nof cases, users are not fetching from their own\nrepositories, but rather from other people's repositories;\nthey have neither the power nor interest to fix a broken\nconfiguration, and the extra components just make the\nmessage more confusing. Users who do want to debug can and\nshould use GIT_CURL_VERBOSE to get more complete information\non the actual URLs visited.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is obviously a more stringent version of the last patch, and could\njust replace it. I think the last one is a no-brainer, because it lets\nwell-configured sites replace these messages. This one is less obvious,\nbecause we may be hitting some random poorly configured server that\nsomebody is trying to set up.\n\nStill, if you think about the number of people fetching (against any\nserver) versus the number of people configuring new servers, I think the\nadvice to run update-server-info has a vanishingly small chance of being\nuseful (and IMHO is misleading if you do not know how dumb-http works,\nas it makes you think the server is misconfigured, when it is much more\nlikely to be a user error).\n\n remote-curl.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 083efdf..5d9f961 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -209,10 +209,8 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tcase HTTP_OK:\n \t\tbreak;\n \tcase HTTP_MISSING_TARGET:\n-\t\tif (!show_http_message(&type, &buffer))\n-\t\t\tdie(\"repository '%s' not found\", url);\n-\t\tdie(\"%s not found: did you run git update-server-info on the\"\n-\t\t    \" server?\", refs_url);\n+\t\tshow_http_message(&type, &buffer);\n+\t\tdie(\"repository '%s' not found\", url);\n \tcase HTTP_NOAUTH:\n \t\tshow_http_message(&type, &buffer);\n \t\tdie(\"Authentication failed\");\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213345","messageId":"20130405222114.GE22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 5/9] remote-curl: consistently report repo url for http errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:21:14Z","receivedAt":"2013-04-05T22:21:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we report http errors in fetching the initial ref\nadvertisement, we show the full URL we attempted to use,\nincluding \"info/refs?service=git-upload-pack\". While this\nmay be useful for debugging a broken server, it is\nunnecessarily verbose and confusing for most cases, in which\nthe client user is not even the same person as the owner of\nthe repository.\n\nLet's just show the repository URL; debugging can happen\nwith GIT_CURL_VERBOSE, which shows way more useful\ninformation, anyway.\n\nAt the same time, let's also make sure to mention the\nrepository URL when we report failed authentication\n(previously we said only \"Authentication failed\"). Knowing\nthe URL can help the user realize why authentication failed\n(e.g., they meant to push to remote A, not remote B).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the same rationale as the latter half of the last patch; if we\ntake them all, I'd be happy to see them squashed together.\n\n remote-curl.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 5d9f961..6c6714b 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -213,10 +213,10 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\tdie(\"repository '%s' not found\", url);\n \tcase HTTP_NOAUTH:\n \t\tshow_http_message(&type, &buffer);\n-\t\tdie(\"Authentication failed\");\n+\t\tdie(\"Authentication failed for '%s'\", url);\n \tdefault:\n \t\tshow_http_message(&type, &buffer);\n-\t\thttp_error(refs_url, http_ret);\n+\t\thttp_error(url, http_ret);\n \t\tdie(\"HTTP request failed\");\n \t}\n \n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213332","messageId":"20130405222134.GF22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 6/9] http: simplify http_error helper function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:21:34Z","receivedAt":"2013-04-05T22:21:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This helper function should really be a one-liner that\nprints an error message, but it has ended up unnecessarily\ncomplicated:\n\n  1. We call error() directly when we fail to start the curl\n     request, so we must later avoid printing a duplicate\n     error in http_error().\n\n     It would be much simpler in this case to just stuff the\n     error message into our usual curl_errorstr buffer\n     rather than printing it ourselves. This means that\n     http_error does not even have to care about curl's exit\n     value (the interesting part is in the errorstr buffer\n     already).\n\n  2. We return the \"ret\" value passed in to us, but none of\n     the callers actually cares about our return value. We\n     can just drop this entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-push.c   |  2 +-\n http.c        | 11 ++++-------\n http.h        |  5 ++---\n remote-curl.c |  2 +-\n 4 files changed, 8 insertions(+), 12 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex bd66f6a..439a555 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1551,7 +1551,7 @@ static int remote_exists(const char *path)\n \t\tret = 0;\n \t\tbreak;\n \tcase HTTP_ERROR:\n-\t\thttp_error(url, HTTP_ERROR);\n+\t\thttp_error(url);\n \tdefault:\n \t\tret = -1;\n \t}\ndiff --git a/http.c b/http.c\nindex 45cc7c7..5e6f67d 100644\n--- a/http.c\n+++ b/http.c\n@@ -857,7 +857,8 @@ static int http_request(const char *url, struct strbuf *type,\n \t\trun_active_slot(slot);\n \t\tret = handle_curl_result(&results);\n \t} else {\n-\t\terror(\"Unable to start HTTP request for %s\", url);\n+\t\tsnprintf(curl_errorstr, sizeof(curl_errorstr),\n+\t\t\t \"failed to start HTTP request\");\n \t\tret = HTTP_START_FAILED;\n \t}\n \n@@ -940,13 +941,9 @@ int http_error(const char *url, int ret)\n \treturn ret;\n }\n \n-int http_error(const char *url, int ret)\n+void http_error(const char *url)\n {\n-\t/* http_request has already handled HTTP_START_FAILED. */\n-\tif (ret != HTTP_START_FAILED)\n-\t\terror(\"%s while accessing %s\", curl_errorstr, url);\n-\n-\treturn ret;\n+\terror(\"%s while accessing %s\", curl_errorstr, url);\n }\n \n int http_fetch_ref(const char *base, struct ref *ref)\ndiff --git a/http.h b/http.h\nindex 0fe54f4..fa65128 100644\n--- a/http.h\n+++ b/http.h\n@@ -136,10 +136,9 @@ int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf\n int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);\n \n /*\n- * Prints an error message using error() containing url and curl_errorstr,\n- * and returns ret.\n+ * Prints an error message using error() containing url and curl_errorstr.\n  */\n-int http_error(const char *url, int ret);\n+void http_error(const char *url);\n \n extern int http_fetch_ref(const char *base, struct ref *ref);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6c6714b..9abe4b7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -216,7 +216,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\tdie(\"Authentication failed for '%s'\", url);\n \tdefault:\n \t\tshow_http_message(&type, &buffer);\n-\t\thttp_error(url, http_ret);\n+\t\thttp_error(url);\n \t\tdie(\"HTTP request failed\");\n \t}\n \n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213307","messageId":"20130405222201.GG22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 7/9] http: re-word http error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:22:01Z","receivedAt":"2013-04-05T22:22:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we report an http error code, we say something like:\n\n  error: The requested URL reported failure: 403 Forbidden while accessing http://example.com/repo.git\n\nEverything between \"error:\" and \"while\" is written by curl,\nand the resulting sentence is hard to read (especially\nbecause there is no punctuation between curl's sentence and\nthe remainder of ours). Instead, let's re-order this to give\nbetter flow:\n\n  error: unable to access 'http://example.com/repo.git: The requested URL reported failure: 403 Forbidden\n\nThis is still annoyingly long, but at least reads more\nclearly left to right.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs I mentioned in the cover letter, I'm still not that excited about the\n\"after\" result here. Suggestions welcome.\n\n http.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 5e6f67d..64068a2 100644\n--- a/http.c\n+++ b/http.c\n@@ -943,7 +943,7 @@ void http_error(const char *url)\n \n void http_error(const char *url)\n {\n-\terror(\"%s while accessing %s\", curl_errorstr, url);\n+\terror(\"unable to access '%s': %s\", url, curl_errorstr);\n }\n \n int http_fetch_ref(const char *base, struct ref *ref)\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213288","messageId":"20130405222215.GH22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 8/9] remote-curl: die directly with http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:22:15Z","receivedAt":"2013-04-05T22:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we encounter an unknown http error (e.g., a 403), we\nhand the error code to http_error, which then prints it with\nerror(). After that we die with the redundant message \"HTTP\nrequest failed\".\n\nInstead, let's just drop http_error entirely, which does\nnothing but pass arguments to error(), and instead die\ndirectly with a useful message.\n\nSo before:\n\n  $ git clone https://example.com/repo.git\n  Cloning into 'repo'...\n  error: unable to access 'https://example.com/repo.git': The requested URL returned error: 403 Forbidden\n  fatal: HTTP request failed\n\nand after:\n\n  $ git clone https://example.com/repo.git\n  Cloning into 'repo'...\n  fatal: unable to access 'https://example.com/repo.git': The requested URL returned error: 403 Forbidden\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote-curl.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 9abe4b7..60eda63 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -216,8 +216,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \t\tdie(\"Authentication failed for '%s'\", url);\n \tdefault:\n \t\tshow_http_message(&type, &buffer);\n-\t\thttp_error(url);\n-\t\tdie(\"HTTP request failed\");\n+\t\tdie(\"unable to access '%s': %s\", url, curl_errorstr);\n \t}\n \n \tlast= xcalloc(1, sizeof(*last_discovery));\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213330","messageId":"20130405222231.GI22163@sigill.intra.peff.net","threadId":"33397","inReplyTo":"20130405221331.GA21209@sigill.intra.peff.net","subject":"[PATCH 9/9] http: drop http_error function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T22:22:31Z","receivedAt":"2013-04-05T22:22:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function is a single-liner and is only called from one\nplace. Just inline it, which makes the code more obvious.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a cleanup made possible by the previous patch.\n\n http-push.c | 2 +-\n http.c      | 5 -----\n http.h      | 5 -----\n 3 files changed, 1 insertion(+), 11 deletions(-)\n\ndiff --git a/http-push.c b/http-push.c\nindex 439a555..395a8cf 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1551,7 +1551,7 @@ static int remote_exists(const char *path)\n \t\tret = 0;\n \t\tbreak;\n \tcase HTTP_ERROR:\n-\t\thttp_error(url);\n+\t\terror(\"unable to access '%s': %s\", url, curl_errorstr);\n \tdefault:\n \t\tret = -1;\n \t}\ndiff --git a/http.c b/http.c\nindex 64068a2..58c063c 100644\n--- a/http.c\n+++ b/http.c\n@@ -941,11 +941,6 @@ cleanup:\n \treturn ret;\n }\n \n-void http_error(const char *url)\n-{\n-\terror(\"unable to access '%s': %s\", url, curl_errorstr);\n-}\n-\n int http_fetch_ref(const char *base, struct ref *ref)\n {\n \tchar *url;\ndiff --git a/http.h b/http.h\nindex fa65128..4dc5353 100644\n--- a/http.h\n+++ b/http.h\n@@ -135,11 +135,6 @@ int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf\n  */\n int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);\n \n-/*\n- * Prints an error message using error() containing url and curl_errorstr.\n- */\n-void http_error(const char *url);\n-\n extern int http_fetch_ref(const char *base, struct ref *ref);\n \n /* Helpers for fetching packs */\n-- \n1.8.2.rc0.33.gd915649\n"}]}