[PATCH 1/2] parse-options: add int value pointer to struct option

[PATCH 1/2] parse-options: add int value pointer to struct option

[René Scharfe]

Add an int pointer, value_int, to struct option to provide a typed value
pointer for the various integer options.  It allows type checks at
compile time, which is not possible with the void pointer, value.  Its
use is optional for now.

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

diff --git a/parse-options.c b/parse-options.c
index e8e076c3a6..2552745804 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -82,10 +82,11 @@ static enum parse_opt_result opt_command_mode_error(
 	 * already, and report that this is not compatible with it.
 	 */
 	for (that = all_opts; that->type != OPTION_END; that++) {
+		int *value_int = opt->value_int ? opt->value_int : opt->value;
 		if (that == opt ||
 		    !(that->flags & PARSE_OPT_CMDMODE) ||
 		    that->value != opt->value ||
-		    that->defval != *(int *)opt->value)
+		    that->defval != *value_int)
 			continue;

 		if (that->long_name)
@@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	const char *s, *arg;
 	const int unset = flags & OPT_UNSET;
 	int err;
+	int *value_int = opt->value_int ? opt->value_int : opt->value;

 	if (unset && p->opt)
 		return error(_("%s takes no value"), optname(opt, flags));
@@ -122,7 +124,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	 * is not a grave error, so let it pass.
 	 */
 	if ((opt->flags & PARSE_OPT_CMDMODE) &&
-	    *(int *)opt->value && *(int *)opt->value != opt->defval)
+	    *value_int && *value_int != opt->defval)
 		return opt_command_mode_error(opt, all_opts, flags);

 	switch (opt->type) {
@@ -131,33 +133,33 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,

 	case OPTION_BIT:
 		if (unset)
-			*(int *)opt->value &= ~opt->defval;
+			*value_int &= ~opt->defval;
 		else
-			*(int *)opt->value |= opt->defval;
+			*value_int |= opt->defval;
 		return 0;

 	case OPTION_NEGBIT:
 		if (unset)
-			*(int *)opt->value |= opt->defval;
+			*value_int |= opt->defval;
 		else
-			*(int *)opt->value &= ~opt->defval;
+			*value_int &= ~opt->defval;
 		return 0;

 	case OPTION_BITOP:
 		if (unset)
 			BUG("BITOP can't have unset form");
-		*(int *)opt->value &= ~opt->extra;
-		*(int *)opt->value |= opt->defval;
+		*value_int &= ~opt->extra;
+		*value_int |= opt->defval;
 		return 0;

 	case OPTION_COUNTUP:
-		if (*(int *)opt->value < 0)
-			*(int *)opt->value = 0;
-		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
+		if (*value_int < 0)
+			*value_int = 0;
+		*value_int = unset ? 0 : *value_int + 1;
 		return 0;

 	case OPTION_SET_INT:
-		*(int *)opt->value = unset ? 0 : opt->defval;
+		*value_int = unset ? 0 : opt->defval;
 		return 0;

 	case OPTION_STRING:
@@ -206,11 +208,11 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	}
 	case OPTION_INTEGER:
 		if (unset) {
-			*(int *)opt->value = 0;
+			*value_int = 0;
 			return 0;
 		}
 		if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
-			*(int *)opt->value = opt->defval;
+			*value_int = opt->defval;
 			return 0;
 		}
 		if (get_arg(p, opt, flags, &arg))
@@ -218,7 +220,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		if (!*arg)
 			return error(_("%s expects a numerical value"),
 				     optname(opt, flags));
-		*(int *)opt->value = strtol(arg, (char **)&s, 10);
+		*value_int = strtol(arg, (char **)&s, 10);
 		if (*s)
 			return error(_("%s expects a numerical value"),
 				     optname(opt, flags));
