Re: [PATCH 3/8] builtin/config: use `OPT_CMDMODE()` to specify modes
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 7, 2024, 07:02 UTC
- Message-ID
- <ZelmnLhGl4Vpb1Qf@tanuki>
- In-Reply-To
- <ZekBznq74P3QVsSC@nand.local>
On Wed, Mar 06, 2024 at 06:52:46PM -0500, Taylor Blau wrote:
Show 25 quoted lines
> 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.
Show 6 quoted lines
> > + 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.
Show 11 quoted lines
> > + 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