Re: [PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc.
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Mar 30, 2018, 05:27 UTC
- Message-ID
- <20180330052719.GA6628@syl.local>
- In-Reply-To
- <20180329221122.GL2939@sigill.intra.peff.net>
On Thu, Mar 29, 2018 at 06:11:22PM -0400, Jeff King wrote:
Show 17 quoted lines
> > +Valid `[type]`'s include: > > ++ > > +- 'bool': canonicalize values as either "true" or "false". > > +- 'int': canonicalize values as simple decimla numbers. An optional suffix of > > + 'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or > > + 1073741824 prior to output. > > +- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described > > + above. > > +- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and > > + `~user` to the home directory for the specified user. This specifier has no > > + effect when setting the value (but you can use `git config section.variable > > + ~/` from the command line to let your shell do the expansion.) > > +- 'expiry-date': canonicalize by converting from a fixed or relative ate-string > > + to a timestamp. This specifier has no effect when setting the value. > > ++ > > Yay. It's nice to have this in only one place now.
Thanks! Agreed :-).
> s/ate-string/d&/ :)
Ack.
My excuse for this is that I have started using iTerm2.app with Vim in my terminal using the new graphics acceleration options, and have had trouble getting Vim to render the underline for misspelled words consistently.
Show 8 quoted lines
> > +static int type_name_to_specifier(char *name)
> > +{
> > + if (!(strcmp(name, "bool")))
> > + return TYPE_BOOL;
>
> We'd usually drop the extra level of parentheses, and just write:
>
> if (!strcmp(name, "bool"))Sounds good; I have adjusted this in the appropriate location in v2.
Show 33 quoted lines
> > @@ -601,6 +618,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> > usage_with_options(builtin_config_usage, builtin_config_options);
> > }
> >
> > + if (type) {
> > + if (types != 0) {
> > + error("usage of --type is ambiguous");
> > + usage_with_options(builtin_config_usage, builtin_config_options);
> > + }
> > + types = type_name_to_specifier(type);
> > + }
>
> This error message left me scratching my head for a minute. Ambiguous
> how? I think this is covering the case of:
>
> git config --int --type=bool
>
> So maybe "--type cannot be used with other type options" or something?
>
> Let's take a step back, though. As part of this, should we convert the
> parsing of type options to last-one-wins? The fact that they are all
> OPT_BIT() is quite silly, since you cannot have more than one bit set.
> So if you do:
>
> git config --int --bool
>
> you get an error. Whereas normal behavior for most options would be for
> --bool to override --int. And that is what happens with:
>
> git config --type=int --type=bool
>
> I don't think there are any backwards compatibility issues to deal with
> here; we'd be changing a case which is now always an error.Agreed.
> And then after that, you truly can make (and document, if we want) that > "--int" is a true synonym for "--type=int".
Great point; for me this is the primary motivating factor for making this change. In addition to simplifying our work when we check:
if (types != 0 && type_str) { ... }it would be nice not to have to compare the two in order to ensure that they are the same, continuing on if so, and causing an error if not.
Show 15 quoted lines
> I think it would be pretty simple. One of:
>
> - convert OPT_BIT("bool") into OPT_CALLBACK("bool") and just assign
> "bool" to the "type" string, which will then later get parsed into
> TYPE_BOOL.
>
> or
>
> - convert OPT_BIT("bool") into OPT_SET_INT("bool") to set TYPE_BOOL
>
> directly. Convert OPT_STRING("type") into OPT_CALLBACK(), and have
> it assign the result of type_name_to_specifier() directly.
>
> I'd probably do the latter, but would be fine with either (and I'd make
> the OPT_SET_INT thing its own preparatory patch).I adopted your advice and used the later, converting each `OPT_BIT` into an `OPT_SET_INT`, and adding a callback for `--type`, which does _not_ complain if `type` is already non-zero.
> If you really want to go all-out, I think the ACTION flags could use the > same cleanup. We treat them as bitflags, and then issue an error when > you set more than one, which is just silly.
Agreed, and I think that this is a good candidate for a future patch. Thoughts? :-).
Thanks, Taylor