| Message ID | 41e5bf1d6aa35a32f961b7f9d82a70781674eed0.1709724089.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | builtin/config: introduce subcommands | expand |
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
On Wed, Mar 06, 2024 at 06:52:46PM -0500, Taylor Blau wrote: > 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. Yup. > > + 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". Yup. > > + 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. Yes, although I think putting it into the `--type` parameter might be even better. I'm still a bit torn on whether we should do this all as part of this patch series or as part of a follow-up patch series. But if anybody feels strongly one way or the other I'm happy to adapt accordingly. Patrick
diff --git a/builtin/config.c b/builtin/config.c index fcd6190f12..8a2d1a5de7 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -631,20 +631,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_BIT(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET), - OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL), - OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP), - OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH), - OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL), - OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD), - OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET), - OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL), - OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION), - OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION), - OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST), - OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), - OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR), - OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL), + 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), + 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), + 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), 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 (actions == 0) switch (argc) { case 1: actions = ACTION_GET; break; 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 +' + test_done
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> --- builtin/config.c | 32 ++++++++++++++------------------ t/t1300-config.sh | 13 +++++++++++++ 2 files changed, 27 insertions(+), 18 deletions(-)