Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR

From: Jeff King <peff@peff.net>
Date: Jan 4, 2019, 10:49 UTC
Message-ID: <20190104104907.GC26185@sigill.intra.peff.net>
In-Reply-To: <20181229194447.157763-3-masayasuzuki@google.com>

On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:

Show 5 quoted lines

> 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