From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Tue, 07 Jul 2026 08:50:41 GMT
Subject: Re: [PATCH GSoC v15 02/13] git-compat-util: add `strtoumax_szt()` with error handling
Message-ID: <CAN5EUNTYeDrQMor29eYMhJD0jcdRQq36ZA6BgupV8gG9xs9rFQ@mail.gmail.com>
In-Reply-To: <xmqqse62obwh.fsf@gitster.g>

El mié, 1 jul 2026 a las 19:30, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
> > From: Eric Ju <eric.peijian@gmail.com>
> >
> > We already have `strtoul_ui()` and similar functions that provide proper
> > error handling using `strtoul` from the standard library. However,
> > there isn't currently a variant that returns a `size_t`.
> >
> > Using `strtoul` is unreliable because `size_`t is platform-dependent,
> > `unsigned long` could be too big to fit into a `size_t` or too small to
> > hold a `size_t`.
>
> It is somehow annoying to see that the commit log desciption, which
> is *clearly* meant to be plaintext, is so heavily riddled with
> backquoted references to code/program symbols.  Yes, `literal` is a
> correct way to format them in both AsciiDoc and Markdown, so we very
> much welcome them in our documentation, but not in proposed log
> messages.
>
> In any case, you dropped 't' in 'size_t' outside the pair of
> backquotes.

Ok, I'll drop the backquotes for the commit messages of this series.

>
> > Use `strtoumax` which returns a `uintmax_t` guaranteed to be at least as
>
> `strtoumax()`, as the convention you used above for strtoul_ui() is
> to suffix function names with ().

ACK.

>
> > large as `size_t`, add a range check against `SIZE_MAX` to prevent
> > `size_t` overflow.
>
> OK.
>
> > This variant is needed in a subsequent commit to enable returning a
> > `size_t` with proper error handling.
> >
> > Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> > Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com>
> > Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> > ---
> >  git-compat-util.h | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 8809776407..5ecce5bbd2 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -975,6 +975,26 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
> >       return 0;
> >  }
> >
> > +/*
> > + * Convert a string to a size_t using the standard library's strtoumax, with
> > + * additional error handling to ensure robustness.
> > + */
> > +static inline int strtoumax_szt(char const *s, int base, size_t *result)
> > +{
> > +     uintmax_t uim;
> > +     char *p;
> > +
> > +     errno = 0;
> > +     /* negative values would be accepted by strtoul */
> > +     if (strchr(s, '-'))
> > +             return -1;
>
> Hmph, wouldn't
>
>         if (*s == '-' || !*s)
>                 return -1
>
> cut it?  Since your call to strtoumax() checks that the string was
> parsed to the end by insisting *p is NUL?
>
> 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 "-":

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.

>
> > +     uim = strtoumax(s, &p, base);
> > +     if ((errno || *p || p == s) || uim > SIZE_MAX)
> > +             return -1;
>
> And with !s[0] upfront, we can discard (p==s) case from here.  Other
> strto*() wrappers we have may need the "cannot be empty" check,
> because they do not need any upfront validation of s[] like we do
> here (we do so to reject negative numbers), but since we do need to
> check s[] before calling the system strto*() function anyway, it is
> OK to be different here from the others.

Agreed

>
> If uintmax_t and size_t are of the same width, then (SIZE_MAX < uim)
> becomes mathmatically impossible, but hopefully no compiler or
> static checker is stupid enough to warn against it.
>
> > +     *result = uim;
> > +     return 0;
> > +}
> > +
> >  static inline int strtol_i(char const *s, int base, int *result)
> >  {
> >       long ul;

Thanks for the feedback,
Pablo