Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR

From: Masaya Suzuki <masayasuzuki@google.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
Date: Mon, 7 Jan 2019 15:24:19 -0800	[thread overview]
Message-ID: <CAJB1erVjLwnKqy5zSMTBNySSUrJXiiL8Xrpw9puh7o4CDaeA2Q@mail.gmail.com> (raw)
In-Reply-To: <20190104104907.GC26185@sigill.intra.peff.net>

On Fri, Jan 4, 2019 at 2:49 AM Jeff King <peff@peff.net> 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
(https://github.com/curl/curl/blob/dea3f94298ac0859464768959488938c4e104545/lib/http.c#L3691).
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
FAILONERROR.

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