Re: [PATCH v3 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau <me@ttaylorr.com>]

On Wed, Apr 04, 2018 at 03:27:48AM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 2:07 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > In this patch, we prefer `--type=[int|bool|bool-or-int|...]` over
> > `--int`, `--bool`, and etc. This allows the aforementioned other patch
> > to add `--color` (in the non-traditional sense) via `--type=color`,
> > instead of `--color`.
>
> I always find this last sentence confusing since it's not clear to
> which ilk of "--color" option you refer. Perhaps say instead something
> like:
>
>     This normalization will allow the aforementioned upcoming patch to
>     support querying a color value with a default via "--type=color
>     --default=...".

I agree that this is much clearer. I have amended this patch to include
your wording here.

> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -160,30 +158,34 @@ See also <<FILES>>.
> > +--type [type]::
> > +  'git config' will ensure that any input output is valid under the given type
> > +  constraint(s), and will canonicalize outgoing values in `[type]`'s canonical
> > +  form.
>
> Do the brackets in "[type]" mean that the argument is optional? If so,
> what does 'type' default to when not specified? The documentation
> should discuss this.

This is my mistake; I was unaware of the semantic difference between '[]'
and '<>'. If my understanding is correct that '<>' means "required" and
'[]' means "optional", than this is a misspelling of "<type>".

I have addressed this during the re-roll.

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -61,6 +61,33 @@ static int show_origin;
> > +static int option_parse_type(const struct option *opt, const char *arg,
> > +                            int unset)
> > +{
> > +       [...]
> > +       if (!strcmp(arg, "bool"))
> > +               *type = TYPE_BOOL;
> > +       else if (!strcmp(arg, "int"))
> > +               *type = TYPE_INT;
> > +       else if (!strcmp(arg, "bool-or-int"))
> > +               *type = TYPE_BOOL_OR_INT;
> > +       else if (!strcmp(arg, "path"))
> > +               *type = TYPE_PATH;
> > +       else if (!strcmp(arg, "expiry-date"))
> > +               *type = TYPE_EXPIRY_DATE;
> > +       else {
> > +               die(_("unexpected --type argument, %s"), arg);
>
> "unexpected" doesn't seem like the best word choice since an argument
> to --type _is_ expected. Perhaps you mean "unrecognized"?

Sure; I think "unrecognized" is a much better fit. I have updated this
in the re-roll.

Thanks,
Taylor