From: Eric Sunshine Date: Fri, 27 Apr 2018 05:57:14 GMT Subject: Re: [PATCH v2 2/5] builtin/config.c: support `--type=` as preferred alias for `--` Message-ID: In-Reply-To: <404da8bdeb9808d2dfcfdf09bc62c76195c9a08c.1524722247.git.me@ttaylorr.com> On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau 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=` 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 `--` flags are given, as well as extend this to > conflicting new-style `--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 --, thus retaining the restriction, and for 2/5 simply to augment option_parse_type() to also understand --type=. > Signed-off-by: Taylor Blau