From 894221d2af0e2d218c5ce0a9e8246eadd3710fc7 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 19 Mar 2025 18:23:46 -0400 Subject: [PATCH 1/4] http.c: remove unnecessary casts to long When parsing 'http.lowSpeedLimit' and 'http.lowSpeedTime', we explicitly cast the result of 'git_config_int()' to a long before assignment. This cast has been in place since all the way back in 58e60dd203 (Add support for pushing to a remote repository using HTTP/DAV, 2005-11-02). But that cast has always been unnecessary, since long is guaranteed to be at least as wide as int. Let's drop the cast accordingly. Noticed-by: Patrick Steinhardt Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- http.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/http.c b/http.c index 0c9a872809..0cbcb079b2 100644 --- a/http.c +++ b/http.c @@ -438,11 +438,11 @@ static int http_options(const char *var, const char *value, return 0; } if (!strcmp("http.lowspeedlimit", var)) { - curl_low_speed_limit = (long)git_config_int(var, value, ctx->kvi); + curl_low_speed_limit = git_config_int(var, value, ctx->kvi); return 0; } if (!strcmp("http.lowspeedtime", var)) { - curl_low_speed_time = (long)git_config_int(var, value, ctx->kvi); + curl_low_speed_time = git_config_int(var, value, ctx->kvi); return 0; } From 572795cff930f11b1566f4f3e47fa9fa33772d1f Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 19 Mar 2025 18:23:50 -0400 Subject: [PATCH 2/4] http.c: introduce `set_long_from_env()` for convenience In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c gained a new "set_from_env()" function as a convenience function around conditionally assigning an environment variable to some variable if and only if the environment variable was set to begin with. But prior to 7059cd99fc, there were two spots which need to first strtol() whatever is set in the environment before assigning it to a long pointer. Both instances stored the result of getenv() in a temporary variable, and conditionally strtol() it depending on whether or not getenv() returned NULL. Replace those two instances with a new cousin of 'set_from_env()' called 'set_long_from_env()', which does what its name suggests. This allows us to remove the temporary variables and clean up some minor code duplication while also adding more robust error handling. More importantly, however, it prepares us for a future commit which will introduce more instances of assigning an environment variable to a long. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- http.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 0cbcb079b2..17b676a1d5 100644 --- a/http.c +++ b/http.c @@ -1256,10 +1256,30 @@ static void set_from_env(char **var, const char *envname) } } +static void set_long_from_env(long *var, const char *envname) +{ + const char *val = getenv(envname); + if (val) { + long tmp; + char *endp; + int saved_errno = errno; + + errno = 0; + tmp = strtol(val, &endp, 10); + + if (errno) + warning_errno(_("failed to parse %s"), envname); + else if (*endp || endp == val) + warning(_("failed to parse %s"), envname); + else + *var = tmp; + + errno = saved_errno; + } +} + void http_init(struct remote *remote, const char *url, int proactive_auth) { - char *low_speed_limit; - char *low_speed_time; char *normalized_url; struct urlmatch_config config = URLMATCH_CONFIG_INIT; @@ -1338,12 +1358,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) set_from_env(&user_agent, "GIT_HTTP_USER_AGENT"); - low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT"); - if (low_speed_limit) - curl_low_speed_limit = strtol(low_speed_limit, NULL, 10); - low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME"); - if (low_speed_time) - curl_low_speed_time = strtol(low_speed_time, NULL, 10); + set_long_from_env(&curl_low_speed_limit, "GIT_HTTP_LOW_SPEED_LIMIT"); + set_long_from_env(&curl_low_speed_time, "GIT_HTTP_LOW_SPEED_TIME"); if (curl_ssl_verify == -1) curl_ssl_verify = 1; From bfdd2591b013ec029b861c1da619da50a28f3887 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 19 Mar 2025 18:23:53 -0400 Subject: [PATCH 3/4] http.c: inline `set_curl_keepalive()` At the end of `get_curl_handle()` we call `set_curl_keepalive()` to enable TCP keepalive probes on our CURL handle. `set_curl_keepalive()` dates back to 47ce115370 (http: use curl's tcp keepalive if available, 2013-10-14), which conditionally compiled different variants of `set_curl_keepalive()` depending on what version of curl we were compiled with[^1]. As of f7c094060c (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), we no longer conditionally compile `set_curl_keepalive()` since we no longer support pre-7.25.0 versions of curl. But the version of that function that we kept is really just a thin wrapper around setting the TCP_KEEPALIVE option, so there's no reason to keep it in its own function. Inline the definition of `set_curl_keepalive()` to within `get_curl_handle()` so that the setup of our CURL handle is self-contained. [1]: The details are spelled out in 47ce115370, but the gist is curl 7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions that predate even that option do nothing. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- http.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/http.c b/http.c index 17b676a1d5..b4267bfdb0 100644 --- a/http.c +++ b/http.c @@ -704,10 +704,6 @@ static int has_proxy_cert_password(void) return 1; } -static void set_curl_keepalive(CURL *c) -{ - curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1); -} /* Return 1 if redactions have been made, 0 otherwise. */ static int redact_sensitive_header(struct strbuf *header, size_t offset) @@ -1242,7 +1238,7 @@ static CURL *get_curl_handle(void) } init_curl_proxy_auth(result); - set_curl_keepalive(result); + curl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1); return result; } From 46e6f9af3ec063529738f4b5b0b97c28c005c365 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Wed, 19 Mar 2025 18:23:56 -0400 Subject: [PATCH 4/4] http.c: allow custom TCP keepalive behavior via config curl supports a few options to control when and how often it should instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and KEEPCNT. Until this point, there hasn't been a way for users to change what values are used for these options, forcing them to rely on curl's defaults. But we do unconditionally enable TCP keepalives without giving users an ability to tweak any fine-grained parameters. Ordinarily this isn't a problem, particularly for users that have fast-enough connections, and/or are talking to a server that has generous or nonexistent thresholds for killing a connection it hasn't heard from in a while. But it can present a problem when one or both of those assumptions fail. For instance, I can reliably get an in-progress clone to be killed from the remote end when cloning from some forges while using trickle to limit my clone's bandwidth. For those users and others who wish to more finely tune the OS's keepalive behavior, expose configuration and environment variables which allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options. Note that while KEEPIDLE and KEEPINTVL were added in curl 7.25.0, KEEPCNT was added much more recently in curl 8.9.0. Per f7c094060c (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), both KEEPIDLE and KEEPINTVL are set unconditionally. But since we may be compiled with a curl that isn't as new as 8.9.0, only set KEEPCNT when we have CURLOPT_TCP_KEEPCNT to begin with. Signed-off-by: Taylor Blau Acked-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/config/http.adoc | 18 ++++++++++++++++++ git-curl-compat.h | 7 +++++++ http.c | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc index 22a8803dea..67393282fa 100644 --- a/Documentation/config/http.adoc +++ b/Documentation/config/http.adoc @@ -296,6 +296,24 @@ http.lowSpeedLimit, http.lowSpeedTime:: Can be overridden by the `GIT_HTTP_LOW_SPEED_LIMIT` and `GIT_HTTP_LOW_SPEED_TIME` environment variables. +http.keepAliveIdle:: + Specifies how long in seconds to wait on an idle connection + before sending TCP keepalive probes (if supported by the OS). If + unset, curl's default value is used. Can be overridden by the + `GIT_HTTP_KEEPALIVE_IDLE` environment variable. + +http.keepAliveInterval:: + Specifies how long in seconds to wait between TCP keepalive + probes (if supported by the OS). If unset, curl's default value + is used. Can be overridden by the `GIT_HTTP_KEEPALIVE_INTERVAL` + environment variable. + +http.keepAliveCount:: + Specifies how many TCP keepalive probes to send before giving up + and terminating the connection (if supported by the OS). If + unset, curl's default value is used. Can be overridden by the + `GIT_HTTP_KEEPALIVE_COUNT` environment variable. + http.noEPSV:: A boolean which disables using of EPSV ftp command by curl. This can be helpful with some "poor" ftp servers which don't diff --git a/git-curl-compat.h b/git-curl-compat.h index 703756ba85..aa8eed7ed2 100644 --- a/git-curl-compat.h +++ b/git-curl-compat.h @@ -45,4 +45,11 @@ #define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1 #endif +/** + * CURLOPT_TCP_KEEPCNT was added in 8.9.0, released in July, 2024. + */ +#if LIBCURL_VERSION_NUM >= 0x080900 +#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT +#endif + #endif diff --git a/http.c b/http.c index b4267bfdb0..d21e3a3bad 100644 --- a/http.c +++ b/http.c @@ -104,6 +104,10 @@ static struct { }; #endif +static long curl_tcp_keepidle = -1; +static long curl_tcp_keepintvl = -1; +static long curl_tcp_keepcnt = -1; + enum proactive_auth { PROACTIVE_AUTH_NONE = 0, PROACTIVE_AUTH_IF_CREDENTIALS, @@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value, return 0; } + if (!strcmp("http.keepaliveidle", var)) { + curl_tcp_keepidle = git_config_int(var, value, ctx->kvi); + return 0; + } + if (!strcmp("http.keepaliveinterval", var)) { + curl_tcp_keepintvl = git_config_int(var, value, ctx->kvi); + return 0; + } + if (!strcmp("http.keepalivecount", var)) { + curl_tcp_keepcnt = git_config_int(var, value, ctx->kvi); + return 0; + } + /* Fall back on the default ones */ return git_default_config(var, value, ctx, data); } @@ -704,7 +721,6 @@ static int has_proxy_cert_password(void) return 1; } - /* Return 1 if redactions have been made, 0 otherwise. */ static int redact_sensitive_header(struct strbuf *header, size_t offset) { @@ -1240,6 +1256,17 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1); + if (curl_tcp_keepidle > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPIDLE, + curl_tcp_keepidle); + if (curl_tcp_keepintvl > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL, + curl_tcp_keepintvl); +#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT + if (curl_tcp_keepcnt > -1) + curl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt); +#endif + return result; } @@ -1382,6 +1409,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth) ssl_cert_password_required = 1; } + set_long_from_env(&curl_tcp_keepidle, "GIT_TCP_KEEPIDLE"); + set_long_from_env(&curl_tcp_keepintvl, "GIT_TCP_KEEPINTVL"); + set_long_from_env(&curl_tcp_keepcnt, "GIT_TCP_KEEPCNT"); + curl_default = get_curl_handle(); }