[BUG] Cloning with git HEAD fails for some repositories

[BUG] Cloning with git HEAD fails for some repositories

[Heinrich Schuchardt]

Initial cloning via

    git clone http://git.denx.de/u-boot-efi.git

fails with git version 2.21.0.196.g041f5ea1cf

git version 2.1.4 works fine.

Bisection points to this first bad commit:
17966c0a63d25b1cc2dd1e98d30873e643bd581f
http: avoid disconnecting on 404s for loose objects

Please, fix the regression.

$ git clone http://git.denx.de/u-boot-efi.git
Cloning into 'u-boot-efi'...
warning: alternate disabled by http.followRedirects:
http://git.denx.de/u-boot.git/
error: Unable to find a00d15757d7a513e410f15f2f910cb52333361a3 under
http://git.denx.de/u-boot-efi.git
Cannot obtain needed object a00d15757d7a513e410f15f2f910cb52333361a3
error: fetch failed.
$ git clone git://git.denx.de/u-boot-efi.git
Cloning into 'u-boot-efi'...
remote: Counting objects: 602001, done.
remote: Compressing objects: 100% (96111/96111), done.
remote: Total 602001 (delta 498315), reused 600868 (delta 497450)
Receiving objects: 100% (602001/602001), 120.33 MiB | 11.28 MiB/s, done.
Resolving deltas: 100% (498315/498315), done.
$ cd u-boot-efi/
/u-boot-efi$ git remote remove origin
/u-boot-efi$ git remote add origin http://git.denx.de/u-boot-efi.git
/u-boot-efi$ git fetch
From http://git.denx.de/u-boot-efi
 * [new branch]            efi-2019-04 -> origin/efi-2019-04
 * [new branch]            efi-2019-07 -> origin/efi-2019-07
 * [new branch]            master      -> origin/master
/u-boot-efi$

Setting

    git config --global \
    http.http://git.denx.de/u-boot-efi.git.followRedirects true

avoids the warning but does not solve the problem:

$ git clone http://git.denx.de/u-boot-efi.git
Cloning into 'u-boot-efi'...
warning: adding alternate object store: http://git.denx.de/u-boot.git/
error: Unable to find a00d15757d7a513e410f15f2f910cb52333361a3 under
http://git.denx.de/u-boot-efi.git
Cannot obtain needed object a00d15757d7a513e410f15f2f910cb52333361a3
error: fetch failed.

Best regards

Heinrich

Re: [BUG] Cloning with git HEAD fails for some repositories

[Jeff King]

On Fri, Mar 22, 2019 at 07:02:41AM +0100, Heinrich Schuchardt wrote:

> Initial cloning via
> 
>     git clone http://git.denx.de/u-boot-efi.git
> 
> fails with git version 2.21.0.196.g041f5ea1cf
> 
> git version 2.1.4 works fine.
> 
> Bisection points to this first bad commit:
> 17966c0a63d25b1cc2dd1e98d30873e643bd581f
> http: avoid disconnecting on 404s for loose objects
> 
> Please, fix the regression.

I think 17966c0a63d needed to massage the CURLcode in more places, since
missing_target() cares about it. The patch below gets me past the error
you saw, but later dies with:

  error: Request for 283d73eda8011371f12daa2cd9fc3107705637dc aborted
  error: Unable to find 283d73eda8011371f12daa2cd9fc3107705637dc under https://git.denx.de/u-boot-efi.git
  Cannot obtain needed object 283d73eda8011371f12daa2cd9fc3107705637dc
  while processing commit d9af3c87df93e1a8126b1a52adf8db978e9a0d40.
  error: fetch failed.

However, so does the parent of 17966c0a63d. So I don't know if the
u-boot-efi repo is just broken, or if there's some other different bug
at play.

-Peff

