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

[Eric Sunshine]

On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau  wrote:
> [...]
> For consistency, let's introduce `--type=color` and encourage its use
> with `--default` together over `--get-color` alone.
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -228,6 +232,8 @@ Valid ``'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=]` 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 []
Deprecated alias for `--type=color [--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 &&
> +   echo "" >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
> +'

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

[Taylor Blau]

As of this commit, the canonical way to retreive an ANSI-compatible
color escape sequence from a configuration file is with the
`--get-color` action.

This is to allow Git to "fall back" on a default value for the color
should the given section not exist in the specified configuration(s).

With the addition of `--default`, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce `--type=color` and encourage its use
with `--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau 
---
 Documentation/git-config.txt |  6 ++
 builtin/config.c | 22 ++
 t/t1300-repo-config.sh   | 30 ++
 3 files changed, 58 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index c3adafd78a..18ddc78f42 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,6 +177,10 @@ Valid ``'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative 
date-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': When getting a value, canonicalize by converting to an ANSI color
+  escape sequence. When setting a value, a sanity-check is performed to ensure
+  that the given value is canonicalize-able as an ANSI color, but it is written
+  as-is.
 +
 
 --bool::
@@ -228,6 +232,8 @@ Valid ``'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=]` is preferred over `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 4f222a0ca9..bb62816bba 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT   3
 #define TYPE_PATH  4
 #define TYPE_EXPIRY_DATE   5
+#define TYPE_COLOR 6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \
@@ -94,6 +95,8 @@ static int option_parse_type(const struct option *opt, const 
char *arg,
new_type = TYPE_PATH;
else if (!strcmp(arg, "expiry-date"))
new_type = TYPE_EXPIRY_DATE;
+   else if (!strcmp(arg, "color"))
+   new_type = TYPE_COLOR;
else
die(_("unrecognized --type argument, %s"), arg);
}
@@ -229,6 +232,11 @@ static int format_config(struct strbuf *buf, const char 
*key_, const char *value
if (git_config_expiry_date(, key_, value_) < 0)
return -1;
strbuf_addf(buf, "%"PRItime, t);
+   } else if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key_, value_) < 0)
+   return -1;
+   strbuf_addstr(buf, v);
} else if (value_) {
strbuf_addstr(buf, value_);
} else {
@@ -374,6 +382,20 @@ static char *normalize_value(const char *key, const char 
*value)
else
return xstrdup(v ? "true" : "false");
}
+   if (type == TYPE_COLOR) {
+   char v[COLOR_MAXLEN];
+   if (git_config_color(v, key, value))
+   die("cannot parse color '%s'", value);
+
+   /*
+* The contents of `v` now contain an ANSI escape
+* sequence, not suitable for including within a
+* configuration file. Treat the above as a
+* "sanity-check", and return the given value, which we
+* know is representable as valid color code.
+*/
+   return xstrdup(value);
+   }
 
die("BUG: cannot normalize type %d", type);
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1e3a1ace14..2acfd513f1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --type=color' '
+   rm .git/config &&
+   git config foo.color "red" &&
+   git config --get --type=color foo.color >actual.raw &&
+   test_decode_color actual &&
+   echo "" >expect &&
+   test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+   color = red
+EOF
+
+test_expect_success 'set --type=color' '
+   rm .git/config &&
+   git config --type=color foo.color "red" &&
+   test_cmp expect .git/config
+'
+