{"thread":{"id":"45247","subject":"[PATCH] http: attempt updating base URL only if no error","startedAt":"2017-02-28T03:25:41Z","lastAt":"2017-02-28T21:00:50Z","messageCount":4,"participants":["Jonathan Tan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312818","messageId":"20170228025311.6347-1-jonathantanmy@google.com","threadId":"45247","inReplyTo":null,"subject":"[PATCH] http: attempt updating base URL only if no error","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-28T02:53:11Z","receivedAt":"2017-02-28T03:25:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"http.c supports HTTP redirects of the form\n\n  http://foo/info/refs?service=git-upload-pack\n  -> http://anything\n  -> http://bar/info/refs?service=git-upload-pack\n\n(that is to say, as long as the Git part of the path and the query\nstring is preserved in the final redirect destination, the intermediate\nsteps can have any URL). However, if one of the intermediate steps\nresults in an HTTP exception, a confusing \"unable to update url base\nfrom redirection\" message is printed instead of a Curl error message\nwith the HTTP exception code.\n\nThis was introduced by 2 commits. Commit c93c92f (\"http: update base\nURLs when we see redirects\", 2013-09-28) introduced a best-effort\noptimization that required checking if only the \"base\" part of the URL\ndiffered between the initial request and the final redirect destination,\nbut it performed the check before any HTTP status checking was done. If\nsomething went wrong, the normal code path was still followed, so this\ndid not cause any confusing error messages until commit 6628eb4 (\"http:\nalways update the base URL for redirects\", 2016-12-06), which taught\nhttp to die if the non-\"base\" part of the URL differed.\n\nTherefore, teach http to check the HTTP status before attempting to\ncheck if only the \"base\" part of the URL differed. This commit teaches\nhttp_request_reauth to return early without updating options->base_url\nupon an error; the only invoker of this function that passes a non-NULL\n\"options\" is remote-curl.c (through \"http_get_strbuf\"), which only uses\noptions->base_url for an informational message in the situations that\nthis commit cares about (that is, when the return value is not HTTP_OK).\n\nThe included test checks that the redirect scheme at the beginning of\nthis commit message works, and that returning a 502 in the middle of the\nredirect scheme produces the correct result. Note that this is different\nfrom the test in commit 6628eb4 (\"http: always update the base URL for\nredirects\", 2016-12-06) in that this commit tests that a Git-shaped URL\n(http://.../info/refs?service=git-upload-pack) works, whereas commit\n6628eb4 tests that a non-Git-shaped URL\n(http://.../info/refs/foo?service=git-upload-pack) does not work (even\nthough Git is processing that URL) and is an error that is fatal, not\nsilently swallowed.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n http.c                     | 3 +++\n t/lib-httpd/apache.conf    | 9 +++++++++\n t/t5550-http-fetch-dumb.sh | 9 +++++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 90a1c0f11..1df186894 100644\n--- a/http.c\n+++ b/http.c\n@@ -1727,6 +1727,9 @@ static int http_request_reauth(const char *url,\n {\n \tint ret = http_request(url, result, target, options);\n \n+\tif (ret != HTTP_OK && ret != HTTP_REAUTH)\n+\t\treturn ret;\n+\n \tif (options && options->effective_url && options->base_url) {\n \t\tif (update_url_from_redirect(options->base_url,\n \t\t\t\t\t     url, options->effective_url)) {\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 69174c6e3..0642ae7e6 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -133,6 +133,15 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]\n RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]\n RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]\n \n+# redir-to/502/x?y -> really-redir-to?path=502/x&qs=y which returns 502\n+# redir-to/x?y -> really-redir-to?path=x&qs=y -> x?y\n+RewriteCond %{QUERY_STRING} ^(.*)$\n+RewriteRule ^/redir-to/(.*)$ /really-redir-to?path=$1&qs=%1 [R=302]\n+RewriteCond %{QUERY_STRING} ^path=502/(.*)&qs=(.*)$\n+RewriteRule ^/really-redir-to$ - [R=502,L]\n+RewriteCond %{QUERY_STRING} ^path=(.*)&qs=(.*)$\n+RewriteRule ^/really-redir-to$ /%1?%2 [R=302]\n+\n # The first rule issues a client-side redirect to something\n # that _doesn't_ look like a git repo. The second rule is a\n # server-side rewrite, so that it turns out the odd-looking\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex aeb3a63f7..2d3b1e9f9 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -378,5 +378,14 @@ test_expect_success 'http-alternates triggers not-from-user protocol check' '\n \t\tclone $HTTPD_URL/dumb/evil.git evil-user\n '\n \n+test_expect_success 'can redirect through non-\"info/refs?service=git-upload-pack\" URL' '\n+\tgit clone \"$HTTPD_URL/redir-to/dumb/repo.git\"\n+'\n+\n+test_expect_success 'print HTTP error when any intermediate redirect throws error' '\n+\ttest_must_fail git clone \"$HTTPD_URL/redir-to/502\" 2> stderr &&\n+\ttest_i18ngrep \"unable to access.*/redir-to/502\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.11.0.483.g087da7b7c-goog\n\n"},{"id":"312843","messageId":"20170228132814.wp3cq4ilp7syinqy@sigill.intra.peff.net","threadId":"45247","inReplyTo":"20170228025311.6347-1-jonathantanmy@google.com","subject":"Re: [PATCH] http: attempt updating base URL only if no error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T13:28:14Z","receivedAt":"2017-02-28T13:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2017 at 06:53:11PM -0800, Jonathan Tan wrote:\n\n> http.c supports HTTP redirects of the form\n> \n>   http://foo/info/refs?service=git-upload-pack\n>   -> http://anything\n>   -> http://bar/info/refs?service=git-upload-pack\n> \n> (that is to say, as long as the Git part of the path and the query\n> string is preserved in the final redirect destination, the intermediate\n> steps can have any URL). However, if one of the intermediate steps\n> results in an HTTP exception, a confusing \"unable to update url base\n> from redirection\" message is printed instead of a Curl error message\n> with the HTTP exception code.\n\nRight, your patch makes sense. A real HTTP error should take precedence\nover the url-update trickery.\n\nAcked-by: Jeff King <peff@peff.net>\n\n> Therefore, teach http to check the HTTP status before attempting to\n> check if only the \"base\" part of the URL differed. This commit teaches\n> http_request_reauth to return early without updating options->base_url\n> upon an error; the only invoker of this function that passes a non-NULL\n> \"options\" is remote-curl.c (through \"http_get_strbuf\"), which only uses\n> options->base_url for an informational message in the situations that\n> this commit cares about (that is, when the return value is not HTTP_OK).\n\nRunning your included test, we get:\n\n  fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The\n  requested URL returned error: 502\n\nbut the error really happened in the intermediate step. I wonder if we\nshould show the effective_url in that case, as it's more likely to\npinpoint the problem. OTOH, we do not mention the intermediate redirect\nat all, so they might be confused about where that URL came from. If you\nreally want to debug HTTP confusion, you should use GIT_TRACE_CURL.\n\nAt any rate, that's orthogonal to your patch, which is obviously the\nright thing to do.\n\n-Peff\n"},{"id":"312868","messageId":"6127c0a7-800d-fcf3-c6e2-17533347f07e@google.com","threadId":"45247","inReplyTo":"20170228132814.wp3cq4ilp7syinqy@sigill.intra.peff.net","subject":"Re: [PATCH] http: attempt updating base URL only if no error","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-02-28T18:48:52Z","receivedAt":"2017-02-28T18:50:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 02/28/2017 05:28 AM, Jeff King wrote:\n> Right, your patch makes sense. A real HTTP error should take precedence\n> over the url-update trickery.\n>\n> Acked-by: Jeff King <peff@peff.net>\n\nThanks!\n\n> Running your included test, we get:\n>\n>   fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The\n>   requested URL returned error: 502\n>\n> but the error really happened in the intermediate step. I wonder if we\n> should show the effective_url in that case, as it's more likely to\n> pinpoint the problem. OTOH, we do not mention the intermediate redirect\n> at all, so they might be confused about where that URL came from. If you\n> really want to debug HTTP confusion, you should use GIT_TRACE_CURL.\n\nYeah, if we mention the effective_url, I think that there would need to \nbe a lot more explaining to be done (e.g. why does my URL have \n\"info/refs?service=git-upload-pack\" tacked on at the end). It might be \nbetter to just recommend GIT_TRACE_CURL.\n"},{"id":"312895","messageId":"20170228195800.tghcnbtjg3qjrhrv@sigill.intra.peff.net","threadId":"45247","inReplyTo":"6127c0a7-800d-fcf3-c6e2-17533347f07e@google.com","subject":"Re: [PATCH] http: attempt updating base URL only if no error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-28T19:58:00Z","receivedAt":"2017-02-28T21:00:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 28, 2017 at 10:48:52AM -0800, Jonathan Tan wrote:\n\n> > Running your included test, we get:\n> > \n> >   fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The\n> >   requested URL returned error: 502\n> > \n> > but the error really happened in the intermediate step. I wonder if we\n> > should show the effective_url in that case, as it's more likely to\n> > pinpoint the problem. OTOH, we do not mention the intermediate redirect\n> > at all, so they might be confused about where that URL came from. If you\n> > really want to debug HTTP confusion, you should use GIT_TRACE_CURL.\n> \n> Yeah, if we mention the effective_url, I think that there would need to be a\n> lot more explaining to be done (e.g. why does my URL have\n> \"info/refs?service=git-upload-pack\" tacked on at the end). It might be\n> better to just recommend GIT_TRACE_CURL.\n\nIndeed. Your comment made me realize that my suggestion was the exact\nopposite of the earlier d5ccbe4df (remote-curl: consistently report repo\nurl for http errors, 2013-04-05). :)\n\nGiven that we don't see a lot of questions on the list about this,\neither it doesn't come up much, or they are capable of finding\nGIT_TRACE_CURL or GIT_CURL_VERBOSE on their own. So I think we can leave\nthe message as-is.\n\n-Peff\n"}]}