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:

quoted

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.

quoted

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