From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 04 Dec 2025 11:23:20 GMT
Subject: Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers
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]
> 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.

>   - 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.

> 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.

> 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]
> +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