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

[Taylor Blau]

On Thu, Apr 26, 2018 at 02:32:54PM +0900, Junio C Hamano wrote:
> Taylor Blau  writes:
>
> > diff --git a/builtin/config.c b/builtin/config.c
> > index d7acf912cd..ec5c11293b 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 | \
> > @@ -231,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 {
> > @@ -376,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);
> >  }
>
> Hmmm, option_parse_type() introduced in [PATCH 2/5] used to learn
> to parse "color" in this step, but it no longer does.

Oof, again. I dropped this hunk on the floor when integrating. I put it
back in v2.


Thanks,
Taylor

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

[Junio C Hamano]

Taylor Blau  writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index d7acf912cd..ec5c11293b 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_PATH4
>  #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 | \
> @@ -231,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 {
> @@ -376,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);
>  }

Hmmm, option_parse_type() introduced in [PATCH 2/5] used to learn
to parse "color" in this step, but it no longer does.  

Here is the difference between what has been queued and the result
of applying these latest patches I found.

diff --git a/builtin/config.c b/builtin/config.c
index 69e7270356..ec5c11293b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -72,7 +72,8 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 int unset)
 {
-   int new_type, *to_type;
+   int new_type;
+   int *to_type;
 
if (unset) {
*((int *) opt->value) = 0;
@@ -95,13 +96,11 @@ 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);
}
 
-   to_type = opt->value;
+   *to_type = opt->value;
if (*to_type && *to_type != new_type) {
/*
 * Complain when there is a new type not equal to the old type.

[PATCH 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 | 20 
 t/t1300-repo-config.sh   | 30 ++
 3 files changed, 56 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 d7acf912cd..ec5c11293b 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 | \
@@ -231,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 {
@@ -376,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
+'
+
+test_expect_success 'get --type=color barfs on non-color' '
+   echo "[foo]bar=not-a-color" >.git/config &&
+   test_must_fail git config --get --type=color foo.bar
+'
+
+test_expect_success 'set --type=color barfs on non-color' '
+   test_must_fail git config --type=color foo.color "not-a-color" 2>error 
&&
+   test_i18ngrep "cannot parse color" error
+'
+
 cat > expect << EOF
 [quote]
leading = " test"
-- 
2.17.0