git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 6/6] http-walker: complain about non-404 loose object errors

From
Jeff King <peff@peff.net>
Date
Dec 1, 2016, 09:04 UTC
Message-ID
<20161201090436.cwh2pxio7afowtfs@sigill.intra.peff.net>
In-Reply-To
<20161201090336.xjbb47bublfcpglo@sigill.intra.peff.net>

Since commit 17966c0a6 (http: avoid disconnecting on 404s for loose objects, 2016-07-11), we turn off curl's FAILONERROR option and instead manually deal with failing HTTP codes.

However, the logic to do so only recognizes HTTP 404 as a failure. This is probably the most common result, but if we were to get another code, the curl result remains CURLE_OK, and we treat it as success. We still end up detecting the failure when we try to zlib-inflate the object (which will fail), but instead of reporting the HTTP error, we just claim that the object is corrupt.

Instead, let's catch anything in the 300's or above as an error (300's are redirects which are not an error at the HTTP level, but are an indication that we've explicitly disabled redirects, so we should treat them as such; we certainly don't have the resulting object content).

Note that we also fill in req->errorstr, which we didn't do before. Without FAILONERROR, curl will not have filled this in, and it will remain a blank string. This never mattered for the 404 case, because in the logic below we hit the "missing_target()" branch and print nothing. But for other errors, we'd want to say _something_, if only to fill in the blank slot in the error message.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-walker.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 25a8b1ad4..c2f81cd6a 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -482,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
 	 * we turned off CURLOPT_FAILONERROR to avoid losing a
 	 * persistent connection and got CURLE_OK.
 	 */
-	if (req->http_code == 404 && req->curl_result == CURLE_OK &&
+	if (req->http_code >= 300 && req->curl_result == CURLE_OK &&
 			(starts_with(req->url, "http://") ||
-			 starts_with(req->url, "https://")))
+			 starts_with(req->url, "https://"))) {
 		req->curl_result = CURLE_HTTP_RETURNED_ERROR;
+		xsnprintf(req->errorstr, sizeof(req->errorstr),
+			  "HTTP request failed");
+	}
 
 	if (obj_req->state == ABORTED) {
 		ret = error("Request for %s aborted", hex);
-- 
2.11.0.319.g1f4e1e0
Previous: Ramsay JonesNext: Jeff King
Message 12 of 18 in “restricting http redirects”
  1. 0/6 restricting http redirectsJeff King, Dec 1, 2016
  2. 3/6 remote-curl: rename shadowed options variableJeff King, Dec 1, 2016
  3. 1/6 http: simplify update_url_from_redirectJeff King, Dec 1, 2016
  4. 2/6 http: always update the base URL for redirectsJeff King, Dec 1, 2016
  5. Ramsay JonesDec 1, 2016
  6. Brandon WilliamsDec 1, 2016
  7. Philip OakleyDec 1, 2016
  8. Junio C HamanoDec 1, 2016
  9. Ramsay JonesDec 2, 2016
  10. Jeff KingDec 2, 2016
  11. Ramsay JonesDec 2, 2016
  12. 6/6 http-walker: complain about non-404 loose object errorsJeff King, Dec 1, 2016
  13. 4/6 http: make redirects more obviousJeff King, Dec 1, 2016
  14. Ramsay JonesDec 1, 2016
  15. 5/6 http: treat http-alternates like redirectsJeff King, Dec 1, 2016
  16. Brandon WilliamsDec 1, 2016
  17. Jeff KingDec 2, 2016
  18. Jeff KingDec 5, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.