On Wed, Mar 06, 2024 at 07:10:09PM -0500, Taylor Blau wrote:

On Wed, Mar 06, 2024 at 12:31:50PM +0100, Patrick Steinhardt wrote:

quoted

@@ -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.

The transition period will be a tad weird, I agree. I think initially
I'd prefer to keep the old behaviour just to ensure we don't regress
anything, but after a couple of releases I think we should revisit this
and become more aggressive about using the new style.

quoted

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

The reason I didn't is that we don't always have an action set once we
support subcommands. We can of course figure it out by walking the array
of functions pointers to find the one corresponding to this action. But
I figured it's easier to just use function pointers exclusively.

Patrick