Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully

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

Jeff King <peff@peff.net> writes:

> This all looks pretty reasonable to me.
>
> I couldn't help but think, though, that surely we have some helpers for
> this already? But the closest seems to be git_parse_int(), which also
> allows unit factors. I'm not sure if allowing "-n 1k" would be a feature
> or a bug. ;)

The change in question is meant to be a pure fix to replace a careless
use of atoi().  I do not mind to see a separate patch to add such a
feature later on top.

> I guess "strtol_i()" maybe is that helper already, though I did not even
> know it existed. Looks like it goes back to 2007, and is seldom used.

I didn't know about it, either ;-) I used it only because there is
one use of it, among places that used atoi() I wanted to tighten.

> I wonder if there are more spots that could benefit.

"git grep -e 'atoi('" would give somebody motivated a decent set of
#microproject ideas, but many hits are not suited for strtol_i(),
which is designed to parse an integer at the end of a string.  Some
places use atoi() immediately followed by strspn() to skip over
digits, which means they are parsing an integer and want to continue
reading after the integer, which is incompatible with what
strtol_i() wants to do.  They need either a separate helper or an
updated strtol_i() that optionally allows you to parse the prefix
and report where the integer ended, e.g., something like:

#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)
static inline int strtol_i2(char const *s, int base, int *result, char **endp)
{
	long ul;
        char *dummy = NULL;

        if (!endp)
		endp = &dummy;
	errno = 0;
	ul = strtol(s, &endp, base);
	if (errno ||
	    /*
	     * if we are told to parse to the end of the string by
	     * passing NULL to endp, it is an error to have any
	     * remaining character after the digits.
	     */
            (dummy && *dummy) ||
	    endp == s || (int) ul != ul)
		return -1;
	*result = ul;
	return 0;
}

perhaps.