git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Masaya Suzuki <masayasuzuki@google.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com, sunshine@sunshineco.com
Subject: Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR
Date: Fri, 4 Jan 2019 05:49:07 -0500	[thread overview]
Message-ID: <20190104104907.GC26185@sigill.intra.peff.net> (raw)
In-Reply-To: <20181229194447.157763-3-masayasuzuki@google.com>

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

  reply	other threads:[~2019-01-04 10:49 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 [this message]
2019-01-07 23:24       ` Masaya Suzuki
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=20190104104907.GC26185@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=masayasuzuki@google.com \
    --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).