Re: [PATCH v2 5/5] builtin/config: introduce `color` type specifier

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> For consistency, let's introduce `--type=color` and encourage its use
> with `--default` together over `--get-color` alone.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -228,6 +232,8 @@ Valid `<type>`'s include:
>         output it as the ANSI color escape sequence to the standard
>         output.  The optional `default` parameter is used instead, if
>         there is no color configured for `name`.
> ++
> +`--type=color [--default=<default>]` is preferred over `--get-color`.

Rather than augmenting the documentation of --get-color like this, I
wonder if it would instead make more sense to replace it entirely,
like this:

    --get-color name [<default>]
        Deprecated alias for `--type=color [--default=<default>]`.

Not necessarily worth a re-roll; could be done as a follow-up if
someone cares enough.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> @@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
> +test_expect_success 'get --type=color' '
> +       rm .git/config &&

I would feel more confident about the robustness of this test if it
instead used 'rm -f .git/config', as is already done elsewhere in this
test script.

> +       git config foo.color "red" &&
> +       git config --get --type=color foo.color >actual.raw &&
> +       test_decode_color <actual.raw >actual &&
> +       echo "<RED>" >expect &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'set --type=color' '
> +       rm .git/config &&

Ditto.

> +       git config --type=color foo.color "red" &&
> +       test_cmp expect .git/config
> +'