Re: [PATCH] http: display the response body on POST failure
- From
Jeff King <peff@peff.net>
- Date
- May 23, 2024, 09:29 UTC
- Message-ID
- <20240523092957.GF1306938@coredump.intra.peff.net>
- In-Reply-To
- <pull.1722.git.git.1716239367046.gitgitgadget@gmail.com>
On Mon, May 20, 2024 at 09:09:26PM +0000, Boris Mbarga via GitGitGadget wrote:
Show 5 quoted lines
> From: elhmn <elhmn@github.com> > > When Git sends a GET request and receives an HTTP code indicating > failure (and that failure does not indicate an authentication problem), > it shows the body of the response, i.e. the error message.
Hmm, do we always do so? Long ago, I implemented that for the initial HTTP connection in 426e70d4a1 (remote-curl: show server content on http errors, 2013-04-05). That disables CURLOPT_FAILONERROR for that request, and then shows the result even if we saw an error.
After that, we turned off CURLOPT_FAILONERROR for all requests in e6cf87b12d (http: enable keep_error for HTTP requests, 2019-01-10), with the rationale that we'd show them with GIT_CURL_VERBOSE (and presumably GIT_TRACE_CURL, too). But do we actually write them out in most cases?
I'm not opposed to doing so, but just trying to understand what the implications might be (and whether we are really bringing POSTs in line, or if this is a new area).
In particular...
Show 9 quoted lines
> @@ -837,6 +840,8 @@ static int run_slot(struct active_request_slot *slot,
> strbuf_addch(&msg, ' ');
> strbuf_addstr(&msg, curl_errorstr);
> }
> + if (slot->errstr && slot->errstr->len)
> + error(_("%s"), slot->errstr->buf);
> }
> error(_("RPC failed; %s"), msg.buf);
> strbuf_release(&msg);If I understand correctly, slot->errstr is just the raw body content returned by the request. That _might_ be something human-readable, but it might not. For that initial connection, we have show_http_message(), which shows only messages that come back as text/plain, does some light cleanup, and shows the error with the "remote:" prefix. We'd want to use that here, I'd think?
-Peff