[PATCH 0/6] parse-options: add more precision handling

[PATCH 0/6] parse-options: add more precision handling

[René Scharfe]

Extend precision handling to all options that currently blindly use an
int pointer to access value variables.  This allows the safe use of
smaller and larger integer types.

Sign handling might be nice as well (especially for OPTION_COUNTUP), but
is out of scope for this series.

  parse-options: add precision handling for PARSE_OPT_CMDMODE
  parse-options: add precision handling for OPTION_SET_INT
  parse-options: add precision handling for OPTION_BIT
  parse-options: add precision handling for OPTION_NEGBIT
  parse-options: add precision handling for OPTION_BITOP
  parse-options: add precision handling for OPTION_COUNTUP

 builtin/am.c                  |   1 +
 builtin/rebase.c              |   1 +
 builtin/update-index.c        |   6 ++
 builtin/write-tree.c          |   1 +
 parse-options.c               | 142 ++++++++++++++++++++++++----------
 parse-options.h               |   7 ++
 t/helper/test-parse-options.c |  17 +++-
 7 files changed, 129 insertions(+), 46 deletions(-)

-- 
2.50.0

[PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE

[René Scharfe]

Build on 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) to support value variables of different
sizes for PARSE_OPT_CMDMODE options.  Do that by requiring their
"precision" to be set and casting their "value" pointer accordingly.

get_value() needs to access all PARSE_OPT_CMDMODE values in addition to
the actual value it is supposed to get to detect conflicting changes.
Give it an example struct option pointer in cmdmode_list instead of just
the "value" pointer to allow it to use the proper "precision".

Use optbug() in get_int_value() to report options with unsupported
"precision" values without requiring enum opt_parsed flags, as we don't
have them in build_cmdmode_list().  Use BUG right afterwards to abort
for uses outside of build_cmdmode_list() by aborting immediately.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c                  |  1 +
 parse-options.c               | 36 ++++++++++++++++++++++++++---------
 parse-options.h               |  1 +
 t/helper/test-parse-options.c | 13 ++++++++++---
 4 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a800003340..c9d925f7b9 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2406,6 +2406,7 @@ int cmd_am(int argc,
 			.type = OPTION_CALLBACK,
 			.long_name = "show-current-patch",
 			.value = &resume_mode,
+			.precision = sizeof(resume_mode),
 			.argh = "(diff|raw)",
 			.help = N_("show the patch being applied"),
 			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..da07a000a3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -68,6 +68,23 @@ static char *fix_filename(const char *prefix, const char *file)
 		return prefix_filename_except_for_dash(prefix, file);
 }
 
+static intmax_t get_int_value(const struct option *opt)
+{
+	switch (opt->precision) {
+	case sizeof(int8_t):
+		return *(int8_t *)opt->value;
+	case sizeof(int16_t):
+		return *(int16_t *)opt->value;
+	case sizeof(int32_t):
+		return *(int32_t *)opt->value;
+	case sizeof(int64_t):
+		return *(int64_t *)opt->value;
+	default:
+		optbug(opt, "has invalid precision");
+		BUG("invalid 'struct option'");
+	}
+}
+
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  const struct option *opt,
 					  enum opt_parsed flags,
@@ -266,8 +283,8 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 }
 
 struct parse_opt_cmdmode_list {
-	int value, *value_ptr;
-	const struct option *opt;
+	intmax_t value;
+	const struct option *opt, *reference_opt;
 	const char *arg;
 	enum opt_parsed flags;
 	struct parse_opt_cmdmode_list *next;
@@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 	for (; opts->type != OPTION_END; opts++) {
 		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
-		int *value_ptr = opts->value;
 
-		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
+		if (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)
 			continue;
 
-		while (elem && elem->value_ptr != value_ptr)
+		while (elem && elem->reference_opt->value != opts->value)
 			elem = elem->next;
 		if (elem)
 			continue;
 
 		CALLOC_ARRAY(elem, 1);
-		elem->value_ptr = value_ptr;
-		elem->value = *value_ptr;
+		elem->reference_opt = opts;
+		elem->value = get_int_value(opts);
 		elem->next = ctx->cmdmode_list;
 		ctx->cmdmode_list = elem;
 	}
@@ -317,7 +333,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	char *opt_name, *other_opt_name;
 
 	for (; elem; elem = elem->next) {
-		if (*elem->value_ptr == elem->value)
+		intmax_t new_value = get_int_value(elem->reference_opt);
+
+		if (new_value == elem->value)
 			continue;
 
 		if (elem->opt &&
@@ -327,7 +345,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		elem->opt = opt;
 		elem->arg = arg;
 		elem->flags = flags;
-		elem->value = *elem->value_ptr;
+		elem->value = new_value;
 	}
 
 	if (result || !elem)
diff --git a/parse-options.h b/parse-options.h
index 91c3e3c29b..c75a473c9e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -269,6 +269,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index f2663dd0c0..1e03ff88f6 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -148,9 +148,16 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
 		OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
 		OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
-		OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)",
-			"set integer to 3 or 4 (cmdmode option)",
-			PARSE_OPT_CMDMODE, mode34_callback),
+		{
+			.type = OPTION_CALLBACK,
+			.long_name = "mode34",
+			.value = &integer,
+			.precision = sizeof(integer),
+			.argh = "(3|4)",
+			.help = "set integer to 3 or 4 (cmdmode option)",
+			.flags = PARSE_OPT_CMDMODE,
+			.callback = mode34_callback,
+		},
 		OPT_CALLBACK('L', "length", &integer, "str",
 			"get length of <str>", length_callback),
 		OPT_FILENAME('F', "file", &file, "set file to <file>"),
-- 
2.50.0

