[PATCH 1/2] Add strtoimax() compatibility function.

[PATCH 1/2] Add strtoimax() compatibility function.

[Nix]

Since systems that omit strtoumax() will likely omit strtomax() too,
and likewise for strtoull() and strtoll(), we also adjust the
compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
unsigned functions.

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 Makefile           |   36 ++++++++++++++++++------------------
 compat/strtoimax.c |   10 ++++++++++
 compat/strtoumax.c |    2 +-
 3 files changed, 29 insertions(+), 19 deletions(-)
 create mode 100644 compat/strtoimax.c

diff --git a/Makefile b/Makefile
index 6bf7d6c..0959e07 100644
--- a/Makefile
+++ b/Makefile
@@ -57,9 +57,9 @@ all::
 #
 # Define NO_STRLCPY if you don't have strlcpy.
 #
-# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
-# If your compiler also does not support long long or does not have
-# strtoull, define NO_STRTOULL.
+# Define NO_STRTOMAX if you don't have both strtoimax and strtoumax in the
+# C library.  If your compiler also does not support long long or does
+# not have strtoll or strtoull, define NO_STRTOLL.
 #
 # Define NO_SETENV if you don't have setenv in the C library.
 #
@@ -804,7 +804,7 @@ ifeq ($(uname_S),OSF1)
 	# Need this for u_short definitions et al
 	BASIC_CFLAGS += -D_OSF_SOURCE
 	SOCKLEN_T = int
-	NO_STRTOULL = YesPlease
+	NO_STRTOLL = YesPlease
 	NO_NSEC = YesPlease
 endif
 ifeq ($(uname_S),Linux)
@@ -890,7 +890,7 @@ ifeq ($(uname_S),SunOS)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
 		NO_STRLCPY = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.7)
@@ -900,19 +900,19 @@ ifeq ($(uname_S),SunOS)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
 		NO_STRLCPY = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.8)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	ifeq ($(uname_R),5.9)
 		NO_UNSETENV = YesPlease
 		NO_SETENV = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 		GIT_TEST_CMP = cmp
 	endif
 	INSTALL = /usr/ucb/install
@@ -954,7 +954,7 @@ ifeq ($(uname_S),FreeBSD)
 	ifeq ($(shell expr "$(uname_R)" : '4\.'),2)
 		PTHREAD_LIBS = -pthread
 		NO_UINTMAX_T = YesPlease
-		NO_STRTOUMAX = YesPlease
+		NO_STRTOMAX = YesPlease
 	endif
 	PYTHON_PATH = /usr/local/bin/python
 	HAVE_PATHS_H = YesPlease
@@ -1092,8 +1092,8 @@ ifeq ($(uname_S),Windows)
 	NO_MEMMEM = YesPlease
 	# NEEDS_LIBICONV = YesPlease
 	NO_ICONV = YesPlease
-	NO_STRTOUMAX = YesPlease
-	NO_STRTOULL = YesPlease
+	NO_STRTOMAX = YesPlease
+	NO_STRTOLL = YesPlease
 	NO_MKDTEMP = YesPlease
 	NO_MKSTEMPS = YesPlease
 	SNPRINTF_RETURNS_BOGUS = YesPlease
@@ -1139,7 +1139,7 @@ ifeq ($(uname_S),Interix)
 	NO_IPV6 = YesPlease
 	NO_MEMMEM = YesPlease
 	NO_MKDTEMP = YesPlease
-	NO_STRTOUMAX = YesPlease
+	NO_STRTOMAX = YesPlease
 	NO_NSEC = YesPlease
 	NO_MKSTEMPS = YesPlease
 	ifeq ($(uname_R),3.5)
@@ -1184,7 +1184,7 @@ ifneq (,$(findstring MINGW,$(uname_S)))
 	NO_MEMMEM = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	OLD_ICONV = YesPlease
-	NO_STRTOUMAX = YesPlease
+	NO_STRTOMAX = YesPlease
 	NO_MKDTEMP = YesPlease
 	NO_MKSTEMPS = YesPlease
 	NO_SVN_TESTS = YesPlease
