Re: What's cooking in git.git (Apr 2013, #05; Mon, 15)
- From
Jeff King <peff@peff.net>
- Date
- Apr 16, 2013, 00:30 UTC
- Message-ID
- <20130416003038.GA5336@sigill.intra.peff.net>
- In-Reply-To
- <20130415232532.GA7134@sigill.intra.peff.net>
On Mon, Apr 15, 2013 at 07:25:32PM -0400, Jeff King wrote:
Show 11 quoted lines
> 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