git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Jeff KingNext: Phillip Wood
Message 53 of 64 in “asan bonanza”
  1. 0/9 asan bonanzaJeff King, Nov 12, 2025
  2. 1/9 compat/mmap: mark unused argument in git_munmap()Jeff King, Nov 12, 2025
  3. 2/9 pack-bitmap: handle name-hash lookups in incremental bitmapsJeff King, Nov 12, 2025
  4. Patrick SteinhardtNov 12, 2025
  5. Taylor BlauNov 13, 2025
  6. Jeff KingNov 18, 2025
  7. 3/9 Makefile: turn on NO_MMAP when building with ASanJeff King, Nov 12, 2025
  8. Collin FunkNov 12, 2025
  9. Jeff KingNov 12, 2025
  10. Collin FunkNov 12, 2025
  11. Patrick SteinhardtNov 12, 2025
  12. Taylor BlauNov 13, 2025
  13. Patrick SteinhardtNov 13, 2025
  14. Jeff KingNov 18, 2025
  15. Junio C HamanoNov 13, 2025
  16. Patrick SteinhardtNov 14, 2025
  17. Jeff KingNov 15, 2025
  18. 4/9 cache-tree: avoid strtol() on non-string bufferJeff King, Nov 12, 2025
  19. Patrick SteinhardtNov 12, 2025
  20. Taylor BlauNov 13, 2025
  21. Jeff KingNov 18, 2025
  22. Jeff KingNov 18, 2025
  23. 5/9 fsck: assert newline presence in fsck_ident()Jeff King, Nov 12, 2025
  24. 6/9 fsck: avoid strcspn() in fsck_ident()Jeff King, Nov 12, 2025
  25. 7/9 fsck: remove redundant date timestamp checkJeff King, Nov 12, 2025
  26. 8/9 fsck: avoid parse_timestamp() on buffer that isn't NUL-terminatedJeff King, Nov 12, 2025
  27. Patrick SteinhardtNov 12, 2025
  28. Junio C HamanoNov 12, 2025
  29. Jeff KingNov 15, 2025
  30. 9/9 t: enable ASan's strict_string_checks optionJeff King, Nov 12, 2025
  31. Taylor BlauNov 13, 2025
  32. 0/9 asan bonanzaJeff King, Nov 18, 2025
  33. 1/9 compat/mmap: mark unused argument in git_munmap()Jeff King, Nov 18, 2025
  34. 2/9 pack-bitmap: handle name-hash lookups in incremental bitmapsJeff King, Nov 18, 2025
  35. 3/9 Makefile: turn on NO_MMAP when building with ASanJeff King, Nov 18, 2025
  36. 4/9 cache-tree: avoid strtol() on non-string bufferJeff King, Nov 18, 2025
  37. Phillip WoodNov 18, 2025
  38. Junio C HamanoNov 23, 2025
  39. Phillip WoodNov 23, 2025
  40. Junio C HamanoNov 23, 2025
  41. Jeff KingNov 24, 2025
  42. Junio C HamanoNov 24, 2025
  43. Jeff KingNov 26, 2025
  44. Junio C HamanoNov 26, 2025
  45. 0/4 more robust functions for parsing int from bufJeff King, Nov 30, 2025
  46. 1/4 parse: prefer bool to int for boolean returnsJeff King, Nov 30, 2025
  47. Patrick SteinhardtDec 4, 2025
  48. 2/4 parse: add functions for parsing from non-string buffersJeff King, Nov 30, 2025
  49. my complaints with clarJeff King, Nov 30, 2025
  50. Phillip WoodDec 1, 2025
  51. Patrick SteinhardtDec 4, 2025
  52. Jeff KingDec 5, 2025
  53. Patrick SteinhardtDec 4, 2025
  54. Phillip WoodDec 5, 2025
  55. Junio C HamanoJan 20, 2026
  56. Jeff KingJan 21, 2026
  57. 3/4 cache-tree: use parse_int_from_buf()Jeff King, Nov 30, 2025
  58. 4/4 fsck: use parse_unsigned_from_buf() for parsing timestampJeff King, Nov 30, 2025
  59. 5/9 fsck: assert newline presence in fsck_ident()Jeff King, Nov 18, 2025
  60. 6/9 fsck: avoid strcspn() in fsck_ident()Jeff King, Nov 18, 2025
  61. 7/9 fsck: remove redundant date timestamp checkJeff King, Nov 18, 2025
  62. 8/9 fsck: avoid parse_timestamp() on buffer that isn't NUL-terminatedJeff King, Nov 18, 2025
  63. 9/9 t: enable ASan's strict_string_checks optionJeff King, Nov 18, 2025
  64. Junio C HamanoNov 23, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.