Re: [PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE

[Patrick Steinhardt]

On Sun, Jun 29, 2025 at 01:50:31PM +0200, René Scharfe wrote:
> Build on 09705696f7 (parse-options: introduce precision handling for
> `OPTION_INTEGER`, 2025-04-17) to support value variables of different
> sizes for PARSE_OPT_CMDMODE options.  Do that by requiring their
> "precision" to be set and casting their "value" pointer accordingly.

Makes sense.

> get_value() needs to access all PARSE_OPT_CMDMODE values in addition to
> the actual value it is supposed to get to detect conflicting changes.
> Give it an example struct option pointer in cmdmode_list instead of just
> the "value" pointer to allow it to use the proper "precision".
> 
> Use optbug() in get_int_value() to report options with unsupported
> "precision" values without requiring enum opt_parsed flags, as we don't
> have them in build_cmdmode_list().  Use BUG right afterwards to abort
> for uses outside of build_cmdmode_list() by aborting immediately.

Hm. I have a bit of a hard time understanding these two paragraphs, to
be honest. Might be that my brain is still in vacation mode.

> @@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
>  
>  	for (; opts->type != OPTION_END; opts++) {
>  		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
> -		int *value_ptr = opts->value;
>  
> -		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
> +		if (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)
>  			continue;
>  
> -		while (elem && elem->value_ptr != value_ptr)
> +		while (elem && elem->reference_opt->value != opts->value)
>  			elem = elem->next;

Hm. Previously we checked for the pointers to be equal, now we check for
the value to be equal. Are we sure that this is always equivalent? Can't
it ever be that two elements might have the same value?

Patrick

Re: [PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE

[René Scharfe]

On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> On Sun, Jun 29, 2025 at 01:50:31PM +0200, René Scharfe wrote:
>> Build on 09705696f7 (parse-options: introduce precision handling for
>> `OPTION_INTEGER`, 2025-04-17) to support value variables of different
>> sizes for PARSE_OPT_CMDMODE options.  Do that by requiring their
>> "precision" to be set and casting their "value" pointer accordingly.
> 
> Makes sense.
> 
>> get_value() needs to access all PARSE_OPT_CMDMODE values in addition to
>> the actual value it is supposed to get to detect conflicting changes.
>> Give it an example struct option pointer in cmdmode_list instead of just
>> the "value" pointer to allow it to use the proper "precision".
>>
>> Use optbug() in get_int_value() to report options with unsupported
>> "precision" values without requiring enum opt_parsed flags, as we don't
>> have them in build_cmdmode_list().  Use BUG right afterwards to abort
>> for uses outside of build_cmdmode_list() by aborting immediately.
> 
> Hm. I have a bit of a hard time understanding these two paragraphs, to
> be honest. Might be that my brain is still in vacation mode.

Or it might be my needing-a-vacation mode.

get_value() checks all PARSE_OPT_CMDMODE value variables.  It does that
to detect changes to such a variable by non-PARSE_OPT_CMDMODE options.
"All" sounds grand, but actual commands only have at most a single
PARSE_OPT_CMDMODE value variable.  But they could have many more.
Anyway, the patch gives it a struct option so that it can use the
proper precision when dereferencing value, a void pointer.

optbug() calls the macro "bug".  It reports a bug just like BUG, but
does not abort.  It's intended for cases where we want to report all the
bugs that we can find instead of forcing developers to fix them one by
one and recompile in between.  That would be nice in
build_cmdmode_list().  I still force an abort by following it with BUG,
though, because handling invalid "precision" values would be tedious
in the other callers.

> 
>> @@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
>>  
>>  	for (; opts->type != OPTION_END; opts++) {
>>  		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
>> -		int *value_ptr = opts->value;
>>  
>> -		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
>> +		if (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)
>>  			continue;
>>  
>> -		while (elem && elem->value_ptr != value_ptr)
>> +		while (elem && elem->reference_opt->value != opts->value)
>>  			elem = elem->next;
> 
> Hm. Previously we checked for the pointers to be equal, now we check for
> the value to be equal. Are we sure that this is always equivalent? Can't
> it ever be that two elements might have the same value?

The "value" member of struct option is a void pointer.  We still compare
pointers here.

René

[PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_SET_INT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Factor out the casting code from the part of do_get_value() that handles
OPTION_INTEGER to avoid code duplication.  We're going to use it in the
next patches as well.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/update-index.c        |  6 ++++
 parse-options.c               | 56 ++++++++++++++++++++++-------------
 parse-options.h               |  2 ++
 t/helper/test-parse-options.c |  1 +
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 538b619ba4..0c1d4ed55b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -981,6 +981,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("mark files as \"not changing\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -989,6 +990,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("clear assumed-unchanged bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -997,6 +999,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("mark files as \"index-only\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1005,6 +1008,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("clear skip-worktree bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -1079,6 +1083,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("mark files as fsmonitor valid"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1087,6 +1092,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("clear fsmonitor valid bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
diff --git a/parse-options.c b/parse-options.c
index da07a000a3..bbb68603cc 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)
 	}
 }
 
+static enum parse_opt_result set_int_value(const struct option *opt,
+					   enum opt_parsed flags,
+					   intmax_t value)
+{
+	switch (opt->precision) {
+	case sizeof(int8_t):
+		*(int8_t *)opt->value = value;
+		return 0;
+	case sizeof(int16_t):
+		*(int16_t *)opt->value = value;
+		return 0;
+	case sizeof(int32_t):
+		*(int32_t *)opt->value = value;
+		return 0;
+	case sizeof(int64_t):
+		*(int64_t *)opt->value = value;
+		return 0;
+	default:
+		BUG("invalid precision for option %s", optname(opt, flags));
+	}
+}
+
+static int signed_int_fits(intmax_t value, size_t size)
+{
+	size_t bits = size * CHAR_BIT;
+	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
+	intmax_t lower_bound = -upper_bound - 1;
+	return lower_bound <= value && value <= upper_bound;
+}
+
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  const struct option *opt,
 					  enum opt_parsed flags,
@@ -133,8 +163,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_SET_INT:
-		*(int *)opt->value = unset ? 0 : opt->defval;
-		return 0;
+		return set_int_value(opt, flags, unset ? 0 : opt->defval);
 
 	case OPTION_STRING:
 		if (unset)
@@ -216,23 +245,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
 
-		switch (opt->precision) {
-		case 1:
-			*(int8_t *)opt->value = value;
-			return 0;
-		case 2:
-			*(int16_t *)opt->value = value;
-			return 0;
-		case 4:
-			*(int32_t *)opt->value = value;
-			return 0;
-		case 8:
-			*(int64_t *)opt->value = value;
-			return 0;
-		default:
-			BUG("invalid precision for option %s",
-			    optname(opt, flags));
-		}
+		return set_int_value(opt, flags, value);
 	}
 	case OPTION_UNSIGNED:
 	{
@@ -604,10 +617,13 @@ static void parse_options_check(const struct option *opts)
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
+		case OPTION_SET_INT:
+			if (!signed_int_fits(opts->defval, opts->precision))
+				optbug(opts, "has invalid defval");
+			/* fallthru */
 		case OPTION_COUNTUP:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
-		case OPTION_SET_INT:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/parse-options.h b/parse-options.h
index c75a473c9e..71516e4b5b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -190,6 +190,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | (f), \
 	.defval = (i), \
@@ -260,6 +261,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \
 	.defval = 1, \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 1e03ff88f6..2ba2546d70 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -131,6 +131,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.short_name = 'B',
 			.long_name = "no-fear",
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "be brave",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = 1,
-- 
2.50.0

Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT

[Patrick Steinhardt]

On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:
> diff --git a/parse-options.c b/parse-options.c
> index da07a000a3..bbb68603cc 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)
>  	}
>  }
>  
> +static enum parse_opt_result set_int_value(const struct option *opt,
> +					   enum opt_parsed flags,
> +					   intmax_t value)
> +{
> +	switch (opt->precision) {
> +	case sizeof(int8_t):
> +		*(int8_t *)opt->value = value;
> +		return 0;
> +	case sizeof(int16_t):
> +		*(int16_t *)opt->value = value;
> +		return 0;
> +	case sizeof(int32_t):
> +		*(int32_t *)opt->value = value;
> +		return 0;
> +	case sizeof(int64_t):
> +		*(int64_t *)opt->value = value;
> +		return 0;
> +	default:
> +		BUG("invalid precision for option %s", optname(opt, flags));
> +	}
> +}

The function only ever dies or returns successfully, so can't we make it
return nothing instead? On the other hand it does make a couple of
callsites a bit nicer to read.

> +static int signed_int_fits(intmax_t value, size_t size)
> +{
> +	size_t bits = size * CHAR_BIT;
> +	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
> +	intmax_t lower_bound = -upper_bound - 1;
> +	return lower_bound <= value && value <= upper_bound;
> +}
> +

Should we s/size/precision/ so that it's clear what kind of size this
exactly is?

Patrick

Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT

[René Scharfe]

On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:
>> diff --git a/parse-options.c b/parse-options.c
>> index da07a000a3..bbb68603cc 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)
>>  	}
>>  }
>>  
>> +static enum parse_opt_result set_int_value(const struct option *opt,
>> +					   enum opt_parsed flags,
>> +					   intmax_t value)
>> +{
>> +	switch (opt->precision) {
>> +	case sizeof(int8_t):
>> +		*(int8_t *)opt->value = value;
>> +		return 0;
>> +	case sizeof(int16_t):
>> +		*(int16_t *)opt->value = value;
>> +		return 0;
>> +	case sizeof(int32_t):
>> +		*(int32_t *)opt->value = value;
>> +		return 0;
>> +	case sizeof(int64_t):
>> +		*(int64_t *)opt->value = value;
>> +		return 0;
>> +	default:
>> +		BUG("invalid precision for option %s", optname(opt, flags));
>> +	}
>> +}
> 
> The function only ever dies or returns successfully, so can't we make it
> return nothing instead? On the other hand it does make a couple of
> callsites a bit nicer to read.

