From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Daniel Stenberg <daniel@haxx.se>
Subject: Re: [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle
Date: Tue, 2 Apr 2024 23:20:45 -0400 [thread overview]
Message-ID: <20240403032045.GA1559972@coredump.intra.peff.net> (raw)
In-Reply-To: <20240402200517.GA875182@coredump.intra.peff.net>
On Tue, Apr 02, 2024 at 04:05:17PM -0400, Jeff King wrote:
> So everything works, but we're better off resetting the size manually
> for a few reasons:
>
> - there was a regression in curl 8.7.0 where the chunked header
> detection didn't kick in, causing any large HTTP requests made by
> Git to fail. This has since been fixed (but not yet released). In
> the issue, curl folks recommended setting it explicitly to -1:
>
> https://github.com/curl/curl/issues/13229#issuecomment-2029826058
>
> and it indeed works around the regression. So even though it won't
> be strictly necessary after the fix there, this will help folks who
> end up using the affected libcurl versions.
Hmph. This isn't quite enough to make things work with 8.7.0, because
there are two things wrong there:
1. curl uses the leftover POSTFIELDSIZE, which this patch fixes. So
HTTP/1.1 is fine (including t5551).
2. In HTTP/2 mode, it sends chunked data, even though HTTP/2 does not
use "Transfer-Encoding: chunked" (it correctly does not send the
header, but it puts unnecessary chunk framing around the data).
t5559 (which covers HTTP/2) fails, and so does accessing HTTP/2
capable hosts like github.com (though score another win for t5559;
it has introduced some hassles, but it diagnosed a real problem we
would not have otherwise seen in the test suite).
This second problem can be fixed by eliminating the manual
transfer-encoding header, which is what's confusing curl's HTTP/2 code.
But as I said earlier...
> Note that the recommendation in the curl issue is to actually drop the
> manual Transfer-Encoding header. Modern libcurl will add the header
> itself when streaming from a READFUNCTION. However, that code wasn't
> added until 802aa5ae2 (HTTP: use chunked Transfer-Encoding for HTTP_POST
> if size unknown, 2019-07-22), which is in curl 7.66.0. We claim to
> support back to 7.19.5, so those older versions still need the manual
> header.
...we can't do so unconditionally. So we'd need something like:
diff --git a/remote-curl.c b/remote-curl.c
index 31b02b8840..215bcc6e10 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -955,7 +955,9 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
/* The request body is large and the size cannot be predicted.
* We must use chunked encoding to send it.
*/
+#if LIBCURL_VERSION_NUM < 0x074200
headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+#endif
rpc->initial_buffer = 1;
curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
I was hoping not to have to carry any version-specific cruft (especially
since newer versions of curl should fix the regression). But it's not
too bad, and it actually marks the code as something that we can ditch
in the future when that version of curl becomes obsolete.
I can try to prepare a cleaner patch for it tomorrow, but comments
welcome in the meantime.
-Peff
next prev parent reply other threads:[~2024-04-03 3:20 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 0:02 tests broken with curl-8.7.0 Jeff King
2024-03-30 8:54 ` Daniel Stenberg
2024-04-02 20:02 ` [PATCH 0/2] git+curl 8.7.0 workaround Jeff King
2024-04-02 20:05 ` [PATCH 1/2] http: reset POSTFIELDSIZE when clearing curl handle Jeff King
2024-04-02 20:27 ` Junio C Hamano
2024-04-03 3:20 ` Jeff King [this message]
2024-04-03 6:30 ` Patrick Steinhardt
2024-04-03 6:34 ` Patrick Steinhardt
2024-04-03 20:18 ` Jeff King
2024-04-02 20:06 ` [PATCH 2/2] INSTALL: bump libcurl version to 7.21.3 Jeff King
2024-04-02 20:21 ` [PATCH 0/2] git+curl 8.7.0 workaround rsbecker
2024-04-02 20:31 ` Jeff King
2024-04-05 20:04 ` [PATCH 3/2] remote-curl: add Transfer-Encoding header only for older curl Jeff King
2024-04-05 21:30 ` Daniel Stenberg
2024-04-05 21:49 ` 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240403032045.GA1559972@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=daniel@haxx.se \
--cc=git@vger.kernel.org \
/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.
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).