[3/2] remote-curl: add Transfer-Encoding header only for older curl

Commit Message

Jeff King April 5, 2024, 8:04 p.m. UTC

On Tue, Apr 02, 2024 at 04:02:54PM -0400, Jeff King wrote:

> Ultimately the issue will be fixed by moving to a different version of
> libcurl, but here's an easy workaround in the meantime, with a small doc
> cleanup I found along the way.
> 
>   [1/2]: http: reset POSTFIELDSIZE when clearing curl handle
>   [2/2]: INSTALL: bump libcurl version to 7.21.3

Here's a final patch on top that gives us a workaround for the HTTP/2
half of this. It's a cleaned-up version of what I showed earlier.

-- >8 --
Subject: remote-curl: add Transfer-Encoding header only for older curl

As of curl 7.66.0, we don't need to manually specify a "chunked"
Transfer-Encoding header. Instead, modern curl deduces the need for it
in a POST that has a POSTFIELDSIZE of -1 and uses READFUNCTION rather
than POSTFIELDS.

That version is recent enough that we can't just drop the header; we
need to do so conditionally. Since it's only a single line, it seems
like the simplest thing would just be to keep setting it unconditionally
(after all, the #ifdefs are much longer than the actual code). But
there's another wrinkle: HTTP/2.

Curl may choose to use HTTP/2 under the hood if the server supports it.
And in that protocol, we do not use the chunked encoding for streaming
at all. Most versions of curl handle this just fine by recognizing and
removing the header. But there's a regression in curl 8.7.0 and 8.7.1
where it doesn't, and large requests over HTTP/2 are broken (which t5559
notices). That regression has since been fixed upstream, but not yet
released.

Make the setting of this header conditional, which will let Git work
even with those buggy curl versions. And as a bonus, it serves as a
reminder that we can eventually clean up the code as we bump the
supported curl versions.

Signed-off-by: Jeff King <[e-mail]>
---
Manually tested against curl 8.7.0 (where skipping the header fixes
t5559), as well as 7.65.0 (which continues to pass t5551, but would fail
if we didn't set the header).

 git-curl-compat.h | 9 +++++++++
 remote-curl.c     | 3 +++
 2 files changed, 12 insertions(+)

Comments

Daniel Stenberg April 5, 2024, 9:30 p.m. UTC | #1

On Fri, 5 Apr 2024, Jeff King wrote:

> Here's a final patch on top that gives us a workaround for the HTTP/2
> half of this. It's a cleaned-up version of what I showed earlier.

+1 from me.

(and appologies for this regression, but you know...)

Junio C Hamano April 5, 2024, 9:49 p.m. UTC | #2

Jeff King <[e-mail]> writes:

> ... And as a bonus, it serves as a
> reminder that we can eventually clean up the code as we bump the
> supported curl versions.

;-).

The way git-curl-compat.h lays out various features/workarounds and
the lowest version that needs them makes it very easy to do that.

Thanks, will queue.

Patch

diff --git a/git-curl-compat.h b/git-curl-compat.h
index fd96b3cdff..e1d0bdd273 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,6 +126,15 @@ 
 #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
 #endif
 
+/**
+ * Versions before curl 7.66.0 (September 2019) required manually setting the
+ * transfer-encoding for a streaming POST; after that this is handled
+ * automatically.
+ */
+#if LIBCURL_VERSION_NUM < 0x074200
+#define GIT_CURL_NEED_TRANSFER_ENCODING_HEADER
+#endif
+
 /**
  * CURLOPT_PROTOCOLS_STR and CURLOPT_REDIR_PROTOCOLS_STR were added in 7.85.0,
  * released in August 2022.
diff --git a/remote-curl.c b/remote-curl.c
index 31b02b8840..0b6d7815fd 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1,4 +1,5 @@ 
 #include "git-compat-util.h"
+#include "git-curl-compat.h"
 #include "config.h"
 #include "environment.h"
 #include "gettext.h"
@@ -955,7 +956,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.
 		 */
+#ifdef GIT_CURL_NEED_TRANSFER_ENCODING_HEADER
 		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);