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
