diff mbox series

[6/8] builtin/config: introduce subcommands

Message ID f79c0f94e415de5c1c7b4120af5270fe4900d825.1709724089.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series builtin/config: introduce subcommands | expand

Commit Message

Patrick Steinhardt March 6, 2024, 11:31 a.m. UTC
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(-)

Comments

Karthik Nayak March 6, 2024, 9:38 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> @@ -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);
> +

I wonder if we can drop the PARSE_OPT_SUBCOMMAND_OPTIONAL in the second
iteration to make it stricter. But this is OK.
Patrick Steinhardt March 7, 2024, 7:14 a.m. UTC | #2
On Wed, Mar 06, 2024 at 01:38:25PM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > @@ -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);
> > +
> 
> I wonder if we can drop the PARSE_OPT_SUBCOMMAND_OPTIONAL in the second
> iteration to make it stricter. But this is OK.

We can't due to a restriction in the parse-options interface: when
subcommands are present, then PARSE_OPT_KEEP_UNKNOWN_OPT requires us to
also pass PARSE_OPT_SUBCOMMAND_OPTIONAL. Otherwise we trigger a `BUG()`.

Patrick
diff mbox series

Patch

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)) {