diff mbox series

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

Message ID ba22a121fa699e490de00eba988552b6c10fe2fd.1742336481.git.me@ttaylorr.com (mailing list archive)
State Superseded
Headers show
Series http: support fine-tuning curl's keepalive behavior | expand

Commit Message

Taylor Blau March 18, 2025, 10:21 p.m. UTC 
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(-)

Comments

Elijah Newren March 19, 2025, 4 p.m. UTC | #1 
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.
Patrick Steinhardt March 19, 2025, 4 p.m. UTC | #2 
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
Taylor Blau March 19, 2025, 6:02 p.m. UTC | #3 
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
diff mbox series

Patch

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;