Re: [PATCH 5/8] builtin/config: track subcommands by action
From: Jean-Noël AVILA <hidden>
Date: 2024-03-06 21:54:17
Le mercredi 6 mars 2024, 12:31:50 CET Patrick Steinhardt a écrit :
quoted hunk ↗ jump to hunk
Part of `cmd_config()` is a rather unwieldy switch statement that invokes the correct subcommand function based on which action has been requested by the user. Now that we have converted actions to be tracked via a `OPT_CMDMODE()`, we know that the `actions` variable will only ever have at most one bit set. This allows us to convert the variable to use an `enum` instead, and thus to create an array that maps from this newly introduced `enum` to the corresponding subcommand function. Refactor the code to do so. Besides allowing us to get rid of the giant switch statement, this refactoring will also make it easier to introduce proper subcommands to git-config(1). Signed-off-by: Patrick Steinhardt <redacted> --- builtin/config.c | 207 +++++++++++++++++++++++------------------------ 1 file changed, 99 insertions(+), 108 deletions(-)diff --git a/builtin/config.c b/builtin/config.c index a6ab9b8204..0d58397ef5 100644 --- a/builtin/config.c +++ b/builtin/config.c@@ -20,6 +20,26 @@ static const char *const builtin_config_usage[] = { NULL }; +enum config_action { + ACTION_NONE, + ACTION_GET, + ACTION_GET_ALL, + ACTION_GET_REGEXP, + ACTION_REPLACE_ALL, + ACTION_ADD, + ACTION_UNSET, + ACTION_UNSET_ALL, + ACTION_RENAME_SECTION, + ACTION_REMOVE_SECTION, + ACTION_LIST, + ACTION_EDIT, + ACTION_SET, + ACTION_SET_ALL, + ACTION_GET_COLOR, + ACTION_GET_COLORBOOL, + ACTION_GET_URLMATCH, +}; + static char *key; static regex_t *key_regexp; static const char *value_pattern;@@ -33,10 +53,12 @@ static char delim = '='; static char key_delim = ' '; static char term = '\n'; +static parse_opt_subcommand_fn *subcommand; +static enum config_action action = ACTION_NONE; static int use_global_config, use_system_config, use_local_config; static int use_worktree_config; static struct git_config_source given_config_source; -static int actions, type; +static int type; static char *default_value; static int end_nul; static int respect_includes_opt = -1;@@ -46,30 +68,6 @@ static int show_scope; static int fixed_value; static int config_flags; -#define ACTION_GET (1<<0) -#define ACTION_GET_ALL (1<<1) -#define ACTION_GET_REGEXP (1<<2) -#define ACTION_REPLACE_ALL (1<<3) -#define ACTION_ADD (1<<4) -#define ACTION_UNSET (1<<5) -#define ACTION_UNSET_ALL (1<<6) -#define ACTION_RENAME_SECTION (1<<7) -#define ACTION_REMOVE_SECTION (1<<8) -#define ACTION_LIST (1<<9) -#define ACTION_EDIT (1<<10) -#define ACTION_SET (1<<11) -#define ACTION_SET_ALL (1<<12) -#define ACTION_GET_COLOR (1<<13) -#define ACTION_GET_COLORBOOL (1<<14) -#define ACTION_GET_URLMATCH (1<<15) - -/* - * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than - * one line of output and which should therefore be paged. - */ -#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \ - ACTION_GET_REGEXP | ACTION_GET_URLMATCH) - #define TYPE_BOOL 1 #define TYPE_INT 2 #define TYPE_BOOL_OR_INT 3@@ -842,6 +840,25 @@ static int cmd_config_get_colorbool(int argc, const char **argv, const char *pre return get_colorbool(argv[0], argc == 2); } +static parse_opt_subcommand_fn *subcommands_by_action[] = { + [ACTION_LIST] = cmd_config_list, + [ACTION_EDIT] = cmd_config_edit, + [ACTION_SET] = cmd_config_set, + [ACTION_SET_ALL] = cmd_config_set_all, + [ACTION_ADD] = cmd_config_add, + [ACTION_REPLACE_ALL] = cmd_config_replace_all, + [ACTION_GET] = cmd_config_get, + [ACTION_GET_ALL] = cmd_config_get_all, + [ACTION_GET_REGEXP] = cmd_config_get_regexp, + [ACTION_GET_URLMATCH] = cmd_config_get_urlmatch, + [ACTION_UNSET] = cmd_config_unset, + [ACTION_UNSET_ALL] = cmd_config_unset_all, + [ACTION_RENAME_SECTION] = cmd_config_rename_section, + [ACTION_REMOVE_SECTION] = cmd_config_remove_section, + [ACTION_GET_COLOR] = cmd_config_get_color, + [ACTION_GET_COLORBOOL] = cmd_config_get_colorbool, +}; + static struct option builtin_config_options[] = { OPT_GROUP(N_("Config file location")), OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),@@ -851,20 +868,20 @@ static struct option builtin_config_options[] = { OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")), OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")), OPT_GROUP(N_("Action")), - OPT_CMDMODE(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET), - OPT_CMDMODE(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL), - OPT_CMDMODE(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP), - OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), - OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL), - OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD), - OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET), - OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL), - OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), - OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), - OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST), - OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), - OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR), - OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL), + OPT_CMDMODE(0, "get", &action, N_("get value: name [value-pattern]"), ACTION_GET),
I guess value-pattern is a placeholder. So for uniformity with the style guidelines, it should be formatted <value-pattern>. This also applies to other placeholders below.
quoted hunk ↗ jump to hunk
+ OPT_CMDMODE(0, "get-all", &action, N_("get all values: key [value-pattern]"), ACTION_GET_ALL), + OPT_CMDMODE(0, "get-regexp", &action, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP), + OPT_CMDMODE(0, "get-urlmatch", &action, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), + OPT_CMDMODE(0, "replace-all", &action, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL), + OPT_CMDMODE(0, "add", &action, N_("add a new variable: name value"), ACTION_ADD), + OPT_CMDMODE(0, "unset", &action, N_("remove a variable: name [value-pattern]"), ACTION_UNSET), + OPT_CMDMODE(0, "unset-all", &action, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL), + OPT_CMDMODE(0, "rename-section", &action, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), + OPT_CMDMODE(0, "remove-section", &action, N_("remove a section: name"), ACTION_REMOVE_SECTION), + OPT_CMDMODE('l', "list", &action, N_("list all"), ACTION_LIST), + OPT_CMDMODE('e', "edit", &action, N_("open an editor"), ACTION_EDIT), + OPT_CMDMODE(0, "get-color", &action, N_("find the color configured: slot [default]"), ACTION_GET_COLOR), + OPT_CMDMODE(0, "get-colorbool", &action, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL), OPT_GROUP(N_("Type")), OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type), OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),@@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) key_delim = '\n'; }