[PATCH 0/3] http: support fine-tuning curl's keepalive behavior

[PATCH 0/3] http: support fine-tuning curl's keepalive behavior

[Taylor Blau]

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 (3):
  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 ++++++++++++
 http.c                         | 54 +++++++++++++++++++++++++---------
 2 files changed, 58 insertions(+), 14 deletions(-)


base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.49.0.3.gbb7a4a684c.dirty

[PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience

[Taylor Blau]

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. 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@ttaylorr.com>
---
 http.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index 0c9a872809..be564fd520 100644
--- a/http.c
+++ b/http.c
@@ -1256,10 +1256,15 @@ 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);
+}
+
 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 +1343,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;
-- 
2.49.0.3.gbb7a4a684c.dirty

[PATCH 2/3] http.c: inline `set_curl_keepalive()`

[Taylor Blau]

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 <me@ttaylorr.com>
---
 http.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/http.c b/http.c
index be564fd520..526f9680f9 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;
 }
-- 
2.49.0.3.gbb7a4a684c.dirty

[PATCH 3/3] http.c: allow custom TCP keepalive behavior via config

[Taylor Blau]

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.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/http.adoc | 18 ++++++++++++++++++
 http.c                         | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 48 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/http.c b/http.c
index 526f9680f9..c13c7da530 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 = (long)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);
+		return 0;
+	}
+	if (!strcmp("http.keepalivecount", var)) {
+		curl_tcp_keepcnt = (long)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,15 @@ 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);
+	if (curl_tcp_keepcnt > -1)
+		curl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);
+
 	return result;
 }
 
@@ -1367,6 +1392,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();
 }
 
-- 
2.49.0.3.gbb7a4a684c.dirty

Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience

[Elijah Newren]

On Tue, Mar 18, 2025 at 3:21 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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. 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@ttaylorr.com>
> ---
>  http.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/http.c b/http.c
> index 0c9a872809..be564fd520 100644
> --- a/http.c
> +++ b/http.c
> @@ -1256,10 +1256,15 @@ 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);
> +}
> +
>  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 +1343,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;
> --
> 2.49.0.3.gbb7a4a684c.dirty

No behavioral changes, just the use of a new convenience function;
simple enough.

Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience

[Patrick Steinhardt]

On Tue, Mar 18, 2025 at 06:21:34PM -0400, Taylor Blau wrote:
> 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. More importantly, however, it prepares us for a future
> commit which will introduce more instances of assigning an environment
> variable to a long.

Okay, makes sense.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  http.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/http.c b/http.c
> index 0c9a872809..be564fd520 100644
> --- a/http.c
> +++ b/http.c
> @@ -1256,10 +1256,15 @@ 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);
> +}

Hm. We don't perform any error checking at all for whether or not the
value of the environment variable is a valid integer. This isn't a new
issue introduced by your patch, but now that we have a central place
where it's being parsed I wonder whether we should be checking for
errors?

Patrick

Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config

[Patrick Steinhardt]

On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:
> 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.

Does this mean that our defaults are insufficient, as well? It's nice to
add a way to adapt those settings for the future, but ideally no user
should ever have to manually tweak them and Git should work out of the
box.

> diff --git a/http.c b/http.c
> index 526f9680f9..c13c7da530 100644
> --- a/http.c
> +++ b/http.c
> @@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,
>  		return 0;
>  	}
>  
> +	if (!strcmp("http.keepaliveidle", var)) {
> +		curl_tcp_keepidle = (long)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);
> +		return 0;
> +	}
> +	if (!strcmp("http.keepalivecount", var)) {
> +		curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);
> +		return 0;
> +	}
> +
>  	/* Fall back on the default ones */
>  	return git_default_config(var, value, ctx, data);
>  }

Are the casts really necessary? The compiler shouldn't complain when
promoting from `int` to `long`.

Patrick

Re: [PATCH 2/3] http.c: inline `set_curl_keepalive()`

[Elijah Newren]

On Tue, Mar 18, 2025 at 3:21 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> 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 <me@ttaylorr.com>
> ---
>  http.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/http.c b/http.c
> index be564fd520..526f9680f9 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;
>  }
> --
> 2.49.0.3.gbb7a4a684c.dirty