We can.  Not sure we should, obviously, but I don't have strong feelings
either way.

> 
>> +static int signed_int_fits(intmax_t value, size_t size)
>> +{
>> +	size_t bits = size * CHAR_BIT;
>> +	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
>> +	intmax_t lower_bound = -upper_bound - 1;
>> +	return lower_bound <= value && value <= upper_bound;
>> +}
>> +
> 
> Should we s/size/precision/ so that it's clear what kind of size this
> exactly is?
It's the width of an integer variable as in sizeof(), so the name fits.
We can inline this single-caller function if it's indeed confusing.

René

Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT

[Patrick Steinhardt]

On Tue, Jul 01, 2025 at 05:54:41PM +0200, René Scharfe wrote:
> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> > On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:
> >> diff --git a/parse-options.c b/parse-options.c
> >> index da07a000a3..bbb68603cc 100644
> >> --- a/parse-options.c
> >> +++ b/parse-options.c
> >> +static int signed_int_fits(intmax_t value, size_t size)
> >> +{
> >> +	size_t bits = size * CHAR_BIT;
> >> +	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
> >> +	intmax_t lower_bound = -upper_bound - 1;
> >> +	return lower_bound <= value && value <= upper_bound;
> >> +}
> >> +
> > 
> > Should we s/size/precision/ so that it's clear what kind of size this
> > exactly is?
> It's the width of an integer variable as in sizeof(), so the name fits.
> We can inline this single-caller function if it's indeed confusing.

The issue to me is rather that it's unclear what the unit is. Is it size
in bytes, bits, nibbles? You wouldn't know that the expectation is that
the caller passes in `sizeof()` without taking a deeper look.

In any case, this is only a minor nit in the first place, not worth much
bikeshedding.

Patrick

