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

[Patrick Steinhardt <ps@pks.im>]

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

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

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

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