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.

Got it, this whole hunk is changing "&actions" to "&action", and nothing
else.

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