[PATCH 3/6] parse-options: add precision handling for OPTION_BIT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_BIT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/write-tree.c |  1 +
 parse-options.c      | 11 +++++++----
 parse-options.h      |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 5a8dc377ec..cfec044710 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -35,6 +35,7 @@ int cmd_write_tree(int argc,
 			.type = OPTION_BIT,
 			.long_name = "ignore-cache-tree",
 			.value = &flags,
+			.precision = sizeof(flags),
 			.help = N_("only useful for debugging"),
 			.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG,
 			.defval = WRITE_TREE_IGNORE_CACHE_TREE,
diff --git a/parse-options.c b/parse-options.c
index bbb68603cc..47a77d2cea 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -136,11 +136,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return opt->ll_callback(p, opt, NULL, unset);
 
 	case OPTION_BIT:
+	{
+		intmax_t value = get_int_value(opt);
 		if (unset)
-			*(int *)opt->value &= ~opt->defval;
+			value &= ~opt->defval;
 		else
-			*(int *)opt->value |= opt->defval;
-		return 0;
+			value |= opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_NEGBIT:
 		if (unset)
@@ -618,11 +621,11 @@ static void parse_options_check(const struct option *opts)
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
 		case OPTION_SET_INT:
+		case OPTION_BIT:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
-		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
diff --git a/parse-options.h b/parse-options.h
index 71516e4b5b..6501ca3c27 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -172,6 +172,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 	.callback = NULL, \
-- 
2.50.0

[PATCH 4/6] parse-options: add precision handling for OPTION_NEGBIT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_NEGBIT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/rebase.c |  1 +
 parse-options.c  | 11 +++++++----
 parse-options.h  |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2e8c4ee678..e90562a3b8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1128,6 +1128,7 @@ int cmd_rebase(int argc,
 			.short_name = 'n',
 			.long_name = "no-stat",
 			.value = &options.flags,
+			.precision = sizeof(options.flags),
 			.help = N_("do not show diffstat of what changed upstream"),
 			.flags = PARSE_OPT_NOARG,
 			.defval = REBASE_DIFFSTAT,
diff --git a/parse-options.c b/parse-options.c
index 47a77d2cea..6bd7158806 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -146,11 +146,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_NEGBIT:
+	{
+		intmax_t value = get_int_value(opt);
 		if (unset)
-			*(int *)opt->value |= opt->defval;
+			value |= opt->defval;
 		else
-			*(int *)opt->value &= ~opt->defval;
-		return 0;
+			value &= ~opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_BITOP:
 		if (unset)
@@ -622,11 +625,11 @@ static void parse_options_check(const struct option *opts)
 		switch (opts->type) {
 		case OPTION_SET_INT:
 		case OPTION_BIT:
+		case OPTION_NEGBIT:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
-		case OPTION_NEGBIT:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/parse-options.h b/parse-options.h
index 6501ca3c27..076f88b384 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -250,6 +250,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG, \
 	.defval = (b), \
-- 
2.50.0

[PATCH 5/6] parse-options: add precision handling for OPTION_BITOP

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_BITOP.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Checking "defval" has the side-effect of also requiring PARSE_OPT_NOARG.
This is sensible, as OPTION_BITOP doesn't handle any arguments, so we
take this unintended benefit.

Don't check "extra", though, as its value is only used to clear bits, so
cannot lead to an overflow.  Not checking continues to allow e.g., using
-1 to clear all bits even if the value variable has a narrower type than
intptr_t.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c | 10 +++++++---
 parse-options.h |  1 +
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6bd7158806..0dc9b0324a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -156,11 +156,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_BITOP:
+	{
+		intmax_t value = get_int_value(opt);
 		if (unset)
 			BUG("BITOP can't have unset form");
-		*(int *)opt->value &= ~opt->extra;
-		*(int *)opt->value |= opt->defval;
-		return 0;
+		value &= ~opt->extra;
+		value |= opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_COUNTUP:
 		if (*(int *)opt->value < 0)
@@ -626,6 +629,7 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_SET_INT:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
+		case OPTION_BITOP:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
diff --git a/parse-options.h b/parse-options.h
index 076f88b384..8bdf469ae9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -240,6 +240,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|PARSE_OPT_NONEG, \
 	.defval = (set), \
-- 
2.50.0

Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP

[Patrick Steinhardt]

On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:
> Similar to 09705696f7 (parse-options: introduce precision handling for
> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
> for OPTION_BITOP.  Do that by requiring their "precision" to be set,
> casting their "value" pointer accordingly and checking whether the value
> fits.
> 
> Checking "defval" has the side-effect of also requiring PARSE_OPT_NOARG.

Hm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in
this patch at all, so I'm a but puzzled.

Patrick

Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP

[René Scharfe]

On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:
>> Similar to 09705696f7 (parse-options: introduce precision handling for
>> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
>> for OPTION_BITOP.  Do that by requiring their "precision" to be set,
>> casting their "value" pointer accordingly and checking whether the value
>> fits.
>>
>> Checking "defval" has the side-effect of also requiring PARSE_OPT_NOARG.
> 
> Hm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in
> this patch at all, so I'm a but puzzled.

For options with OPTION_BITOP.  Adding the defval check also adds the
no-argument check by falling through to it:

diff --git a/parse-options.c b/parse-options.c
index 6bd7158806..0dc9b0324a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -620,18 +623,19 @@ static void parse_options_check(const struct option *opts)
 			optbug(opts, "uses feature "
 			       "not supported for dashless options");
 		if (opts->type == OPTION_SET_INT && !opts->defval &&
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
 		case OPTION_SET_INT:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
+		case OPTION_BITOP:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
 				optbug(opts, "should not accept an argument");
 			break;

Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP

[Patrick Steinhardt]

On Tue, Jul 01, 2025 at 05:21:11PM +0200, René Scharfe wrote:
> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> > On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:
> >> Similar to 09705696f7 (parse-options: introduce precision handling for
> >> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
> >> for OPTION_BITOP.  Do that by requiring their "precision" to be set,
> >> casting their "value" pointer accordingly and checking whether the value
> >> fits.
> >>
> >> Checking "defval" has the side-effect of also requiring PARSE_OPT_NOARG.
> > 
> > Hm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in
> > this patch at all, so I'm a but puzzled.
> 
> For options with OPTION_BITOP.  Adding the defval check also adds the
> no-argument check by falling through to it:
> 
> diff --git a/parse-options.c b/parse-options.c
> index 6bd7158806..0dc9b0324a 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -620,18 +623,19 @@ static void parse_options_check(const struct option *opts)
>  			optbug(opts, "uses feature "
>  			       "not supported for dashless options");
>  		if (opts->type == OPTION_SET_INT && !opts->defval &&
>  		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
>  			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
>  		switch (opts->type) {
>  		case OPTION_SET_INT:
>  		case OPTION_BIT:
>  		case OPTION_NEGBIT:
> +		case OPTION_BITOP:
>  			if (!signed_int_fits(opts->defval, opts->precision))
>  				optbug(opts, "has invalid defval");
>  			/* fallthru */
>  		case OPTION_COUNTUP:
>  		case OPTION_NUMBER:
>  			if ((opts->flags & PARSE_OPT_OPTARG) ||
>  			    !(opts->flags & PARSE_OPT_NOARG))
>  				optbug(opts, "should not accept an argument");
>  			break;

Ah, now I see it, the extended context definitely helps. Might be nice
to point out explicitly in the commit message that it's about the
fallthrough behaviour.

Patrick

[PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_COUNTUP.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c               | 22 +++++++++++++++++-----
 parse-options.h               |  1 +
 t/helper/test-parse-options.c |  3 +++
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 0dc9b0324a..0dd08a3a77 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_COUNTUP:
-		if (*(int *)opt->value < 0)
-			*(int *)opt->value = 0;
-		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
-		return 0;
+	{
+		size_t bits = CHAR_BIT * opt->precision;
+		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
+		intmax_t value = get_int_value(opt);
+
+		if (value < 0)
+			value = 0;
+		if (unset)
+			value = 0;
+		else if (value < upper_bound)
+			value++;
+		else
+			return error(_("value for %s exceeds %"PRIdMAX),
+				     optname(opt, flags), upper_bound);
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_SET_INT:
 		return set_int_value(opt, flags, unset ? 0 : opt->defval);
@@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_BITOP:
+		case OPTION_COUNTUP:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
-		case OPTION_COUNTUP:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/parse-options.h b/parse-options.h
index 8bdf469ae9..312045604d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -183,6 +183,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 }
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 2ba2546d70..68579d83f3 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -178,6 +178,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.short_name = '+',
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "same as -b",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,
 		},
@@ -185,6 +186,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "positive ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},
@@ -192,6 +194,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "no-ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "negative ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},
-- 
2.50.0

Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP

[Patrick Steinhardt]

On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 0dc9b0324a..0dd08a3a77 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>  	}
>  
>  	case OPTION_COUNTUP:
> -		if (*(int *)opt->value < 0)
> -			*(int *)opt->value = 0;
> -		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> -		return 0;
> +	{
> +		size_t bits = CHAR_BIT * opt->precision;
> +		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
> +		intmax_t value = get_int_value(opt);
> +
> +		if (value < 0)
> +			value = 0;
> +		if (unset)
> +			value = 0;
> +		else if (value < upper_bound)
> +			value++;
> +		else
> +			return error(_("value for %s exceeds %"PRIdMAX),
> +				     optname(opt, flags), upper_bound);
> +		return set_int_value(opt, flags, value);
> +	}
>  
>  	case OPTION_SET_INT:
>  		return set_int_value(opt, flags, unset ? 0 : opt->defval);
> @@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)
>  		case OPTION_BIT:
>  		case OPTION_NEGBIT:
>  		case OPTION_BITOP:
> +		case OPTION_COUNTUP:
>  			if (!signed_int_fits(opts->defval, opts->precision))
>  				optbug(opts, "has invalid defval");
>  			/* fallthru */
> -		case OPTION_COUNTUP:
>  		case OPTION_NUMBER:
>  			if ((opts->flags & PARSE_OPT_OPTARG) ||
>  			    !(opts->flags & PARSE_OPT_NOARG))
> diff --git a/parse-options.h b/parse-options.h
> index 8bdf469ae9..312045604d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -183,6 +183,7 @@ struct option {
>  	.short_name = (s), \
>  	.long_name = (l), \
>  	.value = (v), \

It's a bit surprising that `COUNTUP` accepts a signed integer, so should
we maybe add `BARF_UNLESS_SIGNED(*(v))` here?

> +	.precision = sizeof(*v), \
>  	.help = (h), \
>  	.flags = PARSE_OPT_NOARG|(f), \
>  }

Patrick

Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP

[René Scharfe]

On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:
>> diff --git a/parse-options.c b/parse-options.c
>> index 0dc9b0324a..0dd08a3a77 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
>>  	}
>>  
>>  	case OPTION_COUNTUP:
>> -		if (*(int *)opt->value < 0)
>> -			*(int *)opt->value = 0;
>> -		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>> -		return 0;
>> +	{
>> +		size_t bits = CHAR_BIT * opt->precision;
>> +		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
>> +		intmax_t value = get_int_value(opt);
>> +
>> +		if (value < 0)
>> +			value = 0;
>> +		if (unset)
>> +			value = 0;
>> +		else if (value < upper_bound)
>> +			value++;
>> +		else
>> +			return error(_("value for %s exceeds %"PRIdMAX),
>> +				     optname(opt, flags), upper_bound);
>> +		return set_int_value(opt, flags, value);
>> +	}
>>  
>>  	case OPTION_SET_INT:
>>  		return set_int_value(opt, flags, unset ? 0 : opt->defval);
>> @@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)
>>  		case OPTION_BIT:
>>  		case OPTION_NEGBIT:
>>  		case OPTION_BITOP:
>> +		case OPTION_COUNTUP:
>>  			if (!signed_int_fits(opts->defval, opts->precision))
>>  				optbug(opts, "has invalid defval");
>>  			/* fallthru */
>> -		case OPTION_COUNTUP:
>>  		case OPTION_NUMBER:
>>  			if ((opts->flags & PARSE_OPT_OPTARG) ||
>>  			    !(opts->flags & PARSE_OPT_NOARG))
>> diff --git a/parse-options.h b/parse-options.h
>> index 8bdf469ae9..312045604d 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -183,6 +183,7 @@ struct option {
>>  	.short_name = (s), \
>>  	.long_name = (l), \
>>  	.value = (v), \
> 
> It's a bit surprising that `COUNTUP` accepts a signed integer, so should
> we maybe add `BARF_UNLESS_SIGNED(*(v))` here?

Perhaps, but that would require more changes to callers that use unsigned
variables than I can stomach.  That's why I declared it out of scope for
this series in its cover letter.  Later, unless (hopefully) someone beats
me to it.

> 
>> +	.precision = sizeof(*v), \
>>  	.help = (h), \
>>  	.flags = PARSE_OPT_NOARG|(f), \
>>  }
> 
> Patrick

Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP

[Patrick Steinhardt]

On Tue, Jul 01, 2025 at 06:01:44PM +0200, René Scharfe wrote:
> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> > On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:
> >> diff --git a/parse-options.c b/parse-options.c
> >> index 0dc9b0324a..0dd08a3a77 100644
> >> --- a/parse-options.c
> >> +++ b/parse-options.c
> >> diff --git a/parse-options.h b/parse-options.h
> >> index 8bdf469ae9..312045604d 100644
> >> --- a/parse-options.h
> >> +++ b/parse-options.h
> >> @@ -183,6 +183,7 @@ struct option {
> >>  	.short_name = (s), \
> >>  	.long_name = (l), \
> >>  	.value = (v), \
> > 
> > It's a bit surprising that `COUNTUP` accepts a signed integer, so should
> > we maybe add `BARF_UNLESS_SIGNED(*(v))` here?
> 
> Perhaps, but that would require more changes to callers that use unsigned
> variables than I can stomach.  That's why I declared it out of scope for
> this series in its cover letter.  Later, unless (hopefully) someone beats
> me to it.

Fine with me, thanks!

Patrick

[PATCH v2 0/7] parse-options: add more precision handling

[René Scharfe]

Extend precision handling to all options that currently blindly use an
int pointer to access value variables.  This allows the safe use of
smaller and larger integer types.

Sign handling might be nice as well (especially for OPTION_COUNTUP), but
is out of scope for this series.

Changes since v1:
- Enable PARSE_OPT_NOARG checking for OPTION_BITOP explicitly in a new
  separate patch instead of as a side-effect.
- Split out do_get_int_value().
- This allows us to use optbug() only where it makes sense, in
  build_cmdmode_list().  This lets the function report all
  PARSE_OPT_CMDMODE options with invalid precision in one go.
- Use the same BUG call in get_int_value() as in set_int_value(),
  for consistency.
- Store the precision in build_cmdmode_list() instead of an example
  option, which is hopefully easier to follow.
- Rename the size parameter of signed_int_fits() for consistency.

  parse-options: require PARSE_OPT_NOARG for OPTION_BITOP
  parse-options: add precision handling for PARSE_OPT_CMDMODE
  parse-options: add precision handling for OPTION_SET_INT
  parse-options: add precision handling for OPTION_BIT
  parse-options: add precision handling for OPTION_NEGBIT
  parse-options: add precision handling for OPTION_BITOP
  parse-options: add precision handling for OPTION_COUNTUP

 builtin/am.c                  |   1 +
 builtin/rebase.c              |   1 +
 builtin/update-index.c        |   6 ++
 builtin/write-tree.c          |   1 +
 parse-options.c               | 155 +++++++++++++++++++++++++---------
 parse-options.h               |   7 ++
 t/helper/test-parse-options.c |  17 +++-
 7 files changed, 146 insertions(+), 42 deletions(-)

Interdiff against v1:
diff --git a/parse-options.c b/parse-options.c
index 0dd08a3a77..5224203ffe 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -68,23 +68,34 @@ static char *fix_filename(const char *prefix, const char *file)
 		return prefix_filename_except_for_dash(prefix, file);
 }
 
-static intmax_t get_int_value(const struct option *opt)
+static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
 {
-	switch (opt->precision) {
+	switch (precision) {
 	case sizeof(int8_t):
-		return *(int8_t *)opt->value;
+		*ret = *(int8_t *)value;
+		return 0;
 	case sizeof(int16_t):
-		return *(int16_t *)opt->value;
+		*ret = *(int16_t *)value;
+		return 0;
 	case sizeof(int32_t):
-		return *(int32_t *)opt->value;
+		*ret = *(int32_t *)value;
+		return 0;
 	case sizeof(int64_t):
-		return *(int64_t *)opt->value;
+		*ret = *(int64_t *)value;
+		return 0;
 	default:
-		optbug(opt, "has invalid precision");
-		BUG("invalid 'struct option'");
+		return -1;
 	}
 }
 
+static intmax_t get_int_value(const struct option *opt, enum opt_parsed flags)
+{
+	intmax_t ret;
+	if (do_get_int_value(opt->value, opt->precision, &ret))
+		BUG("invalid precision for option %s", optname(opt, flags));
+	return ret;
+}
+
 static enum parse_opt_result set_int_value(const struct option *opt,
 					   enum opt_parsed flags,
 					   intmax_t value)
@@ -107,9 +118,9 @@ static enum parse_opt_result set_int_value(const struct option *opt,
 	}
 }
 
-static int signed_int_fits(intmax_t value, size_t size)
+static int signed_int_fits(intmax_t value, size_t precision)
 {
-	size_t bits = size * CHAR_BIT;
+	size_t bits = precision * CHAR_BIT;
 	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
 	intmax_t lower_bound = -upper_bound - 1;
 	return lower_bound <= value && value <= upper_bound;
@@ -137,7 +148,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 
 	case OPTION_BIT:
 	{
-		intmax_t value = get_int_value(opt);
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
 			value &= ~opt->defval;
 		else
@@ -147,7 +158,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 
 	case OPTION_NEGBIT:
 	{
-		intmax_t value = get_int_value(opt);
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
 			value |= opt->defval;
 		else
@@ -157,7 +168,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 
 	case OPTION_BITOP:
 	{
-		intmax_t value = get_int_value(opt);
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
 			BUG("BITOP can't have unset form");
 		value &= ~opt->extra;
@@ -169,7 +180,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	{
 		size_t bits = CHAR_BIT * opt->precision;
 		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
-		intmax_t value = get_int_value(opt);
+		intmax_t value = get_int_value(opt, flags);
 
 		if (value < 0)
 			value = 0;
@@ -318,7 +329,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 
 struct parse_opt_cmdmode_list {
 	intmax_t value;
-	const struct option *opt, *reference_opt;
+	void *value_ptr;
+	size_t precision;
+	const struct option *opt;
 	const char *arg;
 	enum opt_parsed flags;
 	struct parse_opt_cmdmode_list *next;
@@ -331,21 +344,25 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 	for (; opts->type != OPTION_END; opts++) {
 		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
+		void *value_ptr = opts->value;
 
-		if (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)
+		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
 			continue;
 
-		while (elem && elem->reference_opt->value != opts->value)
+		while (elem && elem->value_ptr != value_ptr)
 			elem = elem->next;
 		if (elem)
 			continue;
 
 		CALLOC_ARRAY(elem, 1);
-		elem->reference_opt = opts;
-		elem->value = get_int_value(opts);
+		elem->value_ptr = value_ptr;
+		elem->precision = opts->precision;
+		if (do_get_int_value(value_ptr, opts->precision, &elem->value))
+			optbug(opts, "has invalid precision");
 		elem->next = ctx->cmdmode_list;
 		ctx->cmdmode_list = elem;
 	}
+	BUG_if_bug("invalid 'struct option'");
 }
 
 static char *optnamearg(const struct option *opt, const char *arg,
@@ -367,7 +384,11 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	char *opt_name, *other_opt_name;
 
 	for (; elem; elem = elem->next) {
-		intmax_t new_value = get_int_value(elem->reference_opt);
+		intmax_t new_value;
+
+		if (do_get_int_value(elem->value_ptr, elem->precision,
+				     &new_value))
+			BUG("impossible: invalid precision");
 
 		if (new_value == elem->value)
 			continue;
-- 
2.50.0

[PATCH v2 1/7] parse-options: require PARSE_OPT_NOARG for OPTION_BITOP

[René Scharfe]

OPTION_BITOP options don't take arguments.  Make sure they are declared
that way using the flag PARSE_OPT_NOARG.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..68ff494492 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -591,6 +591,7 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_NEGBIT:
 		case OPTION_SET_INT:
 		case OPTION_NUMBER:
+		case OPTION_BITOP:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
 				optbug(opts, "should not accept an argument");
-- 
2.50.0

Re: [PATCH v2 1/7] parse-options: require PARSE_OPT_NOARG for OPTION_BITOP

[Patrick Steinhardt]

On Wed, Jul 09, 2025 at 11:44:09AM +0200, René Scharfe wrote:
> OPTION_BITOP options don't take arguments.  Make sure they are declared
> that way using the flag PARSE_OPT_NOARG.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>

Nice, splitting out this commit makes it way easier to understand.

Patrick

[PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE

[René Scharfe]

Build on 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) to support value variables of different
sizes for PARSE_OPT_CMDMODE options.  Do that by requiring their
"precision" to be set and casting their "value" pointer accordingly.

Call the function that does the raw casting do_get_int_value() to
reserve the name get_int_value() for a more friendly wrapper we're
going to introduce in one of the next patches.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c                  |  1 +
 parse-options.c               | 41 ++++++++++++++++++++++++++++++-----
 parse-options.h               |  1 +
 t/helper/test-parse-options.c | 13 ++++++++---
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a800003340..c9d925f7b9 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2406,6 +2406,7 @@ int cmd_am(int argc,
 			.type = OPTION_CALLBACK,
 			.long_name = "show-current-patch",
 			.value = &resume_mode,
+			.precision = sizeof(resume_mode),
 			.argh = "(diff|raw)",
 			.help = N_("show the patch being applied"),
 			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
diff --git a/parse-options.c b/parse-options.c
index 68ff494492..ddac008a5e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)
 		return prefix_filename_except_for_dash(prefix, file);
 }
 
+static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
+{
+	switch (precision) {
+	case sizeof(int8_t):
+		*ret = *(int8_t *)value;
+		return 0;
+	case sizeof(int16_t):
+		*ret = *(int16_t *)value;
+		return 0;
+	case sizeof(int32_t):
+		*ret = *(int32_t *)value;
+		return 0;
+	case sizeof(int64_t):
+		*ret = *(int64_t *)value;
+		return 0;
+	default:
+		return -1;
+	}
+}
+
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  const struct option *opt,
 					  enum opt_parsed flags,
@@ -266,7 +286,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 }
 
 struct parse_opt_cmdmode_list {
-	int value, *value_ptr;
+	intmax_t value;
+	void *value_ptr;
+	size_t precision;
 	const struct option *opt;
 	const char *arg;
 	enum opt_parsed flags;
@@ -280,7 +302,7 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 	for (; opts->type != OPTION_END; opts++) {
 		struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
-		int *value_ptr = opts->value;
+		void *value_ptr = opts->value;
 
 		if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
 			continue;
@@ -292,10 +314,13 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
 
 		CALLOC_ARRAY(elem, 1);
 		elem->value_ptr = value_ptr;
-		elem->value = *value_ptr;
+		elem->precision = opts->precision;
+		if (do_get_int_value(value_ptr, opts->precision, &elem->value))
+			optbug(opts, "has invalid precision");
 		elem->next = ctx->cmdmode_list;
 		ctx->cmdmode_list = elem;
 	}
+	BUG_if_bug("invalid 'struct option'");
 }
 
 static char *optnamearg(const struct option *opt, const char *arg,
@@ -317,7 +342,13 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	char *opt_name, *other_opt_name;
 
 	for (; elem; elem = elem->next) {
-		if (*elem->value_ptr == elem->value)
+		intmax_t new_value;
+
+		if (do_get_int_value(elem->value_ptr, elem->precision,
+				     &new_value))
+			BUG("impossible: invalid precision");
+
+		if (new_value == elem->value)
 			continue;
 
 		if (elem->opt &&
@@ -327,7 +358,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		elem->opt = opt;
 		elem->arg = arg;
 		elem->flags = flags;
-		elem->value = *elem->value_ptr;
+		elem->value = new_value;
 	}
 
 	if (result || !elem)
diff --git a/parse-options.h b/parse-options.h
index 91c3e3c29b..c75a473c9e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -269,6 +269,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index f2663dd0c0..1e03ff88f6 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -148,9 +148,16 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_SET_INT(0, "set23", &integer, "set integer to 23", 23),
 		OPT_CMDMODE(0, "mode1", &integer, "set integer to 1 (cmdmode option)", 1),
 		OPT_CMDMODE(0, "mode2", &integer, "set integer to 2 (cmdmode option)", 2),
-		OPT_CALLBACK_F(0, "mode34", &integer, "(3|4)",
-			"set integer to 3 or 4 (cmdmode option)",
-			PARSE_OPT_CMDMODE, mode34_callback),
+		{
+			.type = OPTION_CALLBACK,
+			.long_name = "mode34",
+			.value = &integer,
+			.precision = sizeof(integer),
+			.argh = "(3|4)",
+			.help = "set integer to 3 or 4 (cmdmode option)",
+			.flags = PARSE_OPT_CMDMODE,
+			.callback = mode34_callback,
+		},
 		OPT_CALLBACK('L', "length", &integer, "str",
 			"get length of <str>", length_callback),
 		OPT_FILENAME('F', "file", &file, "set file to <file>"),
-- 
2.50.0

Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE

[Patrick Steinhardt]

On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:
> Build on 09705696f7 (parse-options: introduce precision handling for
> `OPTION_INTEGER`, 2025-04-17) to support value variables of different
> sizes for PARSE_OPT_CMDMODE options.  Do that by requiring their
> "precision" to be set and casting their "value" pointer accordingly.
> 
> Call the function that does the raw casting do_get_int_value() to
> reserve the name get_int_value() for a more friendly wrapper we're
> going to introduce in one of the next patches.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/am.c                  |  1 +
>  parse-options.c               | 41 ++++++++++++++++++++++++++++++-----
>  parse-options.h               |  1 +
>  t/helper/test-parse-options.c | 13 ++++++++---
>  4 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/am.c b/builtin/am.c
> index a800003340..c9d925f7b9 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2406,6 +2406,7 @@ int cmd_am(int argc,
>  			.type = OPTION_CALLBACK,
>  			.long_name = "show-current-patch",
>  			.value = &resume_mode,
> +			.precision = sizeof(resume_mode),
>  			.argh = "(diff|raw)",
>  			.help = N_("show the patch being applied"),
>  			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> diff --git a/parse-options.c b/parse-options.c
> index 68ff494492..ddac008a5e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)
>  		return prefix_filename_except_for_dash(prefix, file);
>  }
>  
> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)

Nit: after the fourth patch we have `do_get_int_value()` and
`get_int_value()`, where the major difference is that the latter dies if
we failed to parse the value. It might be easier to discern which is
which if we called them `get_int_value()` and `get_int_value_or_die()`.

Patrick

Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE

[René Scharfe]

On 7/9/25 3:58 PM, Patrick Steinhardt wrote:
> On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:
>>
>> Call the function that does the raw casting do_get_int_value() to
>> reserve the name get_int_value() for a more friendly wrapper we're
>> going to introduce in one of the next patches.

>> diff --git a/parse-options.c b/parse-options.c
>> index 68ff494492..ddac008a5e 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)
>>  		return prefix_filename_except_for_dash(prefix, file);
>>  }
>>  
>> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
> 
> Nit: after the fourth patch we have `do_get_int_value()` and
> `get_int_value()`, where the major difference is that the latter dies if
> we failed to parse the value. It might be easier to discern which is
> which if we called them `get_int_value()` and `get_int_value_or_die()`.

That would be misleading because get_int_value() doesn't die() like a
function from write-or-die.c, it BUGs instead.  I don't think it makes
sense to advertise the presence of assertions in a function's name.
But we do have a tradition of using a prefix of "do_" with wrapped
functions that have a more raw interface and do the actual work.

Nit: They don't parse, but cast a void pointer to the appropriate type
and dereference it.

René

Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE

[Patrick Steinhardt]

On Wed, Jul 09, 2025 at 05:05:35PM +0200, René Scharfe wrote:
> On 7/9/25 3:58 PM, Patrick Steinhardt wrote:
> > On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:
> >>
> >> Call the function that does the raw casting do_get_int_value() to
> >> reserve the name get_int_value() for a more friendly wrapper we're
> >> going to introduce in one of the next patches.
> 
> >> diff --git a/parse-options.c b/parse-options.c
> >> index 68ff494492..ddac008a5e 100644
> >> --- a/parse-options.c
> >> +++ b/parse-options.c
> >> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)
> >>  		return prefix_filename_except_for_dash(prefix, file);
> >>  }
> >>  
> >> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
> > 
> > Nit: after the fourth patch we have `do_get_int_value()` and
> > `get_int_value()`, where the major difference is that the latter dies if
> > we failed to parse the value. It might be easier to discern which is
> > which if we called them `get_int_value()` and `get_int_value_or_die()`.
> 
> That would be misleading because get_int_value() doesn't die() like a
> function from write-or-die.c, it BUGs instead.  I don't think it makes
> sense to advertise the presence of assertions in a function's name.
> But we do have a tradition of using a prefix of "do_" with wrapped
> functions that have a more raw interface and do the actual work.
> 
> Nit: They don't parse, but cast a void pointer to the appropriate type
> and dereference it.

Fair enough. I don't mind it much either way, thanks!

Patrick

Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> diff --git a/builtin/am.c b/builtin/am.c
>> index a800003340..c9d925f7b9 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2406,6 +2406,7 @@ int cmd_am(int argc,
>>  			.type = OPTION_CALLBACK,
>>  			.long_name = "show-current-patch",
>>  			.value = &resume_mode,
>> +			.precision = sizeof(resume_mode),
>>  			.argh = "(diff|raw)",
>>  			.help = N_("show the patch being applied"),
>>  			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>> diff --git a/parse-options.c b/parse-options.c
>> index 68ff494492..ddac008a5e 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)
>>  		return prefix_filename_except_for_dash(prefix, file);
>>  }
>>  
>> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
>
> Nit: after the fourth patch we have `do_get_int_value()` and
> `get_int_value()`, where the major difference is that the latter dies if
> we failed to parse the value. It might be easier to discern which is
> which if we called them `get_int_value()` and `get_int_value_or_die()`.

Seeing the symmetry between set_int_value() and do_get_int_value(),
I tend to agree that it would be easier to remember what the latter
does if it were named get_int_value().

I am not so sure about _or_die(), though.  The only kind of error
the current get_int_value() detects and acts on is the mismatched
precision bug, which is a programmer error; there is no "die" but
we call "BUG".

get_int_value_with_precision_check() is quite a mouthful, but it is
known that I am bad at naming X-<, so I dunno.

[PATCH v2 3/7] parse-options: add precision handling for OPTION_SET_INT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_SET_INT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Factor out the casting code from the part of do_get_value() that handles
OPTION_INTEGER to avoid code duplication.  We're going to use it in the
next patches as well.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/update-index.c        |  6 ++++
 parse-options.c               | 56 ++++++++++++++++++++++-------------
 parse-options.h               |  2 ++
 t/helper/test-parse-options.c |  1 +
 4 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 538b619ba4..0c1d4ed55b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -981,6 +981,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("mark files as \"not changing\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -989,6 +990,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-assume-unchanged",
 			.value = &mark_valid_only,
+			.precision = sizeof(mark_valid_only),
 			.help = N_("clear assumed-unchanged bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -997,6 +999,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("mark files as \"index-only\""),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1005,6 +1008,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-skip-worktree",
 			.value = &mark_skip_worktree_only,
+			.precision = sizeof(mark_skip_worktree_only),
 			.help = N_("clear skip-worktree bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
@@ -1079,6 +1083,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("mark files as fsmonitor valid"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = MARK_FLAG,
@@ -1087,6 +1092,7 @@ int cmd_update_index(int argc,
 			.type = OPTION_SET_INT,
 			.long_name = "no-fsmonitor-valid",
 			.value = &mark_fsmonitor_only,
+			.precision = sizeof(mark_fsmonitor_only),
 			.help = N_("clear fsmonitor valid bit"),
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = UNMARK_FLAG,
diff --git a/parse-options.c b/parse-options.c
index ddac008a5e..639f41b83b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -88,6 +88,36 @@ static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
 	}
 }
 
+static enum parse_opt_result set_int_value(const struct option *opt,
+					   enum opt_parsed flags,
+					   intmax_t value)
+{
+	switch (opt->precision) {
+	case sizeof(int8_t):
+		*(int8_t *)opt->value = value;
+		return 0;
+	case sizeof(int16_t):
+		*(int16_t *)opt->value = value;
+		return 0;
+	case sizeof(int32_t):
+		*(int32_t *)opt->value = value;
+		return 0;
+	case sizeof(int64_t):
+		*(int64_t *)opt->value = value;
+		return 0;
+	default:
+		BUG("invalid precision for option %s", optname(opt, flags));
+	}
+}
+
+static int signed_int_fits(intmax_t value, size_t precision)
+{
+	size_t bits = precision * CHAR_BIT;
+	intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
+	intmax_t lower_bound = -upper_bound - 1;
+	return lower_bound <= value && value <= upper_bound;
+}
+
 static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  const struct option *opt,
 					  enum opt_parsed flags,
@@ -136,8 +166,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return 0;
 
 	case OPTION_SET_INT:
-		*(int *)opt->value = unset ? 0 : opt->defval;
-		return 0;
+		return set_int_value(opt, flags, unset ? 0 : opt->defval);
 
 	case OPTION_STRING:
 		if (unset)
@@ -219,23 +248,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
 
-		switch (opt->precision) {
-		case 1:
-			*(int8_t *)opt->value = value;
-			return 0;
-		case 2:
-			*(int16_t *)opt->value = value;
-			return 0;
-		case 4:
-			*(int32_t *)opt->value = value;
-			return 0;
-		case 8:
-			*(int64_t *)opt->value = value;
-			return 0;
-		default:
-			BUG("invalid precision for option %s",
-			    optname(opt, flags));
-		}
+		return set_int_value(opt, flags, value);
 	}
 	case OPTION_UNSIGNED:
 	{
@@ -617,10 +630,13 @@ static void parse_options_check(const struct option *opts)
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
+		case OPTION_SET_INT:
+			if (!signed_int_fits(opts->defval, opts->precision))
+				optbug(opts, "has invalid defval");
+			/* fallthru */
 		case OPTION_COUNTUP:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
-		case OPTION_SET_INT:
 		case OPTION_NUMBER:
 		case OPTION_BITOP:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
diff --git a/parse-options.h b/parse-options.h
index c75a473c9e..71516e4b5b 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -190,6 +190,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | (f), \
 	.defval = (i), \
@@ -260,6 +261,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \
 	.defval = 1, \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 1e03ff88f6..2ba2546d70 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -131,6 +131,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.short_name = 'B',
 			.long_name = "no-fear",
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "be brave",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 			.defval = 1,
-- 
2.50.0

[PATCH v2 4/7] parse-options: add precision handling for OPTION_BIT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_BIT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/write-tree.c |  1 +
 parse-options.c      | 19 +++++++++++++++----
 parse-options.h      |  1 +
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 5a8dc377ec..cfec044710 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -35,6 +35,7 @@ int cmd_write_tree(int argc,
 			.type = OPTION_BIT,
 			.long_name = "ignore-cache-tree",
 			.value = &flags,
+			.precision = sizeof(flags),
 			.help = N_("only useful for debugging"),
 			.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG,
 			.defval = WRITE_TREE_IGNORE_CACHE_TREE,
diff --git a/parse-options.c b/parse-options.c
index 639f41b83b..b5c877d5e1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -88,6 +88,14 @@ static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)
 	}
 }
 
+static intmax_t get_int_value(const struct option *opt, enum opt_parsed flags)
+{
+	intmax_t ret;
+	if (do_get_int_value(opt->value, opt->precision, &ret))
+		BUG("invalid precision for option %s", optname(opt, flags));
+	return ret;
+}
+
 static enum parse_opt_result set_int_value(const struct option *opt,
 					   enum opt_parsed flags,
 					   intmax_t value)
@@ -139,11 +147,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 		return opt->ll_callback(p, opt, NULL, unset);
 
 	case OPTION_BIT:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
-			*(int *)opt->value &= ~opt->defval;
+			value &= ~opt->defval;
 		else
-			*(int *)opt->value |= opt->defval;
-		return 0;
+			value |= opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_NEGBIT:
 		if (unset)
@@ -631,11 +642,11 @@ static void parse_options_check(const struct option *opts)
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
 		switch (opts->type) {
 		case OPTION_SET_INT:
+		case OPTION_BIT:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
-		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_NUMBER:
 		case OPTION_BITOP:
diff --git a/parse-options.h b/parse-options.h
index 71516e4b5b..6501ca3c27 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -172,6 +172,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 	.callback = NULL, \
-- 
2.50.0

[PATCH v2 5/7] parse-options: add precision handling for OPTION_NEGBIT

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_NEGBIT.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/rebase.c |  1 +
 parse-options.c  | 11 +++++++----
 parse-options.h  |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2e8c4ee678..e90562a3b8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1128,6 +1128,7 @@ int cmd_rebase(int argc,
 			.short_name = 'n',
 			.long_name = "no-stat",
 			.value = &options.flags,
+			.precision = sizeof(options.flags),
 			.help = N_("do not show diffstat of what changed upstream"),
 			.flags = PARSE_OPT_NOARG,
 			.defval = REBASE_DIFFSTAT,
diff --git a/parse-options.c b/parse-options.c
index b5c877d5e1..ba89dc4d09 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -157,11 +157,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_NEGBIT:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
-			*(int *)opt->value |= opt->defval;
+			value |= opt->defval;
 		else
-			*(int *)opt->value &= ~opt->defval;
-		return 0;
+			value &= ~opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_BITOP:
 		if (unset)
@@ -643,11 +646,11 @@ static void parse_options_check(const struct option *opts)
 		switch (opts->type) {
 		case OPTION_SET_INT:
 		case OPTION_BIT:
+		case OPTION_NEGBIT:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
-		case OPTION_NEGBIT:
 		case OPTION_NUMBER:
 		case OPTION_BITOP:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
diff --git a/parse-options.h b/parse-options.h
index 6501ca3c27..076f88b384 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -250,6 +250,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG, \
 	.defval = (b), \
-- 
2.50.0

[PATCH v2 6/7] parse-options: add precision handling for OPTION_BITOP

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_BITOP.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Check if "devfal" fits into an integer variable with the given
"precision", but don't check "extra", as its value is only used to clear
bits, so cannot lead to an overflow.  Not checking continues to allow
e.g., using -1 to clear all bits even if the value variable has a
narrower type than intptr_t.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c | 11 +++++++----
 parse-options.h |  1 +
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index ba89dc4d09..a813511b1b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -167,11 +167,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_BITOP:
+	{
+		intmax_t value = get_int_value(opt, flags);
 		if (unset)
 			BUG("BITOP can't have unset form");
-		*(int *)opt->value &= ~opt->extra;
-		*(int *)opt->value |= opt->defval;
-		return 0;
+		value &= ~opt->extra;
+		value |= opt->defval;
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_COUNTUP:
 		if (*(int *)opt->value < 0)
@@ -647,12 +650,12 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_SET_INT:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
+		case OPTION_BITOP:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
 		case OPTION_COUNTUP:
 		case OPTION_NUMBER:
-		case OPTION_BITOP:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
 				optbug(opts, "should not accept an argument");
diff --git a/parse-options.h b/parse-options.h
index 076f88b384..8bdf469ae9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -240,6 +240,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|PARSE_OPT_NONEG, \
 	.defval = (set), \
-- 
2.50.0

[PATCH v2 7/7] parse-options: add precision handling for OPTION_COUNTUP

[René Scharfe]

Similar to 09705696f7 (parse-options: introduce precision handling for
`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes
for OPTION_COUNTUP.  Do that by requiring their "precision" to be set,
casting their "value" pointer accordingly and checking whether the value
fits.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c               | 22 +++++++++++++++++-----
 parse-options.h               |  1 +
 t/helper/test-parse-options.c |  3 +++
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index a813511b1b..5224203ffe 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -177,10 +177,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 	}
 
 	case OPTION_COUNTUP:
-		if (*(int *)opt->value < 0)
-			*(int *)opt->value = 0;
-		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
-		return 0;
+	{
+		size_t bits = CHAR_BIT * opt->precision;
+		intmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);
+		intmax_t value = get_int_value(opt, flags);
+
+		if (value < 0)
+			value = 0;
+		if (unset)
+			value = 0;
+		else if (value < upper_bound)
+			value++;
+		else
+			return error(_("value for %s exceeds %"PRIdMAX),
+				     optname(opt, flags), upper_bound);
+		return set_int_value(opt, flags, value);
+	}
 
 	case OPTION_SET_INT:
 		return set_int_value(opt, flags, unset ? 0 : opt->defval);
@@ -651,10 +663,10 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_BITOP:
+		case OPTION_COUNTUP:
 			if (!signed_int_fits(opts->defval, opts->precision))
 				optbug(opts, "has invalid defval");
 			/* fallthru */
-		case OPTION_COUNTUP:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
diff --git a/parse-options.h b/parse-options.h
index 8bdf469ae9..312045604d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -183,6 +183,7 @@ struct option {
 	.short_name = (s), \
 	.long_name = (l), \
 	.value = (v), \
+	.precision = sizeof(*v), \
 	.help = (h), \
 	.flags = PARSE_OPT_NOARG|(f), \
 }
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 2ba2546d70..68579d83f3 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -178,6 +178,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.short_name = '+',
 			.value = &boolean,
+			.precision = sizeof(boolean),
 			.help = "same as -b",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,
 		},
@@ -185,6 +186,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "positive ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},
@@ -192,6 +194,7 @@ int cmd__parse_options(int argc, const char **argv)
 			.type = OPTION_COUNTUP,
 			.long_name = "no-ambiguous",
 			.value = &ambiguous,
+			.precision = sizeof(ambiguous),
 			.help = "negative ambiguity",
 			.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,
 		},
-- 
2.50.0