@@ -1440,12 +1440,12 @@ ifdef NO_STRLCPY
 	COMPAT_CFLAGS += -DNO_STRLCPY
 	COMPAT_OBJS += compat/strlcpy.o
 endif
-ifdef NO_STRTOUMAX
-	COMPAT_CFLAGS += -DNO_STRTOUMAX
-	COMPAT_OBJS += compat/strtoumax.o
+ifdef NO_STRTOMAX
+	COMPAT_CFLAGS += -DNO_STRTOMAX
+	COMPAT_OBJS += compat/strtoumax.o compat/strtoimax.o
 endif
-ifdef NO_STRTOULL
-	COMPAT_CFLAGS += -DNO_STRTOULL
+ifdef NO_STRTOLL
+	COMPAT_CFLAGS += -DNO_STRTOLL
 endif
 ifdef NO_STRTOK_R
 	COMPAT_CFLAGS += -DNO_STRTOK_R
diff --git a/compat/strtoimax.c b/compat/strtoimax.c
new file mode 100644
index 0000000..fca07a3
--- /dev/null
+++ b/compat/strtoimax.c
@@ -0,0 +1,10 @@
+#include "../git-compat-util.h"
+
+intmax_t gitstrtoimax (const char *nptr, char **endptr, int base)
+{
+#if defined(NO_STRTOLL)
+	return strtol(nptr, endptr, base);
+#else
+	return strtoll(nptr, endptr, base);
+#endif
+}
diff --git a/compat/strtoumax.c b/compat/strtoumax.c
index 5541353..7feedfd 100644
--- a/compat/strtoumax.c
+++ b/compat/strtoumax.c
@@ -2,7 +2,7 @@
 
 uintmax_t gitstrtoumax (const char *nptr, char **endptr, int base)
 {
-#if defined(NO_STRTOULL)
+#if defined(NO_STRTOLL)
 	return strtoul(nptr, endptr, base);
 #else
 	return strtoull(nptr, endptr, base);
-- 
1.7.6.1.139.gcb612

Re: [PATCH 1/2] Add strtoimax() compatibility function.

[Junio C Hamano]

Nix <nix@esperi.org.uk> writes:

> Since systems that omit strtoumax() will likely omit strtomax() too,
> and likewise for strtoull() and strtoll(), we also adjust the
> compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
> NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
> unsigned functions.

What would happen to people who know their systems lack strtoumax and have
happily using NO_STRTOUMAX in their config.mak already? Do their build
suddenly start breaking after this patch is applied and they all have to
adjust to the new name?

Even though "no strtoumax() likely means no strtoimax()" may be a good
heuristics, I am not sure what we would gain by renaming these Makefile
variables. Can't you get the same effect by making existing NO_STRTOUMAX
imply not having strtoimax(), and if you did so, wouldn't it be much less
likely that you would break existing people's build?

Re: [PATCH 1/2] Add strtoimax() compatibility function.

[Nix]

On 6 Sep 2011, Junio C. Hamano spake thusly:

> Nix <nix@esperi.org.uk> writes:
>
>> Since systems that omit strtoumax() will likely omit strtomax() too,
>> and likewise for strtoull() and strtoll(), we also adjust the
>> compatibility #defines from NO_STRTOUMAX to NO_STRTOMAX and from
>> NO_STRTOULL to NO_STRTOLL, and have them cover both the signed and
>> unsigned functions.
>
> What would happen to people who know their systems lack strtoumax and have
> happily using NO_STRTOUMAX in their config.mak already? Do their build
> suddenly start breaking after this patch is applied and they all have to
> adjust to the new name?

Uh. Yeah. Oops.

> Even though "no strtoumax() likely means no strtoimax()" may be a good
> heuristics, I am not sure what we would gain by renaming these Makefile
> variables. Can't you get the same effect by making existing NO_STRTOUMAX
> imply not having strtoimax(), and if you did so, wouldn't it be much less
> likely that you would break existing people's build?

Yes, but I thought that might be too confusing (and having four
variables for this one case seemed ridiculous). I'm happy to rename it
back.

-- 
NULL && (void)

[PATCH 0/2] Support sizes >=2G in various config options, v2

[Nick Alcock]

New in this version:

 - overflow detection, as suggested by Johannes Sixt (on 32-bit
   platforms too).
 - no renaming of NO_STRTOUMAX nor NO_STRTOULL.

I think this covers all the bases, including detection of configuration
values that overflow signed but not unsigned type only after
factor-application (as '3g' would on a 32-bit Linux box).

No new git testsuite failures. (No tests either, because I can't think
of one that will reliably induce overflow on a 64-bit box without being
totally ludicrous.)

Nick Alcock (2):
  Add strtoimax() compatibility function.
  Support sizes >=2G in various config options accepting 'g' sizes.

 Makefile           |    6 +++---
 compat/strtoimax.c |   10 ++++++++++
 config.c           |   43 +++++++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 13 deletions(-)
 create mode 100644 compat/strtoimax.c

-- 
1.7.6.1.138.g03ab.dirty

[PATCH 1/2] Add strtoimax() compatibility function.

[Nick Alcock]

Since systems that omit strtoumax() will likely omit strtomax() too,
and likewise for strtoull() and strtoll(), we arrange for the
compatibility #defines NO_STRTOUMAX and NO_STRTOULL to cover both
the signed and unsigned functions. (We cannot change their names
without breaking existing makefile configurations.)

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 Makefile           |    6 +++---
 compat/strtoimax.c |   10 ++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 compat/strtoimax.c

diff --git a/Makefile b/Makefile
index 303a8df..a1f7e34 100644
--- a/Makefile
+++ b/Makefile
@@ -58,8 +58,8 @@ include /home/compiler/.configure/site.mk
 #
 # Define NO_STRLCPY if you don't have strlcpy.
 #
-# Define NO_STRTOUMAX if you don't have strtoumax in the C library.
-# If your compiler also does not support long long or does not have
+# Define NO_STRTOUMAX if you don't have both strtoimax and strtoumax in the
+# C library. If your compiler also does not support long long or does not have
 # strtoull, define NO_STRTOULL.
 #
 # Define NO_SETENV if you don't have setenv in the C library.
@@ -1459,7 +1459,7 @@ ifdef NO_STRLCPY
 endif
 ifdef NO_STRTOUMAX
 	COMPAT_CFLAGS += -DNO_STRTOUMAX
-	COMPAT_OBJS += compat/strtoumax.o
+	COMPAT_OBJS += compat/strtoumax.o compat/strtoimax.o
 endif
 ifdef NO_STRTOULL
 	COMPAT_CFLAGS += -DNO_STRTOULL
diff --git a/compat/strtoimax.c b/compat/strtoimax.c
new file mode 100644
index 0000000..ac09ed8
--- /dev/null
+++ b/compat/strtoimax.c
@@ -0,0 +1,10 @@
+#include "../git-compat-util.h"
+
+intmax_t gitstrtoimax (const char *nptr, char **endptr, int base)
+{
+#if defined(NO_STRTOULL)
+	return strtol(nptr, endptr, base);
+#else
+	return strtoll(nptr, endptr, base);
+#endif
+}
-- 
1.7.6.1.138.g03ab.dirty

[PATCH 2/2] Support sizes >=2G in various config options accepting 'g' sizes.

[Nick Alcock]

The config options core.packedGitWindowSize, core.packedGitLimit,
core.deltaBaseCacheLimit, core.bigFileThreshold, pack.windowMemory and
pack.packSizeLimit all claim to support suffixes up to and including
'g'.  This implies that they should accept sizes >=2G on 64-bit
systems: certainly, specifying a size of 3g should not silently be
translated to zero or transformed into a large negative value due to
integer overflow.  However, due to use of git_config_int() rather than
git_config_ulong(), that is exactly what happens:

% git config core.bigFileThreshold 2g
% git gc --aggressive # with extra debugging code to print out
                      # core.bigfilethreshold after parsing
bigfilethreshold: -2147483648
[...]

This is probably irrelevant for core.deltaBaseCacheLimit, but is
problematic for the other values.  (It is particularly problematic for
core.packedGitLimit, which can't even be set to its default value in
the config file due to this bug.)

This fixes things for 32-bit platforms as well.  They get the usual bad
config error if an overlarge value is specified, e.g.:

fatal: bad config value for 'core.bigfilethreshold' in /home/nix/.gitconfig

This is detected in all cases, even if the 32-bit platform has no size
larger than 'long'.  For signed integral configuration values, we also
detect the case where the value is too large for the signed type but
not the unsigned type.

Signed-off-by: Nick Alcock <nix@esperi.org.uk>
---
 config.c |   41 +++++++++++++++++++++++++++++++----------
 1 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/config.c b/config.c
index edf9914..84ca156 100644
--- a/config.c
+++ b/config.c
@@ -333,7 +333,7 @@ static int git_parse_file(config_fn_t fn, void *data)
 	die("bad config file line %d in %s", cf->linenr, cf->name);
 }
 
