Re: [PATCH 4/4] builtin/config: introduce `--color` type specifier
From: Jeff King <hidden>
Date: 2018-03-06 07:01:10
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.
quoted hunk ↗ jump to hunk
@@ -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().
quoted hunk ↗ jump to hunk
@@ -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