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

From: Jeff King <peff@peff.net>
Date: Mar 6, 2018, 07:00 UTC
Message-ID: <20180306070057.GE3522@sigill.intra.peff.net>
In-Reply-To: <20180306021729.45813-5-me@ttaylorr.com>

On Mon, Mar 05, 2018 at 06:17:29PM -0800, Taylor Blau wrote:

Show 17 quoted lines

> 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.

Show 10 quoted lines

> @@ -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().

Show 39 quoted lines

> @@ -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