In contrast to the last patch, this simply dispenses with rather than
adds a convenience function.  That makes sense in this case, since it
was only a single line of code anyway.

Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config

[Elijah Newren]

On Wed, Mar 19, 2025 at 9:00 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:
> > 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.
>
> Does this mean that our defaults are insufficient, as well? It's nice to
> add a way to adapt those settings for the future, but ideally no user
> should ever have to manually tweak them and Git should work out of the
> box.

Was going to comment with the same question.

Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience

[Taylor Blau]

On Wed, Mar 19, 2025 at 05:00:43PM +0100, Patrick Steinhardt wrote:
> > diff --git a/http.c b/http.c
> > index 0c9a872809..be564fd520 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -1256,10 +1256,15 @@ 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);
> > +}
>
> Hm. We don't perform any error checking at all for whether or not the
> value of the environment variable is a valid integer. This isn't a new
> issue introduced by your patch, but now that we have a central place
> where it's being parsed I wonder whether we should be checking for
> errors?

Yeah, I guess it's technically not "new" in the sense that we were
already doing:

    xyz = getenv("XYZ");
    if (xyz)
        *var = strtol(xyz, NULL, 10);

I suppose we could do something like:

--- 8< ---
diff --git a/http.c b/http.c
index c13c7da530..6b01ad7a53 100644
--- a/http.c
+++ b/http.c
@@ -1280,8 +1280,20 @@ 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;
+		errno = 0;
+		tmp = strtol(val, &endp, 10);
+		if (errno)
+			warning_errno(_("failed to parse '%s' (%s) as long"),
+				      envname, val);
+		else if (endp == val)
+			warning(_("failed to parse '%s' (%s) as long"), envname,
+				val);
+		else
+			*var = tmp;
+	}
 }

 void http_init(struct remote *remote, const char *url, int proactive_auth)
--- >8 ---

On top, but TBH I'm not sure how much value it adds. This is only used
for reading GIT_XYZ variables out of the environment, and we're already
pretty lax about strtol() errors in other places. Since this isn't the
interface we expect users to use, I'm OK to punt on it for now unless
you feel strongly otherwise.

Thanks,
Taylor

Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config

[Taylor Blau]

On Wed, Mar 19, 2025 at 05:00:51PM +0100, Patrick Steinhardt wrote:
> On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:
> > 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.
>
> Does this mean that our defaults are insufficient, as well? It's nice to
> add a way to adapt those settings for the future, but ideally no user
> should ever have to manually tweak them and Git should work out of the
> box.

No; the defaults are sufficient for most users. It's the users that have
extremely slow connections and/or are talking to hosts that have very
short timeouts for inactive TCP connections that would want to change
these values.

> > diff --git a/http.c b/http.c
> > index 526f9680f9..c13c7da530 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,
> >  		return 0;
> >  	}
> >
> > +	if (!strcmp("http.keepaliveidle", var)) {
> > +		curl_tcp_keepidle = (long)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);
> > +		return 0;
> > +	}
> > +	if (!strcmp("http.keepalivecount", var)) {
> > +		curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);
> > +		return 0;
> > +	}
> > +
> >  	/* Fall back on the default ones */
> >  	return git_default_config(var, value, ctx, data);
> >  }
>
> Are the casts really necessary? The compiler shouldn't complain when
> promoting from `int` to `long`.

No, they're not. They match the style of the rest of this file, but we
shouldn't have explicit casts anywhere. I'll send a new round with an
additional patch that drops the unnecessary casts, too.

Thanks,
Taylor

[PATCH v2 0/4] http: support fine-tuning curl's keepalive behavior

[Taylor Blau]

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@ttaylorr.com>
     
    @@ 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@ttaylorr.com>
     
      ## 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

[PATCH v2 1/4] http.c: remove unnecessary casts to long

[Taylor Blau]

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 <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 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;
 	}
 
-- 
2.49.0.4.ge59cf92f8d

[PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience

[Taylor Blau]

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 <me@ttaylorr.com>
---
 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;
-- 
2.49.0.4.ge59cf92f8d

[PATCH v2 3/4] http.c: inline `set_curl_keepalive()`

[Taylor Blau]

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 <me@ttaylorr.com>
---
 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;
 }
