Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 4, 2025, 11:23 UTC
- Message-ID
- <aTFvKOHlm4zfT9dU@pks.im>
- In-Reply-To
- <20251130131537.GB199335@coredump.intra.peff.net>
On Sun, Nov 30, 2025 at 08:15:37AM -0500, Jeff King wrote: [snip]
Show 16 quoted lines
> For the interface: > > - What do we call it? We have git_parse_int() and friends, which aim > to make parsing less error-prone. And in some ways, these are just > buffer (rather than string) versions of those functions. But not > entirely. Those functions are aimed at parsing a single user-facing > value. So they accept a unit prefix (e.g., "10k"), which we won't > always want. And they insist that the whole string is consumed > (rather than passing back an "end" pointer). > > We also have strtol_i() and strtoul_ui() wrappers, which try to make > error handling simpler (especially around overflow), but mostly > behave like their libc counterparts. These also don't pass out an > end pointer, though. > > So I started a new namespace, "parse_<type>_from_buf".
I think it would be nice if we could eventually converge towards a common namespace here. E.g. `strotol_i()` would then become `parse_<type>()`, without the `_from_buf()` suffix. That would make it a bit more discoverable.
Similarly, `git_parse_int()` could become `parse_<type>_with_units()` eventually.
That certainly doesn't have to be part of this series though.
Show 11 quoted lines
> - Like those other functions above, we use an out-parameter to store > the result, which lets us return an error code directly. This avoids > the complicated errno dance for detecting overflow that you get with > strtol(). > > What should the error code look like? git_parse_int() uses a bool > for success/failure. But strtol_ui() uses the syscall-like "0 is > success, -1 is error" convention. > > I went with the bool approach here. Since the names are closest to > those functions, I thought it would cause the least confusion.
I think that's a sensible choice.
Show 37 quoted lines
> diff --git a/Makefile b/Makefile
> index 237b56fc9d..751bd40a9f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1510,6 +1510,7 @@ CLAR_TEST_SUITES += u-mem-pool
> CLAR_TEST_SUITES += u-oid-array
> CLAR_TEST_SUITES += u-oidmap
> CLAR_TEST_SUITES += u-oidtree
> +CLAR_TEST_SUITES += u-parse-int
> CLAR_TEST_SUITES += u-prio-queue
> CLAR_TEST_SUITES += u-reftable-basics
> CLAR_TEST_SUITES += u-reftable-block
> diff --git a/parse.c b/parse.c
> index f626846def..1dcbcf64a1 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -209,3 +209,99 @@ unsigned long git_env_ulong(const char *k, unsigned long val)
> die(_("failed to parse %s"), k);
> return val;
> }
> +
> +/*
> + * Helper that handles both signed/unsigned cases. If "negate" is NULL,
> + * negative values are disallowed. If not NULL and the input is negative,
> + * the value is range-checked but the caller is responsible for actually doing
> + * the negatiion. You probably don't want to use this! Use one of
> + * parse_signed_from_buf() or parse_unsigned_from_buf() below.
> + */
> +static bool parse_from_buf_internal(const char *buf, size_t len,
> + const char **ep, bool *negate,
> + uintmax_t *ret, uintmax_t max)
> +{
> + const char *end = buf + len;
> + uintmax_t val = 0;
> +
> + while (buf < end && isspace(*buf))
> + buf++;Hm. Do we really want to retain the behaviour of skipping leading spaces? I think it's a rather weird edge case of `strtol()` and friends, and if we can avoid it I'd prefer to not replicate this behaviour.
Show 6 quoted lines
> diff --git a/t/unit-tests/u-parse-int.c b/t/unit-tests/u-parse-int.c > new file mode 100644 > index 0000000000..a1601bb16b > --- /dev/null > +++ b/t/unit-tests/u-parse-int.c > @@ -0,0 +1,98 @@
[snip]
Show 15 quoted lines
> +void test_parse_int__basic(void)
> +{
> + cl_invoke(check_int_full("0", 0));
> + cl_invoke(check_int_full("11", 11));
> + cl_invoke(check_int_full("-23", -23));
> + cl_invoke(check_int_full("+23", 23));
> +
> + cl_invoke(check_int_str(" 31337 ", 7, 0, 31337));
> +
> + cl_invoke(check_int_err(" garbage", EINVAL));
> + cl_invoke(check_int_err("", EINVAL));
> + cl_invoke(check_int_err("-", EINVAL));
> +
> + cl_invoke(check_int("123", 2, 2, 0, 12));
> +}As Phillip suggested, it might make sense to wrap these `cl_invoke()` calls into a macro.
Patrick