From patchwork Sun Mar 24 12:08:38 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10867421 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 6DE62925 for ; Sun, 24 Mar 2019 12:08:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5833F294FA for ; Sun, 24 Mar 2019 12:08:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4C44029516; Sun, 24 Mar 2019 12:08:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id DD8C9294FA for ; Sun, 24 Mar 2019 12:08:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728509AbfCXMIk (ORCPT ); Sun, 24 Mar 2019 08:08:40 -0400 Received: from cloud.peff.net ([104.130.231.41]:34194 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726160AbfCXMIk (ORCPT ); Sun, 24 Mar 2019 08:08:40 -0400 Received: (qmail 4490 invoked by uid 109); 24 Mar 2019 12:08:40 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sun, 24 Mar 2019 12:08:40 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1518 invoked by uid 111); 24 Mar 2019 12:09:03 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sun, 24 Mar 2019 08:09:03 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sun, 24 Mar 2019 08:08:38 -0400 Date: Sun, 24 Mar 2019 08:08:38 -0400 From: Jeff King To: Eric Wong Cc: Wolfgang Denk , Heinrich Schuchardt , git@vger.kernel.org, Junio C Hamano , Takahiro Akashi Subject: [PATCH 1/3] http: factor out curl result code normalization Message-ID: <20190324120838.GA312@sigill.intra.peff.net> References: <20190324120757.GA18684@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190324120757.GA18684@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP We make some requests with CURLOPT_FAILONERROR and some without, and then handle_curl_result() normalizes any failures to a uniform CURLcode. There are some other code paths in the dumb-http walker which don't use handle_curl_result(); let's pull the normalization into its own function so it can be reused. Arguably those code paths would benefit from the rest of handle_curl_result(), notably the auth handling. But retro-fitting it now would be a lot of work, and in practice it doesn't matter too much (whatever authentication we needed to make the initial contact with the server is generally sufficient for the rest of the dumb-http requests). Signed-off-by: Jeff King --- http.c | 18 ++++++++++++------ http.h | 9 +++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/http.c b/http.c index a32ad36ddf..89fcd36a80 100644 --- a/http.c +++ b/http.c @@ -1544,7 +1544,8 @@ char *get_remote_object_url(const char *url, const char *hex, return strbuf_detach(&buf, NULL); } -static int handle_curl_result(struct slot_results *results) +void normalize_curl_result(CURLcode *result, long http_code, + char *errorstr, size_t errorlen) { /* * If we see a failing http code with CURLE_OK, we have turned off @@ -1554,19 +1555,24 @@ static int handle_curl_result(struct slot_results *results) * Likewise, if we see a redirect (30x code), that means we turned off * redirect-following, and we should treat the result as an error. */ - if (results->curl_result == CURLE_OK && - results->http_code >= 300) { - results->curl_result = CURLE_HTTP_RETURNED_ERROR; + if (*result == CURLE_OK && http_code >= 300) { + *result = CURLE_HTTP_RETURNED_ERROR; /* * Normally curl will already have put the "reason phrase" * from the server into curl_errorstr; unfortunately without * FAILONERROR it is lost, so we can give only the numeric * status code. */ - xsnprintf(curl_errorstr, sizeof(curl_errorstr), + xsnprintf(errorstr, errorlen, "The requested URL returned error: %ld", - results->http_code); + http_code); } +} + +static int handle_curl_result(struct slot_results *results) +{ + normalize_curl_result(&results->curl_result, results->http_code, + curl_errorstr, sizeof(curl_errorstr)); if (results->curl_result == CURLE_OK) { credential_approve(&http_auth); diff --git a/http.h b/http.h index 4eb4e808e5..f0d271bb7b 100644 --- a/http.h +++ b/http.h @@ -136,6 +136,15 @@ static inline int missing__target(int code, int result) #define missing_target(a) missing__target((a)->http_code, (a)->curl_result) +/* + * Normalize curl results to handle CURL_FAILONERROR (or lack thereof). Failing + * http codes have their "result" converted to CURLE_HTTP_RETURNED_ERROR, and + * an appropriate string placed in the errorstr buffer (pass curl_errorstr if + * you don't have a custom buffer). + */ +void normalize_curl_result(CURLcode *result, long http_code, char *errorstr, + size_t errorlen); + /* Helpers for modifying and creating URLs */ extern void append_remote_object_url(struct strbuf *buf, const char *url, const char *hex, From patchwork Sun Mar 24 12:09:46 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10867423 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 54429925 for ; Sun, 24 Mar 2019 12:09:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3829F29516 for ; Sun, 24 Mar 2019 12:09:50 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 260E42952F; Sun, 24 Mar 2019 12:09:50 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B062F29516 for ; Sun, 24 Mar 2019 12:09:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727481AbfCXMJs (ORCPT ); Sun, 24 Mar 2019 08:09:48 -0400 Received: from cloud.peff.net ([104.130.231.41]:34220 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726090AbfCXMJs (ORCPT ); Sun, 24 Mar 2019 08:09:48 -0400 Received: (qmail 4565 invoked by uid 109); 24 Mar 2019 12:09:48 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sun, 24 Mar 2019 12:09:48 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1557 invoked by uid 111); 24 Mar 2019 12:10:11 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sun, 24 Mar 2019 08:10:11 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sun, 24 Mar 2019 08:09:46 -0400 Date: Sun, 24 Mar 2019 08:09:46 -0400 From: Jeff King To: Eric Wong Cc: Wolfgang Denk , Heinrich Schuchardt , git@vger.kernel.org, Junio C Hamano , Takahiro Akashi Subject: [PATCH 2/3] http: normalize curl results for dumb loose and alternates fetches Message-ID: <20190324120946.GB312@sigill.intra.peff.net> References: <20190324120757.GA18684@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190324120757.GA18684@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP If the dumb-http walker encounters a 404 when fetching a loose object, it then looks at any http-alternates for the object. The 404 check is implemented by missing_target(), which checks not only the http code, but also that we got an http error from the CURLcode. That broke when we stopped using CURLOPT_FAILONERROR in 17966c0a63 (http: avoid disconnecting on 404s for loose objects, 2016-07-11), since our CURLcode will now be CURLE_OK. As a result, fetching over dumb-http from a repository with alternates could result in Git printing "Unable to find abcd1234..." and aborting. We could probably fix this just by loosening missing_target(). However, there's other code which looks at the curl result, and it would have to be tweaked as well. Instead, let's just normalize the result the same way the smart-http code does. There's a similar case in processing the alternates (where we failover from "info/http-alternates" to "info/alternates"). We'll give it the same treatment. After this patch, we should be hitting all code paths that need this normalization (notably absent here is the http_pack_request path, but it does not use FAILONERROR, nor missing_target()). Signed-off-by: Jeff King --- http-walker.c | 8 ++++++++ t/t5550-http-fetch-dumb.sh | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/http-walker.c b/http-walker.c index 8ae5d76c6a..745436921d 100644 --- a/http-walker.c +++ b/http-walker.c @@ -98,6 +98,11 @@ static void process_object_response(void *callback_data) process_http_object_request(obj_req->req); obj_req->state = COMPLETE; + normalize_curl_result(&obj_req->req->curl_result, + obj_req->req->http_code, + obj_req->req->errorstr, + sizeof(obj_req->req->errorstr)); + /* Use alternates if necessary */ if (missing_target(obj_req->req)) { fetch_alternates(walker, alt->base); @@ -208,6 +213,9 @@ static void process_alternates_response(void *callback_data) char *data; int i = 0; + normalize_curl_result(&slot->curl_result, slot->http_code, + curl_errorstr, sizeof(curl_errorstr)); + if (alt_req->http_specific) { if (slot->curl_result != CURLE_OK || !alt_req->buffer->len) { diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index 6d7d88ccc9..694b77c855 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -408,5 +408,21 @@ test_expect_success 'print HTTP error when any intermediate redirect throws erro test_i18ngrep "unable to access.*/redir-to/502" stderr ' +test_expect_success 'fetching via http alternates works' ' + parent=$HTTPD_DOCUMENT_ROOT_PATH/alt-parent.git && + git init --bare "$parent" && + git -C "$parent" --work-tree=. commit --allow-empty -m foo && + git -C "$parent" update-server-info && + commit=$(git -C "$parent" rev-parse HEAD) && + + child=$HTTPD_DOCUMENT_ROOT_PATH/alt-child.git && + git init --bare "$child" && + echo "../../alt-parent.git/objects" >"$child/objects/info/alternates" && + git -C "$child" update-ref HEAD $commit && + git -C "$child" update-server-info && + + git -c http.followredirects=true clone "$HTTPD_URL/dumb/alt-child.git" +' + stop_httpd test_done From patchwork Sun Mar 24 12:13:16 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 10867425 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id B0FB9925 for ; Sun, 24 Mar 2019 12:13:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9109A2952F for ; Sun, 24 Mar 2019 12:13:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 84A9F295DB; Sun, 24 Mar 2019 12:13:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 24AA42952F for ; Sun, 24 Mar 2019 12:13:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727137AbfCXMNS (ORCPT ); Sun, 24 Mar 2019 08:13:18 -0400 Received: from cloud.peff.net ([104.130.231.41]:34234 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726160AbfCXMNS (ORCPT ); Sun, 24 Mar 2019 08:13:18 -0400 Received: (qmail 4682 invoked by uid 109); 24 Mar 2019 12:13:18 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Sun, 24 Mar 2019 12:13:18 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1585 invoked by uid 111); 24 Mar 2019 12:13:41 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Sun, 24 Mar 2019 08:13:41 -0400 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Sun, 24 Mar 2019 08:13:16 -0400 Date: Sun, 24 Mar 2019 08:13:16 -0400 From: Jeff King To: Eric Wong Cc: Wolfgang Denk , Heinrich Schuchardt , git@vger.kernel.org, Junio C Hamano , Takahiro Akashi Subject: [PATCH 3/3] http: use normalize_curl_result() instead of manual conversion Message-ID: <20190324121316.GC312@sigill.intra.peff.net> References: <20190324120757.GA18684@sigill.intra.peff.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190324120757.GA18684@sigill.intra.peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP When we switched off CURLOPT_FAILONERROR in 17966c0a63 (http: avoid disconnecting on 404s for loose objects, 2016-07-11), the fetch_object() function started manually handling 404's. Since we now have normalize_curl_result() for use elsewhere, we can use it here as well, shortening the code. Note that we lose the check for http/https in the URL here. None of the other result-normalizing code paths bother with this. Looking at missing_target(), which checks specifically for an FTP-specific CURLcode and "http" code 550, it seems likely that git-over-ftp has been subtly broken since 17966c0a63. This patch does nothing to fix that, but nor should it make anything worse (in fact, it may be slightly better because we'll actually recognize an error as such, rather than assuming CURLE_OK means we actually got some data). Signed-off-by: Jeff King --- I can't bring myself to care too much about whether git-over-ftp works with alternates, but if somebody wants to dig into it, be my guest. I suspect other bits may be broken, too, as we check http_code in several places and assume it has some sensible http-based number in it (notably, for 401 triggering authentication). It may not even work at all. I didn't try (and I'd be kind of surprised if somebody is actually using it in practice). I'm content to let it bit-rot unless somebody using it shows up to report a bug. (I'd also be fine with officially deprecating it. But then I kind of feel the same way about the dumb-http code). http-walker.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/http-walker.c b/http-walker.c index 745436921d..48b1b3a193 100644 --- a/http-walker.c +++ b/http-walker.c @@ -526,17 +526,8 @@ static int fetch_object(struct walker *walker, unsigned char *sha1) req->localfile = -1; } - /* - * we turned off CURLOPT_FAILONERROR to avoid losing a - * persistent connection and got CURLE_OK. - */ - if (req->http_code >= 300 && req->curl_result == CURLE_OK && - (starts_with(req->url, "http://") || - starts_with(req->url, "https://"))) { - req->curl_result = CURLE_HTTP_RETURNED_ERROR; - xsnprintf(req->errorstr, sizeof(req->errorstr), - "HTTP request failed"); - } + normalize_curl_result(&req->curl_result, req->http_code, + req->errorstr, sizeof(req->errorstr)); if (obj_req->state == ABORTED) { ret = error("Request for %s aborted", hex);