Re: [PATCH 00/14] numparse module: systematically tighten up integer parsing

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> * It allows leading whitespace.

This might be blessing in disguise.  Our friends on MacOS may be
relying on that

    git cmd --abbrev="$(wc -c <foo)"

to work "as expected", even though their "wc" gives leading spaces,
for example.

> * It allows arbitrary trailing characters.

Which is why we have introduced strtoul_ui() and such.

> * It allows a leading sign character ('+' or '-'), even though the
>   result is unsigned.

I do not particularly see it a bad thing to accept "--abbrev=+7".
Using unsigned type to accept a width and parsing "--abbrev=-7" into
a large positive integer _is_ a problem, and we may want to fix it,
but I think that is still within the scope of the original "better
strtol/strtoul", and I do not see it as a justification to introduce
a set of functions with completely unfamiliar name.

> * If the string doesn't contain a number at all, it sets its "endptr"
>   argument to point at the start of the string but doesn't set errno.

Why is that a problem?  A caller that cares is supposed to check
endptr and the string it gave, no?  Now, if strtoul_ui() and friends
do not do so, I again think that is still within the scope of the
original "better strtol/strtoul".

> * If the value is larger than fits in an unsigned long, it returns the
>   value clamped to the range 0..ULONG_MAX (setting errno to ERANGE).

Ditto.

> * If the value is between -ULONG_MAX and 0, it returns the positive
>   integer with the same bit pattern, without setting errno(!) (I can
>   hardly think of a scenario where this would be useful.)

Ditto.

> * For base=0 (autodetect base), it allows a base specifier prefix "0x"
>   or "0" and parses the number accordingly. For base=16 it also allows
>   a "0x" prefix.

Why is it a problem to allow "git cmd --hexval=0x1234", even if "git
cmd --hexval=1234" would suffice?

> When I looked around, I found scores of sites that call atoi(),
> strtoul(), and strtol() carelessly. And it's understandable. Calling
> any of these functions safely is so much work as to be completely
> impractical in day-to-day code.

Yes, these burdens on the caller were exactly why strtoul_ui()
etc. wanted to reduce---and it will be a welcome change to do
necessary checks that are currently not done.

> Please see the docstrings in numparse.h in the first commit for
> detailed API docs. Briefly, there are eight new functions:
>
>     parse_{l,ul,i,ui}(const char *s, unsigned int flags,
>                       T *result, char **endptr);
>     convert_{l,ul,i,ui}(const char *s, unsigned int flags, T *result);
>
> The parse_*() functions are for parsing a number from the start of a
> string; the convert_*() functions are for parsing a string that
> consists of a single number.

I am not sure if I follow.  Why not unify them into one 4-function
set, with optional endptr that could be NULL?

While we are on the topic of improving number parsing, one thing
that I found somewhat frustrating is that config.c layer knows the
scaling suffix but option parsing layer does not.  These functions
should offer an option (via their "flags") to say "I allow scaled
numbers like 2k, 4M, etc.".