On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau <me@xxxxxxxxxxxx> wrote:
> `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...
> 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@xxxxxxxxxxxx>
