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

[Jeff King <peff@peff.net>]

On Tue, Dec 12, 2023 at 07:09:02AM -0800, Junio C Hamano wrote:

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

Oh, I mostly meant that I would have turned to git_parse_int() as that
already-existing helper, but it is not suitable because of the extra
unit-handling. I think your patch draws the line in the right place.

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

Yeah, I agree this might be a good microproject (or leftoverbits) area,
and the semantics for the helper you propose make sense to me.

-Peff