-- 
2.49.0.4.ge59cf92f8d

[PATCH v2 4/4] http.c: allow custom TCP keepalive behavior via config

[Taylor Blau]

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 <me@ttaylorr.com>
---
 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();
 }
 
-- 
2.49.0.4.ge59cf92f8d

Re: [PATCH v2 0/4] http: support fine-tuning curl's keepalive behavior

[Elijah Newren]

On Wed, Mar 19, 2025 at 3:37 PM Taylor Blau <me@ttaylorr.com> 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@ttaylorr.com>
>
>     @@ 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@ttaylorr.com>
>
>       ## 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.

Re: [PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience

[Patrick Steinhardt]

On Wed, Mar 19, 2025 at 06:23:50PM -0400, Taylor Blau wrote:
> 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;
> +	}
> +}

The `saved_errno` dance feels a bit unnecessary, but other than that I'm
okay with this approach of only printing a warning instead of dying
right away.

The other patches look good to me, too, thanks!

Patrick

Re: [PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience

[Jeff King]

On Wed, Mar 19, 2025 at 06:23:50PM -0400, Taylor Blau wrote:

> +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;
> +	}
> +}

If we are going to add error checking, should it be reusing the existing
code in parse.[ch]? As a bonus, that would support units like "K" (which
could perhaps be useful for LOW_SPEED_LIMIT).

We'd need to extend that code for "long" (it has wrappers for "int" and
"unsigned long", but not "long). But that seems like a good thing
overall. Something like:

diff --git a/http.c b/http.c
index 0c9a872809..a40e45e9cb 100644
--- a/http.c
+++ b/http.c
@@ -1256,10 +1256,15 @@ 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 && !git_parse_long(val, var))
+		warning(_("failed to parse %s"), envname);
+}
+
 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 +1343,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;
diff --git a/parse.c b/parse.c
index 7a60a4f816..4d9758132e 100644
--- a/parse.c
+++ b/parse.c
@@ -116,6 +116,15 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 	return 1;
 }
 
+int git_parse_long(const char *value, long *ret)
+{
+	intmax_t tmp;
+	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long)))
+		return 0;
+	*ret = tmp;
+	return 1;
+}
+
 int git_parse_ssize_t(const char *value, ssize_t *ret)
 {
 	intmax_t tmp;
diff --git a/parse.h b/parse.h
index 6bb9a54d9a..0e536536a2 100644
--- a/parse.h
+++ b/parse.h
@@ -4,6 +4,7 @@
 int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
 int git_parse_ssize_t(const char *, ssize_t *);
 int git_parse_ulong(const char *, unsigned long *);
+int git_parse_long(const char *, long *);
 int git_parse_int(const char *value, int *ret);
 int git_parse_int64(const char *value, int64_t *ret);
 int git_parse_double(const char *value, double *ret);

You could even add git_env_long() to match the existing git_env_ulong(),
although:

  - that family of functions calls die() on error, rather than warning

  - the calling convention is a bit different; it always assigns the
    result, but takes a default value. So you'd need to pass in the
    existing value, which is a little awkward:

      curl_low_speed_limit = git_env_long("GIT_HTTP_LOW_SPEED_LIMIT",
				          curl_low_speed_limit);

-Peff

Re: [PATCH v2 3/4] http.c: inline `set_curl_keepalive()`

[Jeff King]

On Wed, Mar 19, 2025 at 06:23:53PM -0400, Taylor Blau wrote:

> 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.

As the author of 47ce115370, I approve. This ideally would have been
cleaned up as part of the series with f7c094060c, but sometimes it's
hard to see these simplifications when you're focused on getting rid of
the complicated bits. Better late than never.

-Peff

Re: [PATCH v2 4/4] http.c: allow custom TCP keepalive behavior via config

[Jeff King]

On Wed, Mar 19, 2025 at 06:23:56PM -0400, Taylor Blau wrote:

> 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.

OK. I have never wanted those myself, but I have no problem with other
people using them. ;)

The implementation mostly looks as I'd expect, but...

> +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.

...while this environment variable (and its siblings) makes sense to
me....

> @@ -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");

...we seem to use different names in the code.

I think the ones mentioning HTTP make the most sense, to match the
config (and since this only affects curl).

-Peff