Re: [PATCH 3/8] builtin/config: use `OPT_CMDMODE()` to specify modes

[Taylor Blau <me@ttaylorr.com>]

On Wed, Mar 06, 2024 at 12:31:42PM +0100, Patrick Steinhardt wrote:
> The git-config(1) command has various different modes which are
> accessible via e.g. `--get-urlmatch` or `--unset-all`. These modes are
> declared with `OPT_BIT()`, which causes two minor issues:
>
>   - The respective modes also have a negated form `--no-get-urlmatch`,
>     which is unintended.
>
>   - We have to manually handle exclusiveness of the modes.
>
> Switch these options to instead use `OPT_CMDMODE()`, which is made
> exactly for this usecase. Remove the now-unneeded check that only a
> single mode is given, which is now handled by the parse-options
> interface.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

> +	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),
> +	OPT_CMDMODE(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),

Expanding a little on my reply to Junio later on in the thread, I
suspect that these would translate into "get", "get --all", "get
--regexp", and "get --urlmatch", respectively.

> +	OPT_CMDMODE(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
> +	OPT_CMDMODE(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
> +	OPT_CMDMODE(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
> +	OPT_CMDMODE(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),

Same with this one turning into "unset --all".

> +	OPT_CMDMODE(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
> +	OPT_CMDMODE(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
> +	OPT_CMDMODE('l', "list", &actions, N_("list all"), ACTION_LIST),
> +	OPT_CMDMODE('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
> +	OPT_CMDMODE(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
> +	OPT_CMDMODE(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),

These two could probably join the other "get-xyz" modes, and similarly
become "get --color", and "get --colorbool", respectively. It looks like
that's where they lived originally... perhaps I'm missing something as
to why they were moved.

>  	OPT_GROUP(N_("Type")),
>  	OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
>  	OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
> @@ -767,10 +767,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		usage_builtin_config();
>  	}
>
> -	if (HAS_MULTI_BITS(actions)) {
> -		error(_("only one action at a time"));
> -		usage_builtin_config();
> -	}

If you apply just this hunk, I would have expected it to break t1300,
but it doesn't appear to. In fact, it looks like the test suite doesn't
seem to mind, either, which indicates a gap in our test coverage...

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 31c3878687..2d1bc1e27e 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2612,4 +2612,17 @@ test_expect_success 'includeIf.hasconfig:remote.*.url forbids remote url in such
>  	grep "fatal: remote URLs cannot be configured in file directly or indirectly included by includeIf.hasconfig:remote.*.url" err
>  '
>
> +test_expect_success 'negated mode causes failure' '
> +	test_must_fail git config --no-get 2>err &&
> +	grep "unknown option \`no-get${SQ}" err
> +'
> +
> +test_expect_success 'specifying multiple modes causes failure' '
> +	cat >expect <<-EOF &&
> +	error: options ${SQ}--get-all${SQ} and ${SQ}--get${SQ} cannot be used together
> +	EOF
> +	test_must_fail git config --get --get-all 2>err &&
> +	test_cmp expect err
> +'

But this gap is rectified by this hunk here. Thank you!

Thanks,
Taylor