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

[Eric Sunshine]

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 

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

[Junio C Hamano]

Taylor Blau  writes:

> +static int option_parse_type(const struct option *opt, const char *arg,
> +  int unset)
> +{
> + /*
> +  * To support '--' style flags, begin with new_type equal to
> +  * opt->defval.
> +  */
> + int new_type = opt->defval;
> + int *to_type = opt->value;
> +
> + if (unset) {
> + *((int *) opt->value) = 0;

As you moved the definition of to_type higher than the previous
rounds, you can already use it here to avoid cryptic casting, i.e.

*to_type = 0;

no?

> + return 0;
> + }

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

[Taylor Blau]

`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`.

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,
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.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt | 71 
 builtin/config.c | 64 +---
 t/t1300-repo-config.sh   | 58 +++--
 3 files changed, 153 insertions(+), 40 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d5..b759009c75 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
-'git config' [] [type] --add name value
-'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
-'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
-'git config' [] [type] [-z|--null] --get-urlmatch name URL
+'git config' [] [--type=] [--show-origin] [-z|--null] name 
[value [value_regex]]
+'git config' [] [--type=] --add name value
+'git config' [] [--type=] --replace-all name value 
[value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] --get 
name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
--get-all name [value_regex]
+'git config' [] [--type=] [--show-origin] [-z|--null] 
[--name-only] --get-regexp name_regex [value_regex]
+'git config' [] [--type=] [-z|--null] --get-urlmatch name 
URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset. 
 If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given .  If no
+`--type=` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +158,39 @@ See also <>.
 --list::
List all variables set in config file, along with their values.
 
---bool::
-   'git config' will ensure that the output is "true" or "false"
+--type ::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in ``'s
+  canonical form.
++
+Valid ``'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value