| Message ID | 2e39a78e87edaf8f9842e510d05047dce647f4af.1742423021.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 572795cff930f11b1566f4f3e47fa9fa33772d1f |
| Headers | show |
| Series | http: support fine-tuning curl's keepalive behavior | expand |
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
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
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;
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(-)