git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
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

  reply	other threads:[~2019-01-07 23:24 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-28  1:47 [PATCH 1/2] Change how HTTP response body is returned Masaya Suzuki
2018-12-28  1:47 ` [PATCH 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2018-12-28 19:36   ` Eric Sunshine
2018-12-28 19:51     ` Masaya Suzuki
2018-12-28 19:58       ` Eric Sunshine
2018-12-28 20:00         ` Masaya Suzuki
2018-12-29 19:44 ` [PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2018-12-29 19:44   ` [PATCH v2 1/2] Change how HTTP response body is returned Masaya Suzuki
2019-01-03 19:09     ` Junio C Hamano
2019-01-04 10:11       ` Jeff King
2019-01-04 20:13         ` Junio C Hamano
2019-01-04 10:30     ` Jeff King
2018-12-29 19:44   ` [PATCH v2 2/2] Unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-04 10:49     ` Jeff King
2019-01-07 23:24       ` Masaya Suzuki [this message]
2019-01-08  2:47   ` [PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-09 12:15       ` SZEDER Gábor
2019-01-08  2:47     ` [PATCH v3 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-08  2:47     ` [PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 19:33     ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 2/5] http: enable keep_error for HTTP requests Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR Masaya Suzuki
2019-01-10 19:33       ` [PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error Masaya Suzuki
2019-01-10 23:06       ` [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJB1erVjLwnKqy5zSMTBNySSUrJXiiL8Xrpw9puh7o4CDaeA2Q@mail.gmail.com \
    --to=masayasuzuki@google.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).