-static int parse_unit_factor(const char *end, unsigned long *val)
+static int parse_unit_factor(const char *end, uintmax_t *val)
 {
 	if (!*end)
 		return 1;
@@ -356,11 +356,23 @@ static int git_parse_long(const char *value, long *ret)
 {
 	if (value && *value) {
 		char *end;
-		long val = strtol(value, &end, 0);
-		unsigned long factor = 1;
+		intmax_t val;
+		uintmax_t uval;
+		uintmax_t factor = 1;
+
+		errno = 0;
+		val = strtoimax(value, &end, 0);
+		if (errno == ERANGE)
+			return 0;
 		if (!parse_unit_factor(end, &factor))
 			return 0;
-		*ret = val * factor;
+		uval = abs(val);
+		uval *= factor;
+		if ((uval > maximum_signed_value_of_type(long)) ||
+		    (abs(val) > uval))
+			return 0;
+		val *= factor;
+		*ret = val;
 		return 1;
 	}
 	return 0;
@@ -370,9 +382,19 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 {
 	if (value && *value) {
 		char *end;
-		unsigned long val = strtoul(value, &end, 0);
+		uintmax_t val;
+		uintmax_t oldval;
+
+		errno = 0;
+		val = strtoumax(value, &end, 0);
+		if (errno == ERANGE)
+			return 0;
+		oldval = val;
 		if (!parse_unit_factor(end, &val))
 			return 0;
+		if ((val > maximum_unsigned_value_of_type(long)) ||
+		    (oldval > val))
+			return 0;
 		*ret = val;
 		return 1;
 	}
@@ -553,7 +575,7 @@ static int git_default_core_config(const char *var, const char *value)
 
 	if (!strcmp(var, "core.packedgitwindowsize")) {
 		int pgsz_x2 = getpagesize() * 2;
-		packed_git_window_size = git_config_int(var, value);
+		packed_git_window_size = git_config_ulong(var, value);
 
 		/* This value must be multiple of (pagesize * 2) */
 		packed_git_window_size /= pgsz_x2;
@@ -564,18 +586,17 @@ static int git_default_core_config(const char *var, const char *value)
 	}
 
 	if (!strcmp(var, "core.bigfilethreshold")) {
-		long n = git_config_int(var, value);
-		big_file_threshold = 0 < n ? n : 0;
+		big_file_threshold = git_config_ulong(var, value);
 		return 0;
 	}
 
 	if (!strcmp(var, "core.packedgitlimit")) {
-		packed_git_limit = git_config_int(var, value);
+		packed_git_limit = git_config_ulong(var, value);
 		return 0;
 	}
 
 	if (!strcmp(var, "core.deltabasecachelimit")) {
-		delta_base_cache_limit = git_config_int(var, value);
+		delta_base_cache_limit = git_config_ulong(var, value);
 		return 0;
 	}
 
-- 
1.7.6.1.138.g03ab.dirty