From: Pablo Sabater <pabloosabaterr@gmail.com>
Date: Fri, 26 Jun 2026 12:00:01 GMT
Subject: Re: [PATCH GSoC v14 02/13] git-compat-util: add strtoul_szt() with error handling
Message-ID: <CAN5EUNStfvgBCpwokfooa9MY3ZGSf9f=xok+aeKXKK9Zv8uSgw@mail.gmail.com>
In-Reply-To: <xmqqjyrme393.fsf@gitster.g>

El jue, 25 jun 2026 a las 23:09, 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 an unsigned long.
>
> But this one no longer returns an unsigned long anymore ;-)

True, I missed that.

>
> > This variant is needed in a subsequent commit to enable returning an
> > size_t with proper error handling.
>
> I think it would allow a lot of code paths that want to deal with
> size_t not to worry about "is ulong large enough?" to have a
> function like this, but for that to happen, the implementation of
> the function must carefully think through if these steps do sensible
> things on platforms with too small ulong (which often is OK when we
> are coming from decimal string to ulong and then to size_t) and too
> large ulong (which is not OK, when coming from decimal string to
> ulong which might be fine, but will bust the size of the final
> type), etc.

Ok, so the strtoul_szt() is not a bad idea but it is not ok how I did
it. What about using uintmax_t and strtoumax() and after that (before
casting) check if it fits into a size_t?
Something like:

static inline int strtoumax_szt(char const *s, int base, size_t *result)
{
     uintmax_t val;
     char *p;

     errno = 0;
     /* negative values would be accepted by strtoul */
     if (strchr(s, '-'))
             return -1;
     val = strtoumax(s, &p, base);
     if ((errno || *p || p == s) || val > SIZE_MAX)
             return -1;
     *result = val;
     return 0;
}

Alternatively I could go back to the unsigned long version and make
the relevant checks on the caller which is only one place at
`fetch_object_info()`

>
> Also, would it make sense to add yet another "static inline" like
> this?  After the dust settles, we may want to rethink these strtoX
> wrappers we have, benchmark, and possibly make them into a proper
> library function, not "static inline" that may bloat the runtime.

Yeah, I thought the same, I kept it on the header also to avoid
bloating this series too much because it's kinda big already. But a
follow-up after would be a good idea tho.

>
> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 8809776407..7f417f1acf 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 strtoul, with
> > + * additional error handling to ensure robustness.
> > + */
> > +static inline int strtoul_szt(char const *s, int base, size_t *result)
> > +{
> > +     unsigned long ul;
> > +     char *p;
> > +
> > +     errno = 0;
> > +     /* negative values would be accepted by strtoul */
> > +     if (strchr(s, '-'))
> > +             return -1;
> > +     ul = strtoul(s, &p, base);
> > +     if (errno || *p || p == s)
> > +             return -1;
> > +     *result = ul;
> > +     return 0;
> > +}
> > +
> >  static inline int strtol_i(char const *s, int base, int *result)
> >  {
> >       long ul;

Thanks for the review,
Pablo.