{"thread":{"id":"63158","subject":"[PATCH 0/3] http: support fine-tuning curl's keepalive behavior","startedAt":"2025-03-18T22:21:32Z","lastAt":"2025-04-01T09:16:08Z","messageCount":21,"participants":["Taylor Blau","Elijah Newren","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"514561","messageId":"cover.1742336481.git.me@ttaylorr.com","threadId":"63158","inReplyTo":null,"subject":"[PATCH 0/3] http: support fine-tuning curl's keepalive behavior","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-18T22:21:24Z","receivedAt":"2025-03-18T22:21:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series introduces a few new http.* configuration options to\ncontrol curl's behavior around TCP keepalive packets. The details are\nspelled out in the final patch, but the gist is:\n\n  - http.keepAliveIdle specifies how long in seconds to wait on an idle\n    connection before beginning to send keepalive packets.\n\n  - http.keepAliveInterval does the same but controls the interval\n    between successive keepalive packets.\n\n  - http.keepAliveCount specifies how many keepalive packets to send\n    before closing down the connection.\n\nThe first two commits of the series are general code clean-up of a\ncouple of small things I noticed while reading through the http.c code,\nand the final patch implements these new options.\n\nI couldn't think of a great way to test these new configuration\nsettings, and given the simplicity of the final patch I opted for no\ntests there. But if someone has a good idea of how to test this\nbehavior, please let me know.\n\nIn either case, thanks in advance for your review!\n\nTaylor Blau (3):\n  http.c: introduce `set_long_from_env()` for convenience\n  http.c: inline `set_curl_keepalive()`\n  http.c: allow custom TCP keepalive behavior via config\n\n Documentation/config/http.adoc | 18 ++++++++++++\n http.c                         | 54 +++++++++++++++++++++++++---------\n 2 files changed, 58 insertions(+), 14 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.49.0.3.gbb7a4a684c.dirty\n"},{"id":"514562","messageId":"ba22a121fa699e490de00eba988552b6c10fe2fd.1742336481.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742336481.git.me@ttaylorr.com","subject":"[PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-18T22:21:34Z","receivedAt":"2025-03-18T22:21:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c\ngained a new \"set_from_env()\" function as a convenience function around\nconditionally assigning an environment variable to some variable if and\nonly if the environment variable was set to begin with.\n\nBut prior to 7059cd99fc, there were two spots which need to first\nstrtol() whatever is set in the environment before assigning it to a\nlong pointer. Both instances stored the result of getenv() in a\ntemporary variable, and conditionally strtol() it depending on whether\nor not getenv() returned NULL.\n\nReplace those two instances with a new cousin of 'set_from_env()' called\n'set_long_from_env()', which does what its name suggests. This allows us\nto remove the temporary variables and clean up some minor code\nduplication. More importantly, however, it prepares us for a future\ncommit which will introduce more instances of assigning an environment\nvariable to a long.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n http.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0c9a872809..be564fd520 100644\n--- a/http.c\n+++ b/http.c\n@@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)\n \t}\n }\n \n+static void set_long_from_env(long *var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val)\n+\t\t*var = strtol(val, NULL, 10);\n+}\n+\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n-\tchar *low_speed_limit;\n-\tchar *low_speed_time;\n \tchar *normalized_url;\n \tstruct urlmatch_config config = URLMATCH_CONFIG_INIT;\n \n@@ -1338,12 +1343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \n \tset_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n \n-\tlow_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n-\tif (low_speed_limit)\n-\t\tcurl_low_speed_limit = strtol(low_speed_limit, NULL, 10);\n-\tlow_speed_time = getenv(\"GIT_HTTP_LOW_SPEED_TIME\");\n-\tif (low_speed_time)\n-\t\tcurl_low_speed_time = strtol(low_speed_time, NULL, 10);\n+\tset_long_from_env(&curl_low_speed_limit, \"GIT_HTTP_LOW_SPEED_LIMIT\");\n+\tset_long_from_env(&curl_low_speed_time, \"GIT_HTTP_LOW_SPEED_TIME\");\n \n \tif (curl_ssl_verify == -1)\n \t\tcurl_ssl_verify = 1;\n-- \n2.49.0.3.gbb7a4a684c.dirty\n\n"},{"id":"514563","messageId":"a05269552fc2c17519b935c3b9c279c2e231c5c5.1742336481.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742336481.git.me@ttaylorr.com","subject":"[PATCH 2/3] http.c: inline `set_curl_keepalive()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-18T22:21:38Z","receivedAt":"2025-03-18T22:21:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"At the end of `get_curl_handle()` we call `set_curl_keepalive()` to\nenable TCP keepalive probes on our CURL handle. `set_curl_keepalive()`\ndates back to 47ce115370 (http: use curl's tcp keepalive if available,\n2013-10-14), which conditionally compiled different variants of\n`set_curl_keepalive()` depending on what version of curl we were\ncompiled with[^1].\n\nAs of f7c094060c (git-curl-compat: remove check for curl 7.25.0,\n2024-10-23), we no longer conditionally compile `set_curl_keepalive()`\nsince we no longer support pre-7.25.0 versions of curl. But the version\nof that function that we kept is really just a thin wrapper around\nsetting the TCP_KEEPALIVE option, so there's no reason to keep it in its\nown function.\n\nInline the definition of `set_curl_keepalive()` to within\n`get_curl_handle()` so that the setup of our CURL handle is\nself-contained.\n\n[1]: The details are spelled out in 47ce115370, but the gist is curl\n  7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use\n  CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions\n  that predate even that option do nothing.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n http.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex be564fd520..526f9680f9 100644\n--- a/http.c\n+++ b/http.c\n@@ -704,10 +704,6 @@ static int has_proxy_cert_password(void)\n \treturn 1;\n }\n \n-static void set_curl_keepalive(CURL *c)\n-{\n-\tcurl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);\n-}\n \n /* Return 1 if redactions have been made, 0 otherwise. */\n static int redact_sensitive_header(struct strbuf *header, size_t offset)\n@@ -1242,7 +1238,7 @@ static CURL *get_curl_handle(void)\n \t}\n \tinit_curl_proxy_auth(result);\n \n-\tset_curl_keepalive(result);\n+\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);\n \n \treturn result;\n }\n-- \n2.49.0.3.gbb7a4a684c.dirty\n\n"},{"id":"514564","messageId":"d84041580895a653648ee2370e21d7d2aa4fc4bb.1742336481.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742336481.git.me@ttaylorr.com","subject":"[PATCH 3/3] http.c: allow custom TCP keepalive behavior via config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-18T22:21:41Z","receivedAt":"2025-03-18T22:21:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"curl supports a few options to control when and how often it should\ninstruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and\nKEEPCNT. Until this point, there hasn't been a way for users to change\nwhat values are used for these options, forcing them to rely on curl's\ndefaults.\n\nBut we do unconditionally enable TCP keepalives without giving users an\nability to tweak any fine-grained parameters. Ordinarily this isn't a\nproblem, particularly for users that have fast-enough connections,\nand/or are talking to a server that has generous or nonexistent\nthresholds for killing a connection it hasn't heard from in a while.\n\nBut it can present a problem when one or both of those assumptions fail.\nFor instance, I can reliably get an in-progress clone to be killed from\nthe remote end when cloning from some forges while using trickle to\nlimit my clone's bandwidth.\n\nFor those users and others who wish to more finely tune the OS's\nkeepalive behavior, expose configuration and environment variables which\nallow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/http.adoc | 18 ++++++++++++++++++\n http.c                         | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 48 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc\nindex 22a8803dea..67393282fa 100644\n--- a/Documentation/config/http.adoc\n+++ b/Documentation/config/http.adoc\n@@ -296,6 +296,24 @@ http.lowSpeedLimit, http.lowSpeedTime::\n \tCan be overridden by the `GIT_HTTP_LOW_SPEED_LIMIT` and\n \t`GIT_HTTP_LOW_SPEED_TIME` environment variables.\n \n+http.keepAliveIdle::\n+\tSpecifies how long in seconds to wait on an idle connection\n+\tbefore sending TCP keepalive probes (if supported by the OS). If\n+\tunset, curl's default value is used. Can be overridden by the\n+\t`GIT_HTTP_KEEPALIVE_IDLE` environment variable.\n+\n+http.keepAliveInterval::\n+\tSpecifies how long in seconds to wait between TCP keepalive\n+\tprobes (if supported by the OS). If unset, curl's default value\n+\tis used. Can be overridden by the `GIT_HTTP_KEEPALIVE_INTERVAL`\n+\tenvironment variable.\n+\n+http.keepAliveCount::\n+\tSpecifies how many TCP keepalive probes to send before giving up\n+\tand terminating the connection (if supported by the OS). If\n+\tunset, curl's default value is used. Can be overridden by the\n+\t`GIT_HTTP_KEEPALIVE_COUNT` environment variable.\n+\n http.noEPSV::\n \tA boolean which disables using of EPSV ftp command by curl.\n \tThis can be helpful with some \"poor\" ftp servers which don't\ndiff --git a/http.c b/http.c\nindex 526f9680f9..c13c7da530 100644\n--- a/http.c\n+++ b/http.c\n@@ -104,6 +104,10 @@ static struct {\n };\n #endif\n \n+static long curl_tcp_keepidle = -1;\n+static long curl_tcp_keepintvl = -1;\n+static long curl_tcp_keepcnt = -1;\n+\n enum proactive_auth {\n \tPROACTIVE_AUTH_NONE = 0,\n \tPROACTIVE_AUTH_IF_CREDENTIALS,\n@@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.keepaliveidle\", var)) {\n+\t\tcurl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(\"http.keepaliveinterval\", var)) {\n+\t\tcurl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(\"http.keepalivecount\", var)) {\n+\t\tcurl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, ctx, data);\n }\n@@ -704,7 +721,6 @@ static int has_proxy_cert_password(void)\n \treturn 1;\n }\n \n-\n /* Return 1 if redactions have been made, 0 otherwise. */\n static int redact_sensitive_header(struct strbuf *header, size_t offset)\n {\n@@ -1240,6 +1256,15 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);\n \n+\tif (curl_tcp_keepidle > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPIDLE,\n+\t\t\t\t curl_tcp_keepidle);\n+\tif (curl_tcp_keepintvl > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL,\n+\t\t\t\t curl_tcp_keepintvl);\n+\tif (curl_tcp_keepcnt > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);\n+\n \treturn result;\n }\n \n@@ -1367,6 +1392,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\t\tssl_cert_password_required = 1;\n \t}\n \n+\tset_long_from_env(&curl_tcp_keepidle, \"GIT_TCP_KEEPIDLE\");\n+\tset_long_from_env(&curl_tcp_keepintvl, \"GIT_TCP_KEEPINTVL\");\n+\tset_long_from_env(&curl_tcp_keepcnt, \"GIT_TCP_KEEPCNT\");\n+\n \tcurl_default = get_curl_handle();\n }\n \n-- \n2.49.0.3.gbb7a4a684c.dirty\n"},{"id":"514653","messageId":"CABPp-BGcsVAbaAj628ACGb=cgMaUBH0LO316tj9pJsj6=WrH8w@mail.gmail.com","threadId":"63158","inReplyTo":"ba22a121fa699e490de00eba988552b6c10fe2fd.1742336481.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-19T16:00:17Z","receivedAt":"2025-03-19T16:00:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 18, 2025 at 3:21 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c\n> gained a new \"set_from_env()\" function as a convenience function around\n> conditionally assigning an environment variable to some variable if and\n> only if the environment variable was set to begin with.\n>\n> But prior to 7059cd99fc, there were two spots which need to first\n> strtol() whatever is set in the environment before assigning it to a\n> long pointer. Both instances stored the result of getenv() in a\n> temporary variable, and conditionally strtol() it depending on whether\n> or not getenv() returned NULL.\n>\n> Replace those two instances with a new cousin of 'set_from_env()' called\n> 'set_long_from_env()', which does what its name suggests. This allows us\n> to remove the temporary variables and clean up some minor code\n> duplication. More importantly, however, it prepares us for a future\n> commit which will introduce more instances of assigning an environment\n> variable to a long.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  http.c | 17 +++++++++--------\n>  1 file changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 0c9a872809..be564fd520 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)\n>         }\n>  }\n>\n> +static void set_long_from_env(long *var, const char *envname)\n> +{\n> +       const char *val = getenv(envname);\n> +       if (val)\n> +               *var = strtol(val, NULL, 10);\n> +}\n> +\n>  void http_init(struct remote *remote, const char *url, int proactive_auth)\n>  {\n> -       char *low_speed_limit;\n> -       char *low_speed_time;\n>         char *normalized_url;\n>         struct urlmatch_config config = URLMATCH_CONFIG_INIT;\n>\n> @@ -1338,12 +1343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n>\n>         set_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n>\n> -       low_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n> -       if (low_speed_limit)\n> -               curl_low_speed_limit = strtol(low_speed_limit, NULL, 10);\n> -       low_speed_time = getenv(\"GIT_HTTP_LOW_SPEED_TIME\");\n> -       if (low_speed_time)\n> -               curl_low_speed_time = strtol(low_speed_time, NULL, 10);\n> +       set_long_from_env(&curl_low_speed_limit, \"GIT_HTTP_LOW_SPEED_LIMIT\");\n> +       set_long_from_env(&curl_low_speed_time, \"GIT_HTTP_LOW_SPEED_TIME\");\n>\n>         if (curl_ssl_verify == -1)\n>                 curl_ssl_verify = 1;\n> --\n> 2.49.0.3.gbb7a4a684c.dirty\n\nNo behavioral changes, just the use of a new convenience function;\nsimple enough.\n"},{"id":"514654","messageId":"Z9rqK8QSs2dA7t6S@pks.im","threadId":"63158","inReplyTo":"ba22a121fa699e490de00eba988552b6c10fe2fd.1742336481.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-19T16:00:43Z","receivedAt":"2025-03-19T16:00:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 18, 2025 at 06:21:34PM -0400, Taylor Blau wrote:\n> In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c\n> gained a new \"set_from_env()\" function as a convenience function around\n> conditionally assigning an environment variable to some variable if and\n> only if the environment variable was set to begin with.\n> \n> But prior to 7059cd99fc, there were two spots which need to first\n> strtol() whatever is set in the environment before assigning it to a\n> long pointer. Both instances stored the result of getenv() in a\n> temporary variable, and conditionally strtol() it depending on whether\n> or not getenv() returned NULL.\n> \n> Replace those two instances with a new cousin of 'set_from_env()' called\n> 'set_long_from_env()', which does what its name suggests. This allows us\n> to remove the temporary variables and clean up some minor code\n> duplication. More importantly, however, it prepares us for a future\n> commit which will introduce more instances of assigning an environment\n> variable to a long.\n\nOkay, makes sense.\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  http.c | 17 +++++++++--------\n>  1 file changed, 9 insertions(+), 8 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index 0c9a872809..be564fd520 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)\n>  \t}\n>  }\n>  \n> +static void set_long_from_env(long *var, const char *envname)\n> +{\n> +\tconst char *val = getenv(envname);\n> +\tif (val)\n> +\t\t*var = strtol(val, NULL, 10);\n> +}\n\nHm. We don't perform any error checking at all for whether or not the\nvalue of the environment variable is a valid integer. This isn't a new\nissue introduced by your patch, but now that we have a central place\nwhere it's being parsed I wonder whether we should be checking for\nerrors?\n\nPatrick\n"},{"id":"514655","messageId":"Z9rqMxlt1F7lbGk7@pks.im","threadId":"63158","inReplyTo":"d84041580895a653648ee2370e21d7d2aa4fc4bb.1742336481.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-19T16:00:51Z","receivedAt":"2025-03-19T16:00:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:\n> curl supports a few options to control when and how often it should\n> instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and\n> KEEPCNT. Until this point, there hasn't been a way for users to change\n> what values are used for these options, forcing them to rely on curl's\n> defaults.\n> \n> But we do unconditionally enable TCP keepalives without giving users an\n> ability to tweak any fine-grained parameters. Ordinarily this isn't a\n> problem, particularly for users that have fast-enough connections,\n> and/or are talking to a server that has generous or nonexistent\n> thresholds for killing a connection it hasn't heard from in a while.\n> \n> But it can present a problem when one or both of those assumptions fail.\n> For instance, I can reliably get an in-progress clone to be killed from\n> the remote end when cloning from some forges while using trickle to\n> limit my clone's bandwidth.\n\nDoes this mean that our defaults are insufficient, as well? It's nice to\nadd a way to adapt those settings for the future, but ideally no user\nshould ever have to manually tweak them and Git should work out of the\nbox.\n\n> diff --git a/http.c b/http.c\n> index 526f9680f9..c13c7da530 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tif (!strcmp(\"http.keepaliveidle\", var)) {\n> +\t\tcurl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);\n> +\t\treturn 0;\n> +\t}\n> +\tif (!strcmp(\"http.keepaliveinterval\", var)) {\n> +\t\tcurl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);\n> +\t\treturn 0;\n> +\t}\n> +\tif (!strcmp(\"http.keepalivecount\", var)) {\n> +\t\tcurl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \t/* Fall back on the default ones */\n>  \treturn git_default_config(var, value, ctx, data);\n>  }\n\nAre the casts really necessary? The compiler shouldn't complain when\npromoting from `int` to `long`.\n\nPatrick\n"},{"id":"514656","messageId":"CABPp-BEpVhP3rKXAVVZ4MUSsUZUF=YeqA23fnp0-h2S-Hk4bEw@mail.gmail.com","threadId":"63158","inReplyTo":"a05269552fc2c17519b935c3b9c279c2e231c5c5.1742336481.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] http.c: inline `set_curl_keepalive()`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-19T16:01:22Z","receivedAt":"2025-03-19T16:01:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Mar 18, 2025 at 3:21 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> At the end of `get_curl_handle()` we call `set_curl_keepalive()` to\n> enable TCP keepalive probes on our CURL handle. `set_curl_keepalive()`\n> dates back to 47ce115370 (http: use curl's tcp keepalive if available,\n> 2013-10-14), which conditionally compiled different variants of\n> `set_curl_keepalive()` depending on what version of curl we were\n> compiled with[^1].\n>\n> As of f7c094060c (git-curl-compat: remove check for curl 7.25.0,\n> 2024-10-23), we no longer conditionally compile `set_curl_keepalive()`\n> since we no longer support pre-7.25.0 versions of curl. But the version\n> of that function that we kept is really just a thin wrapper around\n> setting the TCP_KEEPALIVE option, so there's no reason to keep it in its\n> own function.\n>\n> Inline the definition of `set_curl_keepalive()` to within\n> `get_curl_handle()` so that the setup of our CURL handle is\n> self-contained.\n>\n> [1]: The details are spelled out in 47ce115370, but the gist is curl\n>   7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use\n>   CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions\n>   that predate even that option do nothing.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  http.c | 6 +-----\n>  1 file changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index be564fd520..526f9680f9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -704,10 +704,6 @@ static int has_proxy_cert_password(void)\n>         return 1;\n>  }\n>\n> -static void set_curl_keepalive(CURL *c)\n> -{\n> -       curl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);\n> -}\n>\n>  /* Return 1 if redactions have been made, 0 otherwise. */\n>  static int redact_sensitive_header(struct strbuf *header, size_t offset)\n> @@ -1242,7 +1238,7 @@ static CURL *get_curl_handle(void)\n>         }\n>         init_curl_proxy_auth(result);\n>\n> -       set_curl_keepalive(result);\n> +       curl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);\n>\n>         return result;\n>  }\n> --\n> 2.49.0.3.gbb7a4a684c.dirty\n\nIn contrast to the last patch, this simply dispenses with rather than\nadds a convenience function.  That makes sense in this case, since it\nwas only a single line of code anyway.\n"},{"id":"514659","messageId":"CABPp-BHsaELBSV-mB=h2e2YY9NQvLfV5fU2XTmOsPSsub-HT8A@mail.gmail.com","threadId":"63158","inReplyTo":"Z9rqMxlt1F7lbGk7@pks.im","subject":"Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-19T16:15:45Z","receivedAt":"2025-03-19T16:15:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 19, 2025 at 9:00 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:\n> > curl supports a few options to control when and how often it should\n> > instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and\n> > KEEPCNT. Until this point, there hasn't been a way for users to change\n> > what values are used for these options, forcing them to rely on curl's\n> > defaults.\n> >\n> > But we do unconditionally enable TCP keepalives without giving users an\n> > ability to tweak any fine-grained parameters. Ordinarily this isn't a\n> > problem, particularly for users that have fast-enough connections,\n> > and/or are talking to a server that has generous or nonexistent\n> > thresholds for killing a connection it hasn't heard from in a while.\n> >\n> > But it can present a problem when one or both of those assumptions fail.\n> > For instance, I can reliably get an in-progress clone to be killed from\n> > the remote end when cloning from some forges while using trickle to\n> > limit my clone's bandwidth.\n>\n> Does this mean that our defaults are insufficient, as well? It's nice to\n> add a way to adapt those settings for the future, but ideally no user\n> should ever have to manually tweak them and Git should work out of the\n> box.\n\nWas going to comment with the same question.\n"},{"id":"514672","messageId":"Z9sGzWNU5Gn5c/+8@nand.local","threadId":"63158","inReplyTo":"Z9rqK8QSs2dA7t6S@pks.im","subject":"Re: [PATCH 1/3] http.c: introduce `set_long_from_env()` for convenience","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T18:02:53Z","receivedAt":"2025-03-19T18:02:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 19, 2025 at 05:00:43PM +0100, Patrick Steinhardt wrote:\n> > diff --git a/http.c b/http.c\n> > index 0c9a872809..be564fd520 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)\n> >  \t}\n> >  }\n> >\n> > +static void set_long_from_env(long *var, const char *envname)\n> > +{\n> > +\tconst char *val = getenv(envname);\n> > +\tif (val)\n> > +\t\t*var = strtol(val, NULL, 10);\n> > +}\n>\n> Hm. We don't perform any error checking at all for whether or not the\n> value of the environment variable is a valid integer. This isn't a new\n> issue introduced by your patch, but now that we have a central place\n> where it's being parsed I wonder whether we should be checking for\n> errors?\n\nYeah, I guess it's technically not \"new\" in the sense that we were\nalready doing:\n\n    xyz = getenv(\"XYZ\");\n    if (xyz)\n        *var = strtol(xyz, NULL, 10);\n\nI suppose we could do something like:\n\n--- 8< ---\ndiff --git a/http.c b/http.c\nindex c13c7da530..6b01ad7a53 100644\n--- a/http.c\n+++ b/http.c\n@@ -1280,8 +1280,20 @@ static void set_from_env(char **var, const char *envname)\n static void set_long_from_env(long *var, const char *envname)\n {\n \tconst char *val = getenv(envname);\n-\tif (val)\n-\t\t*var = strtol(val, NULL, 10);\n+\tif (val) {\n+\t\tlong tmp;\n+\t\tchar *endp;\n+\t\terrno = 0;\n+\t\ttmp = strtol(val, &endp, 10);\n+\t\tif (errno)\n+\t\t\twarning_errno(_(\"failed to parse '%s' (%s) as long\"),\n+\t\t\t\t      envname, val);\n+\t\telse if (endp == val)\n+\t\t\twarning(_(\"failed to parse '%s' (%s) as long\"), envname,\n+\t\t\t\tval);\n+\t\telse\n+\t\t\t*var = tmp;\n+\t}\n }\n\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n--- >8 ---\n\nOn top, but TBH I'm not sure how much value it adds. This is only used\nfor reading GIT_XYZ variables out of the environment, and we're already\npretty lax about strtol() errors in other places. Since this isn't the\ninterface we expect users to use, I'm OK to punt on it for now unless\nyou feel strongly otherwise.\n\nThanks,\nTaylor\n"},{"id":"514673","messageId":"Z9sHgz0kgb2bYVXQ@nand.local","threadId":"63158","inReplyTo":"Z9rqMxlt1F7lbGk7@pks.im","subject":"Re: [PATCH 3/3] http.c: allow custom TCP keepalive behavior via config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T18:05:55Z","receivedAt":"2025-03-19T18:05:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 19, 2025 at 05:00:51PM +0100, Patrick Steinhardt wrote:\n> On Tue, Mar 18, 2025 at 06:21:41PM -0400, Taylor Blau wrote:\n> > curl supports a few options to control when and how often it should\n> > instruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and\n> > KEEPCNT. Until this point, there hasn't been a way for users to change\n> > what values are used for these options, forcing them to rely on curl's\n> > defaults.\n> >\n> > But we do unconditionally enable TCP keepalives without giving users an\n> > ability to tweak any fine-grained parameters. Ordinarily this isn't a\n> > problem, particularly for users that have fast-enough connections,\n> > and/or are talking to a server that has generous or nonexistent\n> > thresholds for killing a connection it hasn't heard from in a while.\n> >\n> > But it can present a problem when one or both of those assumptions fail.\n> > For instance, I can reliably get an in-progress clone to be killed from\n> > the remote end when cloning from some forges while using trickle to\n> > limit my clone's bandwidth.\n>\n> Does this mean that our defaults are insufficient, as well? It's nice to\n> add a way to adapt those settings for the future, but ideally no user\n> should ever have to manually tweak them and Git should work out of the\n> box.\n\nNo; the defaults are sufficient for most users. It's the users that have\nextremely slow connections and/or are talking to hosts that have very\nshort timeouts for inactive TCP connections that would want to change\nthese values.\n\n> > diff --git a/http.c b/http.c\n> > index 526f9680f9..c13c7da530 100644\n> > --- a/http.c\n> > +++ b/http.c\n> > @@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,\n> >  \t\treturn 0;\n> >  \t}\n> >\n> > +\tif (!strcmp(\"http.keepaliveidle\", var)) {\n> > +\t\tcurl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);\n> > +\t\treturn 0;\n> > +\t}\n> > +\tif (!strcmp(\"http.keepaliveinterval\", var)) {\n> > +\t\tcurl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);\n> > +\t\treturn 0;\n> > +\t}\n> > +\tif (!strcmp(\"http.keepalivecount\", var)) {\n> > +\t\tcurl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> >  \t/* Fall back on the default ones */\n> >  \treturn git_default_config(var, value, ctx, data);\n> >  }\n>\n> Are the casts really necessary? The compiler shouldn't complain when\n> promoting from `int` to `long`.\n\nNo, they're not. They match the style of the rest of this file, but we\nshouldn't have explicit casts anywhere. I'll send a new round with an\nadditional patch that drops the unnecessary casts, too.\n\nThanks,\nTaylor\n"},{"id":"514683","messageId":"cover.1742423021.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742336481.git.me@ttaylorr.com","subject":"[PATCH v2 0/4] http: support fine-tuning curl's keepalive behavior","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:23:42Z","receivedAt":"2025-03-19T22:23:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here's a reroll of my series to introduce new http.* knobs to control\ncurl's TCP keepalive behavior.\n\nThis reroll is mostly minor, with the notable differences being limited\nto:\n\n  - Added error handling in the new http.c::set_long_from_env().\n\n  - Removed unnecessary casts from int -> long.\n\n  - Only set CURLOPT_TCP_KEEPCNT when compiled with a version of curl\n    that knows about that option to begin with (>8.9.0).\n\nAs usual, a range-diff is included below for convenience. Thanks again\nfor reviewing!\n\n== Original cover letter\n\nThis short series introduces a few new http.* configuration options to\ncontrol curl's behavior around TCP keepalive packets. The details are\nspelled out in the final patch, but the gist is:\n\n  - http.keepAliveIdle specifies how long in seconds to wait on an idle\n    connection before beginning to send keepalive packets.\n\n  - http.keepAliveInterval does the same but controls the interval\n    between successive keepalive packets.\n\n  - http.keepAliveCount specifies how many keepalive packets to send\n    before closing down the connection.\n\nThe first two commits of the series are general code clean-up of a\ncouple of small things I noticed while reading through the http.c code,\nand the final patch implements these new options.\n\nI couldn't think of a great way to test these new configuration\nsettings, and given the simplicity of the final patch I opted for no\ntests there. But if someone has a good idea of how to test this\nbehavior, please let me know.\n\nIn either case, thanks in advance for your review!\n\nTaylor Blau (4):\n  http.c: remove unnecessary casts to long\n  http.c: introduce `set_long_from_env()` for convenience\n  http.c: inline `set_curl_keepalive()`\n  http.c: allow custom TCP keepalive behavior via config\n\n Documentation/config/http.adoc | 18 ++++++++\n git-curl-compat.h              |  7 ++++\n http.c                         | 75 ++++++++++++++++++++++++++--------\n 3 files changed, 84 insertions(+), 16 deletions(-)\n\nRange-diff against v1:\n-:  ---------- > 1:  204e5e18d2 http.c: remove unnecessary casts to long\n1:  ba22a121fa ! 2:  2e39a78e87 http.c: introduce `set_long_from_env()` for convenience\n    @@ Commit message\n         Replace those two instances with a new cousin of 'set_from_env()' called\n         'set_long_from_env()', which does what its name suggests. This allows us\n         to remove the temporary variables and clean up some minor code\n    -    duplication. More importantly, however, it prepares us for a future\n    -    commit which will introduce more instances of assigning an environment\n    -    variable to a long.\n    +    duplication while also adding more robust error handling.\n    +\n    +    More importantly, however, it prepares us for a future commit which will\n    +    introduce more instances of assigning an environment variable to a long.\n     \n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n    @@ http.c: static void set_from_env(char **var, const char *envname)\n     +static void set_long_from_env(long *var, const char *envname)\n     +{\n     +\tconst char *val = getenv(envname);\n    -+\tif (val)\n    -+\t\t*var = strtol(val, NULL, 10);\n    ++\tif (val) {\n    ++\t\tlong tmp;\n    ++\t\tchar *endp;\n    ++\t\tint saved_errno = errno;\n    ++\n    ++\t\terrno = 0;\n    ++\t\ttmp = strtol(val, &endp, 10);\n    ++\n    ++\t\tif (errno)\n    ++\t\t\twarning_errno(_(\"failed to parse %s\"), envname);\n    ++\t\telse if (*endp || endp == val)\n    ++\t\t\twarning(_(\"failed to parse %s\"), envname);\n    ++\t\telse\n    ++\t\t\t*var = tmp;\n    ++\n    ++\t\terrno = saved_errno;\n    ++\t}\n     +}\n     +\n      void http_init(struct remote *remote, const char *url, int proactive_auth)\n2:  a05269552f = 3:  cdfc9baa8d http.c: inline `set_curl_keepalive()`\n3:  d840415808 ! 4:  3fe62181e5 http.c: allow custom TCP keepalive behavior via config\n    @@ Commit message\n         keepalive behavior, expose configuration and environment variables which\n         allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.\n     \n    +    Note that while KEEPIDLE and KEEPINTVL were added in curl 7.25.0,\n    +    KEEPCNT was added much more recently in curl 8.9.0. Per f7c094060c\n    +    (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), both\n    +    KEEPIDLE and KEEPINTVL are set unconditionally. But since we may be\n    +    compiled with a curl that isn't as new as 8.9.0, only set KEEPCNT when\n    +    we have CURLOPT_TCP_KEEPCNT to begin with.\n    +\n         Signed-off-by: Taylor Blau <me@ttaylorr.com>\n     \n      ## Documentation/config/http.adoc ##\n    @@ Documentation/config/http.adoc: http.lowSpeedLimit, http.lowSpeedTime::\n      \tA boolean which disables using of EPSV ftp command by curl.\n      \tThis can be helpful with some \"poor\" ftp servers which don't\n     \n    + ## git-curl-compat.h ##\n    +@@\n    + #define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n    + #endif\n    + \n    ++/**\n    ++ * CURLOPT_TCP_KEEPCNT was added in 8.9.0, released in July, 2024.\n    ++ */\n    ++#if LIBCURL_VERSION_NUM >= 0x080900\n    ++#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n    ++#endif\n    ++\n    + #endif\n    +\n      ## http.c ##\n     @@ http.c: static struct {\n      };\n    @@ http.c: static int http_options(const char *var, const char *value,\n      \t}\n      \n     +\tif (!strcmp(\"http.keepaliveidle\", var)) {\n    -+\t\tcurl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);\n    ++\t\tcurl_tcp_keepidle = git_config_int(var, value, ctx->kvi);\n     +\t\treturn 0;\n     +\t}\n     +\tif (!strcmp(\"http.keepaliveinterval\", var)) {\n    -+\t\tcurl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);\n    ++\t\tcurl_tcp_keepintvl = git_config_int(var, value, ctx->kvi);\n     +\t\treturn 0;\n     +\t}\n     +\tif (!strcmp(\"http.keepalivecount\", var)) {\n    -+\t\tcurl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);\n    ++\t\tcurl_tcp_keepcnt = git_config_int(var, value, ctx->kvi);\n     +\t\treturn 0;\n     +\t}\n     +\n    @@ http.c: static CURL *get_curl_handle(void)\n     +\tif (curl_tcp_keepintvl > -1)\n     +\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL,\n     +\t\t\t\t curl_tcp_keepintvl);\n    ++#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n     +\tif (curl_tcp_keepcnt > -1)\n     +\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);\n    ++#endif\n     +\n      \treturn result;\n      }\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \n2.49.0.4.ge59cf92f8d\n"},{"id":"514684","messageId":"204e5e18d28951c3cb3c2e70f41873b263b65746.1742423021.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742423021.git.me@ttaylorr.com","subject":"[PATCH v2 1/4] http.c: remove unnecessary casts to long","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:23:46Z","receivedAt":"2025-03-19T22:23:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When parsing 'http.lowSpeedLimit' and 'http.lowSpeedTime', we explicitly\ncast the result of 'git_config_int()' to a long before assignment. This\ncast has been in place since all the way back in 58e60dd203 (Add support\nfor pushing to a remote repository using HTTP/DAV, 2005-11-02).\n\nBut that cast has always been unnecessary, since long is guaranteed to\nbe at least as wide as int. Let's drop the cast accordingly.\n\nNoticed-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n http.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0c9a872809..0cbcb079b2 100644\n--- a/http.c\n+++ b/http.c\n@@ -438,11 +438,11 @@ static int http_options(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.lowspeedlimit\", var)) {\n-\t\tcurl_low_speed_limit = (long)git_config_int(var, value, ctx->kvi);\n+\t\tcurl_low_speed_limit = git_config_int(var, value, ctx->kvi);\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.lowspeedtime\", var)) {\n-\t\tcurl_low_speed_time = (long)git_config_int(var, value, ctx->kvi);\n+\t\tcurl_low_speed_time = git_config_int(var, value, ctx->kvi);\n \t\treturn 0;\n \t}\n \n-- \n2.49.0.4.ge59cf92f8d\n\n"},{"id":"514685","messageId":"2e39a78e87edaf8f9842e510d05047dce647f4af.1742423021.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742423021.git.me@ttaylorr.com","subject":"[PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:23:50Z","receivedAt":"2025-03-19T22:23:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 7059cd99fc (http_init(): Fix config file parsing, 2009-03-09), http.c\ngained a new \"set_from_env()\" function as a convenience function around\nconditionally assigning an environment variable to some variable if and\nonly if the environment variable was set to begin with.\n\nBut prior to 7059cd99fc, there were two spots which need to first\nstrtol() whatever is set in the environment before assigning it to a\nlong pointer. Both instances stored the result of getenv() in a\ntemporary variable, and conditionally strtol() it depending on whether\nor not getenv() returned NULL.\n\nReplace those two instances with a new cousin of 'set_from_env()' called\n'set_long_from_env()', which does what its name suggests. This allows us\nto remove the temporary variables and clean up some minor code\nduplication while also adding more robust error handling.\n\nMore importantly, however, it prepares us for a future commit which will\nintroduce more instances of assigning an environment variable to a long.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n http.c | 32 ++++++++++++++++++++++++--------\n 1 file changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0cbcb079b2..17b676a1d5 100644\n--- a/http.c\n+++ b/http.c\n@@ -1256,10 +1256,30 @@ static void set_from_env(char **var, const char *envname)\n \t}\n }\n \n+static void set_long_from_env(long *var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val) {\n+\t\tlong tmp;\n+\t\tchar *endp;\n+\t\tint saved_errno = errno;\n+\n+\t\terrno = 0;\n+\t\ttmp = strtol(val, &endp, 10);\n+\n+\t\tif (errno)\n+\t\t\twarning_errno(_(\"failed to parse %s\"), envname);\n+\t\telse if (*endp || endp == val)\n+\t\t\twarning(_(\"failed to parse %s\"), envname);\n+\t\telse\n+\t\t\t*var = tmp;\n+\n+\t\terrno = saved_errno;\n+\t}\n+}\n+\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n-\tchar *low_speed_limit;\n-\tchar *low_speed_time;\n \tchar *normalized_url;\n \tstruct urlmatch_config config = URLMATCH_CONFIG_INIT;\n \n@@ -1338,12 +1358,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \n \tset_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n \n-\tlow_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n-\tif (low_speed_limit)\n-\t\tcurl_low_speed_limit = strtol(low_speed_limit, NULL, 10);\n-\tlow_speed_time = getenv(\"GIT_HTTP_LOW_SPEED_TIME\");\n-\tif (low_speed_time)\n-\t\tcurl_low_speed_time = strtol(low_speed_time, NULL, 10);\n+\tset_long_from_env(&curl_low_speed_limit, \"GIT_HTTP_LOW_SPEED_LIMIT\");\n+\tset_long_from_env(&curl_low_speed_time, \"GIT_HTTP_LOW_SPEED_TIME\");\n \n \tif (curl_ssl_verify == -1)\n \t\tcurl_ssl_verify = 1;\n-- \n2.49.0.4.ge59cf92f8d\n\n"},{"id":"514686","messageId":"cdfc9baa8da95505aa4ad61bab07be0b063d916b.1742423021.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742423021.git.me@ttaylorr.com","subject":"[PATCH v2 3/4] http.c: inline `set_curl_keepalive()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:23:53Z","receivedAt":"2025-03-19T22:23:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"At the end of `get_curl_handle()` we call `set_curl_keepalive()` to\nenable TCP keepalive probes on our CURL handle. `set_curl_keepalive()`\ndates back to 47ce115370 (http: use curl's tcp keepalive if available,\n2013-10-14), which conditionally compiled different variants of\n`set_curl_keepalive()` depending on what version of curl we were\ncompiled with[^1].\n\nAs of f7c094060c (git-curl-compat: remove check for curl 7.25.0,\n2024-10-23), we no longer conditionally compile `set_curl_keepalive()`\nsince we no longer support pre-7.25.0 versions of curl. But the version\nof that function that we kept is really just a thin wrapper around\nsetting the TCP_KEEPALIVE option, so there's no reason to keep it in its\nown function.\n\nInline the definition of `set_curl_keepalive()` to within\n`get_curl_handle()` so that the setup of our CURL handle is\nself-contained.\n\n[1]: The details are spelled out in 47ce115370, but the gist is curl\n  7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use\n  CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions\n  that predate even that option do nothing.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n http.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 17b676a1d5..b4267bfdb0 100644\n--- a/http.c\n+++ b/http.c\n@@ -704,10 +704,6 @@ static int has_proxy_cert_password(void)\n \treturn 1;\n }\n \n-static void set_curl_keepalive(CURL *c)\n-{\n-\tcurl_easy_setopt(c, CURLOPT_TCP_KEEPALIVE, 1);\n-}\n \n /* Return 1 if redactions have been made, 0 otherwise. */\n static int redact_sensitive_header(struct strbuf *header, size_t offset)\n@@ -1242,7 +1238,7 @@ static CURL *get_curl_handle(void)\n \t}\n \tinit_curl_proxy_auth(result);\n \n-\tset_curl_keepalive(result);\n+\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);\n \n \treturn result;\n }\n-- \n2.49.0.4.ge59cf92f8d\n\n"},{"id":"514687","messageId":"3fe62181e563d011d4a6374ed7894cc4ea807316.1742423021.git.me@ttaylorr.com","threadId":"63158","inReplyTo":"cover.1742423021.git.me@ttaylorr.com","subject":"[PATCH v2 4/4] http.c: allow custom TCP keepalive behavior via config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-19T22:23:56Z","receivedAt":"2025-03-19T22:23:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"curl supports a few options to control when and how often it should\ninstruct the OS to send TCP keepalives, like KEEPIDLE, KEEPINTVL, and\nKEEPCNT. Until this point, there hasn't been a way for users to change\nwhat values are used for these options, forcing them to rely on curl's\ndefaults.\n\nBut we do unconditionally enable TCP keepalives without giving users an\nability to tweak any fine-grained parameters. Ordinarily this isn't a\nproblem, particularly for users that have fast-enough connections,\nand/or are talking to a server that has generous or nonexistent\nthresholds for killing a connection it hasn't heard from in a while.\n\nBut it can present a problem when one or both of those assumptions fail.\nFor instance, I can reliably get an in-progress clone to be killed from\nthe remote end when cloning from some forges while using trickle to\nlimit my clone's bandwidth.\n\nFor those users and others who wish to more finely tune the OS's\nkeepalive behavior, expose configuration and environment variables which\nallow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.\n\nNote that while KEEPIDLE and KEEPINTVL were added in curl 7.25.0,\nKEEPCNT was added much more recently in curl 8.9.0. Per f7c094060c\n(git-curl-compat: remove check for curl 7.25.0, 2024-10-23), both\nKEEPIDLE and KEEPINTVL are set unconditionally. But since we may be\ncompiled with a curl that isn't as new as 8.9.0, only set KEEPCNT when\nwe have CURLOPT_TCP_KEEPCNT to begin with.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/config/http.adoc | 18 ++++++++++++++++++\n git-curl-compat.h              |  7 +++++++\n http.c                         | 33 ++++++++++++++++++++++++++++++++-\n 3 files changed, 57 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/http.adoc b/Documentation/config/http.adoc\nindex 22a8803dea..67393282fa 100644\n--- a/Documentation/config/http.adoc\n+++ b/Documentation/config/http.adoc\n@@ -296,6 +296,24 @@ http.lowSpeedLimit, http.lowSpeedTime::\n \tCan be overridden by the `GIT_HTTP_LOW_SPEED_LIMIT` and\n \t`GIT_HTTP_LOW_SPEED_TIME` environment variables.\n \n+http.keepAliveIdle::\n+\tSpecifies how long in seconds to wait on an idle connection\n+\tbefore sending TCP keepalive probes (if supported by the OS). If\n+\tunset, curl's default value is used. Can be overridden by the\n+\t`GIT_HTTP_KEEPALIVE_IDLE` environment variable.\n+\n+http.keepAliveInterval::\n+\tSpecifies how long in seconds to wait between TCP keepalive\n+\tprobes (if supported by the OS). If unset, curl's default value\n+\tis used. Can be overridden by the `GIT_HTTP_KEEPALIVE_INTERVAL`\n+\tenvironment variable.\n+\n+http.keepAliveCount::\n+\tSpecifies how many TCP keepalive probes to send before giving up\n+\tand terminating the connection (if supported by the OS). If\n+\tunset, curl's default value is used. Can be overridden by the\n+\t`GIT_HTTP_KEEPALIVE_COUNT` environment variable.\n+\n http.noEPSV::\n \tA boolean which disables using of EPSV ftp command by curl.\n \tThis can be helpful with some \"poor\" ftp servers which don't\ndiff --git a/git-curl-compat.h b/git-curl-compat.h\nindex 703756ba85..aa8eed7ed2 100644\n--- a/git-curl-compat.h\n+++ b/git-curl-compat.h\n@@ -45,4 +45,11 @@\n #define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n #endif\n \n+/**\n+ * CURLOPT_TCP_KEEPCNT was added in 8.9.0, released in July, 2024.\n+ */\n+#if LIBCURL_VERSION_NUM >= 0x080900\n+#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n+#endif\n+\n #endif\ndiff --git a/http.c b/http.c\nindex b4267bfdb0..d21e3a3bad 100644\n--- a/http.c\n+++ b/http.c\n@@ -104,6 +104,10 @@ static struct {\n };\n #endif\n \n+static long curl_tcp_keepidle = -1;\n+static long curl_tcp_keepintvl = -1;\n+static long curl_tcp_keepcnt = -1;\n+\n enum proactive_auth {\n \tPROACTIVE_AUTH_NONE = 0,\n \tPROACTIVE_AUTH_IF_CREDENTIALS,\n@@ -557,6 +561,19 @@ static int http_options(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(\"http.keepaliveidle\", var)) {\n+\t\tcurl_tcp_keepidle = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(\"http.keepaliveinterval\", var)) {\n+\t\tcurl_tcp_keepintvl = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(\"http.keepalivecount\", var)) {\n+\t\tcurl_tcp_keepcnt = git_config_int(var, value, ctx->kvi);\n+\t\treturn 0;\n+\t}\n+\n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, ctx, data);\n }\n@@ -704,7 +721,6 @@ static int has_proxy_cert_password(void)\n \treturn 1;\n }\n \n-\n /* Return 1 if redactions have been made, 0 otherwise. */\n static int redact_sensitive_header(struct strbuf *header, size_t offset)\n {\n@@ -1240,6 +1256,17 @@ static CURL *get_curl_handle(void)\n \n \tcurl_easy_setopt(result, CURLOPT_TCP_KEEPALIVE, 1);\n \n+\tif (curl_tcp_keepidle > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPIDLE,\n+\t\t\t\t curl_tcp_keepidle);\n+\tif (curl_tcp_keepintvl > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL,\n+\t\t\t\t curl_tcp_keepintvl);\n+#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n+\tif (curl_tcp_keepcnt > -1)\n+\t\tcurl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);\n+#endif\n+\n \treturn result;\n }\n \n@@ -1382,6 +1409,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \t\t\tssl_cert_password_required = 1;\n \t}\n \n+\tset_long_from_env(&curl_tcp_keepidle, \"GIT_TCP_KEEPIDLE\");\n+\tset_long_from_env(&curl_tcp_keepintvl, \"GIT_TCP_KEEPINTVL\");\n+\tset_long_from_env(&curl_tcp_keepcnt, \"GIT_TCP_KEEPCNT\");\n+\n \tcurl_default = get_curl_handle();\n }\n \n-- \n2.49.0.4.ge59cf92f8d\n"},{"id":"514692","messageId":"CABPp-BEn+NfGu1c=ZWjwnFBJgmsxRVSq+Roin-KDQGqKPdDhCg@mail.gmail.com","threadId":"63158","inReplyTo":"cover.1742423021.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/4] http: support fine-tuning curl's keepalive behavior","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-19T22:49:12Z","receivedAt":"2025-03-19T22:49:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 19, 2025 at 3:37 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> Here's a reroll of my series to introduce new http.* knobs to control\n> curl's TCP keepalive behavior.\n>\n> This reroll is mostly minor, with the notable differences being limited\n> to:\n>\n>   - Added error handling in the new http.c::set_long_from_env().\n>\n>   - Removed unnecessary casts from int -> long.\n>\n>   - Only set CURLOPT_TCP_KEEPCNT when compiled with a version of curl\n>     that knows about that option to begin with (>8.9.0).\n>\n> As usual, a range-diff is included below for convenience. Thanks again\n> for reviewing!\n>\n> == Original cover letter\n>\n> This short series introduces a few new http.* configuration options to\n> control curl's behavior around TCP keepalive packets. The details are\n> spelled out in the final patch, but the gist is:\n>\n>   - http.keepAliveIdle specifies how long in seconds to wait on an idle\n>     connection before beginning to send keepalive packets.\n>\n>   - http.keepAliveInterval does the same but controls the interval\n>     between successive keepalive packets.\n>\n>   - http.keepAliveCount specifies how many keepalive packets to send\n>     before closing down the connection.\n>\n> The first two commits of the series are general code clean-up of a\n> couple of small things I noticed while reading through the http.c code,\n> and the final patch implements these new options.\n>\n> I couldn't think of a great way to test these new configuration\n> settings, and given the simplicity of the final patch I opted for no\n> tests there. But if someone has a good idea of how to test this\n> behavior, please let me know.\n>\n> In either case, thanks in advance for your review!\n>\n> Taylor Blau (4):\n>   http.c: remove unnecessary casts to long\n>   http.c: introduce `set_long_from_env()` for convenience\n>   http.c: inline `set_curl_keepalive()`\n>   http.c: allow custom TCP keepalive behavior via config\n>\n>  Documentation/config/http.adoc | 18 ++++++++\n>  git-curl-compat.h              |  7 ++++\n>  http.c                         | 75 ++++++++++++++++++++++++++--------\n>  3 files changed, 84 insertions(+), 16 deletions(-)\n>\n> Range-diff against v1:\n> -:  ---------- > 1:  204e5e18d2 http.c: remove unnecessary casts to long\n> 1:  ba22a121fa ! 2:  2e39a78e87 http.c: introduce `set_long_from_env()` for convenience\n>     @@ Commit message\n>          Replace those two instances with a new cousin of 'set_from_env()' called\n>          'set_long_from_env()', which does what its name suggests. This allows us\n>          to remove the temporary variables and clean up some minor code\n>     -    duplication. More importantly, however, it prepares us for a future\n>     -    commit which will introduce more instances of assigning an environment\n>     -    variable to a long.\n>     +    duplication while also adding more robust error handling.\n>     +\n>     +    More importantly, however, it prepares us for a future commit which will\n>     +    introduce more instances of assigning an environment variable to a long.\n>\n>          Signed-off-by: Taylor Blau <me@ttaylorr.com>\n>\n>     @@ http.c: static void set_from_env(char **var, const char *envname)\n>      +static void set_long_from_env(long *var, const char *envname)\n>      +{\n>      +  const char *val = getenv(envname);\n>     -+  if (val)\n>     -+          *var = strtol(val, NULL, 10);\n>     ++  if (val) {\n>     ++          long tmp;\n>     ++          char *endp;\n>     ++          int saved_errno = errno;\n>     ++\n>     ++          errno = 0;\n>     ++          tmp = strtol(val, &endp, 10);\n>     ++\n>     ++          if (errno)\n>     ++                  warning_errno(_(\"failed to parse %s\"), envname);\n>     ++          else if (*endp || endp == val)\n>     ++                  warning(_(\"failed to parse %s\"), envname);\n>     ++          else\n>     ++                  *var = tmp;\n>     ++\n>     ++          errno = saved_errno;\n>     ++  }\n>      +}\n>      +\n>       void http_init(struct remote *remote, const char *url, int proactive_auth)\n> 2:  a05269552f = 3:  cdfc9baa8d http.c: inline `set_curl_keepalive()`\n> 3:  d840415808 ! 4:  3fe62181e5 http.c: allow custom TCP keepalive behavior via config\n>     @@ Commit message\n>          keepalive behavior, expose configuration and environment variables which\n>          allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.\n>\n>     +    Note that while KEEPIDLE and KEEPINTVL were added in curl 7.25.0,\n>     +    KEEPCNT was added much more recently in curl 8.9.0. Per f7c094060c\n>     +    (git-curl-compat: remove check for curl 7.25.0, 2024-10-23), both\n>     +    KEEPIDLE and KEEPINTVL are set unconditionally. But since we may be\n>     +    compiled with a curl that isn't as new as 8.9.0, only set KEEPCNT when\n>     +    we have CURLOPT_TCP_KEEPCNT to begin with.\n>     +\n>          Signed-off-by: Taylor Blau <me@ttaylorr.com>\n>\n>       ## Documentation/config/http.adoc ##\n>     @@ Documentation/config/http.adoc: http.lowSpeedLimit, http.lowSpeedTime::\n>         A boolean which disables using of EPSV ftp command by curl.\n>         This can be helpful with some \"poor\" ftp servers which don't\n>\n>     + ## git-curl-compat.h ##\n>     +@@\n>     + #define GIT_CURL_HAVE_CURLOPT_PROTOCOLS_STR 1\n>     + #endif\n>     +\n>     ++/**\n>     ++ * CURLOPT_TCP_KEEPCNT was added in 8.9.0, released in July, 2024.\n>     ++ */\n>     ++#if LIBCURL_VERSION_NUM >= 0x080900\n>     ++#define GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n>     ++#endif\n>     ++\n>     + #endif\n>     +\n>       ## http.c ##\n>      @@ http.c: static struct {\n>       };\n>     @@ http.c: static int http_options(const char *var, const char *value,\n>         }\n>\n>      +  if (!strcmp(\"http.keepaliveidle\", var)) {\n>     -+          curl_tcp_keepidle = (long)git_config_int(var, value, ctx->kvi);\n>     ++          curl_tcp_keepidle = git_config_int(var, value, ctx->kvi);\n>      +          return 0;\n>      +  }\n>      +  if (!strcmp(\"http.keepaliveinterval\", var)) {\n>     -+          curl_tcp_keepintvl = (long)git_config_int(var, value, ctx->kvi);\n>     ++          curl_tcp_keepintvl = git_config_int(var, value, ctx->kvi);\n>      +          return 0;\n>      +  }\n>      +  if (!strcmp(\"http.keepalivecount\", var)) {\n>     -+          curl_tcp_keepcnt = (long)git_config_int(var, value, ctx->kvi);\n>     ++          curl_tcp_keepcnt = git_config_int(var, value, ctx->kvi);\n>      +          return 0;\n>      +  }\n>      +\n>     @@ http.c: static CURL *get_curl_handle(void)\n>      +  if (curl_tcp_keepintvl > -1)\n>      +          curl_easy_setopt(result, CURLOPT_TCP_KEEPINTVL,\n>      +                           curl_tcp_keepintvl);\n>     ++#ifdef GIT_CURL_HAVE_CURLOPT_TCP_KEEPCNT\n>      +  if (curl_tcp_keepcnt > -1)\n>      +          curl_easy_setopt(result, CURLOPT_TCP_KEEPCNT, curl_tcp_keepcnt);\n>     ++#endif\n>      +\n>         return result;\n>       }\n>\n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n> --\n> 2.49.0.4.ge59cf92f8d\n\nThe new patch and range-diff look good to me.\n"},{"id":"514716","messageId":"Z9umnwmhZTZ82SnU@pks.im","threadId":"63158","inReplyTo":"2e39a78e87edaf8f9842e510d05047dce647f4af.1742423021.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-20T05:24:47Z","receivedAt":"2025-03-20T05:24:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 19, 2025 at 06:23:50PM -0400, Taylor Blau wrote:\n> diff --git a/http.c b/http.c\n> index 0cbcb079b2..17b676a1d5 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1256,10 +1256,30 @@ static void set_from_env(char **var, const char *envname)\n>  \t}\n>  }\n>  \n> +static void set_long_from_env(long *var, const char *envname)\n> +{\n> +\tconst char *val = getenv(envname);\n> +\tif (val) {\n> +\t\tlong tmp;\n> +\t\tchar *endp;\n> +\t\tint saved_errno = errno;\n> +\n> +\t\terrno = 0;\n> +\t\ttmp = strtol(val, &endp, 10);\n> +\n> +\t\tif (errno)\n> +\t\t\twarning_errno(_(\"failed to parse %s\"), envname);\n> +\t\telse if (*endp || endp == val)\n> +\t\t\twarning(_(\"failed to parse %s\"), envname);\n> +\t\telse\n> +\t\t\t*var = tmp;\n> +\n> +\t\terrno = saved_errno;\n> +\t}\n> +}\n\nThe `saved_errno` dance feels a bit unnecessary, but other than that I'm\nokay with this approach of only printing a warning instead of dying\nright away.\n\nThe other patches look good to me, too, thanks!\n\nPatrick\n"},{"id":"515413","messageId":"20250401091020.GA21089@coredump.intra.peff.net","threadId":"63158","inReplyTo":"2e39a78e87edaf8f9842e510d05047dce647f4af.1742423021.git.me@ttaylorr.com","subject":"Re: [PATCH v2 2/4] http.c: introduce `set_long_from_env()` for convenience","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-01T09:10:20Z","receivedAt":"2025-04-01T09:10:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 19, 2025 at 06:23:50PM -0400, Taylor Blau wrote:\n\n> +static void set_long_from_env(long *var, const char *envname)\n> +{\n> +\tconst char *val = getenv(envname);\n> +\tif (val) {\n> +\t\tlong tmp;\n> +\t\tchar *endp;\n> +\t\tint saved_errno = errno;\n> +\n> +\t\terrno = 0;\n> +\t\ttmp = strtol(val, &endp, 10);\n> +\n> +\t\tif (errno)\n> +\t\t\twarning_errno(_(\"failed to parse %s\"), envname);\n> +\t\telse if (*endp || endp == val)\n> +\t\t\twarning(_(\"failed to parse %s\"), envname);\n> +\t\telse\n> +\t\t\t*var = tmp;\n> +\n> +\t\terrno = saved_errno;\n> +\t}\n> +}\n\nIf we are going to add error checking, should it be reusing the existing\ncode in parse.[ch]? As a bonus, that would support units like \"K\" (which\ncould perhaps be useful for LOW_SPEED_LIMIT).\n\nWe'd need to extend that code for \"long\" (it has wrappers for \"int\" and\n\"unsigned long\", but not \"long). But that seems like a good thing\noverall. Something like:\n\ndiff --git a/http.c b/http.c\nindex 0c9a872809..a40e45e9cb 100644\n--- a/http.c\n+++ b/http.c\n@@ -1256,10 +1256,15 @@ static void set_from_env(char **var, const char *envname)\n \t}\n }\n \n+static void set_long_from_env(long *var, const char *envname)\n+{\n+\tconst char *val = getenv(envname);\n+\tif (val && !git_parse_long(val, var))\n+\t\twarning(_(\"failed to parse %s\"), envname);\n+}\n+\n void http_init(struct remote *remote, const char *url, int proactive_auth)\n {\n-\tchar *low_speed_limit;\n-\tchar *low_speed_time;\n \tchar *normalized_url;\n \tstruct urlmatch_config config = URLMATCH_CONFIG_INIT;\n \n@@ -1338,12 +1343,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n \n \tset_from_env(&user_agent, \"GIT_HTTP_USER_AGENT\");\n \n-\tlow_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n-\tif (low_speed_limit)\n-\t\tcurl_low_speed_limit = strtol(low_speed_limit, NULL, 10);\n-\tlow_speed_time = getenv(\"GIT_HTTP_LOW_SPEED_TIME\");\n-\tif (low_speed_time)\n-\t\tcurl_low_speed_time = strtol(low_speed_time, NULL, 10);\n+\tset_long_from_env(&curl_low_speed_limit, \"GIT_HTTP_LOW_SPEED_LIMIT\");\n+\tset_long_from_env(&curl_low_speed_time, \"GIT_HTTP_LOW_SPEED_TIME\");\n \n \tif (curl_ssl_verify == -1)\n \t\tcurl_ssl_verify = 1;\ndiff --git a/parse.c b/parse.c\nindex 7a60a4f816..4d9758132e 100644\n--- a/parse.c\n+++ b/parse.c\n@@ -116,6 +116,15 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \treturn 1;\n }\n \n+int git_parse_long(const char *value, long *ret)\n+{\n+\tintmax_t tmp;\n+\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(long)))\n+\t\treturn 0;\n+\t*ret = tmp;\n+\treturn 1;\n+}\n+\n int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\ndiff --git a/parse.h b/parse.h\nindex 6bb9a54d9a..0e536536a2 100644\n--- a/parse.h\n+++ b/parse.h\n@@ -4,6 +4,7 @@\n int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);\n int git_parse_ssize_t(const char *, ssize_t *);\n int git_parse_ulong(const char *, unsigned long *);\n+int git_parse_long(const char *, long *);\n int git_parse_int(const char *value, int *ret);\n int git_parse_int64(const char *value, int64_t *ret);\n int git_parse_double(const char *value, double *ret);\n\nYou could even add git_env_long() to match the existing git_env_ulong(),\nalthough:\n\n  - that family of functions calls die() on error, rather than warning\n\n  - the calling convention is a bit different; it always assigns the\n    result, but takes a default value. So you'd need to pass in the\n    existing value, which is a little awkward:\n\n      curl_low_speed_limit = git_env_long(\"GIT_HTTP_LOW_SPEED_LIMIT\",\n\t\t\t\t          curl_low_speed_limit);\n\n-Peff\n"},{"id":"515414","messageId":"20250401091216.GB21089@coredump.intra.peff.net","threadId":"63158","inReplyTo":"cdfc9baa8da95505aa4ad61bab07be0b063d916b.1742423021.git.me@ttaylorr.com","subject":"Re: [PATCH v2 3/4] http.c: inline `set_curl_keepalive()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-01T09:12:16Z","receivedAt":"2025-04-01T09:12:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 19, 2025 at 06:23:53PM -0400, Taylor Blau wrote:\n\n> At the end of `get_curl_handle()` we call `set_curl_keepalive()` to\n> enable TCP keepalive probes on our CURL handle. `set_curl_keepalive()`\n> dates back to 47ce115370 (http: use curl's tcp keepalive if available,\n> 2013-10-14), which conditionally compiled different variants of\n> `set_curl_keepalive()` depending on what version of curl we were\n> compiled with[^1].\n> \n> As of f7c094060c (git-curl-compat: remove check for curl 7.25.0,\n> 2024-10-23), we no longer conditionally compile `set_curl_keepalive()`\n> since we no longer support pre-7.25.0 versions of curl. But the version\n> of that function that we kept is really just a thin wrapper around\n> setting the TCP_KEEPALIVE option, so there's no reason to keep it in its\n> own function.\n> \n> Inline the definition of `set_curl_keepalive()` to within\n> `get_curl_handle()` so that the setup of our CURL handle is\n> self-contained.\n> \n> [1]: The details are spelled out in 47ce115370, but the gist is curl\n>   7.25.0 and newer use CURLOPT_TCP_KEEPALIVE, older versions use\n>   CURLOPT_SOCKOPTFUNCTION with a custom callback, and older versions\n>   that predate even that option do nothing.\n\nAs the author of 47ce115370, I approve. This ideally would have been\ncleaned up as part of the series with f7c094060c, but sometimes it's\nhard to see these simplifications when you're focused on getting rid of\nthe complicated bits. Better late than never.\n\n-Peff\n"},{"id":"515415","messageId":"20250401091606.GC21089@coredump.intra.peff.net","threadId":"63158","inReplyTo":"3fe62181e563d011d4a6374ed7894cc4ea807316.1742423021.git.me@ttaylorr.com","subject":"Re: [PATCH v2 4/4] http.c: allow custom TCP keepalive behavior via config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-04-01T09:16:06Z","receivedAt":"2025-04-01T09:16:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 19, 2025 at 06:23:56PM -0400, Taylor Blau wrote:\n\n> For those users and others who wish to more finely tune the OS's\n> keepalive behavior, expose configuration and environment variables which\n> allow setting curl's KEEPIDLE, KEEPINTVL, and KEEPCNT options.\n\nOK. I have never wanted those myself, but I have no problem with other\npeople using them. ;)\n\nThe implementation mostly looks as I'd expect, but...\n\n> +http.keepAliveIdle::\n> +\tSpecifies how long in seconds to wait on an idle connection\n> +\tbefore sending TCP keepalive probes (if supported by the OS). If\n> +\tunset, curl's default value is used. Can be overridden by the\n> +\t`GIT_HTTP_KEEPALIVE_IDLE` environment variable.\n\n...while this environment variable (and its siblings) makes sense to\nme....\n\n> @@ -1382,6 +1409,10 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)\n>  \t\t\tssl_cert_password_required = 1;\n>  \t}\n>  \n> +\tset_long_from_env(&curl_tcp_keepidle, \"GIT_TCP_KEEPIDLE\");\n> +\tset_long_from_env(&curl_tcp_keepintvl, \"GIT_TCP_KEEPINTVL\");\n> +\tset_long_from_env(&curl_tcp_keepcnt, \"GIT_TCP_KEEPCNT\");\n\n...we seem to use different names in the code.\n\nI think the ones mentioning HTTP make the most sense, to match the\nconfig (and since this only affects curl).\n\n-Peff\n"}]}