@@ -483,6 +485,8 @@ static void parse_options_check(const struct option *opts)
 		if (opts->type == OPTION_SET_INT && !opts->defval &&
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
+		if (opts->value && opts->value_int)
+			optbug(opts, "only a single value type supported");
 		switch (opts->type) {
 		case OPTION_COUNTUP:
 		case OPTION_BIT:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..5e7475bd2d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -158,6 +158,8 @@ struct option {
 	parse_opt_ll_cb *ll_callback;
 	intptr_t extra;
 	parse_opt_subcommand_fn *subcommand_fn;
+
+	int *value_int;
 };

 #define OPT_BIT_F(s, l, v, h, b, f) { \
--
2.42.0

[PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
dereferenced as an int pointer in parse-options.c::get_value().  These
two types are incompatible, though -- the storage size of an enum can
vary between platforms.  C23 would allow us to specify the underlying
type of the different enums, making them compatible, but with C99 the
easiest safe option is to actually use int as the value type.

Convert the offending OPT_CMDMODE users and use the typed value_int
point in the macro's definition to enforce that type for future ones.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c         | 2 +-
 builtin/help.c       | 5 +++--
 builtin/ls-tree.c    | 2 +-
 builtin/rebase.c     | 2 +-
 builtin/replace.c    | 3 ++-
 builtin/stripspace.c | 2 +-
 parse-options.h      | 2 +-
 7 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..ebb72ebaaa 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2269,7 +2269,7 @@ enum resume_type {
 };

 struct resume_mode {
-	enum resume_type mode;
+	int mode;
 	enum show_patch_type sub_mode;
 };

diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..e8aedb932c 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -42,7 +42,7 @@ enum show_config_type {
 	SHOW_CONFIG_SECTIONS,
 };

-static enum help_action {
+enum help_action {
 	HELP_ACTION_ALL = 1,
 	HELP_ACTION_GUIDES,
 	HELP_ACTION_CONFIG,
@@ -50,7 +50,8 @@ static enum help_action {
 	HELP_ACTION_DEVELOPER_INTERFACES,
 	HELP_ACTION_CONFIG_FOR_COMPLETION,
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
-} cmd_mode;
+};
+static int cmd_mode;

 static const char *html_path;
 static int verbose = 1;
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..6f8c43f729 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -346,7 +346,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	int i, full_tree = 0;
 	int full_name = !prefix || !*prefix;
 	read_tree_fn_t fn = NULL;
-	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+	int cmdmode = MODE_DEFAULT;
 	int null_termination = 0;
 	struct ls_tree_options options = { 0 };
 	const struct option ls_tree_options[] = {
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..d11e749579 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -111,7 +111,7 @@ struct rebase_options {
 		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
 	} flags;
 	struct strvec git_am_opts;
-	enum action action;
+	int action;
 	char *reflog_action;
 	int signoff;
 	int allow_rerere_autoupdate;
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..d0063d3feb 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -553,7 +553,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		MODE_GRAFT,
 		MODE_CONVERT_GRAFT_FILE,
 		MODE_REPLACE
-	} cmdmode = MODE_UNSPECIFIED;
+	};
+	int cmdmode = MODE_UNSPECIFIED;
 	struct option options[] = {
 		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
 		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..f6de0b17dc 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -32,7 +32,7 @@ enum stripspace_mode {
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
-	enum stripspace_mode mode = STRIP_DEFAULT;
+	int mode = STRIP_DEFAULT;
 	int nongit;

 	const struct option options[] = {
diff --git a/parse-options.h b/parse-options.h
index 5e7475bd2d..349c3fca04 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -262,7 +262,7 @@ struct option {
 	.type = OPTION_SET_INT, \
 	.short_name = (s), \
 	.long_name = (l), \
-	.value = (v), \
+	.value_int = (v), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \
--
2.42.0

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>Convert the offending OPT_CMDMODE users and use the typed value_int
>point in the macro's definition to enforce that type for future ones.
>
that defeats -Wswitch[-enum], though.

the pedantically correct solution would be using setter callbacks.

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>>
> that defeats -Wswitch[-enum], though.

True.  Though I don't fully understand these warnings (why not then
also warn about if without else?), but taking them away is a bit rude
to those who care.

> the pedantically correct solution would be using setter callbacks.

Or to use an int to point to and then copy into a companion enum
variable to after parsing, which would be my choice.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Jeff King]

On Mon, Sep 11, 2023 at 10:11:56PM +0200, René Scharfe wrote:

> Am 10.09.23 um 12:18 schrieb Oswald Buddenhagen:
> > On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
> >> Convert the offending OPT_CMDMODE users and use the typed value_int
> >> point in the macro's definition to enforce that type for future ones.
> >>
> > that defeats -Wswitch[-enum], though.
> 
> True.  Though I don't fully understand these warnings (why not then
> also warn about if without else?), but taking them away is a bit rude
> to those who care.

I think losing warnings is unfortunate, but it's just one example.
We're losing the type information completely from the values. That might
be of use to the compiler (both for -Wswitch, but also for code
generation in general). But it is also of use to human readers, who see
that "foo" is of type "enum bar" and know what it's supposed to contain.

> > the pedantically correct solution would be using setter callbacks.
> 
> Or to use an int to point to and then copy into a companion enum
> variable to after parsing, which would be my choice.

Yeah, I had the same thought. I'm just not sure how to do that in a way
that isn't a pain for the callers.

-Peff

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> True.  Though I don't fully understand these warnings (why not then
>> also warn about if without else?), but taking them away is a bit rude
>> to those who care.
>
> I think losing warnings is unfortunate, but it's just one example.
> We're losing the type information completely from the values.
> ...
>> Or to use an int to point to and then copy into a companion enum
>> variable to after parsing, which would be my choice.
>
> Yeah, I had the same thought. I'm just not sure how to do that in a way
> that isn't a pain for the callers.

The discussion seems to have petered out around this point.
What (if anything) do we want to do with this topic?

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 16.09.23 um 19:45 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>>> True.  Though I don't fully understand these warnings (why not then
>>> also warn about if without else?), but taking them away is a bit rude
>>> to those who care.
>>
>> I think losing warnings is unfortunate, but it's just one example.
>> We're losing the type information completely from the values.
>> ...
>>> Or to use an int to point to and then copy into a companion enum
>>> variable to after parsing, which would be my choice.
>>
>> Yeah, I had the same thought. I'm just not sure how to do that in a way
>> that isn't a pain for the callers.
>
> The discussion seems to have petered out around this point.
> What (if anything) do we want to do with this topic?

Here's a version that preserves the enums by using additional int
variables just for the parsing phase.  No tricks.  The diff is long, but
most changes aren't particularly complicated and the resulting code is
not that ugly.  Except for builtin/am.c perhaps, which changes the
command mode value using a callback as well.

--- >8 ---
Subject: [PATCH v2 2/2] parse-options: use and require int pointer for OPT_CMDMODE

Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
dereferenced as an int pointer in parse-options.c::get_value().  The two
types are incompatible, though -- the storage size of an enum can vary
between platforms.  C23 would allow us to specify the underlying type of
the diffenrent enums, making them compatible, but with C99 the easiest
safe option is to actually use int as the value type.

Convert the offending OPT_CMDMODE users to point to a new int value and
set the enum value after parsing.  Switch to the typed value_int pointer
in the macro's definition to enforce that type for future users.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c         | 24 +++++++++++++-----------
 builtin/help.c       | 16 +++++++++-------
 builtin/ls-tree.c    | 12 +++++++-----
 builtin/rebase.c     | 15 +++++++++------
 builtin/replace.c    | 12 +++++++-----
 builtin/stripspace.c |  6 ++++--
 parse-options.h      |  2 +-
 7 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..00930e2152 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2270,13 +2270,14 @@ enum resume_type {

 struct resume_mode {
 	enum resume_type mode;
+	int mode_int;
 	enum show_patch_type sub_mode;
 };

 static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
 {
 	int *opt_value = opt->value;
-	struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
+	struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);

 	/*
 	 * Please update $__git_showcurrentpatch in git-completion.bash
@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 				     "--show-current-patch", arg);
 	}

-	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
+	if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
 		return error(_("options '%s=%s' and '%s=%s' "
 					   "cannot be used together"),
 					 "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);

-	resume->mode = RESUME_SHOW_PATCH;
+	resume->mode_int = RESUME_SHOW_PATCH;
 	resume->sub_mode = new_value;
 	return 0;
 }
@@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 	int binary = -1;
 	int keep_cr = -1;
 	int patch_format = PATCH_FORMAT_UNKNOWN;
-	struct resume_mode resume = { .mode = RESUME_FALSE };
+	struct resume_mode resume = { .mode_int = RESUME_FALSE };
 	int in_progress;
 	int ret = 0;

@@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG),
 		OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
 			N_("override error message when patch failure occurs")),
-		OPT_CMDMODE(0, "continue", &resume.mode,
+		OPT_CMDMODE(0, "continue", &resume.mode_int,
 			N_("continue applying patches after resolving a conflict"),
 			RESUME_RESOLVED),
-		OPT_CMDMODE('r', "resolved", &resume.mode,
+		OPT_CMDMODE('r', "resolved", &resume.mode_int,
 			N_("synonyms for --continue"),
 			RESUME_RESOLVED),
-		OPT_CMDMODE(0, "skip", &resume.mode,
+		OPT_CMDMODE(0, "skip", &resume.mode_int,
 			N_("skip the current patch"),
 			RESUME_SKIP),
-		OPT_CMDMODE(0, "abort", &resume.mode,
+		OPT_CMDMODE(0, "abort", &resume.mode_int,
 			N_("restore the original branch and abort the patching operation"),
 			RESUME_ABORT),
-		OPT_CMDMODE(0, "quit", &resume.mode,
+		OPT_CMDMODE(0, "quit", &resume.mode_int,
 			N_("abort the patching operation but keep HEAD where it is"),
 			RESUME_QUIT),
-		{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
+		{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
 		  "(diff|raw)",
 		  N_("show the patch being applied"),
 		  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 		  parse_opt_show_current_patch, RESUME_SHOW_PATCH },
-		OPT_CMDMODE(0, "allow-empty", &resume.mode,
+		OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
 			N_("record the empty patch as an empty commit"),
 			RESUME_ALLOW_EMPTY),
 		OPT_BOOL(0, "committer-date-is-author-date",
@@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		am_load(&state);

 	argc = parse_options(argc, argv, prefix, options, usage, 0);
+	resume.mode = resume.mode_int;

 	if (binary >= 0)
 		fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..d76f88d544 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -51,6 +51,7 @@ static enum help_action {
 	HELP_ACTION_CONFIG_FOR_COMPLETION,
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
 } cmd_mode;
+static int cmd_mode_int;

 static const char *html_path;
 static int verbose = 1;
@@ -59,7 +60,7 @@ static int exclude_guides;
 static int show_external_commands = -1;
 static int show_aliases = -1;
 static struct option builtin_help_options[] = {
-	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
+	OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
 		    HELP_ACTION_ALL),
 	OPT_BOOL(0, "external-commands", &show_external_commands,
 		 N_("show external commands in --all")),
@@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
 			HELP_FORMAT_INFO),
 	OPT__VERBOSE(&verbose, N_("print command description")),

-	OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
+	OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
 		    HELP_ACTION_GUIDES),
-	OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
+	OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
 		    N_("print list of user-facing repository, command and file interfaces"),
 		    HELP_ACTION_USER_INTERFACES),
-	OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
+	OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
 		    N_("print list of file formats, protocols and other developer interfaces"),
 		    HELP_ACTION_DEVELOPER_INTERFACES),
-	OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
+	OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
 		    HELP_ACTION_CONFIG),
-	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
+	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
 		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
-	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
+	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
 		    HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),

 	OPT_END(),
@@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
+	cmd_mode = cmd_mode_int;

 	if (cmd_mode != HELP_ACTION_ALL &&
 	    (show_external_commands >= 0 ||
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..c64b38614a 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	int i, full_tree = 0;
 	int full_name = !prefix || !*prefix;
 	read_tree_fn_t fn = NULL;
-	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
+	enum ls_tree_cmdmode cmdmode;
+	int cmdmode_int = MODE_DEFAULT;
 	int null_termination = 0;
 	struct ls_tree_options options = { 0 };
 	const struct option ls_tree_options[] = {
@@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_SHOW_TREES),
 		OPT_BOOL('z', NULL, &null_termination,
 			 N_("terminate entries with NUL byte")),
-		OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
+		OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
 			    MODE_LONG),
-		OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
+		OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
 			    MODE_NAME_ONLY),
-		OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
+		OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
 			    MODE_NAME_STATUS),
-		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
+		OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
 			    MODE_OBJECT_ONLY),
 		OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
 		OPT_BOOL(0, "full-tree", &full_tree,
@@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, ls_tree_options,
 			     ls_tree_usage, 0);
 	options.null_termination = null_termination;
+	cmdmode = cmdmode_int;

 	if (full_tree)
 		prefix = NULL;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..6dbe57f0ac 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
 	struct rebase_options options = REBASE_OPTIONS_INIT;
+	int action;
 	const char *branch_name;
 	int ret, flags, total_argc, in_progress = 0;
 	int keep_base = 0;
@@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
+		OPT_CMDMODE(0, "continue", &action, N_("continue"),
 			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &options.action,
+		OPT_CMDMODE(0, "skip", &action,
 			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &options.action,
+		OPT_CMDMODE(0, "abort", &action,
 			    N_("abort and check out the original branch"),
 			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &options.action,
+		OPT_CMDMODE(0, "quit", &action,
 			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
+		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
 			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &options.action,
+		OPT_CMDMODE(0, "show-current-patch", &action,
 			    N_("show the patch file being applied or merged"),
 			    ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
@@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.type != REBASE_UNSPECIFIED)
 		in_progress = 1;

+	action = options.action;
 	total_argc = argc;
 	argc = parse_options(argc, argv, prefix,
 			     builtin_rebase_options,
 			     builtin_rebase_usage, 0);
+	options.action = action;

 	if (preserve_merges_selected)
 		die(_("--preserve-merges was replaced by --rebase-merges\n"
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..205c337ad3 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		MODE_CONVERT_GRAFT_FILE,
 		MODE_REPLACE
 	} cmdmode = MODE_UNSPECIFIED;
+	int cmdmode_int = cmdmode;
 	struct option options[] = {
-		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
-		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
-		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
-		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
-		OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
+		OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
+		OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
+		OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
+		OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
+		OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
 		OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
 			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
@@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)

 	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);

+	cmdmode = cmdmode_int;
 	if (!cmdmode)
 		cmdmode = argc ? MODE_REPLACE : MODE_LIST;

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..e8efa0e7ac 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	enum stripspace_mode mode = STRIP_DEFAULT;
+	int mode_int = mode;
 	int nongit;

 	const struct option options[] = {
-		OPT_CMDMODE('s', "strip-comments", &mode,
+		OPT_CMDMODE('s', "strip-comments", &mode_int,
 			    N_("skip and remove all lines starting with comment character"),
 			    STRIP_COMMENTS),
-		OPT_CMDMODE('c', "comment-lines", &mode,
+		OPT_CMDMODE('c', "comment-lines", &mode_int,
 			    N_("prepend comment character and space to each line"),
 			    COMMENT_LINES),
 		OPT_END()
@@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 	if (argc)
 		usage_with_options(stripspace_usage, options);

+	mode = mode_int;
 	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
 		setup_git_directory_gently(&nongit);
 		git_config(git_default_config, NULL);
diff --git a/parse-options.h b/parse-options.h
index 5e7475bd2d..349c3fca04 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -262,7 +262,7 @@ struct option {
 	.type = OPTION_SET_INT, \
 	.short_name = (s), \
 	.long_name = (l), \
-	.value = (v), \
+	.value_int = (v), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \
--
2.42.0

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>@@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
> 				     "--show-current-patch", arg);
> 	}
>
>-	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>+	if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>
this illustrates why i don't quite like the approach: the context 
determines which variable to use.

my idea would be to introduce a new type OPTION_SET_ENUM which would 
also use the callback field. one could even adjust the data type and 
elide the callback when c23 mode (or more specifically, the enum size 
feature) is detected.

> 		return error(_("options '%s=%s' and '%s=%s' "
> 					   "cannot be used together"),

> 					 "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>
totally on a tangent: the argument order is bogus here.
and the line wrapping is also funny.

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 18.09.23 um 12:10 schrieb Oswald Buddenhagen:
> On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>>
>>         return error(_("options '%s=%s' and '%s=%s' "
>>                        "cannot be used together"),
> 
>>                      "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
> totally on a tangent: the argument order is bogus here.
> and the line wrapping is also funny.

Hah, good catch!  Care to send a patch?

René

[PATCH] am: fix error message in parse_opt_show_current_patch()

[Oswald Buddenhagen]

The argument order was incorrect. This was introduced by 246cac8505
(i18n: turn even more messages into "cannot be used together" ones,
2022-01-05).

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---
fwiw, this is currently the only message that actually uses the %s=%s
format, so as of now, factoring out the argument names has only
theoretical value.

Cc: Jean-Noël Avila <jn.avila@free.fr>
Cc: Johannes Sixt <j6t@kdbg.org>
Cc: René Scharfe <l.s.r@web.de>
Cc: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 202040b62e..6655059a57 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
 	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
 		return error(_("options '%s=%s' and '%s=%s' "
 					   "cannot be used together"),
-					 "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
+			     "--show-current-patch", arg,
+			     "--show-current-patch", valid_modes[resume->sub_mode]);
 
 	resume->mode = RESUME_SHOW_PATCH;
 	resume->sub_mode = new_value;
-- 
2.42.0.419.g70bf8a5751

Re: [PATCH] am: fix error message in parse_opt_show_current_patch()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> The argument order was incorrect. This was introduced by 246cac8505
> (i18n: turn even more messages into "cannot be used together" ones,
> 2022-01-05).
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> fwiw, this is currently the only message that actually uses the %s=%s
> format, so as of now, factoring out the argument names has only
> theoretical value.

I am not sure I follow, if you mean that the programmer needs to
pass "--show-current-patch" only once if we used something like
"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.

The patch looks good.  It seems that we are seeing a crack in test
coverage, by the way?

Will queue.  Thanks.

> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..6655059a57 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2303,7 +2303,8 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>  	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>  		return error(_("options '%s=%s' and '%s=%s' "
>  					   "cannot be used together"),
> -					 "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
> +			     "--show-current-patch", arg,
> +			     "--show-current-patch", valid_modes[resume->sub_mode]);
>  
>  	resume->mode = RESUME_SHOW_PATCH;
>  	resume->sub_mode = new_value;

Re: [PATCH] am: fix error message in parse_opt_show_current_patch()

[Oswald Buddenhagen]

On Thu, Sep 21, 2023 at 12:09:10PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> fwiw, this is currently the only message that actually uses the %s=%s
>> format, so as of now, factoring out the argument names has only
>> theoretical value.
>
>I am not sure I follow, if you mean that the programmer needs to
>pass "--show-current-patch" only once if we used something like
>"%1$s=%2s and %1$s=%3s", I agree that it probably has little value.
>
no, i mean that that the usual pattern is just "options '%s' and '%s' 
cannot be used together". this format string is indeed used many times, 
so it makes sense to factor out the option names to avoid duplication of 
translatable strings. not so here. but this particular case is still a 
lot less specialized than many of the other strings replaced by the 
referenced patch, and it's at least plausible that further uses would be 
added at some point, so i left it as-is.

i thought about the duplication of the option string as well, but 
compilers should merge the string constants, so that part is indeed 
de-duplicated. the cost of the extra pointer push could be avoided by 
use of the %1$s syntax, but afaict that's unprecedented in git, and i 
kind of expect that some printf implementation would throw up from it.
also, it might reduce the chance of the format being used in another 
place, but who knows.

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Phillip Wood]

Hi René

On 18/09/2023 10:28, René Scharfe wrote:
> Here's a version that preserves the enums by using additional int
> variables just for the parsing phase.  No tricks.  The diff is long, but
> most changes aren't particularly complicated and the resulting code is
> not that ugly.  Except for builtin/am.c perhaps, which changes the
> command mode value using a callback as well.
> 

> diff --git a/builtin/am.c b/builtin/am.c
> index 202040b62e..00930e2152 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2270,13 +2270,14 @@ enum resume_type {
> 
>   struct resume_mode {
>   	enum resume_type mode;
> +	int mode_int;
>   	enum show_patch_type sub_mode;
>   };
> 
>   static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
>   {
>   	int *opt_value = opt->value;
> -	struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
> +	struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
> 
>   	/*
>   	 * Please update $__git_showcurrentpatch in git-completion.bash
> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>   				     "--show-current-patch", arg);
>   	}
> 
> -	if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
> +	if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>   		return error(_("options '%s=%s' and '%s=%s' "
>   					   "cannot be used together"),
>   					 "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
> 
> -	resume->mode = RESUME_SHOW_PATCH;
> +	resume->mode_int = RESUME_SHOW_PATCH;
>   	resume->sub_mode = new_value;
>   	return 0;
>   }

Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is 
valid while parsing the options but then we want to use "mode". I wonder 
if we could get Oswald's idea of using callbacks working in a reasonably 
ergonomic way with a couple of macros. We could add an new 
OPTION_SET_ENUM member to "enum parse_opt_type" that would take a setter 
function as well as the usual void *value. To set the value it would 
pass the value pointer and an integer value to the setter function. We 
could change OPT_CMDMODE to use OPTION_SET_ENUM and take the name of the 
enum as well as the integer value we want to set for that option. The 
name of the enum would be used to generate the name of the setter 
callback which would be defined with another macro. The macro to 
generate the setter would look like

#define MAKE_CMDMODE_SETTER(name) \
	static void parse_cmdmode_ ## name (void * var, int value) {
		enum name *p = var;
		*p = value;
	}

then OPT_CMDMODE would look like

#define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
	.type = OPTION_SET_ENUM, \
	.short_name = (s), \
	.long_name = (l), \
	.value = (v), \
	.help = (h), \
	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
	.defval = (i), \
	.enum_setter = parse_cmdmode_ ## n,
}
#define OPT_CMDMODE(s, l, v, h, n, i)  OPT_CMDMODE_F(s, l, v, h, n, i, 0)


Then in builtin/am.c at the top level we'd add

MAKE_CMDMODE_SETTER(resume_type)

and change the option definitions to look like

OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)


Best Wishes

Phillip

> @@ -2316,7 +2317,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>   	int binary = -1;
>   	int keep_cr = -1;
>   	int patch_format = PATCH_FORMAT_UNKNOWN;
> -	struct resume_mode resume = { .mode = RESUME_FALSE };
> +	struct resume_mode resume = { .mode_int = RESUME_FALSE };
>   	int in_progress;
>   	int ret = 0;
> 
> @@ -2387,27 +2388,27 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>   			PARSE_OPT_NOARG),
>   		OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
>   			N_("override error message when patch failure occurs")),
> -		OPT_CMDMODE(0, "continue", &resume.mode,
> +		OPT_CMDMODE(0, "continue", &resume.mode_int,
>   			N_("continue applying patches after resolving a conflict"),
>   			RESUME_RESOLVED),
> -		OPT_CMDMODE('r', "resolved", &resume.mode,
> +		OPT_CMDMODE('r', "resolved", &resume.mode_int,
>   			N_("synonyms for --continue"),
>   			RESUME_RESOLVED),
> -		OPT_CMDMODE(0, "skip", &resume.mode,
> +		OPT_CMDMODE(0, "skip", &resume.mode_int,
>   			N_("skip the current patch"),
>   			RESUME_SKIP),
> -		OPT_CMDMODE(0, "abort", &resume.mode,
> +		OPT_CMDMODE(0, "abort", &resume.mode_int,
>   			N_("restore the original branch and abort the patching operation"),
>   			RESUME_ABORT),
> -		OPT_CMDMODE(0, "quit", &resume.mode,
> +		OPT_CMDMODE(0, "quit", &resume.mode_int,
>   			N_("abort the patching operation but keep HEAD where it is"),
>   			RESUME_QUIT),
> -		{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
> +		{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode_int,
>   		  "(diff|raw)",
>   		  N_("show the patch being applied"),
>   		  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>   		  parse_opt_show_current_patch, RESUME_SHOW_PATCH },
> -		OPT_CMDMODE(0, "allow-empty", &resume.mode,
> +		OPT_CMDMODE(0, "allow-empty", &resume.mode_int,
>   			N_("record the empty patch as an empty commit"),
>   			RESUME_ALLOW_EMPTY),
>   		OPT_BOOL(0, "committer-date-is-author-date",
> @@ -2439,6 +2440,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>   		am_load(&state);
> 
>   	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +	resume.mode = resume.mode_int;
> 
>   	if (binary >= 0)
>   		fprintf_ln(stderr, _("The -b/--binary option has been a no-op for long time, and\n"
> diff --git a/builtin/help.c b/builtin/help.c
> index dc1fbe2b98..d76f88d544 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -51,6 +51,7 @@ static enum help_action {
>   	HELP_ACTION_CONFIG_FOR_COMPLETION,
>   	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
>   } cmd_mode;
> +static int cmd_mode_int;
> 
>   static const char *html_path;
>   static int verbose = 1;
> @@ -59,7 +60,7 @@ static int exclude_guides;
>   static int show_external_commands = -1;
>   static int show_aliases = -1;
>   static struct option builtin_help_options[] = {
> -	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
> +	OPT_CMDMODE('a', "all", &cmd_mode_int, N_("print all available commands"),
>   		    HELP_ACTION_ALL),
>   	OPT_BOOL(0, "external-commands", &show_external_commands,
>   		 N_("show external commands in --all")),
> @@ -72,19 +73,19 @@ static struct option builtin_help_options[] = {
>   			HELP_FORMAT_INFO),
>   	OPT__VERBOSE(&verbose, N_("print command description")),
> 
> -	OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
> +	OPT_CMDMODE('g', "guides", &cmd_mode_int, N_("print list of useful guides"),
>   		    HELP_ACTION_GUIDES),
> -	OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
> +	OPT_CMDMODE(0, "user-interfaces", &cmd_mode_int,
>   		    N_("print list of user-facing repository, command and file interfaces"),
>   		    HELP_ACTION_USER_INTERFACES),
> -	OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
> +	OPT_CMDMODE(0, "developer-interfaces", &cmd_mode_int,
>   		    N_("print list of file formats, protocols and other developer interfaces"),
>   		    HELP_ACTION_DEVELOPER_INTERFACES),
> -	OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
> +	OPT_CMDMODE('c', "config", &cmd_mode_int, N_("print all configuration variable names"),
>   		    HELP_ACTION_CONFIG),
> -	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
> +	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode_int, "",
>   		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> -	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
> +	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode_int, "",
>   		    HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
> 
>   	OPT_END(),
> @@ -640,6 +641,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>   	argc = parse_options(argc, argv, prefix, builtin_help_options,
>   			builtin_help_usage, 0);
>   	parsed_help_format = help_format;
> +	cmd_mode = cmd_mode_int;
> 
>   	if (cmd_mode != HELP_ACTION_ALL &&
>   	    (show_external_commands >= 0 ||
> diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
> index 209d2dc0d5..c64b38614a 100644
> --- a/builtin/ls-tree.c
> +++ b/builtin/ls-tree.c
> @@ -346,7 +346,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>   	int i, full_tree = 0;
>   	int full_name = !prefix || !*prefix;
>   	read_tree_fn_t fn = NULL;
> -	enum ls_tree_cmdmode cmdmode = MODE_DEFAULT;
> +	enum ls_tree_cmdmode cmdmode;
> +	int cmdmode_int = MODE_DEFAULT;
>   	int null_termination = 0;
>   	struct ls_tree_options options = { 0 };
>   	const struct option ls_tree_options[] = {
> @@ -358,13 +359,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>   			LS_SHOW_TREES),
>   		OPT_BOOL('z', NULL, &null_termination,
>   			 N_("terminate entries with NUL byte")),
> -		OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
> +		OPT_CMDMODE('l', "long", &cmdmode_int, N_("include object size"),
>   			    MODE_LONG),
> -		OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
> +		OPT_CMDMODE(0, "name-only", &cmdmode_int, N_("list only filenames"),
>   			    MODE_NAME_ONLY),
> -		OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
> +		OPT_CMDMODE(0, "name-status", &cmdmode_int, N_("list only filenames"),
>   			    MODE_NAME_STATUS),
> -		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
> +		OPT_CMDMODE(0, "object-only", &cmdmode_int, N_("list only objects"),
>   			    MODE_OBJECT_ONLY),
>   		OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
>   		OPT_BOOL(0, "full-tree", &full_tree,
> @@ -384,6 +385,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
>   	argc = parse_options(argc, argv, prefix, ls_tree_options,
>   			     ls_tree_usage, 0);
>   	options.null_termination = null_termination;
> +	cmdmode = cmdmode_int;
> 
>   	if (full_tree)
>   		prefix = NULL;
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 50cb85751f..6dbe57f0ac 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1061,6 +1061,7 @@ static int check_exec_cmd(const char *cmd)
>   int cmd_rebase(int argc, const char **argv, const char *prefix)
>   {
>   	struct rebase_options options = REBASE_OPTIONS_INIT;
> +	int action;
>   	const char *branch_name;
>   	int ret, flags, total_argc, in_progress = 0;
>   	int keep_base = 0;
> @@ -1116,18 +1117,18 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		OPT_BIT(0, "no-ff", &options.flags,
>   			N_("cherry-pick all commits, even if unchanged"),
>   			REBASE_FORCE),
> -		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
> +		OPT_CMDMODE(0, "continue", &action, N_("continue"),
>   			    ACTION_CONTINUE),
> -		OPT_CMDMODE(0, "skip", &options.action,
> +		OPT_CMDMODE(0, "skip", &action,
>   			    N_("skip current patch and continue"), ACTION_SKIP),
> -		OPT_CMDMODE(0, "abort", &options.action,
> +		OPT_CMDMODE(0, "abort", &action,
>   			    N_("abort and check out the original branch"),
>   			    ACTION_ABORT),
> -		OPT_CMDMODE(0, "quit", &options.action,
> +		OPT_CMDMODE(0, "quit", &action,
>   			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
> -		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
> +		OPT_CMDMODE(0, "edit-todo", &action, N_("edit the todo list "
>   			    "during an interactive rebase"), ACTION_EDIT_TODO),
> -		OPT_CMDMODE(0, "show-current-patch", &options.action,
> +		OPT_CMDMODE(0, "show-current-patch", &action,
>   			    N_("show the patch file being applied or merged"),
>   			    ACTION_SHOW_CURRENT_PATCH),
>   		OPT_CALLBACK_F(0, "apply", &options, NULL,
> @@ -1233,10 +1234,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.type != REBASE_UNSPECIFIED)
>   		in_progress = 1;
> 
> +	action = options.action;
>   	total_argc = argc;
>   	argc = parse_options(argc, argv, prefix,
>   			     builtin_rebase_options,
>   			     builtin_rebase_usage, 0);
> +	options.action = action;
> 
>   	if (preserve_merges_selected)
>   		die(_("--preserve-merges was replaced by --rebase-merges\n"
> diff --git a/builtin/replace.c b/builtin/replace.c
> index da59600ad2..205c337ad3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -554,12 +554,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
>   		MODE_CONVERT_GRAFT_FILE,
>   		MODE_REPLACE
>   	} cmdmode = MODE_UNSPECIFIED;
> +	int cmdmode_int = cmdmode;
>   	struct option options[] = {
> -		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
> -		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
> -		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
> -		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
> -		OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
> +		OPT_CMDMODE('l', "list", &cmdmode_int, N_("list replace refs"), MODE_LIST),
> +		OPT_CMDMODE('d', "delete", &cmdmode_int, N_("delete replace refs"), MODE_DELETE),
> +		OPT_CMDMODE('e', "edit", &cmdmode_int, N_("edit existing object"), MODE_EDIT),
> +		OPT_CMDMODE('g', "graft", &cmdmode_int, N_("change a commit's parents"), MODE_GRAFT),
> +		OPT_CMDMODE(0, "convert-graft-file", &cmdmode_int, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
>   		OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
>   			   PARSE_OPT_NOCOMPLETE),
>   		OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
> @@ -572,6 +573,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
> 
>   	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
> 
> +	cmdmode = cmdmode_int;
>   	if (!cmdmode)
>   		cmdmode = argc ? MODE_REPLACE : MODE_LIST;
> 
> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index 7b700a9fb1..e8efa0e7ac 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -33,13 +33,14 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
>   {
>   	struct strbuf buf = STRBUF_INIT;
>   	enum stripspace_mode mode = STRIP_DEFAULT;
> +	int mode_int = mode;
>   	int nongit;
> 
>   	const struct option options[] = {
> -		OPT_CMDMODE('s', "strip-comments", &mode,
> +		OPT_CMDMODE('s', "strip-comments", &mode_int,
>   			    N_("skip and remove all lines starting with comment character"),
>   			    STRIP_COMMENTS),
> -		OPT_CMDMODE('c', "comment-lines", &mode,
> +		OPT_CMDMODE('c', "comment-lines", &mode_int,
>   			    N_("prepend comment character and space to each line"),
>   			    COMMENT_LINES),
>   		OPT_END()
> @@ -49,6 +50,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
>   	if (argc)
>   		usage_with_options(stripspace_usage, options);
> 
> +	mode = mode_int;
>   	if (mode == STRIP_COMMENTS || mode == COMMENT_LINES) {
>   		setup_git_directory_gently(&nongit);
>   		git_config(git_default_config, NULL);
> diff --git a/parse-options.h b/parse-options.h
> index 5e7475bd2d..349c3fca04 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -262,7 +262,7 @@ struct option {
>   	.type = OPTION_SET_INT, \
>   	.short_name = (s), \
>   	.long_name = (l), \
> -	.value = (v), \
> +	.value_int = (v), \
>   	.help = (h), \
>   	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
>   	.defval = (i), \
> --
> 2.42.0

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> -	resume->mode = RESUME_SHOW_PATCH;
>> +	resume->mode_int = RESUME_SHOW_PATCH;
>>   	resume->sub_mode = new_value;
>>   	return 0;
>>   }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int" is
> valid while parsing the options but then we want to use "mode". I
> wonder if we could get Oswald's idea of using callbacks working in a
> reasonably ergonomic way with a couple of macros. We could add an new
> OPTION_SET_ENUM member to "enum parse_opt_type" that would take a
> setter function as well as the usual void *value. To set the value it
> would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and take
> the name of the enum as well as the integer value we want to set for
> that option. The name of the enum would be used to generate the name
> of the setter callback which would be defined with another macro. The
> macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> 	static void parse_cmdmode_ ## name (void * var, int value) {
> 		enum name *p = var;
> 		*p = value;
> 	}

Ah, OK.  So that's how you defeat "how the size and alignment of an
enum mixes well with int is not known and depends on particular enum
type".  It is a tad sad that this relies on "void *", which means
that the caller of parse_cmdmode_resume_type cannot be forced by the
compilers to pass "enum resume_type *" to the function, though.  And
that is probably inevitable with the design as .enum_setter needs to
be of a single type, and the member in the "struct option" that
points at the destination variable must be "void *" as it has to
be capable of pointing at various different enum types.

> ...
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)

Yup, that is ergonomic and corrects "The shape of a particular enum
may not match 'int'" issue nicely.  I do not know how severe the
problem is that it is not quite type safe that we cannot enforce
resume_type is the same as typeof(resume.mode) here, though.

Thanks.

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Phillip Wood]

On 18/09/2023 18:11, Junio C Hamano wrote:
>> Then in builtin/am.c at the top level we'd add
>>
>> MAKE_CMDMODE_SETTER(resume_type)
>>
>> and change the option definitions to look like
>>
>> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
> 
> Yup, that is ergonomic and corrects "The shape of a particular enum
> may not match 'int'" issue nicely.  I do not know how severe the
> problem is that it is not quite type safe that we cannot enforce
> resume_type is the same as typeof(resume.mode) here, though.

We could use gcc's __builtin_types_compatible_p() if we're prepared to 
have two definitions of OPT_CMDMODE_F

#if defined(__GNUC__)
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
	...
	.defval (i) + \
		BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n, 
__typeof__(v))), \
}
#else
#define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
	...
	.defval (i), \
}
#endif

Best Wishes

Phillip

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 18.09.23 um 21:48 schrieb Phillip Wood:
> On 18/09/2023 18:11, Junio C Hamano wrote:
>>> Then in builtin/am.c at the top level we'd add
>>>
>>> MAKE_CMDMODE_SETTER(resume_type)
>>>
>>> and change the option definitions to look like
>>>
>>> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
>>
>> Yup, that is ergonomic and corrects "The shape of a particular enum
>> may not match 'int'" issue nicely.  I do not know how severe the
>> problem is that it is not quite type safe that we cannot enforce
>> resume_type is the same as typeof(resume.mode) here, though.
>
> We could use gcc's __builtin_types_compatible_p() if we're prepared to have two definitions of OPT_CMDMODE_F
>
> #if defined(__GNUC__)
> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
>     ...
>     .defval (i) + \
>         BUILD_ASSERT_OR_ZERO(__builtin_types_compatible_p(enum n, __typeof__(v))), \
> }
> #else
> #define OPT_CMDMODE_F(s, l, n, v, h, i, f) { \
>     ...
>     .defval (i), \
> }
> #endif
>
> Best Wishes
>
> Phillip

That would work, but we can do a type check without a compiler builtin
by creating a function for that purpose.  How about this?

--- >8 ---
Subject: [PATCH] parse-options: make OPT_CMDMODE type-safe

Some uses of OPT_CMDMODE point to an enum as value, but the option
parser dereferences it as an int pointer.  Depending on the platform,
pointers to int and enums can be incompatible.

Add value accessor functions to struct option to dereference the pointer
safely, use them for PARSE_OPT_CMDMODE error detection and in the new
OPTION_SET_VALUE option type, add a typed version of OPT_CMDMODE named
OPT_CMDMODE_T, make OPT_CMDMODE only accept int and convert enum users
to declare the appropriate type.

The accessor functions for each type must be defined using
DEFINE_OPTION_VALUE_TYPE.  OPT_CMDMODE needs the ones for int, so we
define them centrally in parse-options.h.

builtin/am.c::parse_opt_show_current_patch() uses the value pointer just
to calculate the offset of the enclosing struct resume_mode, making the
pointer type inconsequential.  Use the correct type instead of int *
regardless, for consistency.

The enum used with OPT_CMDMODE in builtin/replace.c was anonymous.  Give
it a name to allow specifying it in DEFINE_OPTION_VALUE_TYPE and then
OPT_CMDMODE_T.

Suggested-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/technical/api-parse-options.txt |  7 ++++
 builtin/am.c                                  | 32 ++++++++++------
 builtin/help.c                                | 37 +++++++++++--------
 builtin/ls-tree.c                             | 18 +++++----
 builtin/rebase.c                              | 33 ++++++++++-------
 builtin/replace.c                             | 37 ++++++++++++-------
 builtin/stripspace.c                          | 14 ++++---
 parse-options.c                               | 20 +++++++++-
 parse-options.h                               | 36 ++++++++++++++++--
 9 files changed, 159 insertions(+), 75 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 61fa6ee167..3958ab7c94 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -278,6 +278,13 @@ There are some macros to easily define options:
 	option has already set its value to the same `int_var`.
 	In new commands consider using subcommands instead.

+`OPT_CMDMODE_T(short, long, type_name, &enum_var, description, enum_val)`::
+	Same as `OPT_CMDMODE`, but allows specifying an enum variable
+	instead of an int.
+	Requires `DEFINE_OPTION_VALUE_TYPE(type_name, enum_type);` at
+	file scope to declare `type_name`; `enum_type` must be the type
+	of `enum_var`.
+
 `OPT_SUBCOMMAND(long, &fn_ptr, subcommand_fn)`::
 	Define a subcommand.  `subcommand_fn` is put into `fn_ptr` when
 	this subcommand is used.
diff --git a/builtin/am.c b/builtin/am.c
index 6655059a57..4642dc5099 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2268,6 +2268,8 @@ enum resume_type {
 	RESUME_ALLOW_EMPTY,
 };

+DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);
+
 struct resume_mode {
 	enum resume_type mode;
 	enum show_patch_type sub_mode;
@@ -2275,7 +2277,7 @@ struct resume_mode {

 static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
 {
-	int *opt_value = opt->value;
+	enum resume_type *opt_value = opt->value;
 	struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);

 	/*
@@ -2388,27 +2390,33 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG),
 		OPT_STRING(0, "resolvemsg", &state.resolvemsg, NULL,
 			N_("override error message when patch failure occurs")),
-		OPT_CMDMODE(0, "continue", &resume.mode,
+		OPT_CMDMODE_T(0, "continue", resume_type, &resume.mode,
 			N_("continue applying patches after resolving a conflict"),
 			RESUME_RESOLVED),
-		OPT_CMDMODE('r', "resolved", &resume.mode,
+		OPT_CMDMODE_T('r', "resolved", resume_type, &resume.mode,
 			N_("synonyms for --continue"),
 			RESUME_RESOLVED),
-		OPT_CMDMODE(0, "skip", &resume.mode,
+		OPT_CMDMODE_T(0, "skip", resume_type, &resume.mode,
 			N_("skip the current patch"),
 			RESUME_SKIP),
-		OPT_CMDMODE(0, "abort", &resume.mode,
+		OPT_CMDMODE_T(0, "abort", resume_type, &resume.mode,
 			N_("restore the original branch and abort the patching operation"),
 			RESUME_ABORT),
-		OPT_CMDMODE(0, "quit", &resume.mode,
+		OPT_CMDMODE_T(0, "quit", resume_type, &resume.mode,
 			N_("abort the patching operation but keep HEAD where it is"),
 			RESUME_QUIT),
-		{ OPTION_CALLBACK, 0, "show-current-patch", &resume.mode,
-		  "(diff|raw)",
-		  N_("show the patch being applied"),
-		  PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
-		  parse_opt_show_current_patch, RESUME_SHOW_PATCH },
-		OPT_CMDMODE(0, "allow-empty", &resume.mode,
+		{
+			.type = OPTION_CALLBACK,
+			.long_name = "show-current-patch",
+			OPTION_VALUE(resume_type, &resume.mode),
+			.argh = "(diff|raw)",
+			.help = N_("show the patch being applied"),
+			.flags = PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG |
+				 PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			.callback = parse_opt_show_current_patch,
+			.defval = RESUME_SHOW_PATCH,
+		},
+		OPT_CMDMODE_T(0, "allow-empty", resume_type, &resume.mode,
 			N_("record the empty patch as an empty commit"),
 			RESUME_ALLOW_EMPTY),
 		OPT_BOOL(0, "committer-date-is-author-date",
diff --git a/builtin/help.c b/builtin/help.c
index dc1fbe2b98..48437c8636 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -52,6 +52,8 @@ static enum help_action {
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
 } cmd_mode;

+DEFINE_OPTION_VALUE_TYPE(help_action, enum help_action);
+
 static const char *html_path;
 static int verbose = 1;
 static enum help_format help_format = HELP_FORMAT_NONE;
@@ -59,8 +61,8 @@ static int exclude_guides;
 static int show_external_commands = -1;
 static int show_aliases = -1;
 static struct option builtin_help_options[] = {
-	OPT_CMDMODE('a', "all", &cmd_mode, N_("print all available commands"),
-		    HELP_ACTION_ALL),
+	OPT_CMDMODE_T('a', "all", help_action, &cmd_mode,
+		      N_("print all available commands"), HELP_ACTION_ALL),
 	OPT_BOOL(0, "external-commands", &show_external_commands,
 		 N_("show external commands in --all")),
 	OPT_BOOL(0, "aliases", &show_aliases, N_("show aliases in --all")),
@@ -72,20 +74,23 @@ static struct option builtin_help_options[] = {
 			HELP_FORMAT_INFO),
 	OPT__VERBOSE(&verbose, N_("print command description")),

-	OPT_CMDMODE('g', "guides", &cmd_mode, N_("print list of useful guides"),
-		    HELP_ACTION_GUIDES),
-	OPT_CMDMODE(0, "user-interfaces", &cmd_mode,
-		    N_("print list of user-facing repository, command and file interfaces"),
-		    HELP_ACTION_USER_INTERFACES),
-	OPT_CMDMODE(0, "developer-interfaces", &cmd_mode,
-		    N_("print list of file formats, protocols and other developer interfaces"),
-		    HELP_ACTION_DEVELOPER_INTERFACES),
-	OPT_CMDMODE('c', "config", &cmd_mode, N_("print all configuration variable names"),
-		    HELP_ACTION_CONFIG),
-	OPT_CMDMODE_F(0, "config-for-completion", &cmd_mode, "",
-		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
-	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
-		    HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+	OPT_CMDMODE_T('g', "guides", help_action, &cmd_mode,
+		      N_("print list of useful guides"), HELP_ACTION_GUIDES),
+	OPT_CMDMODE_T(0, "user-interfaces", help_action, &cmd_mode,
+		      N_("print list of user-facing repository, command and file interfaces"),
+		      HELP_ACTION_USER_INTERFACES),
+	OPT_CMDMODE_T(0, "developer-interfaces", help_action, &cmd_mode,
+		      N_("print list of file formats, protocols and other developer interfaces"),
+		      HELP_ACTION_DEVELOPER_INTERFACES),
+	OPT_CMDMODE_T('c', "config", help_action, &cmd_mode,
+		      N_("print all configuration variable names"),
+		      HELP_ACTION_CONFIG),
+	OPT_CMDMODE_T_F(0, "config-for-completion", help_action, &cmd_mode, "",
+			HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+	OPT_CMDMODE_T_F(0, "config-sections-for-completion",
+			help_action, &cmd_mode, "",
+			HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+			PARSE_OPT_HIDDEN),

 	OPT_END(),
 };
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 209d2dc0d5..2e7faf510d 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -339,6 +339,8 @@ static struct ls_tree_cmdmode_to_fmt ls_tree_cmdmode_format[] = {
 	},
 };

+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum ls_tree_cmdmode);
+
 int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	struct object_id oid;
@@ -358,14 +360,14 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 			LS_SHOW_TREES),
 		OPT_BOOL('z', NULL, &null_termination,
 			 N_("terminate entries with NUL byte")),
-		OPT_CMDMODE('l', "long", &cmdmode, N_("include object size"),
-			    MODE_LONG),
-		OPT_CMDMODE(0, "name-only", &cmdmode, N_("list only filenames"),
-			    MODE_NAME_ONLY),
-		OPT_CMDMODE(0, "name-status", &cmdmode, N_("list only filenames"),
-			    MODE_NAME_STATUS),
-		OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"),
-			    MODE_OBJECT_ONLY),
+		OPT_CMDMODE_T('l', "long", cmdmode, &cmdmode,
+			      N_("include object size"), MODE_LONG),
+		OPT_CMDMODE_T(0, "name-only", cmdmode, &cmdmode,
+			      N_("list only filenames"), MODE_NAME_ONLY),
+		OPT_CMDMODE_T(0, "name-status", cmdmode, &cmdmode,
+			      N_("list only filenames"), MODE_NAME_STATUS),
+		OPT_CMDMODE_T(0, "object-only", cmdmode, &cmdmode,
+			      N_("list only objects"), MODE_OBJECT_ONLY),
 		OPT_BOOL(0, "full-name", &full_name, N_("use full path names")),
 		OPT_BOOL(0, "full-tree", &full_tree,
 			 N_("list entire tree; not just current directory "
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ed15accec9..50739327bd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -75,6 +75,8 @@ enum action {
 	ACTION_SHOW_CURRENT_PATCH
 };

+DEFINE_OPTION_VALUE_TYPE(action, enum action);
+
 static const char *action_names[] = {
 	"undefined",
 	"continue",
@@ -1116,20 +1118,23 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-ff", &options.flags,
 			N_("cherry-pick all commits, even if unchanged"),
 			REBASE_FORCE),
-		OPT_CMDMODE(0, "continue", &options.action, N_("continue"),
-			    ACTION_CONTINUE),
-		OPT_CMDMODE(0, "skip", &options.action,
-			    N_("skip current patch and continue"), ACTION_SKIP),
-		OPT_CMDMODE(0, "abort", &options.action,
-			    N_("abort and check out the original branch"),
-			    ACTION_ABORT),
-		OPT_CMDMODE(0, "quit", &options.action,
-			    N_("abort but keep HEAD where it is"), ACTION_QUIT),
-		OPT_CMDMODE(0, "edit-todo", &options.action, N_("edit the todo list "
-			    "during an interactive rebase"), ACTION_EDIT_TODO),
-		OPT_CMDMODE(0, "show-current-patch", &options.action,
-			    N_("show the patch file being applied or merged"),
-			    ACTION_SHOW_CURRENT_PATCH),
+		OPT_CMDMODE_T(0, "continue", action, &options.action,
+			      N_("continue"), ACTION_CONTINUE),
+		OPT_CMDMODE_T(0, "skip", action, &options.action,
+			      N_("skip current patch and continue"),
+			      ACTION_SKIP),
+		OPT_CMDMODE_T(0, "abort", action, &options.action,
+			      N_("abort and check out the original branch"),
+			      ACTION_ABORT),
+		OPT_CMDMODE_T(0, "quit", action, &options.action,
+			      N_("abort but keep HEAD where it is"),
+			      ACTION_QUIT),
+		OPT_CMDMODE_T(0, "edit-todo", action, &options.action,
+			      N_("edit the todo list during an interactive rebase"),
+			      ACTION_EDIT_TODO),
+		OPT_CMDMODE_T(0, "show-current-patch", action, &options.action,
+			      N_("show the patch file being applied or merged"),
+			      ACTION_SHOW_CURRENT_PATCH),
 		OPT_CALLBACK_F(0, "apply", &options, NULL,
 			N_("use apply strategies to rebase"),
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
diff --git a/builtin/replace.c b/builtin/replace.c
index da59600ad2..09aebc9de2 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -539,27 +539,36 @@ static int convert_graft_file(int force)

 	return -1;
 }
+enum cmdmode {
+	MODE_UNSPECIFIED = 0,
+	MODE_LIST,
+	MODE_DELETE,
+	MODE_EDIT,
+	MODE_GRAFT,
+	MODE_CONVERT_GRAFT_FILE,
+	MODE_REPLACE
+};
+
+DEFINE_OPTION_VALUE_TYPE(cmdmode, enum cmdmode);

 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
 	int force = 0;
 	int raw = 0;
 	const char *format = NULL;
-	enum {
-		MODE_UNSPECIFIED = 0,
-		MODE_LIST,
-		MODE_DELETE,
-		MODE_EDIT,
-		MODE_GRAFT,
-		MODE_CONVERT_GRAFT_FILE,
-		MODE_REPLACE
-	} cmdmode = MODE_UNSPECIFIED;
+	enum cmdmode cmdmode = MODE_UNSPECIFIED;
 	struct option options[] = {
-		OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST),
-		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE),
-		OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT),
-		OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
-		OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
+		OPT_CMDMODE_T('l', "list", cmdmode, &cmdmode,
+			      N_("list replace refs"), MODE_LIST),
+		OPT_CMDMODE_T('d', "delete", cmdmode, &cmdmode,
+			      N_("delete replace refs"), MODE_DELETE),
+		OPT_CMDMODE_T('e', "edit", cmdmode, &cmdmode,
+			      N_("edit existing object"), MODE_EDIT),
+		OPT_CMDMODE_T('g', "graft", cmdmode, &cmdmode,
+			      N_("change a commit's parents"), MODE_GRAFT),
+		OPT_CMDMODE_T(0, "convert-graft-file", cmdmode, &cmdmode,
+			      N_("convert existing graft file"),
+			      MODE_CONVERT_GRAFT_FILE),
 		OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
 			   PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index 7b700a9fb1..82b9a95a4f 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -29,6 +29,8 @@ enum stripspace_mode {
 	COMMENT_LINES
 };

+DEFINE_OPTION_VALUE_TYPE(mode, enum stripspace_mode);
+
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -36,12 +38,12 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)
 	int nongit;

 	const struct option options[] = {
-		OPT_CMDMODE('s', "strip-comments", &mode,
-			    N_("skip and remove all lines starting with comment character"),
-			    STRIP_COMMENTS),
-		OPT_CMDMODE('c', "comment-lines", &mode,
-			    N_("prepend comment character and space to each line"),
-			    COMMENT_LINES),
+		OPT_CMDMODE_T('s', "strip-comments", mode, &mode,
+			      N_("skip and remove all lines starting with comment character"),
+			      STRIP_COMMENTS),
+		OPT_CMDMODE_T('c', "comment-lines", mode, &mode,
+			      N_("prepend comment character and space to each line"),
+			      COMMENT_LINES),
 		OPT_END()
 	};

diff --git a/parse-options.c b/parse-options.c
index e8e076c3a6..63a2247128 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
 		if (that == opt ||
 		    !(that->flags & PARSE_OPT_CMDMODE) ||
 		    that->value != opt->value ||
-		    that->defval != *(int *)opt->value)
+		    that->defval != opt->get_value(opt->value))
 			continue;

 		if (that->long_name)
@@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 	 * is not a grave error, so let it pass.
 	 */
 	if ((opt->flags & PARSE_OPT_CMDMODE) &&
-	    *(int *)opt->value && *(int *)opt->value != opt->defval)
+	    opt->get_value(opt->value) &&
+	    opt->get_value(opt->value) != opt->defval)
 		return opt_command_mode_error(opt, all_opts, flags);

 	switch (opt->type) {
@@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 		*(int *)opt->value = unset ? 0 : opt->defval;
 		return 0;

+	case OPTION_SET_VALUE:
+		opt->set_value(opt->value, unset ? 0 : opt->defval);
+		return 0;
+
 	case OPTION_STRING:
 		if (unset)
 			*(const char **)opt->value = NULL;
@@ -483,11 +488,21 @@ static void parse_options_check(const struct option *opts)
 		if (opts->type == OPTION_SET_INT && !opts->defval &&
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
+		if (opts->type == OPTION_SET_VALUE && !opts->defval &&
+		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
+			optbug(opts, "OPTION_SET_VALUE 0 should not be negatable");
+		if (opts->type == OPTION_SET_VALUE &&
+		    (!opts->get_value || !opts->set_value))
+			optbug(opts, "OPTION_SET_VALUE requires accessors");
+		if ((opts->flags & PARSE_OPT_CMDMODE) &&
+		    (!opts->get_value || !opts->set_value))
+			optbug(opts, "PARSE_OPT_CMDMODE requires accessors");
 		switch (opts->type) {
 		case OPTION_COUNTUP:
 		case OPTION_BIT:
 		case OPTION_NEGBIT:
 		case OPTION_SET_INT:
+		case OPTION_SET_VALUE:
 		case OPTION_NUMBER:
 			if ((opts->flags & PARSE_OPT_OPTARG) ||
 			    !(opts->flags & PARSE_OPT_NOARG))
@@ -611,6 +626,7 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
 		case OPTION_NEGBIT:
 		case OPTION_COUNTUP:
 		case OPTION_SET_INT:
+		case OPTION_SET_VALUE:
 			has_unset_form = 1;
 			break;
 		default:
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..764e7f7896 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -20,6 +20,7 @@ enum parse_opt_type {
 	OPTION_BITOP,
 	OPTION_COUNTUP,
 	OPTION_SET_INT,
+	OPTION_SET_VALUE,
 	/* options with arguments (usually) */
 	OPTION_STRING,
 	OPTION_INTEGER,
@@ -158,8 +159,34 @@ struct option {
 	parse_opt_ll_cb *ll_callback;
 	intptr_t extra;
 	parse_opt_subcommand_fn *subcommand_fn;
+	intptr_t (*get_value)(void *);
+	void (*set_value)(void *, intptr_t);
 };

+#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
+static inline intptr_t type_name##__get(void *void_ptr) \
+{ \
+	type *ptr = void_ptr; \
+	return (intptr_t)*ptr; \
+} \
+static inline void type_name##__set(void *void_ptr, intptr_t value) \
+{ \
+	type *ptr = void_ptr; \
+	*ptr = (type)value; \
+} \
+static inline void *type_name##__check(type *ptr) \
+{ \
+	return ptr; \
+} \
+static inline void *type_name##__check(type *ptr)
+
+DEFINE_OPTION_VALUE_TYPE(int, int);
+
+#define OPTION_VALUE(type_name, v) \
+	.get_value = type_name##__get, \
+	.set_value = type_name##__set, \
+	.value = (1 ? (v) : type_name##__check(v))
+
 #define OPT_BIT_F(s, l, v, h, b, f) { \
 	.type = OPTION_BIT, \
 	.short_name = (s), \
@@ -256,15 +283,18 @@ struct option {
 	.flags = PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, \
 	.defval = 1, \
 }
-#define OPT_CMDMODE_F(s, l, v, h, i, f) { \
-	.type = OPTION_SET_INT, \
+
+#define OPT_CMDMODE_T_F(s, l, t, v, h, i, f) { \
+	.type = OPTION_SET_VALUE, \
 	.short_name = (s), \
 	.long_name = (l), \
-	.value = (v), \
+	OPTION_VALUE(t, (v)), \
 	.help = (h), \
 	.flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
 	.defval = (i), \
 }
+#define OPT_CMDMODE_T(s, l, t, v, h, i) OPT_CMDMODE_T_F(s, l, t, v, h, i, 0)
+#define OPT_CMDMODE_F(s, l, v, h, i, f) OPT_CMDMODE_T_F(s, l, int, v, h, i, f)
 #define OPT_CMDMODE(s, l, v, h, i)  OPT_CMDMODE_F(s, l, v, h, i, 0)

 #define OPT_INTEGER(s, l, v, h)     OPT_INTEGER_F(s, l, v, h, 0)
--
2.42.0

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> +DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);

These are a bit annoying, but because we need a token that can be ## pasted
to form a valid identifier, we cannot help it.

> diff --git a/parse-options.c b/parse-options.c
> index e8e076c3a6..63a2247128 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
>  		if (that == opt ||
>  		    !(that->flags & PARSE_OPT_CMDMODE) ||
>  		    that->value != opt->value ||
> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))
>  			continue;

So, instead of assuming the pointer stuffed in opt->value member can
be dereferenced as inteter pointer, we have the get_value method for
the option and invoke it to grab the value, and compare it with the
default value.

> @@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  	 * is not a grave error, so let it pass.
>  	 */
>  	if ((opt->flags & PARSE_OPT_CMDMODE) &&
> -	    *(int *)opt->value && *(int *)opt->value != opt->defval)
> +	    opt->get_value(opt->value) &&
> +	    opt->get_value(opt->value) != opt->defval)
>  		return opt_command_mode_error(opt, all_opts, flags);

Likewise.

> @@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  		*(int *)opt->value = unset ? 0 : opt->defval;
>  		return 0;
>
> +	case OPTION_SET_VALUE:
> +		opt->set_value(opt->value, unset ? 0 : opt->defval);
> +		return 0;

Here we see the previous way in the precontext of this hunk that is
used for OPTION_SET_INT, but in the new type-safe-enum world order,
that uses OPTION_SET_VALUE, the set_value method should know what to
do with the pointer that is in opt->value.

> diff --git a/parse-options.h b/parse-options.h
> index 57a7fe9d91..764e7f7896 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -20,6 +20,7 @@ enum parse_opt_type {
>  	OPTION_BITOP,
>  	OPTION_COUNTUP,
>  	OPTION_SET_INT,
> +	OPTION_SET_VALUE,
>  	/* options with arguments (usually) */
>  	OPTION_STRING,
>  	OPTION_INTEGER,
> @@ -158,8 +159,34 @@ struct option {
>  	parse_opt_ll_cb *ll_callback;
>  	intptr_t extra;
>  	parse_opt_subcommand_fn *subcommand_fn;
> +	intptr_t (*get_value)(void *);
> +	void (*set_value)(void *, intptr_t);
>  };

OK.

> +#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
> +static inline intptr_t type_name##__get(void *void_ptr) \
> +{ \
> +	type *ptr = void_ptr; \
> +	return (intptr_t)*ptr; \
> +} \
> +static inline void type_name##__set(void *void_ptr, intptr_t value) \
> +{ \
> +	type *ptr = void_ptr; \
> +	*ptr = (type)value; \
> +} \
> +static inline void *type_name##__check(type *ptr) \
> +{ \
> +	return ptr; \
> +} \
> +static inline void *type_name##__check(type *ptr)

Fun.  So a typical pattern is that for "enum foo", the foo__get() is
created from the above template and becomes the .get_value method.

Copying from an earlier hunk, the get_value() method is used like
so:

> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))

We pass opt->value (which is void *) to foo__get(), we have a local
variable "enum foo *ptr" and assign it in there, and dereference it.
We used to dereference the pointer as if it were a pointer to an
integer, so the type of foo__get() could be "int", but because we
compare it with the .defval member, which is of type "intptr_t", the
return type of the get_value() method being "intptr_t" would make it
consistent here.  I am not sure why defval need to be "intptr_t", and
for the purpose of this topic it would have been cleaner if it were
"int", but that is a tangent (probably somebody uses it as the default
value for a pointer variable and points it at some default object).

The setter is also reasonable.  An earlier hunk used it like so:

> +		opt->set_value(opt->value, unset ? 0 : opt->defval);

opt->value which is (void *) is assigned to "enum foo *ptr", and
using that pointer, "(enum foo)opt->defval" (or 0) is assinged
there.  Pretty straight-forward.

> +DEFINE_OPTION_VALUE_TYPE(int, int);
> +
> +#define OPTION_VALUE(type_name, v) \
> +	.get_value = type_name##__get, \
> +	.set_value = type_name##__set, \
> +	.value = (1 ? (v) : type_name##__check(v))

This is cute.  foo__check() is declared to take "enum foo *" and
returns it as "void *", but because the condition to the ternary
operator is constant "true", it is discarded.  The only expected
effect is to force the compiler to catch type errors when v is not
of type "enum foo *".

Unless it is "void *", I presume?  Then foo__check() would be happy,
but typically OPTION_VALUE() is used as an implementation detail of
OPT_CMDMODE_T() and you are expected to say something like "&variable"
for "v" above, so it would be OK (because you cannot have a variable
of type "void").

Thanks for a fun read.

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 18.09.23 um 15:33 schrieb Phillip Wood:
> Hi René
>
> On 18/09/2023 10:28, René Scharfe wrote:
>> Here's a version that preserves the enums by using additional int
>> variables just for the parsing phase.  No tricks.  The diff is long, but
>> most changes aren't particularly complicated and the resulting code is
>> not that ugly.  Except for builtin/am.c perhaps, which changes the
>> command mode value using a callback as well.
>>
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 202040b62e..00930e2152 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2270,13 +2270,14 @@ enum resume_type {
>>
>>   struct resume_mode {
>>       enum resume_type mode;
>> +    int mode_int;
>>       enum show_patch_type sub_mode;
>>   };
>>
>>   static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
>>   {
>>       int *opt_value = opt->value;
>> -    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
>> +    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>>
>>       /*
>>        * Please update $__git_showcurrentpatch in git-completion.bash
>> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>>                        "--show-current-patch", arg);
>>       }
>>
>> -    if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> +    if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>>           return error(_("options '%s=%s' and '%s=%s' "
>>                          "cannot be used together"),
>>                        "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
>> -    resume->mode = RESUME_SHOW_PATCH;
>> +    resume->mode_int = RESUME_SHOW_PATCH;
>>       resume->sub_mode = new_value;
>>       return 0;
>>   }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int"
> is valid while parsing the options but then we want to use "mode".

True.  It feels a bit worse here than for the others because there the
variable is on the stack and here it's in a struct that is passed
around.

> I wonder if we could get Oswald's idea of using callbacks working in
> a reasonably ergonomic way with a couple of macros. We could add an
> new OPTION_SET_ENUM member to "enum parse_opt_type" that would take
> a setter function as well as the usual void *value. To set the value
> it would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and
> take the name of the enum as well as the integer value we want to set
> for that option. The name of the enum would be used to generate the
> name of the setter callback which would be defined with another
> macro. The macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
>     static void parse_cmdmode_ ## name (void * var, int value) {
>         enum name *p = var;
>         *p = value;
>     }
>
> then OPT_CMDMODE would look like
>
> #define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
>     .type = OPTION_SET_ENUM, \
>     .short_name = (s), \
>     .long_name = (l), \
>     .value = (v), \
>     .help = (h), \
>     .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
>     .defval = (i), \
>     .enum_setter = parse_cmdmode_ ## n,
> }
> #define OPT_CMDMODE(s, l, v, h, n, i)  OPT_CMDMODE_F(s, l, v, h, n, i, 0)
>
>
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)

About half of the OPT_CMDMODE users use int, not enum.  They'd have to
be converted.  On the flipside the direct and indirect uses of
OPT_SET_INT_F with enums could be converted to an OPT_SET_ENUM_F based
on the above to avoid their use of incompatible pointers as well (e.g.
OPT_IPVERSION).

For uses of OPT_BIT and OPT_NEGBIT with enums a getter would be needed
as well, though (e.g. git ls-tree -d/-r/-t).

At this point I wonder if the existing callback mechanism would suffice.
Which brings me full circle to the topic of typed callbacks.  Perhaps I
should introduce them first and come back to solving the int/enum issue
at the end of that journey.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
> dereferenced as an int pointer in parse-options.c::get_value().  These
> two types are incompatible, though -- the storage size of an enum can
> vary between platforms.  C23 would allow us to specify the underlying
> type of the different enums, making them compatible, but with C99 the
> easiest safe option is to actually use int as the value type.
>
> Convert the offending OPT_CMDMODE users and use the typed value_int
> point in the macro's definition to enforce that type for future ones.

Interesting.  I wondered if this means that applying [1/2] alone
will immediately break these places that [2/2] fixes, but the answer
is no, as the previous step did not make these places use the typed
pointer.  But it also means that with this step alone to use "int",
instead of various "enum" types that can have representations that
are different from "int", would already "fix" the current code
while still casing back and forth from (void *)?

In any case, the two-patch series looks good, and it does not break
bisectability, either.

Thanks.

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 11.09.23 um 21:12 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
>> dereferenced as an int pointer in parse-options.c::get_value().  These
>> two types are incompatible, though -- the storage size of an enum can
>> vary between platforms.  C23 would allow us to specify the underlying
>> type of the different enums, making them compatible, but with C99 the
>> easiest safe option is to actually use int as the value type.
>>
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>
> Interesting.  I wondered if this means that applying [1/2] alone
> will immediately break these places that [2/2] fixes, but the answer
> is no, as the previous step did not make these places use the typed
> pointer.  But it also means that with this step alone to use "int",
> instead of various "enum" types that can have representations that
> are different from "int", would already "fix" the current code
> while still casing back and forth from (void *)?

Yes and yes.  And the change to use value_int on its own makes the type
mismatch visible via compiler warnings.  It guards against future
violations.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
>dereferenced as an int pointer in parse-options.c::get_value().  These
>two types are incompatible, though
>
s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum

>-- the storage size of an enum can vary between platforms.
>
here's a completely different perspective:
this is merely a theoretical problem, right? gcc for example won't 
actually use non-ints unless -fshort-enums is supplied. so how about 
simply adding a (configure) test to ensure that there is actually no 
problem, and calling it a day?

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 19.09.23 um 11:40 schrieb Oswald Buddenhagen:
> On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>> Some uses of OPT_CMDMODE provide a pointer to an enum.  It is
>> dereferenced as an int pointer in parse-options.c::get_value().  These
>> two types are incompatible, though
>>
> s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum

You're right.  Citing the relevant part: "Each enumerated type [...] is
compatible with one of: char, a signed integer type, or an unsigned
integer type [...]. It is implementation-defined which type is
compatible with any given enumerated type [...]."  So there's a chance
that the underlying type would be compatible by accident.

When we try a few combinations (https://godbolt.org/z/KvKcndY4Y),
Clang warns about incompatible pointers if we use a pointer to an enum
with only positive values as int pointer and about different signs if
use a pointer to an enum with negative values as in unsigned int
pointer and accepts the rest.  GCC accepts the same cases, but all its
warnings are about incompatible pointers.  This seems to be dependent
on the optimization level, though.  MSVC warns about all combinations.

>> -- the storage size of an enum can vary between platforms.
>>
> here's a completely different perspective:
> this is merely a theoretical problem, right? gcc for example won't
> actually use non-ints unless -fshort-enums is supplied. so how about
> simply adding a (configure) test to ensure that there is actually no
> problem, and calling it a day?

That would be an easy, but complex solution.  If the check is done
using -Wincompatible-pointer-types or equivalent then MSVC is out.  If
we base it on type size then we're making assumptions that I find hard
to justify.  Using the same type at both ends of the void and avoiding
compiler warnings that would have been issued if we'd cut out the
middle part is simpler overall.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
> MSVC warns about all combinations.
>
yes, though that's not a problem: after we established that the 
underlying type is int, we can just have a cast in the initializer 
macro.

>> so how about simply adding a (configure) test to ensure that there is 
>> actually no problem, and calling it a day?

> If we base it on type size then we're making assumptions that I find 
> hard to justify.
>
the only one i can think of is signedness. i think this can be safely 
ignored as long as we use only small positive integers.

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>> MSVC warns about all combinations.
>>
> yes, though that's not a problem: after we established that the
> underlying type is int, we can just have a cast in the initializer
> macro.

MSVC does some weird things in general; it's tempting to ignore it.

>>> so how about simply adding a (configure) test to ensure that
>>> there is actually no problem, and calling it a day?
>
>> If we base it on type size then we're making assumptions that I
>> find hard to justify.
>>
> the only one i can think of is signedness. i think this can be safely
> ignored as long as we use only small positive integers.

I don't fully understand the pointer-sign warning, so I'm not
confident enough to silence it.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:
>Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>>> If we base it on type size then we're making assumptions that I
>>> find hard to justify.
>>>
>> the only one i can think of is signedness. i think this can be safely
>> ignored as long as we use only small positive integers.
>
>I don't fully understand the pointer-sign warning, so I'm not
>confident enough to silence it.
>
in theory, differently signed integers may have completely different 
binary representations. but afaik, that only ever mattered for negative 
numbers. and c++20 actually codifies two's complement, which was the 
de-facto standard for decades already.
so in practice it just means that we may be assigning a value that is 
outside the range of the actual type. but small positive values are 
compatible between signed and unsiged types.

regards

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[René Scharfe]

Am 03.10.23 um 11:38 schrieb Oswald Buddenhagen:
> On Tue, Oct 03, 2023 at 10:49:12AM +0200, René Scharfe wrote:
>> Am 21.09.23 um 12:40 schrieb Oswald Buddenhagen:
>>> On Wed, Sep 20, 2023 at 10:18:10AM +0200, René Scharfe wrote:
>>>> If we base it on type size then we're making assumptions that
>>>> I find hard to justify.
>>>>
>>> the only one i can think of is signedness. i think this can be
>>> safely ignored as long as we use only small positive integers.
>>
>> I don't fully understand the pointer-sign warning, so I'm not
>> confident enough to silence it.
>>
> in theory, differently signed integers may have completely different
> binary representations. but afaik, that only ever mattered for
> negative numbers. and c++20 actually codifies two's complement, which
> was the de-facto standard for decades already. so in practice it just
> means that we may be assigning a value that is outside the range of
> the actual type. but small positive values are compatible between
> signed and unsiged types.

C++ is not relevant for Git, but C23 is going to to stop supporting
binary representations other than two's complement as well.

Still I don't feel comfortable overriding compiler warnings for
something pedestrian as a command line parser.  No idea what other
assumptions are made in compilers around enums.  I'd rather honor
the warnings and avoid any forcing or trickery if possible.  Or at
least leave that to more capable hands.

René

Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

[Oswald Buddenhagen]

On Tue, Oct 03, 2023 at 07:54:28PM +0200, René Scharfe wrote:
>C++ is not relevant for Git, but C23 is going to to stop supporting
>binary representations other than two's complement as well.
>
it is relevant insofar as that every platform that comes with a recent 
c++ compiler uses two's complement. this then applies to any language, 
regardless of standard.

>Still I don't feel comfortable overriding compiler warnings for
>something pedestrian as a command line parser.  No idea what other
>assumptions are made in compilers around enums.
>
i'm not sure what you're really worried about. if there was an actual 
problem, we'd have noticed by now. as far as i'm concerned, the question 
is only how to codify the status quo in the most elegant way. putting a 
typecast into a macro certainly qualifies in my book.

regards

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Taylor Blau]

On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
> Add an int pointer, value_int, to struct option to provide a typed value
> pointer for the various integer options.  It allows type checks at
> compile time, which is not possible with the void pointer, value.  Its
> use is optional for now.

This is an interesting direction. I wonder about whether or not you'd
consider changing the option structure to contain a tagged union type
that represents some common cases we'd want from a parse-options
callback, something like:

    struct option {
        /* ... */
        union {
            void *value;
            int *value_int;
            /* etc ... */
        } u;
        enum option_type t;
    };

where option_type has some value corresponding to "void *", another for
"int *", and so on.

Alternatively, perhaps you are thinking that we'd use both the value
pointer and the value_int pointer to point at potentially different
values in the same callback. I don't have strong feelings about it, but
I'd just as soon encourage us to shy away from that approach, since
assigning a single callback parameter to each function seems more
organized.

> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  	const char *s, *arg;
>  	const int unset = flags & OPT_UNSET;
>  	int err;
> +	int *value_int = opt->value_int ? opt->value_int : opt->value;
>
>  	if (unset && p->opt)
>  		return error(_("%s takes no value"), optname(opt, flags));

Reading this hunk, I wonder whether we even need a type tag (the
option_type enum above) if each callback knows a priori what type it
expects. But I think storing them together in a union makes sense to do.

Thanks,
Taylor

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> callback, something like:
>
>     struct option {
>         /* ... */
>         union {
>             void *value;
>             int *value_int;
>             /* etc ... */
>         } u;
>         enum option_type t;
>     };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.

Yup, that does cross my mind, even though I would have used

	union {
		void *void_ptr;
		int *int_ptr;
	} value;

or something without a rather meaningless 'u'.

> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.

We have seen (with Peff's "-Wunused" work) that there are small
number of cases that it would be handy for a callback to be told the
locations of multiple external variables, but I do not think it
would be a good solution to that problem to have "void *value" and
"int value_int" next to each other and allow them to coexist, as it
would work only when these multiple variables happen to be of the
right types.

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Oswald Buddenhagen]

On Mon, Sep 11, 2023 at 12:19:17PM -0700, Junio C Hamano wrote:
>Yup, that does cross my mind, even though I would have used
>
>	union {
>		void *void_ptr;
>		int *int_ptr;
>	} value;
>
>or something without a rather meaningless 'u'.
>
i'd go the opposite way and make it an anonymous union.
that would require c11, though. imo not exactly an outrageous 
proposition in 2023, but ...

regards

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Kristoffer Haugsbakk]

On Tue, Sep 12, 2023, at 00:28, Oswald Buddenhagen wrote:
> i'd go the opposite way and make it an anonymous union.
> that would require c11, though. imo not exactly an outrageous 
> proposition in 2023, but ...

Moving to C11 would get pushback because not all platforms 
that people care about support that compiler.[1]

[1] https://lore.kernel.org/git/004601d8ed6b$13a2f580$3ae8e080$@nexbridge.com/ 
-- 
Kristoffer Haugsbakk

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[René Scharfe]

Am 11.09.23 um 21:19 schrieb Junio C Hamano:
> Taylor Blau <me@ttaylorr.com> writes:
>
>> callback, something like:
>>
>>     struct option {
>>         /* ... */
>>         union {
>>             void *value;
>>             int *value_int;
>>             /* etc ... */
>>         } u;
>>         enum option_type t;
>>     };
>>
>> where option_type has some value corresponding to "void *", another for
>> "int *", and so on.
>
> Yup, that does cross my mind, even though I would have used
>
> 	union {
> 		void *void_ptr;
> 		int *int_ptr;
> 	} value;
>
> or something without a rather meaningless 'u'.

OK, but I neglected to ask what we would get out of throwing different
types into the same bin.  It complicates type safety by making it
impossible for the parser to distinguish the used type.  This will
become relevant once all int options are converted and value_int can be
made mandatory for them.  A named union also requires changing all
users.

It reduces the memory footprint, but only slightly.  Saving a few bytes
for objects with less than a hundred instances total doesn't seem worth
the downsides.

René

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Oswald Buddenhagen]

On Mon, Sep 18, 2023 at 11:53:19AM +0200, René Scharfe wrote:
>Am 11.09.23 um 21:19 schrieb Junio C Hamano:
>> Yup, that does cross my mind, even though I would have used
>>
>> 	union {
>> 		void *void_ptr;
>> 		int *int_ptr;
>> 	} value;
>>
>> or something without a rather meaningless 'u'.
>
>OK, but I neglected to ask what we would get out of throwing different
>types into the same bin.

>It complicates type safety by making it impossible for the parser to 
>distinguish the used type. [...]
>
this is somewhat ironic, given that using a union has some semantic 
value by illustrating that the fields are mutually exclusive.
but i'm not sure that the checking is really important here, given that 
the initializers are inside centralized macros anyway.

regards

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> It reduces the memory footprint, but only slightly.  Saving a few bytes
> for objects with less than a hundred instances total doesn't seem worth
> the downsides.

It makes it impossible to use the both at the same time, which is a
bigger (than reduced memory) advantage.  Otherwise, we would be
tempted to consider that having "void *value" and "int value_int"
next to each other and allow them to coexist may be a good solution
for a narrow corner case (please see at the end of the message you
are responding to).

As you said, use of union has its downsides that may contradict the
objective of the larger picture this topic draws.

Thanks.

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[René Scharfe]

Am 18.09.23 um 18:17 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> It reduces the memory footprint, but only slightly.  Saving a few bytes
>> for objects with less than a hundred instances total doesn't seem worth
>> the downsides.
>
> It makes it impossible to use the both at the same time, which is a
> bigger (than reduced memory) advantage.  Otherwise, we would be
> tempted to consider that having "void *value" and "int value_int"
> next to each other and allow them to coexist may be a good solution
> for a narrow corner case (please see at the end of the message you
> are responding to).

Good point.  The patch adds a check to parse_options_check() to prevent
double use, which adds some runtime overhead and doesn't fully remove
the temptation.

René

Re: [PATCH 1/2] parse-options: add int value pointer to struct option

[René Scharfe]

Am 10.09.23 um 20:40 schrieb Taylor Blau:
> On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
>> Add an int pointer, value_int, to struct option to provide a typed value
>> pointer for the various integer options.  It allows type checks at
>> compile time, which is not possible with the void pointer, value.  Its
>> use is optional for now.
>
> This is an interesting direction. I wonder about whether or not you'd
> consider changing the option structure to contain a tagged union type
> that represents some common cases we'd want from a parse-options
> callback, something like:
>
>     struct option {
>         /* ... */
>         union {
>             void *value;
>             int *value_int;
>             /* etc ... */
>         } u;
>         enum option_type t;
>     };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.

In a hand-made struct option this would only provide a very limited form
of type safety.  It reduces the number of incorrect types to choose from
from basically infinity to a handful, but still allows pointing the
union e.g. to an int for an option that takes a long or a string without
any compiler warning or error.

Convenience macros like OPT_CMDMODE could use the union to provide a
type safe interface, though, true.  This might suffice for our purposes.

> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.

Right, we only need one active value pointer per option.

>> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>>  	const char *s, *arg;
>>  	const int unset = flags & OPT_UNSET;
>>  	int err;
>> +	int *value_int = opt->value_int ? opt->value_int : opt->value;
>>
>>  	if (unset && p->opt)
>>  		return error(_("%s takes no value"), optname(opt, flags));
>
> Reading this hunk, I wonder whether we even need a type tag (the
> option_type enum above) if each callback knows a priori what type it
> expects. But I think storing them together in a union makes sense to do.

Yes, option types (OPTION_INTEGER etc.) already imply a pointer type,
no additional tag needed.

René