Re: [PATCH v2 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 27, 2018, 05:57 UTC
Message-ID: <CAPig+cQticAS2wrucAVkxiEuGuwoDeyvXKdGb=7s4m+iugYmCA@mail.gmail.com>
In-Reply-To: <404da8bdeb9808d2dfcfdf09bc62c76195c9a08c.1524722247.git.me@ttaylorr.com>

On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau <me@ttaylorr.com> wrote:

Show 7 quoted lines

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values can be interpreted as that type, and (2) that outgoing values are
> canonicalized under that type.
>
> In another series, we propose to extend this functionality with
> `--type=color` and `--default` to replace `--get-color`.

Now that you've combined the two series, this sentence no longer makes sense as written. Perhaps say:

    Later patches will extend this functionality...

Show 14 quoted lines

> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
>
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid squatting on this option too soon, so that
> `git config` can support `--color` (in the traditional sense) in the
> future, if that is desired.
>
> In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> addition to `--int`, `--bool`, and etc. This allows the aforementioned
> upcoming patch to support querying a color value with a default via
> `--type=color --default=...`, without squandering `--color`.
>
> We retain the historic behavior of complaining when multiple,

Drop the comma and be more specific: s/multiple,/multiple conflicting/

> legacy-style `--<type>` flags are given, as well as extend this to
> conflicting new-style `--type=<type>` flags. `--int --type=int` (and its
> commutative pair) does not complain, but `--bool --type=int` (and its
> commutative pair) does.

Confusing. Part of the selling point of the commit message of patch 1/5 is the removal of this complaint/restriction, claiming that it intentionally treats "git config --int --bool" simply as "git config --bool", and that that loosening is required to support "git config --int --type=int" without complaining, thus is a good thing. But this commit message (2/5) backpedals and reinstates the original complaint/restriction.

Perhaps I could have understood if 1/5 said that the loosening of the restriction was only temporary and that it would be restored by a later patch rather than using the restriction-removal as a selling point. However, this patch series doesn't need to be crafted such that a feature is temporarily lost and later restored, so I'm having trouble buying the way this series is architected.

What could make more sense would be for 1/5 to introduce option_parse_type() for --<type>, thus retaining the restriction, and for 2/5 simply to augment option_parse_type() to also understand --type=<type>.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>