diff --git a/http-walker.c b/http-walker.c
index 48f2df4d31..83e23e787c 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -93,6 +93,9 @@ 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);
+
 	/* Use alternates if necessary */
 	if (missing_target(obj_req->req)) {
 		fetch_alternates(walker, alt->base);
@@ -189,6 +192,8 @@ static void process_alternates_response(void *callback_data)
 	char *data;
 	int i = 0;
 
+	normalize_curl_result(&slot->curl_result, slot->http_code);
+
 	if (alt_req->http_specific) {
 		if (slot->curl_result != CURLE_OK ||
 		    !alt_req->buffer->len) {
@@ -488,14 +493,7 @@ 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 == 404 && req->curl_result == CURLE_OK &&
-			(starts_with(req->url, "http://") ||
-			 starts_with(req->url, "https://")))
-		req->curl_result = CURLE_HTTP_RETURNED_ERROR;
+	normalize_curl_result(&req->curl_result, req->http_code);
 
 	if (obj_req->state == ABORTED) {
 		ret = error("Request for %s aborted", hex);
diff --git a/http.c b/http.c
index 6f12661a14..1d60a0064a 100644
--- a/http.c
+++ b/http.c
@@ -1116,16 +1116,15 @@ 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)
 {
 	/*
 	 * If we see a failing http code with CURLE_OK, we have turned off
 	 * FAILONERROR (to keep the server's custom error response), and should
 	 * translate the code into failure here.
 	 */
-	if (results->curl_result == CURLE_OK &&
-	    results->http_code >= 400) {
-		results->curl_result = CURLE_HTTP_RETURNED_ERROR;
+	if (*result == CURLE_OK && http_code >= 400) {
+		*result = CURLE_HTTP_RETURNED_ERROR;
 		/*
 		 * Normally curl will already have put the "reason phrase"
 		 * from the server into curl_errorstr; unfortunately without
@@ -1134,8 +1133,13 @@ static int handle_curl_result(struct slot_results *results)
 		 */
 		snprintf(curl_errorstr, sizeof(curl_errorstr),
 			 "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);
 
 	if (results->curl_result == CURLE_OK) {
 		credential_approve(&http_auth);
diff --git a/http.h b/http.h
index 36f558bfb3..1c75e99570 100644
--- a/http.h
+++ b/http.h
@@ -129,6 +129,12 @@ static inline int missing__target(int code, int result)
 
 #define missing_target(a) missing__target((a)->http_code, (a)->curl_result)
 
+/*
+ * Yuck, we cannot just pass the struct containing these because we store the
+ * results in various structs.
+ */
+void normalize_curl_result(CURLcode *result, long http_code);
+
 /* Helpers for modifying and creating URLs */
 extern void append_remote_object_url(struct strbuf *buf, const char *url,
 				     const char *hex,

Re: [BUG] Cloning with git HEAD fails for some repositories

[Wolfgang Denk]

Dear Jeff,

In message <20190322071231.GA26114@sigill.intra.peff.net> you wrote:
>
> However, so does the parent of 17966c0a63d. So I don't know if the
> u-boot-efi repo is just broken, or if there's some other different bug
> at play.

git fsck reports a number of dangling tags, but no other issues:

$ git fsck --strict --full
Checking object directories: 100% (256/256), done.
Checking object directories: 100% (256/256), done.
Checking objects: 100% (602062/602062), done.
Checking connectivity: 602001, done.
dangling tag 452ba0c37615d650cbbc4ce5051b4e8519b8720c
dangling tag 3d6dc0f31fc91f6263419bc954423fe648beb4a3
dangling tag 89e1e16d51cbe039c31fc4e8c86650c02e312f8c
dangling tag f3e3a1693b231fc5da7055537365fac458efc6cd
dangling tag fc41a2b55bf9c9e17cda5bd72ba187ba9196d9d2
dangling tag c4bb2215f2197b3a9db43f32bab969747d5bc8e6
dangling tag c0fee2657d53f7db7d5b0f6824497e46b06f1791
dangling tag 732ec3af41614a70816aa8afcb04a7680e23bd6e
dangling tag aedf24f63bbc92c8168aa0f0b73dd9fb0db4a1b6
dangling tag f3e6c43dcc094d8578951b08b58f4c243ffafb2a
dangling tag 979de5a0a39c596dfa210b19c85a838fa8634052
dangling tag f3b8c6c66c184a8430796cb57394afdd2e541070
dangling tag d2c8c619152019515d9489457345449755535562
dangling tag dadde645865cd3280b1fdd3b575419f4c09573f7
dangling tag 6ed5270bfcb7d38d0c8d88df03c2885462f5f40a
dangling tag 765f888560c78a7d8460fe99faf3e4cbfa61b6b0
dangling tag 3d920830d3809d3cdda14f7546b4ae8285a58842
dangling tag 80a0080837b5c62e40258413b147e8328ca170f5
dangling tag ecbe2871435ac1c56c2a8089f450b469e6a9f9cc
dangling tag 09c5a89a796d2e3d6308f9dce6050c7d4ec5d9df
dangling tag c9d3285563b2b2481d9c771ce0c72481a0af64eb
dangling tag 5483e951b4c87224edafa663cde59fc841664ebf
dangling tag 254f4a15b35151f062ac304ccf199d07910b7c2a
dangling tag 358baae41f3b5cb07aeca741e7d0d791e861a528
dangling tag 95d24ae1624e3eff49ca7d5476d7f5124c489b3a
dangling tag e154abe2a1d1ef87e2a24d2941a877417d2ca3da
dangling tag 04204d1f0467fcef54791c07933002462b954e25
dangling tag abdf4d2a98fb450377c8b45c2aa4cee7f3b3b903
dangling tag 47132ee1703118762a0a26b80586ef544eaaf43e
dangling tag 651f6f06d36957b3ced288b4111d7c6f20764094
dangling tag eb38cf862652e60fa1a3056eab6a9e8f60acb28c
dangling tag b3dd30be5ccae9567d55bcbeaad73e8b94d38ae9
dangling tag 1c511176d278dbbd826c30d6e8dfd483d6eee207
dangling tag 01a35101e2b5279b7bab7c48f80b69dce1863451
dangling tag 1abb11a56c69c935be145b76bba763d3d37b5008
dangling tag a613b2d185434171de1623e5eac2aefac4bf480b
dangling tag 8693d3dc28232d4a10a5538ad840becdd8333923
dangling tag 0f3d74c6bccd90854910781247810a8ec6b2ddc3
dangling tag 4e49f4c5c8dcb02f7920b70e6543d82cae0643c2
dangling tag 0782f493db029eae862eb21d574584d30da73d48
dangling tag f0a414959f25f7ab33cf4ba24a7ff81642e20dd5
dangling tag bd7f95c6689c0c7ed10bd1dafca9d335c239afab
dangling tag 5a1ff6e103ac132ea5d3b2ef768c035f0c83cb9e
dangling tag af4cd6c6c6584e66958bc7ef394ed629c8bb1803
dangling tag 4567960734b2f1349d79057e092c9aac03fb551d
dangling tag e9807710b1d02bce52d0fe31bfbdb2f593833689
dangling tag ba9bd7caa0d05843752fcfe6460131e8f1378606
dangling tag f6443869d551d0d5a6e20ba9970c6c5461788a44
dangling tag bf46b888957dd3c4fa91df0c1988dff7ac06e7a6
dangling tag d2521813943774ea8d0c248b97706cc4fedefb9c
dangling tag 566ab8f50a43a2fd3d2fc830df89b1545ae59532
dangling tag 15ea987e7b8596c2b20a63bc2a5e9adfff48d455
dangling tag 427c593ee160344cecb6d0da72130bc3146cf9a5
dangling tag 4a1cda6347d5db4e51e3b8c86b079abd46caee46
dangling tag a757fc49ca3df6c66f8bcde3b5bff624d764e395
dangling tag 0f6a7c7ea77c2464155aaa5deba8b9d0d5b77403
dangling tag 9219fdc80e88b08ae1f6f5dd49a82d8f63fb8d35
dangling tag 20331ed567cbdd9d55491b9bae022d07b0506d68
dangling tag 82719e3e08a3b6e0b6e309776f18798b01109f22
dangling tag 8d42df34d510ca2e948d3c670d385ee824eccf49
dangling tag a8a39f377012bda355330cf2c6b04497d2f709ef

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Houston, Tranquillity Base here.  The Eagle has landed.
                                                    -- Neil Armstrong

Re: [BUG] Cloning with git HEAD fails for some repositories

[Jeff King]

On Fri, Mar 22, 2019 at 09:21:14AM +0100, Wolfgang Denk wrote:

> Dear Jeff,
> 
> In message <20190322071231.GA26114@sigill.intra.peff.net> you wrote:
> >
> > However, so does the parent of 17966c0a63d. So I don't know if the
> > u-boot-efi repo is just broken, or if there's some other different bug
> > at play.
> 
> git fsck reports a number of dangling tags, but no other issues:

Weird. I had set http.maxrequests to "1" to give more readable output
from GIT_CURL_VERBOSE, etc. It fails with that setting, but not with the
default of 5. Which certainly seems like a bug, but one that has been
there for a while (at least since v2.9.x, which I tested).

-Peff

Re: [BUG] Cloning with git HEAD fails for some repositories

[Eric Wong]

Jeff King <peff@peff.net> wrote:
> On Fri, Mar 22, 2019 at 09:21:14AM +0100, Wolfgang Denk wrote:
> 
> > Dear Jeff,
> > 
> > In message <20190322071231.GA26114@sigill.intra.peff.net> you wrote:
> > >
> > > However, so does the parent of 17966c0a63d. So I don't know if the
> > > u-boot-efi repo is just broken, or if there's some other different bug
> > > at play.
> > 
> > git fsck reports a number of dangling tags, but no other issues:
> 
> Weird. I had set http.maxrequests to "1" to give more readable output
> from GIT_CURL_VERBOSE, etc. It fails with that setting, but not with the
> default of 5. Which certainly seems like a bug, but one that has been
> there for a while (at least since v2.9.x, which I tested).

I couldn't reproduce an error after porting your patch to
master (commit 041f5ea1cf987a40 "The third batch"):
https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw

So you might've hit an ephemeral error (bad connection,
HTTP server restarting, etc).

GIT_CURL_VERBOSE=1 git \
	-c http.maxRequests=1 -c http.followRedirects=true clone \
	http://git.denx.de/u-boot-efi.git

Re: [BUG] Cloning with git HEAD fails for some repositories

[Heinrich Schuchardt]

On 3/22/19 5:50 PM, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
>> On Fri, Mar 22, 2019 at 09:21:14AM +0100, Wolfgang Denk wrote:
>>
>>> Dear Jeff,
>>>
>>> In message <20190322071231.GA26114@sigill.intra.peff.net> you wrote:
>>>>
>>>> However, so does the parent of 17966c0a63d. So I don't know if the
>>>> u-boot-efi repo is just broken, or if there's some other different bug
>>>> at play.
>>>
>>> git fsck reports a number of dangling tags, but no other issues:
>>
>> Weird. I had set http.maxrequests to "1" to give more readable output
>> from GIT_CURL_VERBOSE, etc. It fails with that setting, but not with the
>> default of 5. Which certainly seems like a bug, but one that has been
>> there for a while (at least since v2.9.x, which I tested).
>
> I couldn't reproduce an error after porting your patch to
> master (commit 041f5ea1cf987a40 "The third batch"):
> https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw
>
> So you might've hit an ephemeral error (bad connection,
> HTTP server restarting, etc).
>
> GIT_CURL_VERBOSE=1 git \
> 	-c http.maxRequests=1 -c http.followRedirects=true clone \
> 	http://git.denx.de/u-boot-efi.git
>

I applied the patch to https://github.com/git/git origin/HEAD.

But unfortunately it does not solve the issue:

$ git --version
git version 2.21.0.197.g3845f293e6
$ git clone http://git.denx.de/u-boot-efi.git
Cloning into 'u-boot-efi'...
warning: alternate disabled by http.followRedirects:
http://git.denx.de/u-boot.git/
error: Unable to find a00d15757d7a513e410f15f2f910cb52333361a3 under
http://git.denx.de/u-boot-efi.git
Cannot obtain needed object a00d15757d7a513e410f15f2f910cb52333361a3
error: fetch failed.

In Wireshark I see a lot of '404 Not found' codes.

Best regards

Heinrich

Re: [BUG] Cloning with git HEAD fails for some repositories

[Eric Wong]

Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> On 3/22/19 5:50 PM, Eric Wong wrote:
> > I couldn't reproduce an error after porting your patch to
> > master (commit 041f5ea1cf987a40 "The third batch"):
> > https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw
> >
> > So you might've hit an ephemeral error (bad connection,
> > HTTP server restarting, etc).
> >
> > GIT_CURL_VERBOSE=1 git \
> > 	-c http.maxRequests=1 -c http.followRedirects=true clone \
> > 	http://git.denx.de/u-boot-efi.git
> >
> 
> I applied the patch to https://github.com/git/git origin/HEAD.
> 
> But unfortunately it does not solve the issue:
> 
> $ git --version
> git version 2.21.0.197.g3845f293e6
> $ git clone http://git.denx.de/u-boot-efi.git
> Cloning into 'u-boot-efi'...
> warning: alternate disabled by http.followRedirects:

It looks like you need to enable http.followRedirects
(see the command-line I used above)

> http://git.denx.de/u-boot.git/
> error: Unable to find a00d15757d7a513e410f15f2f910cb52333361a3 under
> http://git.denx.de/u-boot-efi.git
> Cannot obtain needed object a00d15757d7a513e410f15f2f910cb52333361a3
> error: fetch failed.
> 
> In Wireshark I see a lot of '404 Not found' codes.

Re: [BUG] Cloning with git HEAD fails for some repositories

[Heinrich Schuchardt]

On 3/22/19 7:09 PM, Eric Wong wrote:
> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> On 3/22/19 5:50 PM, Eric Wong wrote:
>>> I couldn't reproduce an error after porting your patch to
>>> master (commit 041f5ea1cf987a40 "The third batch"):
>>> https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw
>>>
>>> So you might've hit an ephemeral error (bad connection,
>>> HTTP server restarting, etc).
>>>
>>> GIT_CURL_VERBOSE=1 git \
>>> 	-c http.maxRequests=1 -c http.followRedirects=true clone \
>>> 	http://git.denx.de/u-boot-efi.git
>>>
>>
>> I applied the patch to https://github.com/git/git origin/HEAD.
>>
>> But unfortunately it does not solve the issue:
>>
>> $ git --version
>> git version 2.21.0.197.g3845f293e6
>> $ git clone http://git.denx.de/u-boot-efi.git
>> Cloning into 'u-boot-efi'...
>> warning: alternate disabled by http.followRedirects:
>
> It looks like you need to enable http.followRedirects
> (see the command-line I used above)
>
>> http://git.denx.de/u-boot.git/
>> error: Unable to find a00d15757d7a513e410f15f2f910cb52333361a3 under
>> http://git.denx.de/u-boot-efi.git
>> Cannot obtain needed object a00d15757d7a513e410f15f2f910cb52333361a3
>> error: fetch failed.
>>
>> In Wireshark I see a lot of '404 Not found' codes.
>

Yes, git HEAD + said patch works with this command:
git -c http.followRedirects=true clone http://git.denx.de/u-boot-efi.git

http.followRedirects is documented in man 1 git-config.

Why would git prior to  17966c0a63d25b1cc2dd1e98d30873e643bd581f~1 work
without this redirect parameter?

That following redirects is not secure is already described in
https://github.com/git/git/blob/master/Documentation/RelNotes/2.12.3.txt

Best regards

Heinrich

Re: [BUG] Cloning with git HEAD fails for some repositories

[Eric Wong]

Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Yes, git HEAD + said patch works with this command:
> git -c http.followRedirects=true clone http://git.denx.de/u-boot-efi.git
> 
> http.followRedirects is documented in man 1 git-config.
> 
> Why would git prior to  17966c0a63d25b1cc2dd1e98d30873e643bd581f~1 work
> without this redirect parameter?
> 
> That following redirects is not secure is already described in
> https://github.com/git/git/blob/master/Documentation/RelNotes/2.12.3.txt

17966c0a63d25b1cc2dd1e98d30873e643bd581f predates the
requirement to enable http.followRedirects for http-alternates:
cb4d2d35c4622ec2513c1c352d30ff8f9f9cdb9e
("http: treat http-alternates like redirects")

Re: [BUG] Cloning with git HEAD fails for some repositories

[Jeff King]

On Fri, Mar 22, 2019 at 04:50:34PM +0000, Eric Wong wrote:

> > Weird. I had set http.maxrequests to "1" to give more readable output
> > from GIT_CURL_VERBOSE, etc. It fails with that setting, but not with the
> > default of 5. Which certainly seems like a bug, but one that has been
> > there for a while (at least since v2.9.x, which I tested).
> 
> I couldn't reproduce an error after porting your patch to
> master (commit 041f5ea1cf987a40 "The third batch"):
> https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw
> 
> So you might've hit an ephemeral error (bad connection,
> HTTP server restarting, etc).

No, it was quite reproducible. Curious, I decided to bisect. The problem
started in ad75ebe5b3 (http: maintain curl sessions, 2009-11-27), but
was later fixed by your 2abc848d54 (http: always remove curl easy from
curlm session on release, 2016-09-13).

So trying to build the fix directly on 17966c0a63d (which is in between
those) will run into this unrelated bug. But forward-porting does work.

-Peff

[PATCH 0/3] fix dumb-http fetch with alternates

[Jeff King]

On Sat, Mar 23, 2019 at 04:53:41AM -0400, Jeff King wrote:

> On Fri, Mar 22, 2019 at 04:50:34PM +0000, Eric Wong wrote:
> 
> > > Weird. I had set http.maxrequests to "1" to give more readable output
> > > from GIT_CURL_VERBOSE, etc. It fails with that setting, but not with the
> > > default of 5. Which certainly seems like a bug, but one that has been
> > > there for a while (at least since v2.9.x, which I tested).
> > 
> > I couldn't reproduce an error after porting your patch to
> > master (commit 041f5ea1cf987a40 "The third batch"):
> > https://80x24.org/spew/20190322163449.25362-1-e@80x24.org/raw
> > 
> > So you might've hit an ephemeral error (bad connection,
> > HTTP server restarting, etc).
> 
> No, it was quite reproducible. Curious, I decided to bisect. The problem
> started in ad75ebe5b3 (http: maintain curl sessions, 2009-11-27), but
> was later fixed by your 2abc848d54 (http: always remove curl easy from
> curlm session on release, 2016-09-13).
> 
> So trying to build the fix directly on 17966c0a63d (which is in between
> those) will run into this unrelated bug. But forward-porting does work.

Here's the patch, forward-ported on top of the current master, with
actual commit messages, a test, and a few subtle tweaks around the
curl_errorstr handling.

  [1/3]: http: factor out curl result code normalization
  [2/3]: http: normalize curl results for dumb loose and alternates fetches
  [3/3]: http: use normalize_curl_result() instead of manual conversion

 http-walker.c              | 21 ++++++++++-----------
 http.c                     | 18 ++++++++++++------
 http.h                     |  9 +++++++++
 t/t5550-http-fetch-dumb.sh | 16 ++++++++++++++++
 4 files changed, 47 insertions(+), 17 deletions(-)

[PATCH 1/3] http: factor out curl result code normalization

[Jeff King]

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 <peff@peff.net>
---
 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,
-- 
2.21.0.705.g64cb90f133

[PATCH 2/3] http: normalize curl results for dumb loose and alternates fetches

[Jeff King]

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 <peff@peff.net>
---
 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
-- 
2.21.0.705.g64cb90f133

[PATCH 3/3] http: use normalize_curl_result() instead of manual conversion

[Jeff King]

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 <peff@peff.net>
---
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);
-- 
2.21.0.705.g64cb90f133