{"thread":{"id":"63708","subject":"[PATCH 0/6] parse-options: add more precision handling","startedAt":"2025-06-29T11:43:33Z","lastAt":"2025-07-09T15:58:08Z","messageCount":31,"participants":["René Scharfe","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"520872","messageId":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","threadId":"63708","inReplyTo":null,"subject":"[PATCH 0/6] parse-options: add more precision handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:43:24Z","receivedAt":"2025-06-29T11:43:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extend precision handling to all options that currently blindly use an\nint pointer to access value variables. This allows the safe use of\nsmaller and larger integer types.\n\nSign handling might be nice as well (especially for OPTION_COUNTUP), but\nis out of scope for this series.\n\n parse-options: add precision handling for PARSE_OPT_CMDMODE\n parse-options: add precision handling for OPTION_SET_INT\n parse-options: add precision handling for OPTION_BIT\n parse-options: add precision handling for OPTION_NEGBIT\n parse-options: add precision handling for OPTION_BITOP\n parse-options: add precision handling for OPTION_COUNTUP\n\n builtin/am.c | 1 +\n builtin/rebase.c | 1 +\n builtin/update-index.c | 6 ++\n builtin/write-tree.c | 1 +\n parse-options.c | 142 ++++++++++++++++++++++++----------\n parse-options.h | 7 ++\n t/helper/test-parse-options.c | 17 +++-\n 7 files changed, 129 insertions(+), 46 deletions(-)\n\n-- \n2.50.0\n"},{"id":"520873","messageId":"d92697a6-5367-4412-88db-164ba4dd6230@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:50:31Z","receivedAt":"2025-06-29T11:50:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Build on 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) to support value variables of different\nsizes for PARSE_OPT_CMDMODE options. Do that by requiring their\n\"precision\" to be set and casting their \"value\" pointer accordingly.\n\nget_value() needs to access all PARSE_OPT_CMDMODE values in addition to\nthe actual value it is supposed to get to detect conflicting changes.\nGive it an example struct option pointer in cmdmode_list instead of just\nthe \"value\" pointer to allow it to use the proper \"precision\".\n\nUse optbug() in get_int_value() to report options with unsupported\n\"precision\" values without requiring enum opt_parsed flags, as we don't\nhave them in build_cmdmode_list(). Use BUG right afterwards to abort\nfor uses outside of build_cmdmode_list() by aborting immediately.\n\nSigned-off-by: René Scharfe \n---\n builtin/am.c | 1 +\n parse-options.c | 36 ++++++++++++++++++++++++++---------\n parse-options.h | 1 +\n t/helper/test-parse-options.c | 13 ++++++++++---\n 4 files changed, 39 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex a800003340..c9d925f7b9 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2406,6 +2406,7 @@ int cmd_am(int argc,\n \t\t\t.type = OPTION_CALLBACK,\n \t\t\t.long_name = \"show-current-patch\",\n \t\t\t.value = &resume_mode,\n+\t\t\t.precision = sizeof(resume_mode),\n \t\t\t.argh = \"(diff|raw)\",\n \t\t\t.help = N_(\"show the patch being applied\"),\n \t\t\t.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\ndiff --git a/parse-options.c b/parse-options.c\nindex a9a39ecaef..da07a000a3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -68,6 +68,23 @@ static char *fix_filename(const char *prefix, const char *file)\n \t\treturn prefix_filename_except_for_dash(prefix, file);\n }\n \n+static intmax_t get_int_value(const struct option *opt)\n+{\n+\tswitch (opt->precision) {\n+\tcase sizeof(int8_t):\n+\t\treturn *(int8_t *)opt->value;\n+\tcase sizeof(int16_t):\n+\t\treturn *(int16_t *)opt->value;\n+\tcase sizeof(int32_t):\n+\t\treturn *(int32_t *)opt->value;\n+\tcase sizeof(int64_t):\n+\t\treturn *(int64_t *)opt->value;\n+\tdefault:\n+\t\toptbug(opt, \"has invalid precision\");\n+\t\tBUG(\"invalid 'struct option'\");\n+\t}\n+}\n+\n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t\t const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n@@ -266,8 +283,8 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n }\n \n struct parse_opt_cmdmode_list {\n-\tint value, *value_ptr;\n-\tconst struct option *opt;\n+\tintmax_t value;\n+\tconst struct option *opt, *reference_opt;\n \tconst char *arg;\n \tenum opt_parsed flags;\n \tstruct parse_opt_cmdmode_list *next;\n@@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n-\t\tint *value_ptr = opts->value;\n \n-\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n+\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)\n \t\t\tcontinue;\n \n-\t\twhile (elem && elem->value_ptr != value_ptr)\n+\t\twhile (elem && elem->reference_opt->value != opts->value)\n \t\t\telem = elem->next;\n \t\tif (elem)\n \t\t\tcontinue;\n \n \t\tCALLOC_ARRAY(elem, 1);\n-\t\telem->value_ptr = value_ptr;\n-\t\telem->value = *value_ptr;\n+\t\telem->reference_opt = opts;\n+\t\telem->value = get_int_value(opts);\n \t\telem->next = ctx->cmdmode_list;\n \t\tctx->cmdmode_list = elem;\n \t}\n@@ -317,7 +333,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tchar *opt_name, *other_opt_name;\n \n \tfor (; elem; elem = elem->next) {\n-\t\tif (*elem->value_ptr == elem->value)\n+\t\tintmax_t new_value = get_int_value(elem->reference_opt);\n+\n+\t\tif (new_value == elem->value)\n \t\t\tcontinue;\n \n \t\tif (elem->opt &&\n@@ -327,7 +345,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\telem->opt = opt;\n \t\telem->arg = arg;\n \t\telem->flags = flags;\n-\t\telem->value = *elem->value_ptr;\n+\t\telem->value = new_value;\n \t}\n \n \tif (result || !elem)\ndiff --git a/parse-options.h b/parse-options.h\nindex 91c3e3c29b..c75a473c9e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -269,6 +269,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n \t.defval = (i), \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex f2663dd0c0..1e03ff88f6 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -148,9 +148,16 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n \t\tOPT_CMDMODE(0, \"mode1\", &integer, \"set integer to 1 (cmdmode option)\", 1),\n \t\tOPT_CMDMODE(0, \"mode2\", &integer, \"set integer to 2 (cmdmode option)\", 2),\n-\t\tOPT_CALLBACK_F(0, \"mode34\", &integer, \"(3|4)\",\n-\t\t\t\"set integer to 3 or 4 (cmdmode option)\",\n-\t\t\tPARSE_OPT_CMDMODE, mode34_callback),\n+\t\t{\n+\t\t\t.type = OPTION_CALLBACK,\n+\t\t\t.long_name = \"mode34\",\n+\t\t\t.value = &integer,\n+\t\t\t.precision = sizeof(integer),\n+\t\t\t.argh = \"(3|4)\",\n+\t\t\t.help = \"set integer to 3 or 4 (cmdmode option)\",\n+\t\t\t.flags = PARSE_OPT_CMDMODE,\n+\t\t\t.callback = mode34_callback,\n+\t\t},\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n \t\t\t\"get length of \", length_callback),\n \t\tOPT_FILENAME('F', \"file\", &file, \"set file to \"),\n-- \n2.50.0\n"},{"id":"520874","messageId":"3690df99-8a83-4377-9b03-6766f7958c21@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:50:39Z","receivedAt":"2025-06-29T11:50:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_SET_INT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nFactor out the casting code from the part of do_get_value() that handles\nOPTION_INTEGER to avoid code duplication. We're going to use it in the\nnext patches as well.\n\nSigned-off-by: René Scharfe \n---\n builtin/update-index.c | 6 ++++\n parse-options.c | 56 ++++++++++++++++++++++-------------\n parse-options.h | 2 ++\n t/helper/test-parse-options.c | 1 +\n 4 files changed, 45 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 538b619ba4..0c1d4ed55b 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -981,6 +981,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"assume-unchanged\",\n \t\t\t.value = &mark_valid_only,\n+\t\t\t.precision = sizeof(mark_valid_only),\n \t\t\t.help = N_(\"mark files as \\\"not changing\\\"\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -989,6 +990,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-assume-unchanged\",\n \t\t\t.value = &mark_valid_only,\n+\t\t\t.precision = sizeof(mark_valid_only),\n \t\t\t.help = N_(\"clear assumed-unchanged bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\n@@ -997,6 +999,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"skip-worktree\",\n \t\t\t.value = &mark_skip_worktree_only,\n+\t\t\t.precision = sizeof(mark_skip_worktree_only),\n \t\t\t.help = N_(\"mark files as \\\"index-only\\\"\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -1005,6 +1008,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-skip-worktree\",\n \t\t\t.value = &mark_skip_worktree_only,\n+\t\t\t.precision = sizeof(mark_skip_worktree_only),\n \t\t\t.help = N_(\"clear skip-worktree bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\n@@ -1079,6 +1083,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"fsmonitor-valid\",\n \t\t\t.value = &mark_fsmonitor_only,\n+\t\t\t.precision = sizeof(mark_fsmonitor_only),\n \t\t\t.help = N_(\"mark files as fsmonitor valid\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -1087,6 +1092,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-fsmonitor-valid\",\n \t\t\t.value = &mark_fsmonitor_only,\n+\t\t\t.precision = sizeof(mark_fsmonitor_only),\n \t\t\t.help = N_(\"clear fsmonitor valid bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\ndiff --git a/parse-options.c b/parse-options.c\nindex da07a000a3..bbb68603cc 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)\n \t}\n }\n \n+static enum parse_opt_result set_int_value(const struct option *opt,\n+\t\t\t\t\t enum opt_parsed flags,\n+\t\t\t\t\t intmax_t value)\n+{\n+\tswitch (opt->precision) {\n+\tcase sizeof(int8_t):\n+\t\t*(int8_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int16_t):\n+\t\t*(int16_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int32_t):\n+\t\t*(int32_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int64_t):\n+\t\t*(int64_t *)opt->value = value;\n+\t\treturn 0;\n+\tdefault:\n+\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n+\t}\n+}\n+\n+static int signed_int_fits(intmax_t value, size_t size)\n+{\n+\tsize_t bits = size * CHAR_BIT;\n+\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n+\tintmax_t lower_bound = -upper_bound - 1;\n+\treturn lower_bound <= value && value <= upper_bound;\n+}\n+\n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t\t const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n@@ -133,8 +163,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_SET_INT:\n-\t\t*(int *)opt->value = unset ? 0 : opt->defval;\n-\t\treturn 0;\n+\t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n \n \tcase OPTION_STRING:\n \t\tif (unset)\n@@ -216,23 +245,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn error(_(\"value %s for %s not in range [%\"PRIdMAX\",%\"PRIdMAX\"]\"),\n \t\t\t\t arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);\n \n-\t\tswitch (opt->precision) {\n-\t\tcase 1:\n-\t\t\t*(int8_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 2:\n-\t\t\t*(int16_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 4:\n-\t\t\t*(int32_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 8:\n-\t\t\t*(int64_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tdefault:\n-\t\t\tBUG(\"invalid precision for option %s\",\n-\t\t\t optname(opt, flags));\n-\t\t}\n+\t\treturn set_int_value(opt, flags, value);\n \t}\n \tcase OPTION_UNSIGNED:\n \t{\n@@ -604,10 +617,13 @@ static void parse_options_check(const struct option *opts)\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n+\t\tcase OPTION_SET_INT:\n+\t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n+\t\t\t\toptbug(opts, \"has invalid defval\");\n+\t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n-\t\tcase OPTION_SET_INT:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\ndiff --git a/parse-options.h b/parse-options.h\nindex c75a473c9e..71516e4b5b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -190,6 +190,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG | (f), \\\n \t.defval = (i), \\\n@@ -260,6 +261,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \\\n \t.defval = 1, \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 1e03ff88f6..2ba2546d70 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -131,6 +131,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.short_name = 'B',\n \t\t\t.long_name = \"no-fear\",\n \t\t\t.value = &boolean,\n+\t\t\t.precision = sizeof(boolean),\n \t\t\t.help = \"be brave\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = 1,\n-- \n2.50.0\n"},{"id":"520875","messageId":"7be6f365-7904-4086-992a-607d6c638d11@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 3/6] parse-options: add precision handling for OPTION_BIT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:50:52Z","receivedAt":"2025-06-29T11:50:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_BIT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n builtin/write-tree.c | 1 +\n parse-options.c | 11 +++++++----\n parse-options.h | 1 +\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/write-tree.c b/builtin/write-tree.c\nindex 5a8dc377ec..cfec044710 100644\n--- a/builtin/write-tree.c\n+++ b/builtin/write-tree.c\n@@ -35,6 +35,7 @@ int cmd_write_tree(int argc,\n \t\t\t.type = OPTION_BIT,\n \t\t\t.long_name = \"ignore-cache-tree\",\n \t\t\t.value = &flags,\n+\t\t\t.precision = sizeof(flags),\n \t\t\t.help = N_(\"only useful for debugging\"),\n \t\t\t.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG,\n \t\t\t.defval = WRITE_TREE_IGNORE_CACHE_TREE,\ndiff --git a/parse-options.c b/parse-options.c\nindex bbb68603cc..47a77d2cea 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -136,11 +136,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn opt->ll_callback(p, opt, NULL, unset);\n \n \tcase OPTION_BIT:\n+\t{\n+\t\tintmax_t value = get_int_value(opt);\n \t\tif (unset)\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n+\t\t\tvalue &= ~opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value |= opt->defval;\n-\t\treturn 0;\n+\t\t\tvalue |= opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_NEGBIT:\n \t\tif (unset)\n@@ -618,11 +621,11 @@ static void parse_options_check(const struct option *opts)\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_BIT:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n-\t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\ndiff --git a/parse-options.h b/parse-options.h\nindex 71516e4b5b..6501ca3c27 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -172,6 +172,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|(f), \\\n \t.callback = NULL, \\\n-- \n2.50.0\n"},{"id":"520876","messageId":"b0f69c3d-d0a0-4137-ad09-4f46dedc0dab@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 4/6] parse-options: add precision handling for OPTION_NEGBIT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:51:06Z","receivedAt":"2025-06-29T11:51:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_NEGBIT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n builtin/rebase.c | 1 +\n parse-options.c | 11 +++++++----\n parse-options.h | 1 +\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e8c4ee678..e90562a3b8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1128,6 +1128,7 @@ int cmd_rebase(int argc,\n \t\t\t.short_name = 'n',\n \t\t\t.long_name = \"no-stat\",\n \t\t\t.value = &options.flags,\n+\t\t\t.precision = sizeof(options.flags),\n \t\t\t.help = N_(\"do not show diffstat of what changed upstream\"),\n \t\t\t.flags = PARSE_OPT_NOARG,\n \t\t\t.defval = REBASE_DIFFSTAT,\ndiff --git a/parse-options.c b/parse-options.c\nindex 47a77d2cea..6bd7158806 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -146,11 +146,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_NEGBIT:\n+\t{\n+\t\tintmax_t value = get_int_value(opt);\n \t\tif (unset)\n-\t\t\t*(int *)opt->value |= opt->defval;\n+\t\t\tvalue |= opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n-\t\treturn 0;\n+\t\t\tvalue &= ~opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_BITOP:\n \t\tif (unset)\n@@ -622,11 +625,11 @@ static void parse_options_check(const struct option *opts)\n \t\tswitch (opts->type) {\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n-\t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\ndiff --git a/parse-options.h b/parse-options.h\nindex 6501ca3c27..076f88b384 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -250,6 +250,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG, \\\n \t.defval = (b), \\\n-- \n2.50.0\n"},{"id":"520877","messageId":"7b970f48-05a8-40c6-8e5b-95ce830705c0@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 5/6] parse-options: add precision handling for OPTION_BITOP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:51:19Z","receivedAt":"2025-06-29T11:51:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_BITOP. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nChecking \"defval\" has the side-effect of also requiring PARSE_OPT_NOARG.\nThis is sensible, as OPTION_BITOP doesn't handle any arguments, so we\ntake this unintended benefit.\n\nDon't check \"extra\", though, as its value is only used to clear bits, so\ncannot lead to an overflow. Not checking continues to allow e.g., using\n-1 to clear all bits even if the value variable has a narrower type than\nintptr_t.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 10 +++++++---\n parse-options.h | 1 +\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 6bd7158806..0dc9b0324a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -156,11 +156,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_BITOP:\n+\t{\n+\t\tintmax_t value = get_int_value(opt);\n \t\tif (unset)\n \t\t\tBUG(\"BITOP can't have unset form\");\n-\t\t*(int *)opt->value &= ~opt->extra;\n-\t\t*(int *)opt->value |= opt->defval;\n-\t\treturn 0;\n+\t\tvalue &= ~opt->extra;\n+\t\tvalue |= opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_COUNTUP:\n \t\tif (*(int *)opt->value < 0)\n@@ -626,6 +629,7 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_BITOP:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\ndiff --git a/parse-options.h b/parse-options.h\nindex 076f88b384..8bdf469ae9 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -240,6 +240,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|PARSE_OPT_NONEG, \\\n \t.defval = (set), \\\n-- \n2.50.0\n"},{"id":"520878","messageId":"7322758a-9310-4892-b476-50dc57d559b4@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-06-29T11:51:36Z","receivedAt":"2025-06-29T11:51:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_COUNTUP. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 22 +++++++++++++++++-----\n parse-options.h | 1 +\n t/helper/test-parse-options.c | 3 +++\n 3 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0dc9b0324a..0dd08a3a77 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_COUNTUP:\n-\t\tif (*(int *)opt->value < 0)\n-\t\t\t*(int *)opt->value = 0;\n-\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n-\t\treturn 0;\n+\t{\n+\t\tsize_t bits = CHAR_BIT * opt->precision;\n+\t\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n+\t\tintmax_t value = get_int_value(opt);\n+\n+\t\tif (value < 0)\n+\t\t\tvalue = 0;\n+\t\tif (unset)\n+\t\t\tvalue = 0;\n+\t\telse if (value < upper_bound)\n+\t\t\tvalue++;\n+\t\telse\n+\t\t\treturn error(_(\"value for %s exceeds %\"PRIdMAX),\n+\t\t\t\t optname(opt, flags), upper_bound);\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_SET_INT:\n \t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n@@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_BITOP:\n+\t\tcase OPTION_COUNTUP:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n-\t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\ndiff --git a/parse-options.h b/parse-options.h\nindex 8bdf469ae9..312045604d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -183,6 +183,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|(f), \\\n }\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 2ba2546d70..68579d83f3 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -178,6 +178,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.short_name = '+',\n \t\t\t.value = &boolean,\n+\t\t\t.precision = sizeof(boolean),\n \t\t\t.help = \"same as -b\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,\n \t\t},\n@@ -185,6 +186,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.long_name = \"ambiguous\",\n \t\t\t.value = &ambiguous,\n+\t\t\t.precision = sizeof(ambiguous),\n \t\t\t.help = \"positive ambiguity\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t},\n@@ -192,6 +194,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.long_name = \"no-ambiguous\",\n \t\t\t.value = &ambiguous,\n+\t\t\t.precision = sizeof(ambiguous),\n \t\t\t.help = \"negative ambiguity\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t},\n-- \n2.50.0\n"},{"id":"521008","messageId":"aGO-i6V6_iN678_7@pks.im","threadId":"63708","inReplyTo":"d92697a6-5367-4412-88db-164ba4dd6230@web.de","subject":"Re: [PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-01T10:55:07Z","receivedAt":"2025-07-01T10:55:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 29, 2025 at 01:50:31PM +0200, René Scharfe wrote:\n> Build on 09705696f7 (parse-options: introduce precision handling for\n> `OPTION_INTEGER`, 2025-04-17) to support value variables of different\n> sizes for PARSE_OPT_CMDMODE options. Do that by requiring their\n> \"precision\" to be set and casting their \"value\" pointer accordingly.\n\nMakes sense.\n\n> get_value() needs to access all PARSE_OPT_CMDMODE values in addition to\n> the actual value it is supposed to get to detect conflicting changes.\n> Give it an example struct option pointer in cmdmode_list instead of just\n> the \"value\" pointer to allow it to use the proper \"precision\".\n> \n> Use optbug() in get_int_value() to report options with unsupported\n> \"precision\" values without requiring enum opt_parsed flags, as we don't\n> have them in build_cmdmode_list(). Use BUG right afterwards to abort\n> for uses outside of build_cmdmode_list() by aborting immediately.\n\nHm. I have a bit of a hard time understanding these two paragraphs, to\nbe honest. Might be that my brain is still in vacation mode.\n\n> @@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n> \n> \tfor (; opts->type != OPTION_END; opts++) {\n> \t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n> -\t\tint *value_ptr = opts->value;\n> \n> -\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n> +\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)\n> \t\t\tcontinue;\n> \n> -\t\twhile (elem && elem->value_ptr != value_ptr)\n> +\t\twhile (elem && elem->reference_opt->value != opts->value)\n> \t\t\telem = elem->next;\n\nHm. Previously we checked for the pointers to be equal, now we check for\nthe value to be equal. Are we sure that this is always equivalent? Can't\nit ever be that two elements might have the same value?\n\nPatrick\n"},{"id":"521009","messageId":"aGO-kbdAgMMef-A5@pks.im","threadId":"63708","inReplyTo":"7b970f48-05a8-40c6-8e5b-95ce830705c0@web.de","subject":"Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-01T10:55:13Z","receivedAt":"2025-07-01T10:55:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:\n> Similar to 09705696f7 (parse-options: introduce precision handling for\n> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\n> for OPTION_BITOP. Do that by requiring their \"precision\" to be set,\n> casting their \"value\" pointer accordingly and checking whether the value\n> fits.\n> \n> Checking \"defval\" has the side-effect of also requiring PARSE_OPT_NOARG.\n\nHm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in\nthis patch at all, so I'm a but puzzled.\n\nPatrick\n"},{"id":"521010","messageId":"aGO-l81JiOESvbS9@pks.im","threadId":"63708","inReplyTo":"3690df99-8a83-4377-9b03-6766f7958c21@web.de","subject":"Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-01T10:55:19Z","receivedAt":"2025-07-01T10:55:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:\n> diff --git a/parse-options.c b/parse-options.c\n> index da07a000a3..bbb68603cc 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)\n> \t}\n> }\n> \n> +static enum parse_opt_result set_int_value(const struct option *opt,\n> +\t\t\t\t\t enum opt_parsed flags,\n> +\t\t\t\t\t intmax_t value)\n> +{\n> +\tswitch (opt->precision) {\n> +\tcase sizeof(int8_t):\n> +\t\t*(int8_t *)opt->value = value;\n> +\t\treturn 0;\n> +\tcase sizeof(int16_t):\n> +\t\t*(int16_t *)opt->value = value;\n> +\t\treturn 0;\n> +\tcase sizeof(int32_t):\n> +\t\t*(int32_t *)opt->value = value;\n> +\t\treturn 0;\n> +\tcase sizeof(int64_t):\n> +\t\t*(int64_t *)opt->value = value;\n> +\t\treturn 0;\n> +\tdefault:\n> +\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n> +\t}\n> +}\n\nThe function only ever dies or returns successfully, so can't we make it\nreturn nothing instead? On the other hand it does make a couple of\ncallsites a bit nicer to read.\n\n> +static int signed_int_fits(intmax_t value, size_t size)\n> +{\n> +\tsize_t bits = size * CHAR_BIT;\n> +\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n> +\tintmax_t lower_bound = -upper_bound - 1;\n> +\treturn lower_bound <= value && value <= upper_bound;\n> +}\n> +\n\nShould we s/size/precision/ so that it's clear what kind of size this\nexactly is?\n\nPatrick\n"},{"id":"521011","messageId":"aGO-nSyCN7OD9Zae@pks.im","threadId":"63708","inReplyTo":"7322758a-9310-4892-b476-50dc57d559b4@web.de","subject":"Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-01T10:55:25Z","receivedAt":"2025-07-01T10:55:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:\n> diff --git a/parse-options.c b/parse-options.c\n> index 0dc9b0324a..0dd08a3a77 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n> \t}\n> \n> \tcase OPTION_COUNTUP:\n> -\t\tif (*(int *)opt->value < 0)\n> -\t\t\t*(int *)opt->value = 0;\n> -\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n> -\t\treturn 0;\n> +\t{\n> +\t\tsize_t bits = CHAR_BIT * opt->precision;\n> +\t\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n> +\t\tintmax_t value = get_int_value(opt);\n> +\n> +\t\tif (value < 0)\n> +\t\t\tvalue = 0;\n> +\t\tif (unset)\n> +\t\t\tvalue = 0;\n> +\t\telse if (value < upper_bound)\n> +\t\t\tvalue++;\n> +\t\telse\n> +\t\t\treturn error(_(\"value for %s exceeds %\"PRIdMAX),\n> +\t\t\t\t optname(opt, flags), upper_bound);\n> +\t\treturn set_int_value(opt, flags, value);\n> +\t}\n> \n> \tcase OPTION_SET_INT:\n> \t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n> @@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)\n> \t\tcase OPTION_BIT:\n> \t\tcase OPTION_NEGBIT:\n> \t\tcase OPTION_BITOP:\n> +\t\tcase OPTION_COUNTUP:\n> \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n> \t\t\t\toptbug(opts, \"has invalid defval\");\n> \t\t\t/* fallthru */\n> -\t\tcase OPTION_COUNTUP:\n> \t\tcase OPTION_NUMBER:\n> \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n> \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n> diff --git a/parse-options.h b/parse-options.h\n> index 8bdf469ae9..312045604d 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -183,6 +183,7 @@ struct option {\n> \t.short_name = (s), \\\n> \t.long_name = (l), \\\n> \t.value = (v), \\\n\nIt's a bit surprising that `COUNTUP` accepts a signed integer, so should\nwe maybe add `BARF_UNLESS_SIGNED(*(v))` here?\n\n> +\t.precision = sizeof(*v), \\\n> \t.help = (h), \\\n> \t.flags = PARSE_OPT_NOARG|(f), \\\n> }\n\nPatrick\n"},{"id":"521065","messageId":"e75c8da8-a859-4980-bbd7-1fb5b03a8113@web.de","threadId":"63708","inReplyTo":"aGO-i6V6_iN678_7@pks.im","subject":"Re: [PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-01T15:15:30Z","receivedAt":"2025-07-01T15:15:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> On Sun, Jun 29, 2025 at 01:50:31PM +0200, René Scharfe wrote:\n>> Build on 09705696f7 (parse-options: introduce precision handling for\n>> `OPTION_INTEGER`, 2025-04-17) to support value variables of different\n>> sizes for PARSE_OPT_CMDMODE options. Do that by requiring their\n>> \"precision\" to be set and casting their \"value\" pointer accordingly.\n> \n> Makes sense.\n> \n>> get_value() needs to access all PARSE_OPT_CMDMODE values in addition to\n>> the actual value it is supposed to get to detect conflicting changes.\n>> Give it an example struct option pointer in cmdmode_list instead of just\n>> the \"value\" pointer to allow it to use the proper \"precision\".\n>>\n>> Use optbug() in get_int_value() to report options with unsupported\n>> \"precision\" values without requiring enum opt_parsed flags, as we don't\n>> have them in build_cmdmode_list(). Use BUG right afterwards to abort\n>> for uses outside of build_cmdmode_list() by aborting immediately.\n> \n> Hm. I have a bit of a hard time understanding these two paragraphs, to\n> be honest. Might be that my brain is still in vacation mode.\n\nOr it might be my needing-a-vacation mode.\n\nget_value() checks all PARSE_OPT_CMDMODE value variables. It does that\nto detect changes to such a variable by non-PARSE_OPT_CMDMODE options.\n\"All\" sounds grand, but actual commands only have at most a single\nPARSE_OPT_CMDMODE value variable. But they could have many more.\nAnyway, the patch gives it a struct option so that it can use the\nproper precision when dereferencing value, a void pointer.\n\noptbug() calls the macro \"bug\". It reports a bug just like BUG, but\ndoes not abort. It's intended for cases where we want to report all the\nbugs that we can find instead of forcing developers to fix them one by\none and recompile in between. That would be nice in\nbuild_cmdmode_list(). I still force an abort by following it with BUG,\nthough, because handling invalid \"precision\" values would be tedious\nin the other callers.\n\n> \n>> @@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n>> \n>> \tfor (; opts->type != OPTION_END; opts++) {\n>> \t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n>> -\t\tint *value_ptr = opts->value;\n>> \n>> -\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n>> +\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)\n>> \t\t\tcontinue;\n>> \n>> -\t\twhile (elem && elem->value_ptr != value_ptr)\n>> +\t\twhile (elem && elem->reference_opt->value != opts->value)\n>> \t\t\telem = elem->next;\n> \n> Hm. Previously we checked for the pointers to be equal, now we check for\n> the value to be equal. Are we sure that this is always equivalent? Can't\n> it ever be that two elements might have the same value?\n\nThe \"value\" member of struct option is a void pointer. We still compare\npointers here.\n\nRené\n\n"},{"id":"521068","messageId":"978e38e1-45b4-4c74-856d-255a87d6c817@web.de","threadId":"63708","inReplyTo":"aGO-kbdAgMMef-A5@pks.im","subject":"Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-01T15:21:11Z","receivedAt":"2025-07-01T15:21:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:\n>> Similar to 09705696f7 (parse-options: introduce precision handling for\n>> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\n>> for OPTION_BITOP. Do that by requiring their \"precision\" to be set,\n>> casting their \"value\" pointer accordingly and checking whether the value\n>> fits.\n>>\n>> Checking \"defval\" has the side-effect of also requiring PARSE_OPT_NOARG.\n> \n> Hm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in\n> this patch at all, so I'm a but puzzled.\n\nFor options with OPTION_BITOP. Adding the defval check also adds the\nno-argument check by falling through to it:\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 6bd7158806..0dc9b0324a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -620,18 +623,19 @@ static void parse_options_check(const struct option *opts)\n \t\t\toptbug(opts, \"uses feature \"\n \t\t\t \"not supported for dashless options\");\n \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_BITOP:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\t\toptbug(opts, \"should not accept an argument\");\n \t\t\tbreak;\n\n"},{"id":"521073","messageId":"fb50bdea-f529-447a-9cf1-0fbde3f8e22e@web.de","threadId":"63708","inReplyTo":"aGO-l81JiOESvbS9@pks.im","subject":"Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-01T15:54:41Z","receivedAt":"2025-07-01T15:54:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:\n>> diff --git a/parse-options.c b/parse-options.c\n>> index da07a000a3..bbb68603cc 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -85,6 +85,36 @@ static intmax_t get_int_value(const struct option *opt)\n>> \t}\n>> }\n>> \n>> +static enum parse_opt_result set_int_value(const struct option *opt,\n>> +\t\t\t\t\t enum opt_parsed flags,\n>> +\t\t\t\t\t intmax_t value)\n>> +{\n>> +\tswitch (opt->precision) {\n>> +\tcase sizeof(int8_t):\n>> +\t\t*(int8_t *)opt->value = value;\n>> +\t\treturn 0;\n>> +\tcase sizeof(int16_t):\n>> +\t\t*(int16_t *)opt->value = value;\n>> +\t\treturn 0;\n>> +\tcase sizeof(int32_t):\n>> +\t\t*(int32_t *)opt->value = value;\n>> +\t\treturn 0;\n>> +\tcase sizeof(int64_t):\n>> +\t\t*(int64_t *)opt->value = value;\n>> +\t\treturn 0;\n>> +\tdefault:\n>> +\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n>> +\t}\n>> +}\n> \n> The function only ever dies or returns successfully, so can't we make it\n> return nothing instead? On the other hand it does make a couple of\n> callsites a bit nicer to read.\n\nWe can. Not sure we should, obviously, but I don't have strong feelings\neither way.\n\n> \n>> +static int signed_int_fits(intmax_t value, size_t size)\n>> +{\n>> +\tsize_t bits = size * CHAR_BIT;\n>> +\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n>> +\tintmax_t lower_bound = -upper_bound - 1;\n>> +\treturn lower_bound <= value && value <= upper_bound;\n>> +}\n>> +\n> \n> Should we s/size/precision/ so that it's clear what kind of size this\n> exactly is?\nIt's the width of an integer variable as in sizeof(), so the name fits.\nWe can inline this single-caller function if it's indeed confusing.\n\nRené\n\n\n"},{"id":"521074","messageId":"591d618f-1b86-414c-8069-3621e00add74@web.de","threadId":"63708","inReplyTo":"aGO-nSyCN7OD9Zae@pks.im","subject":"Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-01T16:01:44Z","receivedAt":"2025-07-01T16:01:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 0dc9b0324a..0dd08a3a77 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -166,10 +166,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n>> \t}\n>> \n>> \tcase OPTION_COUNTUP:\n>> -\t\tif (*(int *)opt->value < 0)\n>> -\t\t\t*(int *)opt->value = 0;\n>> -\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n>> -\t\treturn 0;\n>> +\t{\n>> +\t\tsize_t bits = CHAR_BIT * opt->precision;\n>> +\t\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n>> +\t\tintmax_t value = get_int_value(opt);\n>> +\n>> +\t\tif (value < 0)\n>> +\t\t\tvalue = 0;\n>> +\t\tif (unset)\n>> +\t\t\tvalue = 0;\n>> +\t\telse if (value < upper_bound)\n>> +\t\t\tvalue++;\n>> +\t\telse\n>> +\t\t\treturn error(_(\"value for %s exceeds %\"PRIdMAX),\n>> +\t\t\t\t optname(opt, flags), upper_bound);\n>> +\t\treturn set_int_value(opt, flags, value);\n>> +\t}\n>> \n>> \tcase OPTION_SET_INT:\n>> \t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n>> @@ -630,10 +642,10 @@ static void parse_options_check(const struct option *opts)\n>> \t\tcase OPTION_BIT:\n>> \t\tcase OPTION_NEGBIT:\n>> \t\tcase OPTION_BITOP:\n>> +\t\tcase OPTION_COUNTUP:\n>> \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n>> \t\t\t\toptbug(opts, \"has invalid defval\");\n>> \t\t\t/* fallthru */\n>> -\t\tcase OPTION_COUNTUP:\n>> \t\tcase OPTION_NUMBER:\n>> \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n>> \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n>> diff --git a/parse-options.h b/parse-options.h\n>> index 8bdf469ae9..312045604d 100644\n>> --- a/parse-options.h\n>> +++ b/parse-options.h\n>> @@ -183,6 +183,7 @@ struct option {\n>> \t.short_name = (s), \\\n>> \t.long_name = (l), \\\n>> \t.value = (v), \\\n> \n> It's a bit surprising that `COUNTUP` accepts a signed integer, so should\n> we maybe add `BARF_UNLESS_SIGNED(*(v))` here?\n\nPerhaps, but that would require more changes to callers that use unsigned\nvariables than I can stomach. That's why I declared it out of scope for\nthis series in its cover letter. Later, unless (hopefully) someone beats\nme to it.\n\n> \n>> +\t.precision = sizeof(*v), \\\n>> \t.help = (h), \\\n>> \t.flags = PARSE_OPT_NOARG|(f), \\\n>> }\n> \n> Patrick\n\n"},{"id":"521142","messageId":"aGSZfP87vAillq0x@pks.im","threadId":"63708","inReplyTo":"591d618f-1b86-414c-8069-3621e00add74@web.de","subject":"Re: [PATCH 6/6] parse-options: add precision handling for OPTION_COUNTUP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-02T02:29:16Z","receivedAt":"2025-07-02T02:29:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 01, 2025 at 06:01:44PM +0200, René Scharfe wrote:\n> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> > On Sun, Jun 29, 2025 at 01:51:36PM +0200, René Scharfe wrote:\n> >> diff --git a/parse-options.c b/parse-options.c\n> >> index 0dc9b0324a..0dd08a3a77 100644\n> >> --- a/parse-options.c\n> >> +++ b/parse-options.c\n> >> diff --git a/parse-options.h b/parse-options.h\n> >> index 8bdf469ae9..312045604d 100644\n> >> --- a/parse-options.h\n> >> +++ b/parse-options.h\n> >> @@ -183,6 +183,7 @@ struct option {\n> >> \t.short_name = (s), \\\n> >> \t.long_name = (l), \\\n> >> \t.value = (v), \\\n> > \n> > It's a bit surprising that `COUNTUP` accepts a signed integer, so should\n> > we maybe add `BARF_UNLESS_SIGNED(*(v))` here?\n> \n> Perhaps, but that would require more changes to callers that use unsigned\n> variables than I can stomach. That's why I declared it out of scope for\n> this series in its cover letter. Later, unless (hopefully) someone beats\n> me to it.\n\nFine with me, thanks!\n\nPatrick\n"},{"id":"521143","messageId":"aGSZ8jHWz0HnPGnO@pks.im","threadId":"63708","inReplyTo":"fb50bdea-f529-447a-9cf1-0fbde3f8e22e@web.de","subject":"Re: [PATCH 2/6] parse-options: add precision handling for OPTION_SET_INT","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-02T02:31:14Z","receivedAt":"2025-07-02T02:31:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 01, 2025 at 05:54:41PM +0200, René Scharfe wrote:\n> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> > On Sun, Jun 29, 2025 at 01:50:39PM +0200, René Scharfe wrote:\n> >> diff --git a/parse-options.c b/parse-options.c\n> >> index da07a000a3..bbb68603cc 100644\n> >> --- a/parse-options.c\n> >> +++ b/parse-options.c\n> >> +static int signed_int_fits(intmax_t value, size_t size)\n> >> +{\n> >> +\tsize_t bits = size * CHAR_BIT;\n> >> +\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n> >> +\tintmax_t lower_bound = -upper_bound - 1;\n> >> +\treturn lower_bound <= value && value <= upper_bound;\n> >> +}\n> >> +\n> > \n> > Should we s/size/precision/ so that it's clear what kind of size this\n> > exactly is?\n> It's the width of an integer variable as in sizeof(), so the name fits.\n> We can inline this single-caller function if it's indeed confusing.\n\nThe issue to me is rather that it's unclear what the unit is. Is it size\nin bytes, bits, nibbles? You wouldn't know that the expectation is that\nthe caller passes in `sizeof()` without taking a deeper look.\n\nIn any case, this is only a minor nit in the first place, not worth much\nbikeshedding.\n\nPatrick\n"},{"id":"521144","messageId":"aGSadcpxNsp1BLCm@pks.im","threadId":"63708","inReplyTo":"978e38e1-45b4-4c74-856d-255a87d6c817@web.de","subject":"Re: [PATCH 5/6] parse-options: add precision handling for OPTION_BITOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-02T02:33:25Z","receivedAt":"2025-07-02T02:33:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 01, 2025 at 05:21:11PM +0200, René Scharfe wrote:\n> On 7/1/25 12:55 PM, Patrick Steinhardt wrote:\n> > On Sun, Jun 29, 2025 at 01:51:19PM +0200, René Scharfe wrote:\n> >> Similar to 09705696f7 (parse-options: introduce precision handling for\n> >> `OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\n> >> for OPTION_BITOP. Do that by requiring their \"precision\" to be set,\n> >> casting their \"value\" pointer accordingly and checking whether the value\n> >> fits.\n> >>\n> >> Checking \"defval\" has the side-effect of also requiring PARSE_OPT_NOARG.\n> > \n> > Hm, requiring PARSE_OPT_NOARG for what? I cannot see it being touched in\n> > this patch at all, so I'm a but puzzled.\n> \n> For options with OPTION_BITOP. Adding the defval check also adds the\n> no-argument check by falling through to it:\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 6bd7158806..0dc9b0324a 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -620,18 +623,19 @@ static void parse_options_check(const struct option *opts)\n> \t\t\toptbug(opts, \"uses feature \"\n> \t\t\t \"not supported for dashless options\");\n> \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n> \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n> \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n> \t\tswitch (opts->type) {\n> \t\tcase OPTION_SET_INT:\n> \t\tcase OPTION_BIT:\n> \t\tcase OPTION_NEGBIT:\n> +\t\tcase OPTION_BITOP:\n> \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n> \t\t\t\toptbug(opts, \"has invalid defval\");\n> \t\t\t/* fallthru */\n> \t\tcase OPTION_COUNTUP:\n> \t\tcase OPTION_NUMBER:\n> \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n> \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n> \t\t\t\toptbug(opts, \"should not accept an argument\");\n> \t\t\tbreak;\n\nAh, now I see it, the extended context definitely helps. Might be nice\nto point out explicitly in the commit message that it's about the\nfallthrough behaviour.\n\nPatrick\n"},{"id":"521631","messageId":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","threadId":"63708","inReplyTo":"cf5cd57d-733f-4239-80f8-23bdc1523ab2@web.de","subject":"[PATCH v2 0/7] parse-options: add more precision handling","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:26:10Z","receivedAt":"2025-07-09T09:31:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Extend precision handling to all options that currently blindly use an\nint pointer to access value variables. This allows the safe use of\nsmaller and larger integer types.\n\nSign handling might be nice as well (especially for OPTION_COUNTUP), but\nis out of scope for this series.\n\nChanges since v1:\n- Enable PARSE_OPT_NOARG checking for OPTION_BITOP explicitly in a new\n separate patch instead of as a side-effect.\n- Split out do_get_int_value().\n- This allows us to use optbug() only where it makes sense, in\n build_cmdmode_list(). This lets the function report all\n PARSE_OPT_CMDMODE options with invalid precision in one go.\n- Use the same BUG call in get_int_value() as in set_int_value(),\n for consistency.\n- Store the precision in build_cmdmode_list() instead of an example\n option, which is hopefully easier to follow.\n- Rename the size parameter of signed_int_fits() for consistency.\n\n parse-options: require PARSE_OPT_NOARG for OPTION_BITOP\n parse-options: add precision handling for PARSE_OPT_CMDMODE\n parse-options: add precision handling for OPTION_SET_INT\n parse-options: add precision handling for OPTION_BIT\n parse-options: add precision handling for OPTION_NEGBIT\n parse-options: add precision handling for OPTION_BITOP\n parse-options: add precision handling for OPTION_COUNTUP\n\n builtin/am.c | 1 +\n builtin/rebase.c | 1 +\n builtin/update-index.c | 6 ++\n builtin/write-tree.c | 1 +\n parse-options.c | 155 +++++++++++++++++++++++++---------\n parse-options.h | 7 ++\n t/helper/test-parse-options.c | 17 +++-\n 7 files changed, 146 insertions(+), 42 deletions(-)\n\nInterdiff against v1:\ndiff --git a/parse-options.c b/parse-options.c\nindex 0dd08a3a77..5224203ffe 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -68,23 +68,34 @@ static char *fix_filename(const char *prefix, const char *file)\n \t\treturn prefix_filename_except_for_dash(prefix, file);\n }\n \n-static intmax_t get_int_value(const struct option *opt)\n+static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n {\n-\tswitch (opt->precision) {\n+\tswitch (precision) {\n \tcase sizeof(int8_t):\n-\t\treturn *(int8_t *)opt->value;\n+\t\t*ret = *(int8_t *)value;\n+\t\treturn 0;\n \tcase sizeof(int16_t):\n-\t\treturn *(int16_t *)opt->value;\n+\t\t*ret = *(int16_t *)value;\n+\t\treturn 0;\n \tcase sizeof(int32_t):\n-\t\treturn *(int32_t *)opt->value;\n+\t\t*ret = *(int32_t *)value;\n+\t\treturn 0;\n \tcase sizeof(int64_t):\n-\t\treturn *(int64_t *)opt->value;\n+\t\t*ret = *(int64_t *)value;\n+\t\treturn 0;\n \tdefault:\n-\t\toptbug(opt, \"has invalid precision\");\n-\t\tBUG(\"invalid 'struct option'\");\n+\t\treturn -1;\n \t}\n }\n \n+static intmax_t get_int_value(const struct option *opt, enum opt_parsed flags)\n+{\n+\tintmax_t ret;\n+\tif (do_get_int_value(opt->value, opt->precision, &ret))\n+\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n+\treturn ret;\n+}\n+\n static enum parse_opt_result set_int_value(const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n \t\t\t\t\t intmax_t value)\n@@ -107,9 +118,9 @@ static enum parse_opt_result set_int_value(const struct option *opt,\n \t}\n }\n \n-static int signed_int_fits(intmax_t value, size_t size)\n+static int signed_int_fits(intmax_t value, size_t precision)\n {\n-\tsize_t bits = size * CHAR_BIT;\n+\tsize_t bits = precision * CHAR_BIT;\n \tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n \tintmax_t lower_bound = -upper_bound - 1;\n \treturn lower_bound <= value && value <= upper_bound;\n@@ -137,7 +148,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \n \tcase OPTION_BIT:\n \t{\n-\t\tintmax_t value = get_int_value(opt);\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n \t\t\tvalue &= ~opt->defval;\n \t\telse\n@@ -147,7 +158,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \n \tcase OPTION_NEGBIT:\n \t{\n-\t\tintmax_t value = get_int_value(opt);\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n \t\t\tvalue |= opt->defval;\n \t\telse\n@@ -157,7 +168,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \n \tcase OPTION_BITOP:\n \t{\n-\t\tintmax_t value = get_int_value(opt);\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n \t\t\tBUG(\"BITOP can't have unset form\");\n \t\tvalue &= ~opt->extra;\n@@ -169,7 +180,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t{\n \t\tsize_t bits = CHAR_BIT * opt->precision;\n \t\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n-\t\tintmax_t value = get_int_value(opt);\n+\t\tintmax_t value = get_int_value(opt, flags);\n \n \t\tif (value < 0)\n \t\t\tvalue = 0;\n@@ -318,7 +329,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \n struct parse_opt_cmdmode_list {\n \tintmax_t value;\n-\tconst struct option *opt, *reference_opt;\n+\tvoid *value_ptr;\n+\tsize_t precision;\n+\tconst struct option *opt;\n \tconst char *arg;\n \tenum opt_parsed flags;\n \tstruct parse_opt_cmdmode_list *next;\n@@ -331,21 +344,25 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n+\t\tvoid *value_ptr = opts->value;\n \n-\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)\n+\t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n \t\t\tcontinue;\n \n-\t\twhile (elem && elem->reference_opt->value != opts->value)\n+\t\twhile (elem && elem->value_ptr != value_ptr)\n \t\t\telem = elem->next;\n \t\tif (elem)\n \t\t\tcontinue;\n \n \t\tCALLOC_ARRAY(elem, 1);\n-\t\telem->reference_opt = opts;\n-\t\telem->value = get_int_value(opts);\n+\t\telem->value_ptr = value_ptr;\n+\t\telem->precision = opts->precision;\n+\t\tif (do_get_int_value(value_ptr, opts->precision, &elem->value))\n+\t\t\toptbug(opts, \"has invalid precision\");\n \t\telem->next = ctx->cmdmode_list;\n \t\tctx->cmdmode_list = elem;\n \t}\n+\tBUG_if_bug(\"invalid 'struct option'\");\n }\n \n static char *optnamearg(const struct option *opt, const char *arg,\n@@ -367,7 +384,11 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tchar *opt_name, *other_opt_name;\n \n \tfor (; elem; elem = elem->next) {\n-\t\tintmax_t new_value = get_int_value(elem->reference_opt);\n+\t\tintmax_t new_value;\n+\n+\t\tif (do_get_int_value(elem->value_ptr, elem->precision,\n+\t\t\t\t &new_value))\n+\t\t\tBUG(\"impossible: invalid precision\");\n \n \t\tif (new_value == elem->value)\n \t\t\tcontinue;\n-- \n2.50.0\n"},{"id":"521635","messageId":"4d91754b-00a4-47ab-86eb-0da201e7889d@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 1/7] parse-options: require PARSE_OPT_NOARG for OPTION_BITOP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:44:09Z","receivedAt":"2025-07-09T09:44:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"OPTION_BITOP options don't take arguments. Make sure they are declared\nthat way using the flag PARSE_OPT_NOARG.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a9a39ecaef..68ff494492 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -591,6 +591,7 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_BITOP:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\t\toptbug(opts, \"should not accept an argument\");\n-- \n2.50.0\n"},{"id":"521636","messageId":"77b841ea-eab1-4e76-98ff-f7a16653fcb8@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:45:14Z","receivedAt":"2025-07-09T09:45:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Build on 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) to support value variables of different\nsizes for PARSE_OPT_CMDMODE options. Do that by requiring their\n\"precision\" to be set and casting their \"value\" pointer accordingly.\n\nCall the function that does the raw casting do_get_int_value() to\nreserve the name get_int_value() for a more friendly wrapper we're\ngoing to introduce in one of the next patches.\n\nSigned-off-by: René Scharfe \n---\n builtin/am.c | 1 +\n parse-options.c | 41 ++++++++++++++++++++++++++++++-----\n parse-options.h | 1 +\n t/helper/test-parse-options.c | 13 ++++++++---\n 4 files changed, 48 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex a800003340..c9d925f7b9 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2406,6 +2406,7 @@ int cmd_am(int argc,\n \t\t\t.type = OPTION_CALLBACK,\n \t\t\t.long_name = \"show-current-patch\",\n \t\t\t.value = &resume_mode,\n+\t\t\t.precision = sizeof(resume_mode),\n \t\t\t.argh = \"(diff|raw)\",\n \t\t\t.help = N_(\"show the patch being applied\"),\n \t\t\t.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\ndiff --git a/parse-options.c b/parse-options.c\nindex 68ff494492..ddac008a5e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)\n \t\treturn prefix_filename_except_for_dash(prefix, file);\n }\n \n+static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n+{\n+\tswitch (precision) {\n+\tcase sizeof(int8_t):\n+\t\t*ret = *(int8_t *)value;\n+\t\treturn 0;\n+\tcase sizeof(int16_t):\n+\t\t*ret = *(int16_t *)value;\n+\t\treturn 0;\n+\tcase sizeof(int32_t):\n+\t\t*ret = *(int32_t *)value;\n+\t\treturn 0;\n+\tcase sizeof(int64_t):\n+\t\t*ret = *(int64_t *)value;\n+\t\treturn 0;\n+\tdefault:\n+\t\treturn -1;\n+\t}\n+}\n+\n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t\t const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n@@ -266,7 +286,9 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n }\n \n struct parse_opt_cmdmode_list {\n-\tint value, *value_ptr;\n+\tintmax_t value;\n+\tvoid *value_ptr;\n+\tsize_t precision;\n \tconst struct option *opt;\n \tconst char *arg;\n \tenum opt_parsed flags;\n@@ -280,7 +302,7 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n \n \tfor (; opts->type != OPTION_END; opts++) {\n \t\tstruct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;\n-\t\tint *value_ptr = opts->value;\n+\t\tvoid *value_ptr = opts->value;\n \n \t\tif (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)\n \t\t\tcontinue;\n@@ -292,10 +314,13 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,\n \n \t\tCALLOC_ARRAY(elem, 1);\n \t\telem->value_ptr = value_ptr;\n-\t\telem->value = *value_ptr;\n+\t\telem->precision = opts->precision;\n+\t\tif (do_get_int_value(value_ptr, opts->precision, &elem->value))\n+\t\t\toptbug(opts, \"has invalid precision\");\n \t\telem->next = ctx->cmdmode_list;\n \t\tctx->cmdmode_list = elem;\n \t}\n+\tBUG_if_bug(\"invalid 'struct option'\");\n }\n \n static char *optnamearg(const struct option *opt, const char *arg,\n@@ -317,7 +342,13 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \tchar *opt_name, *other_opt_name;\n \n \tfor (; elem; elem = elem->next) {\n-\t\tif (*elem->value_ptr == elem->value)\n+\t\tintmax_t new_value;\n+\n+\t\tif (do_get_int_value(elem->value_ptr, elem->precision,\n+\t\t\t\t &new_value))\n+\t\t\tBUG(\"impossible: invalid precision\");\n+\n+\t\tif (new_value == elem->value)\n \t\t\tcontinue;\n \n \t\tif (elem->opt &&\n@@ -327,7 +358,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,\n \t\telem->opt = opt;\n \t\telem->arg = arg;\n \t\telem->flags = flags;\n-\t\telem->value = *elem->value_ptr;\n+\t\telem->value = new_value;\n \t}\n \n \tif (result || !elem)\ndiff --git a/parse-options.h b/parse-options.h\nindex 91c3e3c29b..c75a473c9e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -269,6 +269,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \\\n \t.defval = (i), \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex f2663dd0c0..1e03ff88f6 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -148,9 +148,16 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_SET_INT(0, \"set23\", &integer, \"set integer to 23\", 23),\n \t\tOPT_CMDMODE(0, \"mode1\", &integer, \"set integer to 1 (cmdmode option)\", 1),\n \t\tOPT_CMDMODE(0, \"mode2\", &integer, \"set integer to 2 (cmdmode option)\", 2),\n-\t\tOPT_CALLBACK_F(0, \"mode34\", &integer, \"(3|4)\",\n-\t\t\t\"set integer to 3 or 4 (cmdmode option)\",\n-\t\t\tPARSE_OPT_CMDMODE, mode34_callback),\n+\t\t{\n+\t\t\t.type = OPTION_CALLBACK,\n+\t\t\t.long_name = \"mode34\",\n+\t\t\t.value = &integer,\n+\t\t\t.precision = sizeof(integer),\n+\t\t\t.argh = \"(3|4)\",\n+\t\t\t.help = \"set integer to 3 or 4 (cmdmode option)\",\n+\t\t\t.flags = PARSE_OPT_CMDMODE,\n+\t\t\t.callback = mode34_callback,\n+\t\t},\n \t\tOPT_CALLBACK('L', \"length\", &integer, \"str\",\n \t\t\t\"get length of \", length_callback),\n \t\tOPT_FILENAME('F', \"file\", &file, \"set file to \"),\n-- \n2.50.0\n"},{"id":"521637","messageId":"fbb7d5fe-e7ae-440f-80c4-991a2aff1d6a@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 3/7] parse-options: add precision handling for OPTION_SET_INT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:45:24Z","receivedAt":"2025-07-09T09:45:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_SET_INT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nFactor out the casting code from the part of do_get_value() that handles\nOPTION_INTEGER to avoid code duplication. We're going to use it in the\nnext patches as well.\n\nSigned-off-by: René Scharfe \n---\n builtin/update-index.c | 6 ++++\n parse-options.c | 56 ++++++++++++++++++++++-------------\n parse-options.h | 2 ++\n t/helper/test-parse-options.c | 1 +\n 4 files changed, 45 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 538b619ba4..0c1d4ed55b 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -981,6 +981,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"assume-unchanged\",\n \t\t\t.value = &mark_valid_only,\n+\t\t\t.precision = sizeof(mark_valid_only),\n \t\t\t.help = N_(\"mark files as \\\"not changing\\\"\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -989,6 +990,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-assume-unchanged\",\n \t\t\t.value = &mark_valid_only,\n+\t\t\t.precision = sizeof(mark_valid_only),\n \t\t\t.help = N_(\"clear assumed-unchanged bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\n@@ -997,6 +999,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"skip-worktree\",\n \t\t\t.value = &mark_skip_worktree_only,\n+\t\t\t.precision = sizeof(mark_skip_worktree_only),\n \t\t\t.help = N_(\"mark files as \\\"index-only\\\"\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -1005,6 +1008,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-skip-worktree\",\n \t\t\t.value = &mark_skip_worktree_only,\n+\t\t\t.precision = sizeof(mark_skip_worktree_only),\n \t\t\t.help = N_(\"clear skip-worktree bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\n@@ -1079,6 +1083,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"fsmonitor-valid\",\n \t\t\t.value = &mark_fsmonitor_only,\n+\t\t\t.precision = sizeof(mark_fsmonitor_only),\n \t\t\t.help = N_(\"mark files as fsmonitor valid\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = MARK_FLAG,\n@@ -1087,6 +1092,7 @@ int cmd_update_index(int argc,\n \t\t\t.type = OPTION_SET_INT,\n \t\t\t.long_name = \"no-fsmonitor-valid\",\n \t\t\t.value = &mark_fsmonitor_only,\n+\t\t\t.precision = sizeof(mark_fsmonitor_only),\n \t\t\t.help = N_(\"clear fsmonitor valid bit\"),\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = UNMARK_FLAG,\ndiff --git a/parse-options.c b/parse-options.c\nindex ddac008a5e..639f41b83b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -88,6 +88,36 @@ static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n \t}\n }\n \n+static enum parse_opt_result set_int_value(const struct option *opt,\n+\t\t\t\t\t enum opt_parsed flags,\n+\t\t\t\t\t intmax_t value)\n+{\n+\tswitch (opt->precision) {\n+\tcase sizeof(int8_t):\n+\t\t*(int8_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int16_t):\n+\t\t*(int16_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int32_t):\n+\t\t*(int32_t *)opt->value = value;\n+\t\treturn 0;\n+\tcase sizeof(int64_t):\n+\t\t*(int64_t *)opt->value = value;\n+\t\treturn 0;\n+\tdefault:\n+\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n+\t}\n+}\n+\n+static int signed_int_fits(intmax_t value, size_t precision)\n+{\n+\tsize_t bits = precision * CHAR_BIT;\n+\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n+\tintmax_t lower_bound = -upper_bound - 1;\n+\treturn lower_bound <= value && value <= upper_bound;\n+}\n+\n static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\t\t\t const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n@@ -136,8 +166,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn 0;\n \n \tcase OPTION_SET_INT:\n-\t\t*(int *)opt->value = unset ? 0 : opt->defval;\n-\t\treturn 0;\n+\t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n \n \tcase OPTION_STRING:\n \t\tif (unset)\n@@ -219,23 +248,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\t\treturn error(_(\"value %s for %s not in range [%\"PRIdMAX\",%\"PRIdMAX\"]\"),\n \t\t\t\t arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);\n \n-\t\tswitch (opt->precision) {\n-\t\tcase 1:\n-\t\t\t*(int8_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 2:\n-\t\t\t*(int16_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 4:\n-\t\t\t*(int32_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tcase 8:\n-\t\t\t*(int64_t *)opt->value = value;\n-\t\t\treturn 0;\n-\t\tdefault:\n-\t\t\tBUG(\"invalid precision for option %s\",\n-\t\t\t optname(opt, flags));\n-\t\t}\n+\t\treturn set_int_value(opt, flags, value);\n \t}\n \tcase OPTION_UNSIGNED:\n \t{\n@@ -617,10 +630,13 @@ static void parse_options_check(const struct option *opts)\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n+\t\tcase OPTION_SET_INT:\n+\t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n+\t\t\t\toptbug(opts, \"has invalid defval\");\n+\t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n-\t\tcase OPTION_SET_INT:\n \t\tcase OPTION_NUMBER:\n \t\tcase OPTION_BITOP:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\ndiff --git a/parse-options.h b/parse-options.h\nindex c75a473c9e..71516e4b5b 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -190,6 +190,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG | (f), \\\n \t.defval = (i), \\\n@@ -260,6 +261,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \\\n \t.defval = 1, \\\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 1e03ff88f6..2ba2546d70 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -131,6 +131,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.short_name = 'B',\n \t\t\t.long_name = \"no-fear\",\n \t\t\t.value = &boolean,\n+\t\t\t.precision = sizeof(boolean),\n \t\t\t.help = \"be brave\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t\t.defval = 1,\n-- \n2.50.0\n"},{"id":"521638","messageId":"b0e933c7-1b95-4bfa-b056-1bf00ad2583c@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 4/7] parse-options: add precision handling for OPTION_BIT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:45:33Z","receivedAt":"2025-07-09T09:45:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_BIT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n builtin/write-tree.c | 1 +\n parse-options.c | 19 +++++++++++++++----\n parse-options.h | 1 +\n 3 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/write-tree.c b/builtin/write-tree.c\nindex 5a8dc377ec..cfec044710 100644\n--- a/builtin/write-tree.c\n+++ b/builtin/write-tree.c\n@@ -35,6 +35,7 @@ int cmd_write_tree(int argc,\n \t\t\t.type = OPTION_BIT,\n \t\t\t.long_name = \"ignore-cache-tree\",\n \t\t\t.value = &flags,\n+\t\t\t.precision = sizeof(flags),\n \t\t\t.help = N_(\"only useful for debugging\"),\n \t\t\t.flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG,\n \t\t\t.defval = WRITE_TREE_IGNORE_CACHE_TREE,\ndiff --git a/parse-options.c b/parse-options.c\nindex 639f41b83b..b5c877d5e1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -88,6 +88,14 @@ static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n \t}\n }\n \n+static intmax_t get_int_value(const struct option *opt, enum opt_parsed flags)\n+{\n+\tintmax_t ret;\n+\tif (do_get_int_value(opt->value, opt->precision, &ret))\n+\t\tBUG(\"invalid precision for option %s\", optname(opt, flags));\n+\treturn ret;\n+}\n+\n static enum parse_opt_result set_int_value(const struct option *opt,\n \t\t\t\t\t enum opt_parsed flags,\n \t\t\t\t\t intmax_t value)\n@@ -139,11 +147,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t\treturn opt->ll_callback(p, opt, NULL, unset);\n \n \tcase OPTION_BIT:\n+\t{\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n+\t\t\tvalue &= ~opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value |= opt->defval;\n-\t\treturn 0;\n+\t\t\tvalue |= opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_NEGBIT:\n \t\tif (unset)\n@@ -631,11 +642,11 @@ static void parse_options_check(const struct option *opts)\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n \t\tswitch (opts->type) {\n \t\tcase OPTION_SET_INT:\n+\t\tcase OPTION_BIT:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n-\t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_NUMBER:\n \t\tcase OPTION_BITOP:\ndiff --git a/parse-options.h b/parse-options.h\nindex 71516e4b5b..6501ca3c27 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -172,6 +172,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|(f), \\\n \t.callback = NULL, \\\n-- \n2.50.0\n"},{"id":"521639","messageId":"c492e2cd-d1aa-43ba-83c0-bd8f1a4d2f19@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 5/7] parse-options: add precision handling for OPTION_NEGBIT","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:45:53Z","receivedAt":"2025-07-09T09:45:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_NEGBIT. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n builtin/rebase.c | 1 +\n parse-options.c | 11 +++++++----\n parse-options.h | 1 +\n 3 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2e8c4ee678..e90562a3b8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1128,6 +1128,7 @@ int cmd_rebase(int argc,\n \t\t\t.short_name = 'n',\n \t\t\t.long_name = \"no-stat\",\n \t\t\t.value = &options.flags,\n+\t\t\t.precision = sizeof(options.flags),\n \t\t\t.help = N_(\"do not show diffstat of what changed upstream\"),\n \t\t\t.flags = PARSE_OPT_NOARG,\n \t\t\t.defval = REBASE_DIFFSTAT,\ndiff --git a/parse-options.c b/parse-options.c\nindex b5c877d5e1..ba89dc4d09 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -157,11 +157,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_NEGBIT:\n+\t{\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n-\t\t\t*(int *)opt->value |= opt->defval;\n+\t\t\tvalue |= opt->defval;\n \t\telse\n-\t\t\t*(int *)opt->value &= ~opt->defval;\n-\t\treturn 0;\n+\t\t\tvalue &= ~opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_BITOP:\n \t\tif (unset)\n@@ -643,11 +646,11 @@ static void parse_options_check(const struct option *opts)\n \t\tswitch (opts->type) {\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_BIT:\n+\t\tcase OPTION_NEGBIT:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n-\t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_NUMBER:\n \t\tcase OPTION_BITOP:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\ndiff --git a/parse-options.h b/parse-options.h\nindex 6501ca3c27..076f88b384 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -250,6 +250,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG, \\\n \t.defval = (b), \\\n-- \n2.50.0\n"},{"id":"521640","messageId":"62725f95-a732-4ad3-ba55-0bfe4de7f1f3@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 6/7] parse-options: add precision handling for OPTION_BITOP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:46:20Z","receivedAt":"2025-07-09T09:46:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_BITOP. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nCheck if \"devfal\" fits into an integer variable with the given\n\"precision\", but don't check \"extra\", as its value is only used to clear\nbits, so cannot lead to an overflow. Not checking continues to allow\ne.g., using -1 to clear all bits even if the value variable has a\nnarrower type than intptr_t.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 11 +++++++----\n parse-options.h | 1 +\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex ba89dc4d09..a813511b1b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -167,11 +167,14 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_BITOP:\n+\t{\n+\t\tintmax_t value = get_int_value(opt, flags);\n \t\tif (unset)\n \t\t\tBUG(\"BITOP can't have unset form\");\n-\t\t*(int *)opt->value &= ~opt->extra;\n-\t\t*(int *)opt->value |= opt->defval;\n-\t\treturn 0;\n+\t\tvalue &= ~opt->extra;\n+\t\tvalue |= opt->defval;\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_COUNTUP:\n \t\tif (*(int *)opt->value < 0)\n@@ -647,12 +650,12 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n+\t\tcase OPTION_BITOP:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n \t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_NUMBER:\n-\t\tcase OPTION_BITOP:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\t\toptbug(opts, \"should not accept an argument\");\ndiff --git a/parse-options.h b/parse-options.h\nindex 076f88b384..8bdf469ae9 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -240,6 +240,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|PARSE_OPT_NONEG, \\\n \t.defval = (set), \\\n-- \n2.50.0\n"},{"id":"521641","messageId":"2f88dc74-e744-4b09-897a-522429f579b1@web.de","threadId":"63708","inReplyTo":"802eba72-c100-429a-80b7-7a0e8b6559ed@web.de","subject":"[PATCH v2 7/7] parse-options: add precision handling for OPTION_COUNTUP","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T09:46:28Z","receivedAt":"2025-07-09T09:46:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Similar to 09705696f7 (parse-options: introduce precision handling for\n`OPTION_INTEGER`, 2025-04-17) support value variables of different sizes\nfor OPTION_COUNTUP. Do that by requiring their \"precision\" to be set,\ncasting their \"value\" pointer accordingly and checking whether the value\nfits.\n\nSigned-off-by: René Scharfe \n---\n parse-options.c | 22 +++++++++++++++++-----\n parse-options.h | 1 +\n t/helper/test-parse-options.c | 3 +++\n 3 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a813511b1b..5224203ffe 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -177,10 +177,22 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,\n \t}\n \n \tcase OPTION_COUNTUP:\n-\t\tif (*(int *)opt->value < 0)\n-\t\t\t*(int *)opt->value = 0;\n-\t\t*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;\n-\t\treturn 0;\n+\t{\n+\t\tsize_t bits = CHAR_BIT * opt->precision;\n+\t\tintmax_t upper_bound = INTMAX_MAX >> (bitsizeof(intmax_t) - bits);\n+\t\tintmax_t value = get_int_value(opt, flags);\n+\n+\t\tif (value < 0)\n+\t\t\tvalue = 0;\n+\t\tif (unset)\n+\t\t\tvalue = 0;\n+\t\telse if (value < upper_bound)\n+\t\t\tvalue++;\n+\t\telse\n+\t\t\treturn error(_(\"value for %s exceeds %\"PRIdMAX),\n+\t\t\t\t optname(opt, flags), upper_bound);\n+\t\treturn set_int_value(opt, flags, value);\n+\t}\n \n \tcase OPTION_SET_INT:\n \t\treturn set_int_value(opt, flags, unset ? 0 : opt->defval);\n@@ -651,10 +663,10 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_BIT:\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_BITOP:\n+\t\tcase OPTION_COUNTUP:\n \t\t\tif (!signed_int_fits(opts->defval, opts->precision))\n \t\t\t\toptbug(opts, \"has invalid defval\");\n \t\t\t/* fallthru */\n-\t\tcase OPTION_COUNTUP:\n \t\tcase OPTION_NUMBER:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\ndiff --git a/parse-options.h b/parse-options.h\nindex 8bdf469ae9..312045604d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -183,6 +183,7 @@ struct option {\n \t.short_name = (s), \\\n \t.long_name = (l), \\\n \t.value = (v), \\\n+\t.precision = sizeof(*v), \\\n \t.help = (h), \\\n \t.flags = PARSE_OPT_NOARG|(f), \\\n }\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 2ba2546d70..68579d83f3 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -178,6 +178,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.short_name = '+',\n \t\t\t.value = &boolean,\n+\t\t\t.precision = sizeof(boolean),\n \t\t\t.help = \"same as -b\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH,\n \t\t},\n@@ -185,6 +186,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.long_name = \"ambiguous\",\n \t\t\t.value = &ambiguous,\n+\t\t\t.precision = sizeof(ambiguous),\n \t\t\t.help = \"positive ambiguity\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t},\n@@ -192,6 +194,7 @@ int cmd__parse_options(int argc, const char **argv)\n \t\t\t.type = OPTION_COUNTUP,\n \t\t\t.long_name = \"no-ambiguous\",\n \t\t\t.value = &ambiguous,\n+\t\t\t.precision = sizeof(ambiguous),\n \t\t\t.help = \"negative ambiguity\",\n \t\t\t.flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n \t\t},\n-- \n2.50.0\n"},{"id":"521671","messageId":"aG51nFpH6kMT95uf@pks.im","threadId":"63708","inReplyTo":"77b841ea-eab1-4e76-98ff-f7a16653fcb8@web.de","subject":"Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-09T13:58:52Z","receivedAt":"2025-07-09T13:58:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:\n> Build on 09705696f7 (parse-options: introduce precision handling for\n> `OPTION_INTEGER`, 2025-04-17) to support value variables of different\n> sizes for PARSE_OPT_CMDMODE options. Do that by requiring their\n> \"precision\" to be set and casting their \"value\" pointer accordingly.\n> \n> Call the function that does the raw casting do_get_int_value() to\n> reserve the name get_int_value() for a more friendly wrapper we're\n> going to introduce in one of the next patches.\n> \n> Signed-off-by: René Scharfe \n> ---\n> builtin/am.c | 1 +\n> parse-options.c | 41 ++++++++++++++++++++++++++++++-----\n> parse-options.h | 1 +\n> t/helper/test-parse-options.c | 13 ++++++++---\n> 4 files changed, 48 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/am.c b/builtin/am.c\n> index a800003340..c9d925f7b9 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2406,6 +2406,7 @@ int cmd_am(int argc,\n> \t\t\t.type = OPTION_CALLBACK,\n> \t\t\t.long_name = \"show-current-patch\",\n> \t\t\t.value = &resume_mode,\n> +\t\t\t.precision = sizeof(resume_mode),\n> \t\t\t.argh = \"(diff|raw)\",\n> \t\t\t.help = N_(\"show the patch being applied\"),\n> \t\t\t.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> diff --git a/parse-options.c b/parse-options.c\n> index 68ff494492..ddac008a5e 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)\n> \t\treturn prefix_filename_except_for_dash(prefix, file);\n> }\n> \n> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n\nNit: after the fourth patch we have `do_get_int_value()` and\n`get_int_value()`, where the major difference is that the latter dies if\nwe failed to parse the value. It might be easier to discern which is\nwhich if we called them `get_int_value()` and `get_int_value_or_die()`.\n\nPatrick\n"},{"id":"521672","messageId":"aG51pI-_xv5abGwz@pks.im","threadId":"63708","inReplyTo":"4d91754b-00a4-47ab-86eb-0da201e7889d@web.de","subject":"Re: [PATCH v2 1/7] parse-options: require PARSE_OPT_NOARG for OPTION_BITOP","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-09T13:59:00Z","receivedAt":"2025-07-09T13:59:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 09, 2025 at 11:44:09AM +0200, René Scharfe wrote:\n> OPTION_BITOP options don't take arguments. Make sure they are declared\n> that way using the flag PARSE_OPT_NOARG.\n> \n> Signed-off-by: René Scharfe \n\nNice, splitting out this commit makes it way easier to understand.\n\nPatrick\n"},{"id":"521682","messageId":"144ecb24-4da2-4311-bf26-3101788f4b12@web.de","threadId":"63708","inReplyTo":"aG51nFpH6kMT95uf@pks.im","subject":"Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-07-09T15:05:35Z","receivedAt":"2025-07-09T15:05:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 7/9/25 3:58 PM, Patrick Steinhardt wrote:\n> On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:\n>>\n>> Call the function that does the raw casting do_get_int_value() to\n>> reserve the name get_int_value() for a more friendly wrapper we're\n>> going to introduce in one of the next patches.\n\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 68ff494492..ddac008a5e 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)\n>> \t\treturn prefix_filename_except_for_dash(prefix, file);\n>> }\n>> \n>> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n> \n> Nit: after the fourth patch we have `do_get_int_value()` and\n> `get_int_value()`, where the major difference is that the latter dies if\n> we failed to parse the value. It might be easier to discern which is\n> which if we called them `get_int_value()` and `get_int_value_or_die()`.\n\nThat would be misleading because get_int_value() doesn't die() like a\nfunction from write-or-die.c, it BUGs instead. I don't think it makes\nsense to advertise the presence of assertions in a function's name.\nBut we do have a tradition of using a prefix of \"do_\" with wrapped\nfunctions that have a more raw interface and do the actual work.\n\nNit: They don't parse, but cast a void pointer to the appropriate type\nand dereference it.\n\nRené\n\n"},{"id":"521697","messageId":"xmqqple9nysq.fsf@gitster.g","threadId":"63708","inReplyTo":"aG51nFpH6kMT95uf@pks.im","subject":"Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-09T15:56:05Z","receivedAt":"2025-07-09T15:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index a800003340..c9d925f7b9 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -2406,6 +2406,7 @@ int cmd_am(int argc,\n>> \t\t\t.type = OPTION_CALLBACK,\n>> \t\t\t.long_name = \"show-current-patch\",\n>> \t\t\t.value = &resume_mode,\n>> +\t\t\t.precision = sizeof(resume_mode),\n>> \t\t\t.argh = \"(diff|raw)\",\n>> \t\t\t.help = N_(\"show the patch being applied\"),\n>> \t\t\t.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 68ff494492..ddac008a5e 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)\n>> \t\treturn prefix_filename_except_for_dash(prefix, file);\n>> }\n>> \n>> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n>\n> Nit: after the fourth patch we have `do_get_int_value()` and\n> `get_int_value()`, where the major difference is that the latter dies if\n> we failed to parse the value. It might be easier to discern which is\n> which if we called them `get_int_value()` and `get_int_value_or_die()`.\n\nSeeing the symmetry between set_int_value() and do_get_int_value(),\nI tend to agree that it would be easier to remember what the latter\ndoes if it were named get_int_value().\n\nI am not so sure about _or_die(), though. The only kind of error\nthe current get_int_value() detects and acts on is the mismatched\nprecision bug, which is a programmer error; there is no \"die\" but\nwe call \"BUG\".\n\nget_int_value_with_precision_check() is quite a mouthful, but it is\nknown that I am bad at naming X-<, so I dunno.\n\n\n"},{"id":"521698","messageId":"aG6RidvxUqV25Fli@pks.im","threadId":"63708","inReplyTo":"144ecb24-4da2-4311-bf26-3101788f4b12@web.de","subject":"Re: [PATCH v2 2/7] parse-options: add precision handling for PARSE_OPT_CMDMODE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-09T15:58:01Z","receivedAt":"2025-07-09T15:58:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 09, 2025 at 05:05:35PM +0200, René Scharfe wrote:\n> On 7/9/25 3:58 PM, Patrick Steinhardt wrote:\n> > On Wed, Jul 09, 2025 at 11:45:14AM +0200, René Scharfe wrote:\n> >>\n> >> Call the function that does the raw casting do_get_int_value() to\n> >> reserve the name get_int_value() for a more friendly wrapper we're\n> >> going to introduce in one of the next patches.\n> \n> >> diff --git a/parse-options.c b/parse-options.c\n> >> index 68ff494492..ddac008a5e 100644\n> >> --- a/parse-options.c\n> >> +++ b/parse-options.c\n> >> @@ -68,6 +68,26 @@ static char *fix_filename(const char *prefix, const char *file)\n> >> \t\treturn prefix_filename_except_for_dash(prefix, file);\n> >> }\n> >> \n> >> +static int do_get_int_value(const void *value, size_t precision, intmax_t *ret)\n> > \n> > Nit: after the fourth patch we have `do_get_int_value()` and\n> > `get_int_value()`, where the major difference is that the latter dies if\n> > we failed to parse the value. It might be easier to discern which is\n> > which if we called them `get_int_value()` and `get_int_value_or_die()`.\n> \n> That would be misleading because get_int_value() doesn't die() like a\n> function from write-or-die.c, it BUGs instead. I don't think it makes\n> sense to advertise the presence of assertions in a function's name.\n> But we do have a tradition of using a prefix of \"do_\" with wrapped\n> functions that have a more raw interface and do the actual work.\n> \n> Nit: They don't parse, but cast a void pointer to the appropriate type\n> and dereference it.\n\nFair enough. I don't mind it much either way, thanks!\n\nPatrick\n"}]}