On Wed, Mar 19, 2025 at 05:00:43PM +0100, Patrick Steinhardt wrote:

quoted

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