Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)

[Jeff King <peff@peff.net>]

On Mon, Apr 15, 2013 at 07:25:32PM -0400, Jeff King wrote:

> On Mon, Apr 15, 2013 at 01:28:53PM -0700, Junio C Hamano wrote:
> 
> > * jk/http-error-messages (2013-04-06) 9 commits
> [...]
> ...the tip of your current master does not currently pass the test
> suite[1]. I bisected the problem to "show server content on http
> errors" from the above topic, but haven't figure it out past that. I
> typically run "make test" before submitting, so I'm guessing it is an
> interaction with another topic that graduated around the same time
> (though it's also possible that I just failed to test after the last
> rebase).

This patch on top of jk/http-error-messages fixes it.

-- >8 --
Subject: [PATCH] http: set curl FAILONERROR each time we select a handle

Because we reuse curl handles for multiple requests, the
setup of a handle happens in two stages: stable, global
setup and per-request setup. The lifecycle of a handle is
something like:

  1. get_curl_handle; do basic global setup that will last
     through the whole program (e.g., setting the user
     agent, ssl options, etc)

  2. get_active_slot; set up a per-request baseline (e.g.,
     clearing the read/write functions, making it a GET
     request, etc)

  3. perform the request with curl_*_perform functions

  4. goto step 2 to perform another request

Breaking it down this way means we can avoid doing global
setup from step (1) repeatedly, but we still finish step (2)
with a predictable baseline setup that callers can rely on.

Until commit 6d052d7 (http: add HTTP_KEEP_ERROR option,
2013-04-05), setting curl's FAILONERROR option was a global
setup; we never changed it. However, 6d052d7 introduced in
option where some requests might turn off FAILONERROR. Later
requests using the same handle would have the option
unexpectedly turned off, which meant they would not notice
http failures at all.

This could easily be seen in the test-suite for the
"half-auth" cases of t5541 and t5551. The initial requests
turned off FAILONERROR, which meant it was erroneously off
for the rpc POST. That worked fine for a successful request,
but meant that we failed to react properly to the HTTP 401
(instead, we treated whatever the server handed us as a
successful message body).

The solution is simple: now that FAILONERROR is a
per-request setting, we move it to get_active_slot to make
sure it is reset for each request.

Signed-off-by: Jeff King <peff@peff.net>
---
Hmph. I have no idea how this ever passed the tests, so I can only
assume that I screwed up in running them. I even recall considering this
issue while writing the patches, but I mixed up which of get_curl_handle
and get_active_slot it needed to be in when I did so.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 58c063c..48d4ff6 100644
--- a/http.c
+++ b/http.c
@@ -282,7 +282,6 @@ static CURL *get_curl_handle(void)
 #endif
 	if (ssl_cainfo != NULL)
 		curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
-	curl_easy_setopt(result, CURLOPT_FAILONERROR, 1);
 
 	if (curl_low_speed_limit > 0 && curl_low_speed_time > 0) {
 		curl_easy_setopt(result, CURLOPT_LOW_SPEED_LIMIT,
@@ -506,6 +505,7 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
 	if (http_auth.password)
 		init_curl_http_auth(slot->curl);
 
-- 
1.8.2.8.g44e4c28