On Wed, Mar 19, 2025 at 3:37 PM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> Here's a reroll of my series to introduce new http.* knobs to control
> curl's TCP keepalive behavior.
>
> This reroll is mostly minor, with the notable differences being limited
> to:
>
> - Added error handling in the new http.c::set_long_from_env().
>
> - Removed unnecessary casts from int -> long.
>
> - Only set CURLOPT_TCP_KEEPCNT when compiled with a version of curl
> that knows about that option to begin with (>8.9.0).
>
> As usual, a range-diff is included below for convenience. Thanks again
> for reviewing!
>
> == Original cover letter
>
> This short series introduces a few new http.* configuration options to
> control curl's behavior around TCP keepalive packets. The details are
> spelled out in the final patch, but the gist is:
>
> - http.keepAliveIdle specifies how long in seconds to wait on an idle
> connection before beginning to send keepalive packets.
>
> - http.keepAliveInterval does the same but controls the interval
> between successive keepalive packets.
>
> - http.keepAliveCount specifies how many keepalive packets to send
> before closing down the connection.
>
> The first two commits of the series are general code clean-up of a
> couple of small things I noticed while reading through the http.c code,
> and the final patch implements these new options.
>
> I couldn't think of a great way to test these new configuration
> settings, and given the simplicity of the final patch I opted for no
> tests there. But if someone has a good idea of how to test this
> behavior, please let me know.
>
> In either case, thanks in advance for your review!
>
> Taylor Blau (4):
> http.c: remove unnecessary casts to long
> http.c: introduce `set_long_from_env()` for convenience
> http.c: inline `set_curl_keepalive()`
> http.c: allow custom TCP keepalive behavior via config
>
> Documentation/config/http.adoc | 18 ++++++++
> git-curl-compat.h | 7 ++++
> http.c | 75 ++++++++++++++++++++++++++--------
> 3 files changed, 84 insertions(+), 16 deletions(-)
>
> Range-diff against v1:
> -: ---------- > 1: 204e5e18d2 http.c: remove unnecessary casts to long
> 1: ba22a121fa ! 2: 2e39a78e87 http.c: introduce `set_long_from_env()` for convenience
> @@ Commit message
> 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. More importantly, however, it prepares us for a future
> - commit which will introduce more instances of assigning an environment
> - variable to a long.
> + 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 <me@xxxxxxxxxxxx>
>
> @@ http.c: 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)
> -+ *var = strtol(val, NULL, 10);
> ++ 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)
> 2: a05269552f = 3: cdfc9baa8d http.c: inline `set_curl_keepalive()`
> 3: d840415808 ! 4: 3fe62181e5 http.c: allow custom TCP keepalive behavior via config
> @@ Commit message
> 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 <me@xxxxxxxxxxxx>
>
> ## Documentation/config/http.adoc ##
> @@ Documentation/config/http.adoc: http.lowSpeedLimit, http.lowSpeedTime::
> A boolean which disables using of EPSV ftp command by curl.
> This can be helpful with some "poor" ftp servers which don't
>
> + ## git-curl-compat.h ##
> +@@
> + #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
> +
> ## http.c ##
> @@ http.c: static struct {
> };
> @@ http.c: static int http_options(const char *var, const char *value,
> }
>
> + if (!strcmp("http.keepaliveidle", var)) {
> -+ curl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);
> ++ curl_tcp_keepidle = git_config_int(var, value, ctx->kvi);
> + return 0;
> + }
> + if (!strcmp("http.keepaliveinterval", var)) {
> -+ curl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);
> ++ curl_tcp_keepintvl = git_config_int(var, value, ctx->kvi);
> + return 0;
> + }
> + if (!strcmp("http.keepalivecount", var)) {
> -+ curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);
> ++ curl_tcp_keepcnt = git_config_int(var, value, ctx->kvi);
> + return 0;
> + }
> +
> @@ http.c: static CURL *get_curl_handle(void)
> + 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;
> }
>
> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
> --
> 2.49.0.4.ge59cf92f8d
The new patch and range-diff look good to me.
