Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers
From: Patrick Steinhardt <hidden>
Date: 2025-12-04 11:23:26
Possibly related (same subject, not in this thread)
- 2026-01-21 · Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers · Jeff King <hidden>
- 2026-01-20 · Re: [PATCH 2/4] parse: add functions for parsing from non-string buffers · Junio C Hamano <hidden>
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.
quoted hunk ↗ jump to hunk
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-blockdiff --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.
quoted hunk ↗ jump to hunk
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