Re: [PATCH GSoC v15 02/13] git-compat-util: add `strtoumax_szt()` with error handling

From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Jul 7, 2026, 19:53 UTC
Message-ID: <CAN5EUNQ=2qtKXSJvxQiNLYqx0N0m6sfyBGLLXm4FB1kwtOsdbQ@mail.gmail.com>
In-Reply-To: <xmqqcxwy4qp5.fsf@gitster.g>

El mar, 7 jul 2026 a las 20:09, Junio C Hamano (<gitster@pobox.com>) escribió:

Show 33 quoted lines

>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> >> If you are trying to more explicitly insist that s[] has only
> >> digits, which may not be a bad idea, as that is what we generally
> >> expect, then
> >>
> >>         if (!s[0] || s[strspn(s, "0123456789")])
> >>                 return -1;
> >>
> >> perhaps.
> >
> > I like the idea of only digits but, even though in this series I only
> > use this function in base 10, I want the function to work in other
> > bases, that's why I left the base in the function signature instead of
> > hardcoding it. strspn(s, "0123456789") rejects bases >10  ("ff" for
> > base 16) while strtoumax does support higher ones.
> > I think that it would be better to explicitly reject what we don't
> > want similarly to "-":
>
> Let's step back a bit and think.
>
> Where do we plan to use this function?  Remember that being a
> superset is not always necessarily good for a helper function that
> serves as a format checker.
>
> In the output of "git diff master...ps/cat-file-remote-object-info",
> there is only one caller, which is fetch_object_info().  It reads
> into object_info_data[].sizep.  Do we expect to express the object
> size in anything but an unsigned decimal integer?  Remember that it
> is better to be unambiguous when designing a protocol.  We do not
> want a third-party reimplementation of whatever is talking to
> fetch_object_info() to send object size in hex ;-).

No haha, we don't want size being sent in hex :), I agree that it is better to be unambiguous. We could hardcode the base 10 but I feel that calling the function strtoumax_szt() when it does not support >10 base (or I hardcode the base to be 10) lies to a future developer that tries to use this function thinking that it behaves as strtoumax_*().

Maybe because it is called only once in this series it is better to have a static function close to its caller that explicitly does what we want and it is unambiguous.

If that sounds reasonable I'll move this function to the commit where it's called, call the function parse_object_size() and keep the strict digits only with strspn proposed.

Show 16 quoted lines

>
> It may also be usable to parse the size of the object payload in
> object-file.c::parse_loose_header() but notice that it is already
> even stricter not to use strto<anything> system function and instead
> handcrafts the trivial number parsing.  This would avoid system
> dependent funnyness, which is a good thing.
>
> > if (!*s || isspace((unsigned char)*s) || *s == '-' || *s == '+')
> >         return -1;
> >
> > About that, strtoumax works fine with "+" and ignores starting
> > whitespaces, but for consistency (we reject "-" and whitespaces
> > between or at the end) rejecting whitespaces and +/- will be better
> > and make the caller format it correctly.
> >
> > I'll do that for the next version.

Thanks, Pablo