Web lists-archives.com

Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR




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?

> 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?

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?

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
analysis.

-Peff