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

From: Jeff King <peff@peff.net>
Date: Apr 1, 2025, 09:10 UTC
Message-ID: <20250401091020.GA21089@coredump.intra.peff.net>
In-Reply-To: <2e39a78e87edaf8f9842e510d05047dce647f4af.1742423021.git.me@ttaylorr.com>

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

Show 21 quoted lines

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