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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 12, 2023, 15:09 UTC
Message-ID: <xmqqjzpjsbjl.fsf@gitster.g>
In-Reply-To: <20231212013045.GE376323@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> 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.