| Message ID | e39774d6495767c6505999bdc33110678139e347.1709724089.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/config: introduce subcommands | expand |
Le mercredi 6 mars 2024, 12:31:50 CET Patrick Steinhardt a écrit : > 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 <ps@pks.im> > --- > 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. > + 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'; > } >
On Wed, Mar 06, 2024 at 12:31:50PM +0100, Patrick Steinhardt wrote: > --- > builtin/config.c | 207 +++++++++++++++++++++++------------------------ > 1 file changed, 99 insertions(+), 108 deletions(-) This is definitely easier (for me, at least) to view with: $ git show --color-moved --color-moved-ws=ignore-all-space which makes clearer that this change does not introduce or change any existing behavior. > 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), > [...] Got it, this whole hunk is changing "&actions" to "&action", and nothing else. > @@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) > key_delim = '\n'; > } > > - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { > - error(_("--get-color and variable type are incoherent")); > - usage_builtin_config(); > - } > - > - if (actions == 0) > + if (action == ACTION_NONE) { > switch (argc) { > - case 1: actions = ACTION_GET; break; > - case 2: actions = ACTION_SET; break; > - case 3: actions = ACTION_SET_ALL; break; > + case 1: action = ACTION_GET; break; > + case 2: action = ACTION_SET; break; > + case 3: action = ACTION_SET_ALL; break; Wondering aloud a little bit... is it safe to set us to "ACTION_SET", for example, if we have exactly two arguments? On the one hand, it seems like the answer is yes. But on the other hand, if we were invoked as: $ git config ste foo We would get something like: $ git config ste foo error: key does not contain a section: ste which is sensible. It would be nice to say something more along the lines of "oops, you probably meant 'set' instead of 'ste'". I think you could claim that the error on the user's part is one of: - (using the new style 'git config') misspelling "set" as "ste", and thus trying to invoke a non-existent sub-command - (using the old style) trying to set the key "ste", which does not contain a section name, and thus is not a valid configuration key I have no idea which is more likely, and I think that there isn't a good way to distinguish between the two at all. So I think the error message is OK as-is, but let me know if you have other thoughts. > default: > usage_builtin_config(); > } > + } > + if (action <= ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action)) > + BUG("invalid action %d", action); > + subcommand = subcommands_by_action[action]; > + > + if (type && (subcommand == cmd_config_get_color || > + subcommand == cmd_config_get_colorbool)) { I don't think there's anything wrong with using the function-pointer equality here to figure out which subcommand we're in, but I wonder if it might be cleaner to write this as: if (type && (action == ACTION_GET_COLOR || action == ACTION_GET_COLORBOOL)) { ... > - if (actions == ACTION_LIST) { > - return cmd_config_list(argc, argv, prefix); > - } else if (actions == ACTION_EDIT) { > - return cmd_config_edit(argc, argv, prefix); > - } else if (actions == ACTION_SET) { > - return cmd_config_set(argc, argv, prefix); > - } else if (actions == ACTION_SET_ALL) { > - return cmd_config_set_all(argc, argv, prefix); > - } else if (actions == ACTION_ADD) { > - return cmd_config_add(argc, argv, prefix); > - } else if (actions == ACTION_REPLACE_ALL) { > - return cmd_config_replace_all(argc, argv, prefix); > - } else if (actions == ACTION_GET) { > - return cmd_config_get(argc, argv, prefix); > - } else if (actions == ACTION_GET_ALL) { > - return cmd_config_get_all(argc, argv, prefix); > - } else if (actions == ACTION_GET_REGEXP) { > - return cmd_config_get_regexp(argc, argv, prefix); > - } else if (actions == ACTION_GET_URLMATCH) { > - return cmd_config_get_urlmatch(argc, argv, prefix); > - } else if (actions == ACTION_UNSET) { > - return cmd_config_unset(argc, argv, prefix); > - } else if (actions == ACTION_UNSET_ALL) { > - return cmd_config_unset_all(argc, argv, prefix); > - } else if (actions == ACTION_RENAME_SECTION) { > - return cmd_config_rename_section(argc, argv, prefix); > - } else if (actions == ACTION_REMOVE_SECTION) { > - return cmd_config_remove_section(argc, argv, prefix); > - } else if (actions == ACTION_GET_COLOR) { > - return cmd_config_get_color(argc, argv, prefix); > - } else if (actions == ACTION_GET_COLORBOOL) { > - return cmd_config_get_colorbool(argc, argv, prefix); > - } > - > - BUG("invalid action"); > + return subcommand(argc, argv, prefix); Very nice. Thanks, Taylor
On Wed, Mar 06, 2024 at 07:10:09PM -0500, Taylor Blau wrote: > On Wed, Mar 06, 2024 at 12:31:50PM +0100, Patrick Steinhardt wrote: > > @@ -976,33 +993,43 @@ int cmd_config(int argc, const char **argv, const char *prefix) > > key_delim = '\n'; > > } > > > > - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { > > - error(_("--get-color and variable type are incoherent")); > > - usage_builtin_config(); > > - } > > - > > - if (actions == 0) > > + if (action == ACTION_NONE) { > > switch (argc) { > > - case 1: actions = ACTION_GET; break; > > - case 2: actions = ACTION_SET; break; > > - case 3: actions = ACTION_SET_ALL; break; > > + case 1: action = ACTION_GET; break; > > + case 2: action = ACTION_SET; break; > > + case 3: action = ACTION_SET_ALL; break; > > Wondering aloud a little bit... is it safe to set us to "ACTION_SET", > for example, if we have exactly two arguments? On the one hand, it seems > like the answer is yes. But on the other hand, if we were invoked as: > > $ git config ste foo > > We would get something like: > > $ git config ste foo > error: key does not contain a section: ste > > which is sensible. It would be nice to say something more along the > lines of "oops, you probably meant 'set' instead of 'ste'". I think you > could claim that the error on the user's part is one of: > > - (using the new style 'git config') misspelling "set" as "ste", and > thus trying to invoke a non-existent sub-command > > - (using the old style) trying to set the key "ste", which does not > contain a section name, and thus is not a valid configuration key > > I have no idea which is more likely, and I think that there isn't a good > way to distinguish between the two at all. So I think the error message > is OK as-is, but let me know if you have other thoughts. The transition period will be a tad weird, I agree. I think initially I'd prefer to keep the old behaviour just to ensure we don't regress anything, but after a couple of releases I think we should revisit this and become more aggressive about using the new style. > > default: > > usage_builtin_config(); > > } > > + } > > + if (action <= ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action)) > > + BUG("invalid action %d", action); > > + subcommand = subcommands_by_action[action]; > > + > > + if (type && (subcommand == cmd_config_get_color || > > + subcommand == cmd_config_get_colorbool)) { > I don't think there's anything wrong with using the function-pointer > equality here to figure out which subcommand we're in, but I wonder if > it might be cleaner to write this as: > > if (type && (action == ACTION_GET_COLOR || > action == ACTION_GET_COLORBOOL)) { > ... The reason I didn't is that we don't always have an action set once we support subcommands. We can of course figure it out by walking the array of functions pointers to find the one corresponding to this action. But I figured it's easier to just use function pointers exclusively. Patrick
On Wed, Mar 06, 2024 at 10:54:01PM +0100, Jean-Noël AVILA wrote: > Le mercredi 6 mars 2024, 12:31:50 CET Patrick Steinhardt a écrit : [snip] > > @@ -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. Ah, indeed, thanks for catching this! Patrick
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), + 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'; } - if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) { - error(_("--get-color and variable type are incoherent")); - usage_builtin_config(); - } - - if (actions == 0) + if (action == ACTION_NONE) { switch (argc) { - case 1: actions = ACTION_GET; break; - case 2: actions = ACTION_SET; break; - case 3: actions = ACTION_SET_ALL; break; + case 1: action = ACTION_GET; break; + case 2: action = ACTION_SET; break; + case 3: action = ACTION_SET_ALL; break; default: usage_builtin_config(); } + } + if (action <= ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action)) + BUG("invalid action %d", action); + subcommand = subcommands_by_action[action]; + + if (type && (subcommand == cmd_config_get_color || + subcommand == cmd_config_get_colorbool)) { + error(_("--get-color and variable type are incoherent")); + usage_builtin_config(); + } + if (omit_values && - !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) { + subcommand != cmd_config_list && + subcommand != cmd_config_get_regexp) { error(_("--name-only is only applicable to --list or --get-regexp")); usage_builtin_config(); } - if (show_origin && !(actions & - (ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST))) { + if (show_origin && + subcommand != cmd_config_get && + subcommand != cmd_config_get_all && + subcommand != cmd_config_get_regexp && + subcommand != cmd_config_list) { error(_("--show-origin is only applicable to --get, --get-all, " "--get-regexp, and --list")); usage_builtin_config(); } - if (default_value && !(actions & ACTION_GET)) { + if (default_value && subcommand != cmd_config_get) { error(_("--default is only applicable to --get")); usage_builtin_config(); } @@ -1011,28 +1038,19 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (fixed_value) { int allowed_usage = 0; - switch (actions) { - /* git config --get <name> <value-pattern> */ - case ACTION_GET: - /* git config --get-all <name> <value-pattern> */ - case ACTION_GET_ALL: - /* git config --get-regexp <name-pattern> <value-pattern> */ - case ACTION_GET_REGEXP: - /* git config --unset <name> <value-pattern> */ - case ACTION_UNSET: - /* git config --unset-all <name> <value-pattern> */ - case ACTION_UNSET_ALL: + if (subcommand == cmd_config_get || + subcommand == cmd_config_get_all || + subcommand == cmd_config_get_regexp || + subcommand == cmd_config_unset || + subcommand == cmd_config_unset_all) { + /* git config --<action> <name> <value-pattern> */ allowed_usage = argc > 1 && !!argv[1]; - break; - - /* git config <name> <value> <value-pattern> */ - case ACTION_SET_ALL: - /* git config --replace-all <name> <value> <value-pattern> */ - case ACTION_REPLACE_ALL: + } else if (subcommand == cmd_config_set_all || + subcommand == cmd_config_replace_all) { + /* git config --<action> <name> <value> <value-pattern> */ allowed_usage = argc > 2 && !!argv[2]; - break; - - /* other options don't allow --fixed-value */ + } else { + /* other options don't allow --fixed-value */ } if (!allowed_usage) { @@ -1043,42 +1061,15 @@ int cmd_config(int argc, const char **argv, const char *prefix) config_flags |= CONFIG_FLAGS_FIXED_VALUE; } - if (actions & PAGING_ACTIONS) + /* + * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than + * one line of output and which should therefore be paged. + */ + if (subcommand == cmd_config_list || + subcommand == cmd_config_get_all || + subcommand == cmd_config_get_regexp || + subcommand == cmd_config_get_urlmatch) setup_auto_pager("config", 1); - if (actions == ACTION_LIST) { - return cmd_config_list(argc, argv, prefix); - } else if (actions == ACTION_EDIT) { - return cmd_config_edit(argc, argv, prefix); - } else if (actions == ACTION_SET) { - return cmd_config_set(argc, argv, prefix); - } else if (actions == ACTION_SET_ALL) { - return cmd_config_set_all(argc, argv, prefix); - } else if (actions == ACTION_ADD) { - return cmd_config_add(argc, argv, prefix); - } else if (actions == ACTION_REPLACE_ALL) { - return cmd_config_replace_all(argc, argv, prefix); - } else if (actions == ACTION_GET) { - return cmd_config_get(argc, argv, prefix); - } else if (actions == ACTION_GET_ALL) { - return cmd_config_get_all(argc, argv, prefix); - } else if (actions == ACTION_GET_REGEXP) { - return cmd_config_get_regexp(argc, argv, prefix); - } else if (actions == ACTION_GET_URLMATCH) { - return cmd_config_get_urlmatch(argc, argv, prefix); - } else if (actions == ACTION_UNSET) { - return cmd_config_unset(argc, argv, prefix); - } else if (actions == ACTION_UNSET_ALL) { - return cmd_config_unset_all(argc, argv, prefix); - } else if (actions == ACTION_RENAME_SECTION) { - return cmd_config_rename_section(argc, argv, prefix); - } else if (actions == ACTION_REMOVE_SECTION) { - return cmd_config_remove_section(argc, argv, prefix); - } else if (actions == ACTION_GET_COLOR) { - return cmd_config_get_color(argc, argv, prefix); - } else if (actions == ACTION_GET_COLORBOOL) { - return cmd_config_get_colorbool(argc, argv, prefix); - } - - BUG("invalid action"); + return subcommand(argc, argv, prefix); }
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 <ps@pks.im> --- builtin/config.c | 207 +++++++++++++++++++++++------------------------ 1 file changed, 99 insertions(+), 108 deletions(-)