Re: [PATCH v3 5/7] parse-options: introduce `OPTION_UNSIGNED`

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 17, 2025, 08:15 UTC
Message-ID: <aAC4hMDS8gIwi7ku@pks.im>
In-Reply-To: <xmqqwmbkgcu5.fsf@gitster.g>

On Wed, Apr 16, 2025 at 11:50:42AM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We have two generic ways to parse integers in the "parse-options"
> > subsystem:
> >
> >   - `OPTION_INTEGER` parses a signed integer.
> >
> >   - `OPTION_MAGNITUDE` parses an unsigned integer, but it also
> >     interprets suffixes like "k" or "g".
> >
> > Notably missing is a middle ground that parses unsigned integers without
> > interpreting suffixes. Introduce a new `OPTION_UNSIGNED` option type to
> > plug this gap. This option type will be used in subsequent commits.
> 
> This takes a turn in a bit unexpected direction.  Because the way to
> spell a scaled quantity is unambiguous in the sense that anything
> that used to be parsed with OPT_INTEGER() couldn't have had anything
> but "^-?[0-9]+$", an obvious alternative is to teach OPTION_INTEGER
> to always allow the scaling suffix if the user wants to use one,
> without adding a new "only numbers but this one does not even allow
> a sign" variant.

Yes, that would be the alternative indeed. I wanted to have this patch so that I can adapt callsites that accidentally pass unsigned integers to the signed option without introducing any kind of incompatibility. So I was mostly aiming for 1:1 compatibility.

But I'm equally fine with extending `OPT_INTEGER()` to support parsing magnitudes. I guess it's even preferable: less overhead on our side, and more consistency for our users.

> Seriously, are there good candidates for an option where we want to
> absolutely refuse to take scaling suffix and insist only on a bare
> number?

I cannot think of any specific reason.

Patrick