Re: [PATCH v3 7/7] parse-options: introduce bounded integer options

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 16, 2025, 19:19 UTC
Message-ID: <xmqqsem7hq2k.fsf@gitster.g>
In-Reply-To: <20250416-b4-pks-parse-options-integers-v3-7-d390746bea79@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 22 quoted lines

> In the preceding commits we have introduced integer precisions. The
> precision merely tracks bounds of the underlying data types so that we
> don't try to for example write a `size_t` into an `unsigned`, which
> could otherwise cause out-of-bounds writes.
>
> Some options may have bounds that are stricter than the underlying data
> type. Right now, users of any such options would have to manually verify
> that the value passed to such an option is inside the expected bounds.
> This is rather tedious, and it leads to code duplication across sites
> that wish to perform such bounds checks.
>
> Introduce `OPT_*_BOUNDED()` options that alleviate this issue. Users
> can optionally specify both a lower and upper bound, and if set we will
> verify that the value passed by the user is in that range.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  parse-options.c               | 40 ++++++++++++++++++++++++++++-----
>  parse-options.h               | 52 +++++++++++++++++++++++++++++++++++++++++++
>  t/helper/test-parse-options.c |  5 +++++
>  t/t0040-parse-options.sh      | 33 +++++++++++++++++++++++++++
>  4 files changed, 125 insertions(+), 5 deletions(-)

It is certainly cute, but unless there are plenty of existing users that use OPT_INTEGER() and friends and perform bounds checks themselves, I am not sure if this can withstand YAGNI criticism. And this step being at the end of the series, plus the above diffstat, tells us that there aren't any existing users converted to use this new mechanism.

OPT_INTEGER that wants to track percentage may want to say the value is between 0 and 100 (inclusive), but instead we take it bounded not to exceed 100, without lower bound. Without a real callsite, we cannot even tell if it is acceptable compromise for the sake of simplicity to forbid 0 as lower or upper bound, for example.

Thanks.