[PATCH 6/8] builtin/config: introduce subcommands

[Patrick Steinhardt <ps@pks.im>]

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

While git-config(1) has several modes, those modes are not exposed with
subcommands but instead by specifying e.g. `--unset` or `--list`. This
user interface is not really in line with how our more modern commands
work, where it is a lot more customary to say e.g. `git remote list`.
Furthermore, to add to the confusion, git-config(1) also allows the user
to request modes implicitly by just specifying the correct number of
arguments. Thus, `git config foo.bar` will retrieve the value of
"foo.bar" while `git config foo.bar baz` will set it to "baz".

Overall, this makes for a confusing interface that could really use a
makeover. It hurts discoverability of what you can do with git-config(1)
and is comparatively easy to get wrong.

Modernize git-config(1) so that it understands proper subcommands like
"list" or "unset". Like this, a user can say "git config get foo.bar" to
retrieve the config key's value and "git config set foo.bar baz" to set
it.

One concern in this context is backwards compatibility. Luckily, we can
introduce subcommands without breaking backwards compatibility at all.
This is because all the implicit modes of git-config(1) require that the
first argument is a properly formatted config key. And as config keys
_must_ have a dot in their name, any value without a dot would have been
discarded by git-config(1) previous to this change. Thus, given that
none of the subcommands do have a dot, they are unambiguous.

Consequently, we introduce subcommands in such a way that git-config(1)
understands both the old and the new syntax at the same time. This
should help to transition to the new-style syntax until we eventually
deprecate and remove the old-style syntax.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/config.c | 52 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 0d58397ef5..10fa933931 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -859,6 +859,26 @@ static parse_opt_subcommand_fn *subcommands_by_action[] = {
 	[ACTION_GET_COLORBOOL] = cmd_config_get_colorbool,
 };
 
+static struct option builtin_subcommand_options[] = {
+	OPT_SUBCOMMAND("list", &subcommand, cmd_config_list),
+	OPT_SUBCOMMAND("get", &subcommand, cmd_config_get),
+	OPT_SUBCOMMAND("get-all", &subcommand, cmd_config_get_all),
+	OPT_SUBCOMMAND("get-color", &subcommand, cmd_config_get_color),
+	OPT_SUBCOMMAND("get-colorbool", &subcommand, cmd_config_get_colorbool),
+	OPT_SUBCOMMAND("get-regexp", &subcommand, cmd_config_get_regexp),
+	OPT_SUBCOMMAND("get-urlmatch", &subcommand, cmd_config_get_urlmatch),
+	OPT_SUBCOMMAND("add", &subcommand, cmd_config_add),
+	OPT_SUBCOMMAND("set", &subcommand, cmd_config_set),
+	OPT_SUBCOMMAND("set-all", &subcommand, cmd_config_set_all),
+	OPT_SUBCOMMAND("unset", &subcommand, cmd_config_unset),
+	OPT_SUBCOMMAND("unset-all", &subcommand, cmd_config_unset_all),
+	OPT_SUBCOMMAND("replace-all", &subcommand, cmd_config_replace_all),
+	OPT_SUBCOMMAND("rename-section", &subcommand, cmd_config_rename_section),
+	OPT_SUBCOMMAND("remove-section", &subcommand, cmd_config_remove_section),
+	OPT_SUBCOMMAND("edit", &subcommand, cmd_config_edit),
+	OPT_END(),
+};
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -910,6 +930,20 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
+	/*
+	 * This is somewhat hacky: we first parse the command line while
+	 * keeping all args intact in order to determine whether a subcommand
+	 * has been specified. If so, we re-parse it a second time, but this
+	 * time we drop KEEP_ARGV0. This is so that we don't munge the command
+	 * line in case no subcommand was given, which would otherwise confuse
+	 * us when parsing the implicit modes.
+	 */
+	argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
+			     PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN_OPT);
+	if (subcommand)
+		argc = parse_options(argc, argv, prefix, builtin_subcommand_options, builtin_config_usage,
+				     PARSE_OPT_SUBCOMMAND_OPTIONAL|PARSE_OPT_KEEP_UNKNOWN_OPT);
+
 	argc = parse_options(argc, argv, prefix, builtin_config_options,
 			     builtin_config_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -993,18 +1027,22 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (action == ACTION_NONE) {
+	if (action != ACTION_NONE && subcommand) {
+		error(_("subcommand and action modes are incompatible"));
+		usage_builtin_config();
+	} else if (action == ACTION_NONE && !subcommand) {
 		switch (argc) {
-		case 1: action = ACTION_GET; break;
-		case 2: action = ACTION_SET; break;
-		case 3: action = ACTION_SET_ALL; break;
+		case 1: subcommand = cmd_config_get; break;
+		case 2: subcommand = cmd_config_set; break;
+		case 3: subcommand = cmd_config_set_all; break;
 		default:
 			usage_builtin_config();
 		}
+	} else if (action != ACTION_NONE) {
+		if (action < ACTION_NONE || action >= ARRAY_SIZE(subcommands_by_action))
+			BUG("invalid action %d", action);
+		subcommand = subcommands_by_action[action];
 	}
-	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)) {
-- 
2.44.0


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