[PATCH 5/8] builtin/config: track subcommands by action

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 13026 bytes --]

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),
+	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);
 }
-- 
2.44.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]