On Mon, Mar 05, 2018 at 06:17:29PM -0800, Taylor Blau wrote:
> As of this commit, the cannonical 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 --color core.section
>
> will be have exactly as:
>
> $ git config --get-color core.section red
>
> For consistency, let's introduce `--color` and encourage `--color`,
> `--default` together over `--get-color` alone.
Sounds good. Do we want to explicitly mark "--get-color" as historical
and/or deprecated in the git-config manpage? We won't remove it for a
long time, but this would start the cycle.
> @@ -168,6 +170,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
> if (git_config_expiry_date(&t, key_, value_) < 0)
> return -1;
> strbuf_addf(buf, "%"PRItime, t);
> + } else if (types == TYPE_COLOR) {
> + char *v = xmalloc(COLOR_MAXLEN);
> + if (git_config_color(&v, key_, value_) < 0)
> + return -1;
> + strbuf_addstr(buf, v);
> + free((char *) v);
No need to cast the argument to free, unless you're getting rid of a
"const" (but here "v" already has type "char *").
However, do we need heap allocation here at all? I think:
char v[COLOR_MAXLEN];
if (git_config_color(v, key_, value_) < 0)
return -1;
strbuf_addstr(buf, v);
would suffice (and would also fix the leak when we return on error).
Ditto for the call in normalize_value().
> @@ -313,6 +321,15 @@ static char *normalize_value(const char *key, const char *value)
> else
> return xstrdup(v ? "true" : "false");
> }
> + if (types == TYPE_COLOR) {
> + char *v = xmalloc(COLOR_MAXLEN);
> + if (!git_config_color(&v, key, value)) {
> + free((char *) v);
> + return xstrdup(value);
> + }
> + free((char *) v);
> + die("cannot parse color '%s'", value);
> + }
>
> die("BUG: cannot normalize type %d", types);
> }
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 4f8e6f5fd..c03f54fbe 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -931,6 +931,22 @@ test_expect_success 'get --expiry-date' '
> test_must_fail git config --expiry-date date.invalid1
> '
>
> +cat >expect <<EOF
> +[foo]
> + color = red
> +EOF
> +
> +test_expect_success 'get --color' '
> + rm .git/config &&
> + git config --color foo.color "red" &&
> + test_cmp expect .git/config
> +'
> +
> +test_expect_success 'get --color barfs on non-color' '
> + echo "[foo]bar=not-a-color" >.git/config &&
> + test_must_fail git config --get --color foo.bar
> +'
Looks good. The out-of-block setup of expect violates our modern style,
but matches the surrounding code.
-Peff
