patch, 2 partsAdd strtoimax() compatibility function.

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

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

32-bit platforms with no type larger than 'long' cannot detect this case and will continue to silently misbehave, but the misbehaviour will be somewhat different and more useful, since bigFileThreshold was also being mistakenly treated as a signed value when it should have been unsigned.

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

Heya,

On Mon, Sep 5, 2011 at 13:45, Nix <nix@esperi.org.uk> wrote:

Is it not possible to detect that the target value won't fit in the max size of an int when parsing the config value?

On 5 Sep 2011, Sverre Rabbelier said:

Well, we're parsing longs, not ints. If sizeof(long)>sizeof(int), or we have long long and sizeof(long long)>sizeof(int), then we can always detect overflows when saving into the appropriate type: but if we don't have long long, or if we have neither strto(u)ll() nor strto[ui]max(), we could only detect overflow by looking at the raw text string and checking it by hand to see if it would fit. I judged this pointless extra complexity for a very rare edge case (machines with neither strot(u)ll() nor strto[ui]max() are generally quite old and people aren't going to be specifying sizes in gigabytes on such machines anyway.)

On Mon, Sep 05, 2011 at 02:56:10PM +0100, Nix wrote:

Is this also true for Windows and other platforms?

And I don't think it's about whether or not people are likely to specify sizes in gigabytes on old machines. People are bound to blindly copy configuration files from one machine to another. In any case, my expectation would be for the configuration options to do what I tell them, or error out if they do not make sense.

Clemens

On 6 Sep 2011, Clemens Buchacher uttered the following:

There, uintmax_t is 'long long' and is longer than 'long', let alone 'int', so this holds there too, or should.

Yeah, and we do that whenever practically possible: but fixing this for the case that int/long is the largest available type (which among other things implies that we're not using GCC or any other C99 compiler or any of the myriad C89 compilers that implmented 'long long') amounts to writing our own strtol() specifically for this one case, to see if the parsed number is too long. And that is probably a maintenance burden too far.

The failure mode if you put a huge number in on such a platform is better than it used to be, too, especially for core.bigfilethreshold. We used to get a negative number that was latched to zero, which then disabled compression entirely (I had an 83Mb pack turn itself into an 837Mb one when that happened). Now we get a number that, while positive, is less positive than we expect, but still likely up in the hundreds of millions.

Am 9/6/2011 11:13, schrieb Nix:

Why so? strtol() can report overflow:

RETURN VALUE

    ...
    If the correct value is outside the range of representable values,
{LONG_MIN}, {LONG_MAX}, {LLONG_MIN}, or {LLONG_MAX} shall be returned
(according to the sign of the value), and errno set to [ERANGE].

-- Hannes

On 6 Sep 2011, Johannes Sixt verbalised:

... it can?!

I've been using it for longer than I care to imagine and I've never once noticed that.

OK, I'll add range checking support then! following which we can detect config value overflow on the most pathetic platform imaginable, except that such a platform would probably not bother to set ERANGE properly ;}

Fixed patch following later today ripping out the STRTOMAX renaming and adding proper range checking.

Am 9/6/2011 12:25, schrieb Nix:

Don't forget to follow this advice noted on the POSIX page (http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html):

Since 0, {LONG_MIN} or {LLONG_MIN}, and {LONG_MAX} or {LLONG_MAX} are returned on error and are also valid returns on success, an application wishing to check for error situations should set errno to 0, then call strtol() or strtoll(), then check errno.

-- Hannes

On 6 Sep 2011, Johannes Sixt stated:

Oh, I've fallen into *that* trap before with math functions setting ERANGE/EDOM. Thanks for the reminder :)

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

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?

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

Uh. Yeah. Oops.

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.