Am 16.09.23 um 19:45 schrieb Junio C Hamano:
> Jeff King <peff@xxxxxxxx> 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@xxxxxx>
---
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
