Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
- Date: Mon, 7 Jan 2019 15:24:19 -0800
- From: Masaya Suzuki <masayasuzuki@xxxxxxxxxx>
- Subject: Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
On Fri, Jan 4, 2019 at 2:49 AM Jeff King <peff@xxxxxxxx> wrote:
> On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:
> > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> > to stderr. However, if the response is an error response and
> > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> > won't dump the headers. Showing HTTP response headers is useful for
> > debugging, especially for non-OK responses.
> Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply
> that curl closes the handle when it sees the bad response code, and
> nobody ever gets to see the rest of the data?
curl disregards the rest of the contents when it sees a bad response
code when CURLOPT_FAILONERROR is set
So nobody gets the rest of the data.
> > This is substantially same as setting http_options.keep_error to all
> > requests. Hence, remove this option.
> The assumption here is that every code path using FAILONERROR is
> prepared to handle the failing http response codes itself (since we no
> longer set it at all in get_active_slot()). Is that so?
I was thinking that I covered the all code paths using FAILONERROR,
but it seems it's not the case. http-walker.c and http-push.c also
calls get_active_slot(). I'll narrow down the scope on removing
> Anything that uses handle_curl_result() is OK. That means run_one_slot()
> is fine, which in turn covers run_slot() for RPCs, and http_request()
> for normal one-at-a-time requests. But what about the parallel multiple
> requests issued by the dumb-http walker code?
Right. I'll keep FAILONERROR in get_active_slot and remove it only for
the code paths that can handle HTTP errors.
> There I think we end up in step_active_slots(), which calls into
> finish_active_slot() for completed requests. I think that
> unconditionally fetches the http code without bothering to look at
> whether curl reported success or not.
> So I _think_ that's probably all of the users of the curl handles
> provided by get_active_slot(). Though given the tangled mess of our HTTP
> code, I won't be surprised if there's a corner case I missed in that