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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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
