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

[Taylor Blau <me@ttaylorr.com>]

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