[PATCH 0/4] Teach `--default` to `git-config(1)`

[PATCH 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi,

Attached is a patch series to introduce `--default` and `--color` to the
`git-config(1)` builtin with the aim of introducing a consistent interface to
replace `--get-color`.

Thank you in advance for reviewing this series; I will be more than happy to
respond to any feedback seeing as I am still quite new to working on Git itself.

Thanks,
Taylor

Taylor Blau (4):
  builtin/config: introduce `--default`
  Documentation: list all type specifiers in config prose
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `--color` type specifier

 Documentation/git-config.txt |  21 +++++---
 builtin/config.c             |  36 +++++++++++++
 config.c                     |  10 ++++
 config.h                     |   1 +
 t/t1300-repo-config.sh       |  16 ++++++
 t/t1310-config-default.sh    | 119 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 6 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.15.1.354.g95ec6b1b3

[PATCH 1/4] builtin/config: introduce `--default`

[Taylor Blau]

In an aim to replace:

  $ git config --get-color slot [default] [...]

with:

  $ git config --default default --color slot [...]

introduce `--defualt` to behave as if the given default were present and
assigned to slot in the case that that slot does not exist.

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuraion.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--color`, which (in conjunction
with `--default`) will be sufficient to replace `--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |   4 ++
 builtin/config.c             |  19 +++++++
 t/t1310-config-default.sh    | 119 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc15..390b49831 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,10 @@ See also <<FILES>>.
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default value::
+  When using `--get`, `--get-all`, and `--get-regexp`, behave as
+  if value were the value assigned to the given slot.
+
 [[FILES]]
 FILES
 -----
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476..76edefc07 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -87,6 +88,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("use default value with missing entry")),
 	OPT_END(),
 };
 
@@ -251,6 +253,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		if (format_config(item, key_, default_value) < 0) {
+			values.nr = 0;
+		}
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
@@ -594,6 +606,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions &
+		(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP))) {
+		error("--default is only applicable to --get, --get-all, "
+			"and --get-regexp.");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (config_with_options(show_all_config, NULL,
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 000000000..57fe63295
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'clear default config' '
+	rm -f .git/config
+'
+
+test_expect_success 'uses default when missing entry' '
+	echo quux >expect &&
+	git config --default quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+	echo bar >expect &&
+	git config --add core.foo bar &&
+	git config --default baz core.foo >actual &&
+	git config --unset core.foo &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as bool' '
+	echo true >expect &&
+	git config --default true --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int' '
+	echo 810 >expect &&
+	git config --default 810 --int core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int (storage unit)' '
+	echo 1048576 >expect &&
+	git config --default 1M --int core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as bool-or-int' '
+	echo "1
+true" >expect &&
+	git config --default 1 --bool-or-int core.foo >actual &&
+	git config --default true --bool-or-int core.foo >>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshal default value as path' '
+	echo /path/to/file >expect &&
+	git config --default /path/to/file --path core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshal default value as expiry-date' '
+	echo 0 >expect &&
+	git config --default never --expiry-date core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshal default value as color' '
+	echo "\033[31m" >expect &&
+	git config --default red --color core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'does not allow --default with --replace-all' '
+	test_must_fail git config --default quux --replace-all a b >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --add' '
+	test_must_fail git config --default quux --add a b >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --unset' '
+	test_must_fail git config --default quux --unset a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --unset-all' '
+	test_must_fail git config --default quux --unset-all a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --rename-section' '
+	test_must_fail git config --default quux --rename-section a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --remove-section' '
+	test_must_fail git config --default quux --remove-section a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --list' '
+	test_must_fail git config --default quux --list >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --edit' '
+	test_must_fail git config --default quux --edit >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --get-color' '
+	test_must_fail git config --default quux --get-color >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --get-colorbool' '
+	test_must_fail git config --default quux --get-colorbool >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.15.1.354.g95ec6b1b3

[PATCH 2/4] Documentation: list all type specifiers in config prose

[Taylor Blau]

The documentation for the `git-config(1)` builtin has not been recently
updated to include new types, such as `--bool-or-int`, and
`--expiry-date`. To ensure completeness when adding a new type
specifier, let's update the existing documentation to include the new
types.

This commit also prepares for the new type specifier `--color`, so that
this section will not lag behind when yet more new specifiers are added.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 390b49831..28d84ded9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The type specifier can be either `--int` or `--bool`, to make 'git config'
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form (simple decimal number for int, a "true" or "false" string for
+bool, either of for --bool-or-int), or `--path`, which does some path expansion
+(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
+type specifier is passed, no checks or transformations are performed on the
+value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
-- 
2.15.1.354.g95ec6b1b3

[PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..fb1f41ab3 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char **dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, *dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..48f8e7684 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char **, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.15.1.354.g95ec6b1b3

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

[Taylor Blau]

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.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 10 +++++++---
 builtin/config.c             | 17 +++++++++++++++++
 t/t1300-repo-config.sh       | 16 ++++++++++++++++
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 28d84ded9..0dfb20324 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -42,9 +42,9 @@ The type specifier can be either `--int` or `--bool`, to make 'git config'
 ensure that the variable(s) are of the given type and convert the value to the
 canonical form (simple decimal number for int, a "true" or "false" string for
 bool, either of for --bool-or-int), or `--path`, which does some path expansion
-(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
-type specifier is passed, no checks or transformations are performed on the
-value.
+(see `--path` below), or `--expiry-date` (see `--expiry-date` below), or
+`--color` (see `--color` below).  If no type specifier is passed, no checks or
+transformations are performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -186,6 +186,10 @@ See also <<FILES>>.
 	a fixed or relative date-string to a timestamp. This option
 	has no effect when setting the value.
 
+--color::
+  `git config` will ensure that the output is converted to an
+  ANSI color escape sequence.
+
 -z::
 --null::
 	For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index 76edefc07..05f97f2cb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -54,6 +54,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_COLOR (1<<5)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -83,6 +84,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
 	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
 	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_BIT(0, "color", &types, N_("value is a color"), TYPE_COLOR),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
@@ -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);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -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
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"
-- 
2.15.1.354.g95ec6b1b3

Re: [PATCH 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

On Mon, Mar 05, 2018 at 06:17:25PM -0800, Taylor Blau wrote:
> Attached is a patch series to introduce `--default` and `--color` to the
> `git-config(1)` builtin with the aim of introducing a consistent interface to
> replace `--get-color`.

This series draws significant inspiration from Soukaina Nait Hmid's work
in:

  https://public-inbox.org/git/0102015fb0bf2f74-cb456171-fe65-4d83-8784-b553c7c9e584-000000@eu-west-1.amazonses.com/

--
- Taylor

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Jeff King]

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

> In an aim to replace:
> 
>   $ git config --get-color slot [default] [...]
> 
> with:
> 
>   $ git config --default default --color slot [...]
> 
> introduce `--defualt` to behave as if the given default were present and
> assigned to slot in the case that that slot does not exist.

I think this motivation skips over the beginning part of the story,
which is why we want "--color --default". :)

IMHO, the reason we want --default is two-fold:

  1. Callers have to handle parsing defaults themselves, like:

       foo=$(git config core.foo || echo 1234)

     For an integer, that's not too bad, since you can write "1048576"
     instead of "1M". For colors, it's abominable, which is why we added
     "--get-color". But as we add more types that are hard to parse
     (like --expiry-date), it would be nice for them to get the same
     defaulting feature without adding --get-expiry-date, etc.

  2. --get-color is a one-off unlike all of the other types. That's bad
     interface design, but the inconsistency also makes it harder to add
     features which treat the types uniformly (like, say, a --stdin
     query mode).

And perhaps minor, but it's also easier to correctly error-check
--default, since the "foo" example above would do the wrong thing if
git-config encountered a fatal error.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 14da5fc15..390b49831 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -233,6 +233,10 @@ See also <<FILES>>.
>  	using `--file`, `--global`, etc) and `on` when searching all
>  	config files.
>  
> +--default value::
> +  When using `--get`, `--get-all`, and `--get-regexp`, behave as
> +  if value were the value assigned to the given slot.

I had thought about this in the context of --get, where a single value
makes sense.

For --get-all, would we want to be able to specify a list of objects?
E.g.:

  git config --default foo --default bar --get-all core.slot

and behave as if we found two entries, "foo" and "bar"?

I'm not really sure what semantics would be most useful.

Ditto for --get-regexp.

This isn't necessarily an objection. I'm just not sure what people would
expect. So it might make sense to start more limited and wait for a real
use case to pop up. But I'm also open to arguments about plausible use
cases. ;)

> diff --git a/builtin/config.c b/builtin/config.c
> index ab5f95476..76edefc07 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c

The general design of the implementation looks good. There's one funny
thing:

> +	if (!values.nr && default_value) {
> +		struct strbuf *item;
> +
> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> +		item = &values.items[values.nr++];
> +		if (format_config(item, key_, default_value) < 0) {
> +			values.nr = 0;
> +		}

We never initialize the strbuf data here, and we can't count on
ALLOC_GROW to even zero it (which I suspect would work, but still isn't
how strbufs are meant to be used).

Do you need:

  strbuf_init(item, 0);

here, similar to what collect_config does?

(As an aside, it seems like this whole thing might be simpler with a
string_list, but that's certainly not a problem that you're introducing
here).

> +test_expect_success 'marshals default value as bool-or-int' '
> +	echo "1
> +true" >expect &&
> +	git config --default 1 --bool-or-int core.foo >actual &&
> +	git config --default true --bool-or-int core.foo >>actual &&
> +	test_cmp expect actual
> +'

Funny indentation. Use:

  {
	echo 1 &&
	echo true
  } >expect &&

or

  cat >expect <<-\EOF
  1
  true
  EOF

-Peff

Re: [PATCH 2/4] Documentation: list all type specifiers in config prose

[Jeff King]

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

> The documentation for the `git-config(1)` builtin has not been recently
> updated to include new types, such as `--bool-or-int`, and
> `--expiry-date`. To ensure completeness when adding a new type
> specifier, let's update the existing documentation to include the new
> types.
> 
> This commit also prepares for the new type specifier `--color`, so that
> this section will not lag behind when yet more new specifiers are added.

Good catch. Thanks for cleaning this up.

> -The type specifier can be either `--int` or `--bool`, to make
> -'git config' ensure that the variable(s) are of the given type and
> -convert the value to the canonical form (simple decimal number for int,
> -a "true" or "false" string for bool), or `--path`, which does some
> -path expansion (see `--path` below).  If no type specifier is passed, no
> -checks or transformations are performed on the value.
> +The type specifier can be either `--int` or `--bool`, to make 'git config'
> +ensure that the variable(s) are of the given type and convert the value to the
> +canonical form (simple decimal number for int, a "true" or "false" string for
> +bool, either of for --bool-or-int), or `--path`, which does some path expansion
> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
> +type specifier is passed, no checks or transformations are performed on the
> +value.

Perhaps it's time to switch to a list format for these?

-Peff

Re: [PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors

[Jeff King]

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

> In preparation for adding `--color` to the `git-config(1)` builtin,
> let's introduce a color parsing utility, `git_config_color` in a similar
> fashion to `git_config_<type>`.

Sounds good...

> @@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
>  	return 0;
>  }
>  
> +int git_config_color(char **dest, const char *var, const char *value)
> +{
> +	if (!value)
> +		return config_error_nonbool(var);
> +	if (color_parse(value, *dest) < 0)
> +		return -1;
> +	return 0;
> +}

Why do we take a pointer-to-pointer here? We don't allocate like
git_config_string() does, but instead fill in the existing buffer.

-Peff

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

[Jeff King]

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

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Eric Sunshine]

On Mon, Mar 5, 2018 at 9:17 PM, Taylor Blau <me@ttaylorr.com> wrote:
> In an aim to replace:
>
>   $ git config --get-color slot [default] [...]
>
> with:
>
>   $ git config --default default --color slot [...]
>
> introduce `--defualt` to behave as if the given default were present and

s/--defualt/--default/

> assigned to slot in the case that that slot does not exist.
>
> Values filled by `--default` behave exactly as if they were present in
> the affected configuration file; they will be parsed by type specifiers
> without the knowledge that they are not themselves present in the
> configuraion.
>
> Specifically, this means that the following will work:
>
>   $ git config --int --default 1M does.not.exist
>   1048576
>
> In subsequent commits, we will offer `--color`, which (in conjunction
> with `--default`) will be sufficient to replace `--get-color`.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH 1/4] builtin/config: introduce `--default`

[Eric Sunshine]

On Tue, Mar 6, 2018 at 1:52 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:
>> In an aim to replace:
>>   $ git config --get-color slot [default] [...]
>> with:
>>   $ git config --default default --color slot [...]
>> introduce `--defualt` to behave as if the given default were present and
>> assigned to slot in the case that that slot does not exist.
>
> I think this motivation skips over the beginning part of the story,
> which is why we want "--color --default". :)
>
> IMHO, the reason we want --default is two-fold:
>
>   1. Callers have to handle parsing defaults themselves, like:
>
>        foo=$(git config core.foo || echo 1234)
>
>      For an integer, that's not too bad, since you can write "1048576"
>      instead of "1M". For colors, it's abominable, which is why we added
>      "--get-color". But as we add more types that are hard to parse
>      (like --expiry-date), it would be nice for them to get the same
>      defaulting feature without adding --get-expiry-date, etc.
>
>   2. --get-color is a one-off unlike all of the other types. That's bad
>      interface design, but the inconsistency also makes it harder to add
>      features which treat the types uniformly (like, say, a --stdin
>      query mode).
>
> And perhaps minor, but it's also easier to correctly error-check
> --default, since the "foo" example above would do the wrong thing if
> git-config encountered a fatal error.

Thanks. For someone (me) who didn't follow the earlier discussion
closely, this motivating explanation really helps; especially since
the commit message mentions only color, which seems to already allow
for a default value, so it wasn't clear what benefit the new --default
provides.

>> +test_expect_success 'marshals default value as bool-or-int' '
>> +     echo "1
>> +true" >expect &&
>> +     git config --default 1 --bool-or-int core.foo >actual &&
>> +     git config --default true --bool-or-int core.foo >>actual &&
>> +     test_cmp expect actual
>> +'
>
> Funny indentation. Use:
>
>   {
>         echo 1 &&
>         echo true
>   } >expect &&
>
> or
>
>   cat >expect <<-\EOF
>   1
>   true
>   EOF

Or, simpler:

    test_write_lines 1 true >expect

Re: [PATCH 2/4] Documentation: list all type specifiers in config prose

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +The type specifier can be either `--int` or `--bool`, to make 'git config'
>> +ensure that the variable(s) are of the given type and convert the value to the
>> +canonical form (simple decimal number for int, a "true" or "false" string for
>> +bool, either of for --bool-or-int), or `--path`, which does some path expansion
>> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no
>> +type specifier is passed, no checks or transformations are performed on the
>> +value.
>
> Perhaps it's time to switch to a list format for these?

A very sensible suggestion.  The original was already bad enough but
with complete set, it does become quite hard to read.  Perhaps along
the lines of...

    A type specifier option can be used to force interpretation of
    values and conversion to canonical form.  Currently supported
    type specifiers are:

    `--int`::
            The value is taken as an integer.

    `--bool`::
            The value is taken as a yes/no value, and are shown as
            "true" or "false".

    ...

[PATCH v2 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi,

Attached is 'v2' of my patch series to add a `--default` option to `git config`.

I have addressed the following concerns since the first iteration:

  1. Better motivation in 'builtin/config: introduce `--default`'. (cc: Peff,
  Eric)

  2. Fixed a typo in the message of 'builtin/config: introduce `--default`'.
  (cc: Eric).

  3. Changed the first mention of type specifiers in `git config`'s
  documentation from a paragraph to a brief list (cc: Peff, Junio).

  4. Changed the signatures of some new functions, particularly in 'config.c:
  introduce 'git_config_color' to parse ANSI colors'.

I have thus-far avoided mentioning any specific deprecation of `--get-color` and
`--get-colorbool`, but would not be opposed to changing that in this series
before queuing.

Thank you again for all of your feedback, and my apologies that it has taken me
so long to respond. I was out of the office last week, and have been quite busy
since catching up on Git LFS-related issues.


Thanks,
Taylor

Taylor Blau (4):
  builtin/config: introduce `--default`
  Documentation: list all type specifiers in config prose
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `--color` type specifier

 Documentation/git-config.txt |  38 +++++++---
 builtin/config.c             |  30 ++++++++
 config.c                     |  10 +++
 config.h                     |   1 +
 t/t1300-repo-config.sh       |  24 +++++++
 t/t1310-config-default.sh    | 131 +++++++++++++++++++++++++++++++++++
 6 files changed, 226 insertions(+), 8 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.16.2.440.gc6284da4f

[PATCH v2 1/4] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the slot asked for does not exist. In
addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
slot and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color slot [default] [...]

with:

  $ git config --default default --color slot [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuraion.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--color`, which (in conjunction
with `--default`) will be sufficient to replace `--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |   6 +-
 builtin/config.c             |  17 +++++
 t/t1310-config-default.sh    | 125 +++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+), 2 deletions(-)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..d9e389a33 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,8 +233,10 @@ See also <<FILES>>.
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
-CONFIGURATION
--------------
+--default value::
+  When using `--get`, behave as if value were the value assigned to the given
+  slot.
+
 `pager.config` is only respected when listing configuration, i.e., when
 using `--list` or any of the `--get-*` which may return multiple results.
 The default is to use a pager.
diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..1410be850 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -94,6 +95,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };
 
@@ -258,6 +260,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0) {
+			values.nr = 0;
+		}
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
@@ -601,6 +613,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 000000000..0e464c206
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,125 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'clear default config' '
+	rm -f .git/config
+'
+
+test_expect_success 'uses default when missing entry' '
+	echo quux >expect &&
+	git config --default quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+	echo bar >expect &&
+	git config --add core.foo bar &&
+	git config --default baz core.foo >actual &&
+	git config --unset core.foo &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as bool' '
+	echo true >expect &&
+	git config --default true --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int' '
+	echo 810 >expect &&
+	git config --default 810 --int core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as int (storage unit)' '
+	echo 1048576 >expect &&
+	git config --default 1M --int core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshals default value as bool-or-int' '
+	{
+		echo 1 &&
+		echo true
+	} >expect &&
+	git config --default 1 --bool-or-int core.foo >actual &&
+	git config --default true --bool-or-int core.foo >>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshal default value as path' '
+	echo /path/to/file >expect &&
+	git config --default /path/to/file --path core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'marshal default value as expiry-date' '
+	echo 0 >expect &&
+	git config --default never --expiry-date core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'does not allow --default with --get-all' '
+	test_must_fail git config --default quux --get-all a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --get-regexp' '
+	test_must_fail git config --default quux --get-regexp a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --replace-all' '
+	test_must_fail git config --default quux --replace-all a b >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --add' '
+	test_must_fail git config --default quux --add a b >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --unset' '
+	test_must_fail git config --default quux --unset a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --unset-all' '
+	test_must_fail git config --default quux --unset-all a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --rename-section' '
+	test_must_fail git config --default quux --rename-section a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --remove-section' '
+	test_must_fail git config --default quux --remove-section a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --list' '
+	test_must_fail git config --default quux --list >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --edit' '
+	test_must_fail git config --default quux --edit >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --get-color' '
+	test_must_fail git config --default quux --get-color >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_expect_success 'does not allow --default with --get-colorbool' '
+	test_must_fail git config --default quux --get-colorbool >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.16.2.440.gc6284da4f

[PATCH v2 2/4] Documentation: list all type specifiers in config prose

[Taylor Blau]

The documentation for the `git-config(1)` builtin has not been recently
updated to include new types, such as `--bool-or-int`, and
`--expiry-date`. To ensure completeness when adding a new type
specifier, let's update the existing documentation to include the new
types.

Since this paragraph is growing in length, let's also convert this to a
list so that it can be read with greater ease. We provide a minimal
description of all valid type specifiers, and reference the <<OPTIONS>>
section where each specifier is described in full detail.

This commit also prepares for the new type specifier `--color`, so that
this section will not lag behind when yet more new specifiers are added.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9e389a33..d9d45aca3 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,12 +38,25 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+A type specifier option can be used to force interpretation of values and
+conversion to canonical form.  Currently supported type specifiers are:
+
+`bool`::
+    The value is taken as a boolean.
+
+`int`::
+    The value is taken as an integer.
+
+`bool-or-int`::
+    The value is taken as a boolean or integer, as above.
+
+`path`::
+    The value is taken as a filepath.
+
+`expiry-date`::
+    The value is taken as a timestamp.
+
+For more information about the above type specifiers, see <<OPTIONS>> below.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
-- 
2.16.2.440.gc6284da4f

[PATCH v2 3/4] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..33366b52c 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..0e060779d 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.16.2.440.gc6284da4f

[PATCH v2 4/4] 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 --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.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |  7 +++++++
 builtin/config.c             | 13 +++++++++++++
 t/t1300-repo-config.sh       | 24 ++++++++++++++++++++++++
 t/t1310-config-default.sh    |  6 ++++++
 4 files changed, 50 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index d9d45aca3..e70c0a855 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -56,6 +56,9 @@ conversion to canonical form.  Currently supported type specifiers are:
 `expiry-date`::
     The value is taken as a timestamp.
 
+`color`::
+    The value is taken as an ANSI color escape sequence.
+
 For more information about the above type specifiers, see <<OPTIONS>> below.
 
 When reading, the values are read from the system, global and
@@ -198,6 +201,10 @@ See also <<FILES>>.
 	a fixed or relative date-string to a timestamp. This option
 	has no effect when setting the value.
 
+--color::
+  `git config` will ensure that the output is converted to an
+  ANSI color escape sequence.
+
 -z::
 --null::
 	For all options that output values and/or keys, always
diff --git a/builtin/config.c b/builtin/config.c
index 1410be850..e8661bc12 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_COLOR (1<<5)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
 	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
 	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_BIT(0, "color", &types, N_("value is a color"), TYPE_COLOR),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
@@ -175,6 +177,11 @@ 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[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (types == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (!git_config_color(v, key, value))
+			return xstrdup(value);
+		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..dab94d0c4 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -931,6 +931,30 @@ test_expect_success 'get --expiry-date' '
 	test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --color' '
+	rm .git/config &&
+	git config foo.color "red" &&
+	git config --get --color foo.color | test_decode_color >actual &&
+	echo "<RED>" >expect &&
+	test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+	color = red
+EOF
+
+test_expect_success 'set --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
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
index 0e464c206..0ebece7d2 100755
--- a/t/t1310-config-default.sh
+++ b/t/t1310-config-default.sh
@@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '
 	test_cmp expect actual
 '
 
+test_expect_success 'marshal default value as color' '
+	echo "\033[31m" >expect &&
+	git config --default red --color core.foo >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'does not allow --default with --get-all' '
 	test_must_fail git config --default quux --get-all a >output 2>&1 &&
 	test_i18ngrep "\-\-default is only applicable to" output
-- 
2.16.2.440.gc6284da4f

Re: [PATCH v2 1/4] builtin/config: introduce `--default`

[Jeff King]

On Fri, Mar 23, 2018 at 08:55:53PM -0400, Taylor Blau wrote:

> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.
> 
> For example, we aim to replace:
> 
>   $ git config --get-color slot [default] [...]
> 
> with:
> 
>   $ git config --default default --color slot [...]
> [...]
> In subsequent commits, we will offer `--color`, which (in conjunction
> with `--default`) will be sufficient to replace `--get-color`.

Not a huge deal, but I suspect you could actually get away with less
detail here (I know, I know, I asked for more in the last review).

The motivation of "--get-color is weirdly special, and other types
should match it" is probably enough without laying out all of the future
plans in this patch (which are going to end up repeated when we actually
do add --color).

I don't think it's worth a re-roll by itself, but just a thought for
future patches.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index e09ed5d7d..d9e389a33 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -233,8 +233,10 @@ See also <<FILES>>.
>  	using `--file`, `--global`, etc) and `on` when searching all
>  	config files.
>  
> -CONFIGURATION
> --------------
> +--default value::
> +  When using `--get`, behave as if value were the value assigned to the given
> +  slot.
> +
>  `pager.config` is only respected when listing configuration, i.e., when
>  using `--list` or any of the `--get-*` which may return multiple results.
>  The default is to use a pager.

Hmm, what's going on with the CONFIGURATION header here?

For the description of "--default" itself, do we want to say something
like:

  When using `--get` and the requested slot is not found, behave as
  if...

That is hopefully a given from the name "--default", but it seems like
an important point to mention.

> @@ -258,6 +260,16 @@ static int get_value(const char *key_, const char *regex_)
>  	config_with_options(collect_config, &values,
>  			    &given_config_source, &config_options);
>  
> +	if (!values.nr && default_value) {
> +		struct strbuf *item;
> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> +		item = &values.items[values.nr++];
> +		strbuf_init(item, 0);
> +		if (format_config(item, key_, default_value) < 0) {
> +			values.nr = 0;
> +		}
> +	}

If we see an error from format_config (which AFAICT is only for a bogus
expiry date; the other formats will just die() immediately), what
happens? I guess we'd eventually report "I didn't find anything". Which
is true, but I suspect we may be better off explicitly saying "your
default value was junk" (i.e., by dying right here).

> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
> new file mode 100755
> index 000000000..0e464c206
> --- /dev/null
> +++ b/t/t1310-config-default.sh
> @@ -0,0 +1,125 @@
> +#!/bin/sh
> +
> +test_description='Test git config in different settings (with --default)'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'clear default config' '
> +	rm -f .git/config
> +'

I know that you pulled this trick from t1300, but I think we should
avoid blowing away .git/config. With the current repo-setup code we'll
still blindly treat it as a git repository, but I'm not convinced that's
not a bug. And if we ever do tighten it, we may start rejecting the
trash directory as a repository, and this would set values in the
git.git repo's config file.

Instead, doing "git config -f file" is probably a safer bet (unless you
really do want to check the system-user-repo lookup, but I don't think
we do that at all here).

> +test_expect_success 'marshals default value as bool' '
> +	echo true >expect &&
> +	git config --default true --bool core.foo >actual &&
> +	test_cmp expect actual
> +'

Maybe "--default 1" would be a better test for --bool, since it would
actually transform the value?

> +test_expect_success 'marshals default value as int' '
> +	echo 810 >expect &&
> +	git config --default 810 --int core.foo >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'marshals default value as int (storage unit)' '
> +	echo 1048576 >expect &&
> +	git config --default 1M --int core.foo >actual &&
> +	test_cmp expect actual
> +'

I'm not sure what we're really testing in the first one. The storage
unit conversion is the interesting bit.

TBH, I'm not sure we need to separately test each possible type
conversion here. If we know that the type conversions are tested
elsewhere (and I would not be surprised if they're not, but let's assume
for a moment...) and we check that type conversions are applied to
"--default" values for some types, I don't think there's any reason to
assume that the others would not work.

> +test_expect_success 'does not allow --default with --get-all' '
> +	test_must_fail git config --default quux --get-all a >output 2>&1 &&
> +	test_i18ngrep "\-\-default is only applicable to" output
> +'
> +
> +test_expect_success 'does not allow --default with --get-regexp' '
> +	test_must_fail git config --default quux --get-regexp a >output 2>&1 &&
> +	test_i18ngrep "\-\-default is only applicable to" output
> +'

For these repeated tests, this _is_ the only place we'd test the
behavior. I do have mixed feelings on this kind of completionism,
though.  While of course any kind of bug is possible, it doesn't seem
like these are likely to catch a regression. The obvious regression IMHO
would be adding a new mode that isn't caught by the --default check. But
then, it's unlikely that somebody introducing that bug would find and
update these tests to catch it.

So I dunno. They are written and they're not that complicated. But at
some point I wonder if we want to be more judicious in adding tests just
to keep the test suite to a reasonable size.

-Peff

Re: [PATCH v2 2/4] Documentation: list all type specifiers in config prose

[Jeff King]

On Fri, Mar 23, 2018 at 08:55:54PM -0400, Taylor Blau wrote:

> The documentation for the `git-config(1)` builtin has not been recently
> updated to include new types, such as `--bool-or-int`, and
> `--expiry-date`. To ensure completeness when adding a new type
> specifier, let's update the existing documentation to include the new
> types.
> 
> Since this paragraph is growing in length, let's also convert this to a
> list so that it can be read with greater ease. We provide a minimal
> description of all valid type specifiers, and reference the <<OPTIONS>>
> section where each specifier is described in full detail.
> 
> This commit also prepares for the new type specifier `--color`, so that
> this section will not lag behind when yet more new specifiers are added.

Thanks for reformatting; the result is much easier to read. One nit:

> -The type specifier can be either `--int` or `--bool`, to make
> -'git config' ensure that the variable(s) are of the given type and
> -convert the value to the canonical form (simple decimal number for int,
> -a "true" or "false" string for bool), or `--path`, which does some
> -path expansion (see `--path` below).  If no type specifier is passed, no
> -checks or transformations are performed on the value.
> +A type specifier option can be used to force interpretation of values and
> +conversion to canonical form.  Currently supported type specifiers are:
> +
> +`bool`::
> +    The value is taken as a boolean.
> +
> +`int`::
> +    The value is taken as an integer.
> +
> +`bool-or-int`::
> +    The value is taken as a boolean or integer, as above.
> +
> +`path`::
> +    The value is taken as a filepath.
> +
> +`expiry-date`::
> +    The value is taken as a timestamp.
> +
> +For more information about the above type specifiers, see <<OPTIONS>> below.

The connection between "bool" here and "--bool" below isn't made
explicit. We probably should mention it.

Some of the details for the types are lost. E.g., what is a filepath?
The fact that we expand is not mentioned. I'm not sure how much that
matters, though. The full descriptions should be covered under "--path"
(and "--int", etc).

In fact, that kind of makes me wonder if this "type" list should not
exist at all. If we instead grouped the options and said "these are the
type options", then we'd only need one list.

I'm OK to punt on that for now, though, in the interest of not holding
up this patch series. I do think we should say something like:

  Each type can be specified with the matching command-line option
  (e.g., `--bool`, `--int`, etc); see <<OPTIONS>> below.

-Peff

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

[Jeff King]

On Fri, Mar 23, 2018 at 08:55:56PM -0400, Taylor Blau wrote:

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

s/retreive/retrieve/

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

I don't think we'll ever get rid of --get-color (at the very least, we'd
need a deprecation period). But it's probably worth adding a note under
the --get-color description to mention that it's a historical synonym,
and that using "--default" should be preferred.

> @@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {
>  	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
>  	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
>  	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
> +	OPT_BIT(0, "color", &types, N_("value is a color"), TYPE_COLOR),
>  	OPT_GROUP(N_("Other")),
>  	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
>  	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),

I just had a funny thought. Normally in Git the "--color" option means
"colorize the output". And we are diverging from that here. I wonder if
anybody would be confused by that, or if we would ever want to later add
an option to colorize git-config output. Would we regret squatting on
--color?

I'm not sure what else to name it. Anything _except_ "--color" would
diverge from the existing scheme of "--<type>".

If we were designing from scratch, I'd consider:

  git config --type=int ...
  git config --type=color ...

etc. I'm not sure if it's worth trying to switch now (on the other hand,
it resolves the documentation issue I mentioned earlier, since that
would naturally group all of the types ;) ).

It would be pretty easy to declare "--type" as the Right Way, and list
"--int" as a historical synonym for "--type=int".

> @@ -175,6 +177,11 @@ 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[COLOR_MAXLEN];
> +			if (git_config_color(v, key_, value_) < 0)
> +				return -1;
> +			strbuf_addstr(buf, v);

Looks good.

> @@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)
>  		else
>  			return xstrdup(v ? "true" : "false");
>  	}
> +	if (types == TYPE_COLOR) {
> +		char v[COLOR_MAXLEN];
> +		if (!git_config_color(v, key, value))
> +			return xstrdup(value);
> +		die("cannot parse color '%s'", value);
> +	}

Interesting. This doesn't actually normalize anything, since we always
pass back the original value (or die). I think that's the right thing to
do, since otherwise you'd end up with ANSI codes in your config file.

I wondered at first if this should go in the "noop" normalization that
TYPE_PATH undergoes. But I like that it actually sanity-checks the
value. We should probably have a comment here explaining that yes, we
parse and then throw away the value (similar to the one near TYPE_PATH).

I suspect that TYPE_EXPIRY_DATE should do the same thing (parse and
complain if you fed nonsense, but always keep the original value).

> +test_expect_success 'get --color' '
> +	rm .git/config &&
> +	git config foo.color "red" &&
> +	git config --get --color foo.color | test_decode_color >actual &&
> +	echo "<RED>" >expect &&
> +	test_cmp expect actual
> +'

We should probably write this as:

  git config --get --color foo.color >actual.raw &&
  test_decode_color <actual.raw >actual

to catch failures from git-config itself (there's a lot of old tests
which pipe, but we've been trying to convert them to be more careful).

> +test_expect_success 'set --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
> +'

After reading the normalize bits above, I think there's one more case to
cover:

  test_must_fail git config --color foo.color not-a-color

> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
> index 0e464c206..0ebece7d2 100755
> --- a/t/t1310-config-default.sh
> +++ b/t/t1310-config-default.sh
> @@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'marshal default value as color' '
> +	echo "\033[31m" >expect &&
> +	git config --default red --color core.foo >actual &&
> +	test_cmp expect actual
> +'

I don't offhand recall whether octal escapes with "echo" are portable.
It _is_ in POSIX, but only for XSI. I think using "printf" would be
portable.

-Peff

Re: [PATCH v2 0/4] Teach `--default` to `git-config(1)`

[Jeff King]

On Fri, Mar 23, 2018 at 08:55:52PM -0400, Taylor Blau wrote:

> Attached is 'v2' of my patch series to add a `--default` option to `git config`.

Thanks, this is looking much better. I did come up with a few
suggestions/fixes. Some minor which would make v3 easy, and some that
would require expanding the series quite a bit. I'm not sure if those
bigger ones are worth doing or not. :)

> Thank you again for all of your feedback, and my apologies that it has taken me
> so long to respond. I was out of the office last week, and have been quite busy
> since catching up on Git LFS-related issues.

No problem. Open source moves at the pace of patch submissions.

-Peff

[PATCH v3 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi all,

Attached is 'v3' of my patch series to add a `--default` option to `git config`.

The most notable change since the last reroll is that this series is now based
upon tb/config-type-specifier-option [1], with discussion about that change here
[2]. This was motivated by my and Peff's shared concern that by adding `--color`
as a top-level type specifier, that we'd be "squatting" on adding `--color` in
the traditional sense of "colorize this output."

As such, `--color` is _not_ added as a top-level type specifier in this version,
rather, it is accessible via `--type=color`. This leaves `--color` unused and
available for other uses in the future.

Other changes since last time include:

  1. Fixing an erroneous diff in Documentation/git-config.txt, where a header
     was removed.

  2. `git config` will now die immediately when the `--default` value is not
      canonicalize able under the given type specifier.

  3. Removing redundancies from t1300 and t1310 that are--as Peff wisely points
     out--unlikely to catch any meaningful regressions in the future.

  4. Documenting behavior in builtin/config.c that we "throw away" the
     canonicalized value during normalization.

As always, thank you for the helpful feedback. I look forward to your thoughts
on this version of the series.


Thanks,
Taylor

Taylor Blau (3):
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt | 15 ++++++++++----
 builtin/config.c             | 38 ++++++++++++++++++++++++++++++++++++
 config.c                     | 10 ++++++++++
 config.h                     |  1 +
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++
 t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++
 6 files changed, 128 insertions(+), 4 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.16.2.440.gc6284da4f

[PATCH v3 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the slot asked for does not exist. In
addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
slot and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color slot [default] [...]

with:

  $ git config --default default --color slot [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--color`, which (in conjunction
with `--default`) will be sufficient to replace `--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 17 ++++++++++++++++
 t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index a4a5ffb41..5aa528878 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -235,6 +235,10 @@ Valid `[type]`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default value::
+  When using `--get`, and the requested slot is not found, behave as if value
+  were the value assigned to the that slot.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index ea7923493..4340f5f3d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -27,6 +27,7 @@ static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, types;
 static char *type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -96,6 +97,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };
 
@@ -260,6 +262,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0) {
+			exit(1);
+		}
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
@@ -618,6 +630,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
+
 	if (type) {
 		if (types != 0) {
 			error("usage of --type is ambiguous");
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 000000000..ab4e35f06
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when missing entry' '
+	echo quux >expect &&
+	git config -f config --default quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=true --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+	echo bar >expect &&
+	git config --add core.foo bar &&
+	git config --default baz core.foo >actual &&
+	git config --unset core.foo &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=int --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "invalid unit" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default quux --unset a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.16.2.440.gc6284da4f

[PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..33366b52c 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..0e060779d 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.16.2.440.gc6284da4f

[PATCH v3 3/3] 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 `--color` and encourage `--type=color`,
`--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 11 +++++++----
 builtin/config.c             | 21 +++++++++++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 5aa528878..aaba36615 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-A type specifier may be given as an argument to `--type` to make 'git config'
-ensure that the variable(s) are of the given type and convert the value to the
-canonical form. If no type specifier is passed, no checks or transformations are
-performed on the value.
+`color`::
+    The value is taken as an ANSI color escape sequence.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -177,6 +175,7 @@ Valid `[type]`'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative ate-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': canonicalize by converting to an ANSI color escape sequence.
 +
 
 --bool::
@@ -184,6 +183,7 @@ Valid `[type]`'s include:
 --bool-or-int::
 --path::
 --expiry-date::
+--color::
   Historical options for selecting a type specifier. Prefer instead `--type`,
   (see: above).
 
@@ -223,6 +223,9 @@ Valid `[type]`'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`.
++
+It is preferred to use `--type=color`, or `--type=color --default=[default]`
+instead of `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 4340f5f3d..1aae228a6 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -62,6 +62,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT (1<<2)
 #define TYPE_PATH (1<<3)
 #define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_COLOR (1<<5)
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -177,6 +178,11 @@ 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[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
@@ -322,6 +328,19 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (types == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (!git_config_color(v, key, 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("cannot parse color '%s'", value);
+	}
 
 	die("BUG: cannot normalize type %d", types);
 }
@@ -519,6 +538,8 @@ static int type_name_to_specifier(char *name)
 		return TYPE_PATH;
 	else if (!(strcmp(name, "expiry-date")))
 		return TYPE_EXPIRY_DATE;
+	else if (!(strcmp(name, "color")))
+		return TYPE_COLOR;
 	die(_("unexpected --type argument, %s"), name);
 }
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 12dc94bd2..12e852a1d 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.raw >actual &&
+	echo "<RED>" >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.16.2.440.gc6284da4f

Re: [PATCH v3 0/4] Teach `--default` to `git-config(1)`

[Taylor Blau]

On Wed, Mar 28, 2018 at 06:16:31PM -0700, Taylor Blau wrote:
> Attached is 'v3' of my patch series to add a `--default` option to `git config`.

My apologies, this patch series is only 3 long, so the subject line of
this message is incorrect.


Thanks,
Taylor

Re: [PATCH v2 1/4] builtin/config: introduce `--default`

[Taylor Blau]

On Mon, Mar 26, 2018 at 04:34:42AM -0400, Jeff King wrote:
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > index e09ed5d7d..d9e389a33 100644
> > --- a/Documentation/git-config.txt
> > +++ b/Documentation/git-config.txt
> > @@ -233,8 +233,10 @@ See also <<FILES>>.
> >  	using `--file`, `--global`, etc) and `on` when searching all
> >  	config files.
> >
> > -CONFIGURATION
> > --------------
> > +--default value::
> > +  When using `--get`, behave as if value were the value assigned to the given
> > +  slot.
> > +
> >  `pager.config` is only respected when listing configuration, i.e., when
> >  using `--list` or any of the `--get-*` which may return multiple results.
> >  The default is to use a pager.
>
> Hmm, what's going on with the CONFIGURATION header here?

My mistake, I have correct this erroneous diff in v3. Thanks for
pointing it out!

> For the description of "--default" itself, do we want to say something
> like:
>
>   When using `--get` and the requested slot is not found, behave as
>   if...
>
> That is hopefully a given from the name "--default", but it seems like
> an important point to mention.

Ditto.

> > +test_expect_success 'marshals default value as int' '
> > +	echo 810 >expect &&
> > +	git config --default 810 --int core.foo >actual &&
> > +	test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'marshals default value as int (storage unit)' '
> > +	echo 1048576 >expect &&
> > +	git config --default 1M --int core.foo >actual &&
> > +	test_cmp expect actual
> > +'
>
> I'm not sure what we're really testing in the first one. The storage
> unit conversion is the interesting bit.
>
> TBH, I'm not sure we need to separately test each possible type
> conversion here. If we know that the type conversions are tested
> elsewhere (and I would not be surprised if they're not, but let's assume
> for a moment...) and we check that type conversions are applied to
> "--default" values for some types, I don't think there's any reason to
> assume that the others would not work.

Agreed. I don't think that this test (and the ones that your comment
below concerns) are testing anything meaningful, or that would catch any
interesting future regressions. I have removed them from this series in
v3.


Thanks,
Taylor

Re: [PATCH v2 2/4] Documentation: list all type specifiers in config prose

[Taylor Blau]

On Mon, Mar 26, 2018 at 04:55:56AM -0400, Jeff King wrote:
> In fact, that kind of makes me wonder if this "type" list should not
> exist at all. If we instead grouped the options and said "these are the
> type options", then we'd only need one list.
>
> I'm OK to punt on that for now, though, in the interest of not holding
> up this patch series. I do think we should say something like:
>
>   Each type can be specified with the matching command-line option
>   (e.g., `--bool`, `--int`, etc); see <<OPTIONS>> below.

I punted on this for now, since rebasing on
tb/config-type-specifier-option makes this commit a no-op.


Thanks,
Taylor

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

[Taylor Blau]

On Mon, Mar 26, 2018 at 05:16:45AM -0400, Jeff King wrote:
> On Fri, Mar 23, 2018 at 08:55:56PM -0400, Taylor Blau wrote:
>
> > 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.
>
> s/retreive/retrieve/

Ack. I forgot to correct this in v3. I will remember to fix this in
anticipation of v4 before this is queued.

> > 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.
>
> I don't think we'll ever get rid of --get-color (at the very least, we'd
> need a deprecation period). But it's probably worth adding a note under
> the --get-color description to mention that it's a historical synonym,
> and that using "--default" should be preferred.

I added a note in v3 under the `--get-color` section that it is now
considered "historical", and it is instead preferred to use

  $ git config --default=<default> --type=color <section>

> > @@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {
> >  	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
> >  	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
> >  	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
> > +	OPT_BIT(0, "color", &types, N_("value is a color"), TYPE_COLOR),
> >  	OPT_GROUP(N_("Other")),
> >  	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
> >  	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
>
> I just had a funny thought. Normally in Git the "--color" option means
> "colorize the output". And we are diverging from that here. I wonder if
> anybody would be confused by that, or if we would ever want to later add
> an option to colorize git-config output. Would we regret squatting on
> --color?
>
> I'm not sure what else to name it. Anything _except_ "--color" would
> diverge from the existing scheme of "--<type>".
>
> If we were designing from scratch, I'd consider:
>
>   git config --type=int ...
>   git config --type=color ...
>
> etc. I'm not sure if it's worth trying to switch now (on the other hand,
> it resolves the documentation issue I mentioned earlier, since that
> would naturally group all of the types ;) ).
>
> It would be pretty easy to declare "--type" as the Right Way, and list
> "--int" as a historical synonym for "--type=int".

Agreed. I have posted a topic to the list containing a patch to the
above effect.

> > @@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)
> >  		else
> >  			return xstrdup(v ? "true" : "false");
> >  	}
> > +	if (types == TYPE_COLOR) {
> > +		char v[COLOR_MAXLEN];
> > +		if (!git_config_color(v, key, value))
> > +			return xstrdup(value);
> > +		die("cannot parse color '%s'", value);
> > +	}
>
> Interesting. This doesn't actually normalize anything, since we always
> pass back the original value (or die). I think that's the right thing to
> do, since otherwise you'd end up with ANSI codes in your config file.
>
> I wondered at first if this should go in the "noop" normalization that
> TYPE_PATH undergoes. But I like that it actually sanity-checks the
> value. We should probably have a comment here explaining that yes, we
> parse and then throw away the value (similar to the one near TYPE_PATH).

That was my original intent, but I agree that this is confusing without
a comment explaining so. I have added such a comment in v3.

> I suspect that TYPE_EXPIRY_DATE should do the same thing (parse and
> complain if you fed nonsense, but always keep the original value).

Where?

> > +test_expect_success 'get --color' '
> > +	rm .git/config &&
> > +	git config foo.color "red" &&
> > +	git config --get --color foo.color | test_decode_color >actual &&
> > +	echo "<RED>" >expect &&
> > +	test_cmp expect actual
> > +'
>
> We should probably write this as:
>
>   git config --get --color foo.color >actual.raw &&
>   test_decode_color <actual.raw >actual
>
> to catch failures from git-config itself (there's a lot of old tests
> which pipe, but we've been trying to convert them to be more careful).

Sounds good; I have added this in v3.

> > +test_expect_success 'set --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
> > +'
>
> After reading the normalize bits above, I think there's one more case to
> cover:
>
>   test_must_fail git config --color foo.color not-a-color

Good idea, I've added this in v3.

> > diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
> > index 0e464c206..0ebece7d2 100755
> > --- a/t/t1310-config-default.sh
> > +++ b/t/t1310-config-default.sh
> > @@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'marshal default value as color' '
> > +	echo "\033[31m" >expect &&
> > +	git config --default red --color core.foo >actual &&
> > +	test_cmp expect actual
> > +'
>
> I don't offhand recall whether octal escapes with "echo" are portable.
> It _is_ in POSIX, but only for XSI. I think using "printf" would be
> portable.

I removed this test in accordance with your earlier suggestions, so this
change is no longer required.


Thanks,
Taylor

Re: [PATCH v3 1/3] builtin/config: introduce `--default`

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> For some use cases, callers of the `git-config(1)` builtin would like to
> fallback to default values when the slot asked for does not exist. In
> addition, users would like to use existing type specifiers to ensure
> that values are parsed correctly when they do exist in the
> configuration.
> ...
> +--default value::
> +  When using `--get`, and the requested slot is not found, behave as if value
> +  were the value assigned to the that slot.

For "diff.<slot>.color", the above is OK, but in general,
configuration variables are not called "slot".  s/slot/variable/.

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -184,6 +183,7 @@ Valid `[type]`'s include:
>  --bool-or-int::
>  --path::
>  --expiry-date::
> +--color::
>    Historical options for selecting a type specifier. Prefer instead `--type`,
>    (see: above).
>  
> @@ -223,6 +223,9 @@ Valid `[type]`'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`.
> ++
> +It is preferred to use `--type=color`, or `--type=color --default=[default]`
> +instead of `--get-color`.

Wasn't the whole point of the preliminary --type=<type> patch to
avoid having to add thse two hunks?

Re: [PATCH v3 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:
> For some use cases, callers of the `git-config(1)` builtin would like to
> fallback to default values when the slot asked for does not exist. In
> addition, users would like to use existing type specifiers to ensure
> that values are parsed correctly when they do exist in the
> configuration.
>
> For example, to fetch a value without a type specifier and fallback to
> `$fallback`, the following is required:
>
>   $ git config core.foo || echo "$fallback"
>
> This is fine for most values, but can be tricky for difficult-to-express
> `$fallback`'s, like ANSI color codes.
>
> This motivates `--get-color`, which is a one-off exception to the normal
> type specifier rules wherein a user specifies both the configuration
> slot and an optional fallback. Both are formatted according to their
> type specifier, which eases the burden on the user to ensure that values
> are correctly formatted.
>
> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.

I'm confused. The cover letter said that this iteration no longer
introduces a --color option (favoring instead --type=color), but this
commit message still talks about --color. Did you mean
s/--color/--type=color/ ?

> For example, we aim to replace:
>
>   $ git config --get-color slot [default] [...]
>
> with:
>
>   $ git config --default default --color slot [...]

Ditto: s/--color/--type=color/

> Values filled by `--default` behave exactly as if they were present in
> the affected configuration file; they will be parsed by type specifiers
> without the knowledge that they are not themselves present in the
> configuration.
>
> Specifically, this means that the following will work:
>
>   $ git config --int --default 1M does.not.exist
>   1048576
>
> In subsequent commits, we will offer `--color`, which (in conjunction
> with `--default`) will be sufficient to replace `--get-color`.

Ditto: s/--color/--type=color/

> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Eric Sunshine]

On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:
> In preparation for adding `--color` to the `git-config(1)` builtin,
> let's introduce a color parsing utility, `git_config_color` in a similar
> fashion to `git_config_<type>`.

Did you mean s/--color/--type=color/ ?

> Signed-off-by: Taylor Blau <me@ttaylorr.com>

Re: [PATCH v3 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Fri, Mar 30, 2018 at 11:06:22AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > For some use cases, callers of the `git-config(1)` builtin would like to
> > fallback to default values when the slot asked for does not exist. In
> > addition, users would like to use existing type specifiers to ensure
> > that values are parsed correctly when they do exist in the
> > configuration.
> > ...
> > +--default value::
> > +  When using `--get`, and the requested slot is not found, behave as if value
> > +  were the value assigned to the that slot.
>
> For "diff.<slot>.color", the above is OK, but in general,
> configuration variables are not called "slot".  s/slot/variable/.

Thanks; I was unaware of this convention. I have changed "slot" to
"variable" as you suggested in the subsequent re-roll.

Thanks,
Taylor

Re: [PATCH v3 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Fri, Mar 30, 2018 at 04:23:56PM -0400, Eric Sunshine wrote:
> On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > This commit (and those following it in this series) aim to eventually
> > replace `--get-color` with a consistent alternative. By introducing
> > `--default`, we allow the `--get-color` action to be promoted to a
> > `--color` type specifier, retaining the "fallback" behavior via the
> > `--default` flag introduced in this commit.
>
> I'm confused. The cover letter said that this iteration no longer
> introduces a --color option (favoring instead --type=color), but this
> commit message still talks about --color. Did you mean
> s/--color/--type=color/ ?

My mistake; I think I rebased this series off of the "--type=" series
and forgot to amend this change. I have updated this and the below in
the subsequent re-roll.

> > For example, we aim to replace:
> >
> >   $ git config --get-color slot [default] [...]
> >
> > with:
> >
> >   $ git config --default default --color slot [...]
>
> Ditto: s/--color/--type=color/

Ack.

> > Values filled by `--default` behave exactly as if they were present in
> > the affected configuration file; they will be parsed by type specifiers
> > without the knowledge that they are not themselves present in the
> > configuration.
> >
> > Specifically, this means that the following will work:
> >
> >   $ git config --int --default 1M does.not.exist
> >   1048576
> >
> > In subsequent commits, we will offer `--color`, which (in conjunction
> > with `--default`) will be sufficient to replace `--get-color`.
>
> Ditto: s/--color/--type=color/

Ack.

Thanks,
Taylor

Re: [PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

On Fri, Mar 30, 2018 at 04:26:09PM -0400, Eric Sunshine wrote:
> On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > In preparation for adding `--color` to the `git-config(1)` builtin,
> > let's introduce a color parsing utility, `git_config_color` in a similar
> > fashion to `git_config_<type>`.
>
> Did you mean s/--color/--type=color/ ?

I did; thanks for pointing this out. I have fixed this to mention
"--type=color" in the subsequent re-roll.

Thanks,
Taylor

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

[Taylor Blau]

On Fri, Mar 30, 2018 at 11:09:43AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -184,6 +183,7 @@ Valid `[type]`'s include:
> >  --bool-or-int::
> >  --path::
> >  --expiry-date::
> > +--color::
> >    Historical options for selecting a type specifier. Prefer instead `--type`,
> >    (see: above).
> >
> > @@ -223,6 +223,9 @@ Valid `[type]`'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`.
> > ++
> > +It is preferred to use `--type=color`, or `--type=color --default=[default]`
> > +instead of `--get-color`.
>
> Wasn't the whole point of the preliminary --type=<type> patch to
> avoid having to add thse two hunks?

For the first hunk, yes, but not for the second. The series that adds
"--type=<type>" was meant to make it possible to say "parse this as a
color" without squatting on the "--color" flag.

So, including "--color" in the list of historical options is a mistake.
But, using "--type=color --default=..." over "--get-color" is the
desired intention of this series.


Thanks,
Taylor

[PATCH v4 0/3] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi,

Attached is a fourth re-roll of my series to add "--default" and
"--type=color" to "git config" in order to replace:

  $ git config --get-color <variable> [default]

with

  $ git config [--default=<default>] --type=color <variable>

Since the last revision, I have:

  - Rebased it upon the latest changes from my series to add
    "--type=<type>" in favor of "--<type>".

  - Fixed various incorrect hunks mentioning "--color" (which does not
    exist), cc: Eric, Junio.

  - Fixed referencing "slot" when the correct spelling is "variable".
    cc: Junio.

As always, thank you for your helpful feedback. This process is still
new to me, so I appreciate an extra degree of scrutiny :-).

I am hopeful that this series is ready for queueing.


Thanks,
Taylor

Taylor Blau (3):
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt | 14 +++++++++----
 builtin/config.c             | 33 +++++++++++++++++++++++++++++++
 config.c                     | 10 ++++++++++
 config.h                     |  1 +
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++
 t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++
 6 files changed, 122 insertions(+), 4 deletions(-)
 create mode 100755 t/t1310-config-default.sh

--
2.17.0

[PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 12 ++++++++++++
 t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index c7e56e3fd..620492d1e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -235,6 +235,10 @@ Valid `<type>`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default value::
+  When using `--get`, and the requested slot is not found, behave as if value
+  were the value assigned to the that slot.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 6e1495eac..1328b568b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -122,6 +123,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };
 
@@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0) {
+			exit(1);
+		}
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 000000000..ab4e35f06
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when missing entry' '
+	echo quux >expect &&
+	git config -f config --default quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=true --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+	echo bar >expect &&
+	git config --add core.foo bar &&
+	git config --default baz core.foo >actual &&
+	git config --unset core.foo &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=int --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "invalid unit" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default quux --unset a >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.17.0

[PATCH v4 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--type=color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..33366b52c 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..0e060779d 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.17.0

[PATCH v4 3/3] 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 `--color` and encourage `--type=color`,
`--default` together over `--get-color` alone.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 10 ++++++----
 builtin/config.c             | 21 +++++++++++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 620492d1e..bde702d2e 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-A type specifier may be given as an argument to `--type` to make 'git config'
-ensure that the variable(s) are of the given type and convert the value to the
-canonical form. If no type specifier is passed, no checks or transformations are
-performed on the value.
+`color`::
+    The value is taken as an ANSI color escape sequence.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -177,6 +175,7 @@ Valid `<type>`'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': canonicalize by converting to an ANSI color escape sequence.
 +
 
 --bool::
@@ -223,6 +222,9 @@ Valid `<type>`'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`.
++
+It is preferred to use `--type=color`, or `--type=color --default=[default]`
+instead of `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 1328b568b..aa3fcabe9 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
 
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		*type = TYPE_PATH;
 	else if (!strcmp(arg, "expiry-date"))
 		*type = TYPE_EXPIRY_DATE;
+	else if (!strcmp(arg, "color"))
+		*type = TYPE_COLOR;
 	else {
 		die(_("unrecognized --type argument, %s"), arg);
 		return 1;
@@ -203,6 +206,11 @@ 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 (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 {
@@ -348,6 +356,19 @@ 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))
+			/*
+			 * 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("cannot parse color '%s'", value);
+	}
 
 	die("BUG: cannot normalize type %d", type);
 }
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index b25ab7b9e..c630bdc77 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.raw >actual &&
+	echo "<RED>" >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

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Jeff King]

On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:

> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
>  	config_with_options(collect_config, &values,
>  			    &given_config_source, &config_options);
>  
> +	if (!values.nr && default_value) {
> +		struct strbuf *item;
> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> +		item = &values.items[values.nr++];
> +		strbuf_init(item, 0);
> +		if (format_config(item, key_, default_value) < 0) {
> +			exit(1);
> +		}
> +	}

Calling exit() explicitly is unusual for our code. Usually we would
either die() or propagate the error. Most of the types in
format_config() would die on bogus input, but a few code paths will
return an error.

What happens if a non-default value has a bogus format? E.g.:

  $ git config foo.bar '~NoSuchUser'
  $ git config --path foo.bar
  fatal: failed to expand user dir in: '~NoSuchUser'

Oops. Despite us checking for an error return from
git_config_pathname(), it will always either return 0 or die(). So
that's not a good example. ;)

Let's try expiry-date:

  $ git config foo.bar 'the first of octember'
  $ git config --expiry-date foo.bar
  error: 'the first of octember' for 'foo.bar' is not a valid timestamp
  fatal: bad config line 7 in file .git/config

OK. So we call format_config() there from the actual collect_config()
callback, and the error gets propagated back to the config parser, which
then gives us an informative die(). What happens with your new code:

  $ ./git config --default 'the first of octember' --type=expiry-date no.such.key
  error: 'the first of octember' for 'no.such.key' is not a valid timestamp

It's obvious in this toy example, but that config call may be buried
deep in a script. It'd probably be nicer for that exit(1) to be
something like:

  die(_("failed to format default config value"));

> +test_expect_success 'does not allow --default without --get' '
> +	test_must_fail git config --default quux --unset a >output 2>&1 &&
> +	test_i18ngrep "\-\-default is only applicable to" output
> +'

I think "a" here needs to be "a.section". I get:

  error: key does not contain a section: a

-Peff

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

[Jeff King]

On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:

> 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 `--color` and encourage `--type=color`,
> `--default` together over `--get-color` alone.

In this last sentence, did you mean "let's introduce --type=color and
encourage its use with --default over --get-color alone"?

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  Documentation/git-config.txt | 10 ++++++----
>  builtin/config.c             | 21 +++++++++++++++++++++
>  t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 620492d1e..bde702d2e 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If
>  you want to handle the lines that do *not* match the regex, just
>  prepend a single exclamation mark in front (see also <<EXAMPLES>>).
>  
> -A type specifier may be given as an argument to `--type` to make 'git config'
> -ensure that the variable(s) are of the given type and convert the value to the
> -canonical form. If no type specifier is passed, no checks or transformations are
> -performed on the value.
> +`color`::
> +    The value is taken as an ANSI color escape sequence.

We'd want to keep that introductory paragraph, right? And there is no
`--color`? So I think this hunk can go away (and is presumably a
leftover mistake during rebasing).

>  When reading, the values are read from the system, global and
>  repository local configuration files by default, and options
> @@ -177,6 +175,7 @@ Valid `<type>`'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': canonicalize by converting to an ANSI color escape sequence.
>  +

This one is part of the --type list, so that's what we expect.

You may want to also cover the behavior when setting the value (we check
that it's sane, but store the original).

> diff --git a/builtin/config.c b/builtin/config.c
> index 1328b568b..aa3fcabe9 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

Not strictly necessary for this series, but if this became an enum as
part of the de-bitifying, you wouldn't have to write the numbers
manually. :)

> @@ -203,6 +206,11 @@ 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 (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 {

OK, formatting shows the converted value. Good.

> @@ -348,6 +356,19 @@ 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))
> +			/*
> +			 * 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("cannot parse color '%s'", value);
> +	}

And this returns the original. Good.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index b25ab7b9e..c630bdc77 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh

The tests look good.

-Peff

Re: [PATCH v4 0/3] Teach `--default` to `git-config(1)`

[Jeff King]

On Wed, Apr 04, 2018 at 07:58:45PM -0700, Taylor Blau wrote:

> Hi,
> 
> Attached is a fourth re-roll of my series to add "--default" and
> "--type=color" to "git config" in order to replace:
> 
>   $ git config --get-color <variable> [default]
> 
> with
> 
>   $ git config [--default=<default>] --type=color <variable>

I think this is getting close, but I found a few minor problems to
address.

-Peff

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--type=color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.
> [...]
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -235,6 +235,10 @@ Valid `<type>`'s include:
> +--default value::

This should be typeset as: --default <value>

> +  When using `--get`, and the requested slot is not found, behave as if value
> +  were the value assigned to the that slot.

And you might use "behaves as if <value>" in the body as well (though
I think Git documentation isn't terribly consistent about typesetting
as "<value>" in the body, so I don't insist upon it).

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

[Eric Sunshine]

On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
>> +     if (type == TYPE_COLOR) {
>> +             char v[COLOR_MAXLEN];
>> +             if (!git_config_color(v, key, 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("cannot parse color '%s'", value);
>> +     }
>
> And this returns the original. Good.

It's entirely subjective, but I find this easier to read and
understand when written like this:

    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);

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

[Jeff King]

On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:

> On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
> >> +     if (type == TYPE_COLOR) {
> >> +             char v[COLOR_MAXLEN];
> >> +             if (!git_config_color(v, key, 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("cannot parse color '%s'", value);
> >> +     }
> >
> > And this returns the original. Good.
> 
> It's entirely subjective, but I find this easier to read and
> understand when written like this:
> 
>     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);

It may be subjective, but I happen to agree with you.

-Peff

[PATCH v5 0/2] *** SUBJECT HERE ***

[Taylor Blau]

*** BLURB HERE ***

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 74 +++++++++++++++++++---------------
 builtin/config.c             | 77 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 121 insertions(+), 59 deletions(-)

-- 
2.17.0

[PATCH v5 0/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

Hi,

(Apologies for the noise of accidentally sending an empty template cover
letter. Here is the real one :-).)

Attached is my fifth, and anticipated final re-roll of my series to add
"--type=<type>" to "git config" in an effort to replace "--bool" with
"--type=bool".

I have changed the following since v4:

  - Remove extra space when redirecting in t1300 (cc: Peff).

  - Clarify the effect of using "--type" in Documentation (cc: Eric).

  - Document "--no-type" in Documentation (cc: Eric).

  - Fix an outstanding typo in Documentation, "input output" (cc: Peff).

Thanks as always for your feedback -- I look forward to having this queued.


Thanks,
Taylor

Taylor Blau (2):
  builtin/config.c: treat type specifiers singularly
  builtin/config.c: prefer `--type=bool` over `--bool`, etc.

 Documentation/git-config.txt | 74 +++++++++++++++++++---------------
 builtin/config.c             | 77 +++++++++++++++++++++++-------------
 t/t1300-repo-config.sh       | 29 ++++++++++++++
 3 files changed, 121 insertions(+), 59 deletions(-)

--
2.17.0

[PATCH v5 1/2] builtin/config.c: treat type specifiers singularly

[Taylor Blau]

Internally, we represent `git config`'s type specifiers as a bitset
using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
allows for the representation of multiple type specifiers in the `int
types` field, but this multi-representation is left unused.

In fact, `git config` will not accept multiple type specifiers at a
time, as indicated by:

  $ git config --int --bool some.section
  error: only one type at a time.

This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
specifier, so that the above command would instead be valid, and a
synonym of:

  $ git config --bool some.section

This change is motivated by two urges: (1) it does not make sense to
represent a singular type specifier internally as a bitset, only to
complain when there are multiple bits in the set. `OPT_SET_INT` is more
well-suited to this task than `OPT_BIT` is. (2) a future patch will
introduce `--type=<type>`, and we would like not to complain in the
following situation:

  $ git config --int --type=int

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 01169dd62..92fb8d56b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
-	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+	OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+	OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+	OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+	OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store
@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)
@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;
@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4f8e6f5fd..24de37d54 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.17.0

[PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

`git config` has long allowed the ability for callers to provide a 'type
specifier', which instructs `git config` to (1) ensure that incoming
values are satisfiable under that type, and (2) that outgoing values are
canonicalized under that type.

In another series, we propose to add extend this functionality with
`--type=color` and `--default` to replace `--get-color`.

However, we traditionally use `--color` to mean "colorize this output",
instead of "this value should be treated as a color".

Currently, `git config` does not support this kind of colorization, but
we should be careful to avoid inhabiting this option too soon, so that
`git config` can support `--type=color` (in the traditional sense) in
the future, if that is desired.

In this patch, we prefer `--type=<int|bool|bool-or-int|...>` over
`--int`, `--bool`, and etc. This allows the aforementioned upcoming
patch to support querying a color value with a default via `--type=color
--default=....`, without squandering `--color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt | 74 ++++++++++++++++++++----------------
 builtin/config.c             | 28 ++++++++++++++
 t/t1300-repo-config.sh       | 18 +++++++++
 3 files changed, 88 insertions(+), 32 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e09ed5d7d..31931ae00 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type] --add name value
+'git config' [<file-option>] [--type] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
@@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type` option requests 'git config' to ensure that the configured values
+assosciated with the given variable(s) are of the given type. When given
+`--type`, 'git config' will convert the value to that type's canonical form.
+ensure that the variable(s) are of the given type and convert the value to the
+canonical form. If no type specifier is passed, no checks or transformations are
+performed on the value. Callers may unset any pre-existing type specifiers with
+`--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
@@ -160,30 +161,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 prior to output.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` 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.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::
diff --git a/builtin/config.c b/builtin/config.c
index 92fb8d56b..6e1495eac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -61,6 +61,33 @@ static int show_origin;
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
 
+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int *type = opt->value;
+
+	if (unset) {
+		*type = 0;
+		return 0;
+	}
+
+	if (!strcmp(arg, "bool"))
+		*type = TYPE_BOOL;
+	else if (!strcmp(arg, "int"))
+		*type = TYPE_INT;
+	else if (!strcmp(arg, "bool-or-int"))
+		*type = TYPE_BOOL_OR_INT;
+	else if (!strcmp(arg, "path"))
+		*type = TYPE_PATH;
+	else if (!strcmp(arg, "expiry-date"))
+		*type = TYPE_EXPIRY_DATE;
+	else {
+		die(_("unrecognized --type argument, %s"), arg);
+		return 1;
+	}
+	return 0;
+}
+
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),
 	OPT_BOOL(0, "global", &use_global_config, N_("use global config file")),
@@ -84,6 +111,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
+	OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
 	OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
 	OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
 	OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 24de37d54..f5ae80e9a 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '
 
 cat >.git/config <<-\EOF &&
 [core]
+foo = true
 number = 10
 EOF
 
@@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '
 	test_cmp expect actual
 '
 
+test_expect_success '--type allows valid type specifiers' '
+	echo "true" >expect &&
+	git config --type=bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--no-type unsets type specifiers' '
+	echo "10" >expect &&
+	git config --type=bool --no-type core.number >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--type rejects unknown specifiers' '
+	test_must_fail git config --type=nonsense core.foo 2>error &&
+	test_i18ngrep "unrecognized --type argument" error
+'
+
 test_done
-- 
2.17.0

Re: [PATCH v5 0/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

On Thu, Apr 05, 2018 at 10:29:01PM -0700, Taylor Blau wrote:
> Hi,

Ack. Clearly I am still getting used to things outside of Git LFS. I have sent
this topic in response to my series to add "--default" instead of the
appropriate series.

I'll avoid re-sending it in an effort to reduce further noise. In the meantime,
thanks for understanding my silly mistakes.


Thanks,
Taylor

Re: [PATCH v5 0/2] *** SUBJECT HERE ***

[Jacob Keller]

On Thu, Apr 5, 2018 at 10:27 PM, Taylor Blau <me@ttaylorr.com> wrote:
> *** BLURB HERE ***
>

Missing subject and cover letter stuff.. did you submit before you
were ready? or did you not mean to include the cover letter? :)

-Jake

> Taylor Blau (2):
>   builtin/config.c: treat type specifiers singularly
>   builtin/config.c: prefer `--type=bool` over `--bool`, etc.
>
>  Documentation/git-config.txt | 74 +++++++++++++++++++---------------
>  builtin/config.c             | 77 +++++++++++++++++++++++-------------
>  t/t1300-repo-config.sh       | 29 ++++++++++++++
>  3 files changed, 121 insertions(+), 59 deletions(-)
>
> --
> 2.17.0

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:29:49PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
> >  	config_with_options(collect_config, &values,
> >  			    &given_config_source, &config_options);
> >
> > +	if (!values.nr && default_value) {
> > +		struct strbuf *item;
> > +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> > +		item = &values.items[values.nr++];
> > +		strbuf_init(item, 0);
> > +		if (format_config(item, key_, default_value) < 0) {
> > +			exit(1);
> > +		}
> > +	}
>
> Calling exit() explicitly is unusual for our code. Usually we would
> either die() or propagate the error. Most of the types in
> format_config() would die on bogus input, but a few code paths will
> return an error.
>
> What happens if a non-default value has a bogus format? E.g.:
>
>   $ git config foo.bar '~NoSuchUser'
>   $ git config --path foo.bar
>   fatal: failed to expand user dir in: '~NoSuchUser'
>
> Oops. Despite us checking for an error return from
> git_config_pathname(), it will always either return 0 or die(). So
> that's not a good example. ;)
>
> Let's try expiry-date:
>
>   $ git config foo.bar 'the first of octember'
>   $ git config --expiry-date foo.bar
>   error: 'the first of octember' for 'foo.bar' is not a valid timestamp
>   fatal: bad config line 7 in file .git/config
>
> OK. So we call format_config() there from the actual collect_config()
> callback, and the error gets propagated back to the config parser, which
> then gives us an informative die(). What happens with your new code:
>
>   $ ./git config --default 'the first of octember' --type=expiry-date no.such.key
>   error: 'the first of octember' for 'no.such.key' is not a valid timestamp
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Aha. Thanks for the explanation :-). I've removed the exit() and changed
it to the die() that you suggested above. The test in t1310 is updated
to grep for the new message, so we know that it is being reported
appropriately.

> > +test_expect_success 'does not allow --default without --get' '
> > +	test_must_fail git config --default quux --unset a >output 2>&1 &&
> > +	test_i18ngrep "\-\-default is only applicable to" output
> > +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

Aha, thanks again. I have updated this in the forthcoming re-roll.


Thanks,
Taylor

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:40:03PM -0400, Eric Sunshine wrote:
> On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > This commit (and those following it in this series) aim to eventually
> > replace `--get-color` with a consistent alternative. By introducing
> > `--default`, we allow the `--get-color` action to be promoted to a
> > `--type=color` type specifier, retaining the "fallback" behavior via the
> > `--default` flag introduced in this commit.
> > [...]
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -235,6 +235,10 @@ Valid `<type>`'s include:
> > +--default value::
>
> This should be typeset as: --default <value>

I see; thank you for letting me know. I have updated this in the
forthcoming re-roll.

> > +  When using `--get`, and the requested slot is not found, behave as if value
> > +  were the value assigned to the that slot.
>
> And you might use "behaves as if <value>" in the body as well (though
> I think Git documentation isn't terribly consistent about typesetting
> as "<value>" in the body, so I don't insist upon it).

Since I'm here, and re-rolling anyway, I have included this change as
well.


Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:36:20PM -0400, Jeff King wrote:
> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
>
> > 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 `--color` and encourage `--type=color`,
> > `--default` together over `--get-color` alone.
>
> In this last sentence, did you mean "let's introduce --type=color and
> encourage its use with --default over --get-color alone"?

I did; thank you :-). I have updated this in the forthcoming re-roll.

> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  Documentation/git-config.txt | 10 ++++++----
> >  builtin/config.c             | 21 +++++++++++++++++++++
> >  t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
> >  3 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > index 620492d1e..bde702d2e 100644
> > --- a/Documentation/git-config.txt
> > +++ b/Documentation/git-config.txt
> > @@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If
> >  you want to handle the lines that do *not* match the regex, just
> >  prepend a single exclamation mark in front (see also <<EXAMPLES>>).
> >
> > -A type specifier may be given as an argument to `--type` to make 'git config'
> > -ensure that the variable(s) are of the given type and convert the value to the
> > -canonical form. If no type specifier is passed, no checks or transformations are
> > -performed on the value.
> > +`color`::
> > +    The value is taken as an ANSI color escape sequence.
>
> We'd want to keep that introductory paragraph, right? And there is no
> `--color`? So I think this hunk can go away (and is presumably a
> leftover mistake during rebasing).

Yes; this entire hunk should not have been staged. I think that it was a
leftover from rebasing. I have removed it in the forthcoming re-roll.

> >  When reading, the values are read from the system, global and
> >  repository local configuration files by default, and options
> > @@ -177,6 +175,7 @@ Valid `<type>`'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': canonicalize by converting to an ANSI color escape sequence.
> >  +
>
> This one is part of the --type list, so that's what we expect.
>
> You may want to also cover the behavior when setting the value (we check
> that it's sane, but store the original).

Good idea; thank you.

> > diff --git a/builtin/config.c b/builtin/config.c
> > index 1328b568b..aa3fcabe9 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
>
> Not strictly necessary for this series, but if this became an enum as
> part of the de-bitifying, you wouldn't have to write the numbers
> manually. :)

Certainly agree that this would be useful. I think that I'd prefer to
accomplish this in a future series, since this one is starting to become
long-lived :-).

> > @@ -203,6 +206,11 @@ 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 (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 {
>
> OK, formatting shows the converted value. Good.
>
> > @@ -348,6 +356,19 @@ 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))
> > +			/*
> > +			 * 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("cannot parse color '%s'", value);
> > +	}
>
> And this returns the original. Good.
>
> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index b25ab7b9e..c630bdc77 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
>
> The tests look good.

Thank you.


Thanks,
Taylor

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

[Taylor Blau]

On Thu, Apr 05, 2018 at 06:53:20PM -0400, Jeff King wrote:
> On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:
>
> > On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:
> > > On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
> > >> +     if (type == TYPE_COLOR) {
> > >> +             char v[COLOR_MAXLEN];
> > >> +             if (!git_config_color(v, key, 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("cannot parse color '%s'", value);
> > >> +     }
> > >
> > > And this returns the original. Good.
> >
> > It's entirely subjective, but I find this easier to read and
> > understand when written like this:
> >
> >     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);
>
> It may be subjective, but I happen to agree with you.

I agree, too :-). I have changed this in the forthcoming re-roll.

Thanks,
Taylor

Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Eric Sunshine]

On Fri, Apr 6, 2018 at 1:29 AM, Taylor Blau <me@ttaylorr.com> wrote:
> builtin/config.c: prefer `--type=bool` over `--bool`, etc.

This patch isn't just preferring --type; it's actually introducing the
functionality. Perhaps:

    builtin/config.c: support --type=<type> as preferred alias for --<type>

(Not worth a re-roll.)

> `git config` has long allowed the ability for callers to provide a 'type
> specifier', which instructs `git config` to (1) ensure that incoming
> values are satisfiable under that type, and (2) that outgoing values are
> canonicalized under that type.
>
> In another series, we propose to add extend this functionality with

"add extend"?

> `--type=color` and `--default` to replace `--get-color`.
>
> However, we traditionally use `--color` to mean "colorize this output",
> instead of "this value should be treated as a color".
>
> Currently, `git config` does not support this kind of colorization, but
> we should be careful to avoid inhabiting this option too soon, so that
> `git config` can support `--type=color` (in the traditional sense) in
> the future, if that is desired.
>
> In this patch, we prefer `--type=<int|bool|bool-or-int|...>` over
> `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> patch to support querying a color value with a default via `--type=color
> --default=....`, without squandering `--color`.

Drop one of the periods in "...." if you happen to re-roll for some reason.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -9,13 +9,13 @@ git-config - Get and set repository or global options
>  SYNOPSIS
>  --------
>  [verse]
> -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
> +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]

It's pretty confusing to see bare "--type" like this which makes it
seem as if it doesn't take an argument at all. Better would be
"[--type=<type>]".

> @@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If
> +The `--type` option requests 'git config' to ensure that the configured values
> +assosciated with the given variable(s) are of the given type. When given

s/assosciated/associated/

> +`--type`, 'git config' will convert the value to that type's canonical form.
> +ensure that the variable(s) are of the given type and convert the value to the
> +canonical form.

Meh, looks like some sort of editing botch here. I doubt you meant to
repeat "canonical form" like this.

> If no type specifier is passed, no checks or transformations are
> +performed on the value. Callers may unset any pre-existing type specifiers with
> +`--no-type`.
>
> +--no-type::
> +  Un-sets the previously set type specifier (if one was previously set). This
> +  option requests that 'git config' not canonicalize the retrieved variable.
> +  `--no-type` has no effect without `--type=<type>` or `--<type>`.

The final sentence doesn't really add value; I'd probably drop it as
extraneous. (Subjective, and not worth a re-roll.)

> diff --git a/builtin/config.c b/builtin/config.c
> @@ -61,6 +61,33 @@ static int show_origin;
> +static int option_parse_type(const struct option *opt, const char *arg,
> +                            int unset)
> +{
> +       [...]
> +       if (!strcmp(arg, "bool"))
> +               *type = TYPE_BOOL;
> +       else if (!strcmp(arg, "int"))
> +               *type = TYPE_INT;
> +       else if (!strcmp(arg, "bool-or-int"))
> +               *type = TYPE_BOOL_OR_INT;
> +       else if (!strcmp(arg, "path"))
> +               *type = TYPE_PATH;
> +       else if (!strcmp(arg, "expiry-date"))
> +               *type = TYPE_EXPIRY_DATE;
> +       else {
> +               die(_("unrecognized --type argument, %s"), arg);
> +               return 1;

Pointless unreachable 'return'.

> +       }
> +       return 0;
> +}

[PATCH v5 0/3] builtin/config: introduce `--default`

[Taylor Blau]

Hi,

Attached is the fifth re-roll of my series to add "--default" and
"--type=color" to "git config" in order to introduce a consistency
between type-specifiers and the historic "--get-color".

I have changed the following since the last series:

  - Replace exit() with die() in the case where the given "--default"
    cannot be parsed. (cc: Peff).

  - Fixed incorrectly calling "git config" in t1310. (cc: Peff).

  - Correct typesetting in Documentation (cc: Eric).

  - Perform guard checking in a negative context (cc: Eric & Peff).

  - Add notes to Documentation stating when we do and do not sanitize
    values into a ".gitconfig". (cc: Peff).

  - Remove a hunk left over during rebasing (cc: Peff).

  - Replace one last outdated reference to "--color" (cc: Peff).

Thanks as always for your review :-).


Thanks,
Taylor

Taylor Blau (3):
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt | 11 ++++++++++
 builtin/config.c             | 41 ++++++++++++++++++++++++++++++++++++
 config.c                     | 10 +++++++++
 config.h                     |  1 +
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++
 t/t1310-config-default.sh    | 38 +++++++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+)
 create mode 100755 t/t1310-config-default.sh

--
2.17.0

[PATCH v5 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 19 ++++++++++++++++++
 t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 31931ae00..28567f75a 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -243,6 +243,10 @@ Valid `<type>`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default <value>::
+  When using `--get`, and the requested variable is not found, behave as if
+  <value> were the value assigned to the that variable.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 6e1495eac..e8da4009e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -122,6 +123,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };
 
@@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0) {
+			die(_("failed to format default config value"));
+		}
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
@@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage,
+			builtin_config_options);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 000000000..b74c93276
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when missing entry' '
+	echo quux >expect &&
+	git config -f config --default quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=true --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'uses entry when available' '
+	echo bar >expect &&
+	git config --add core.foo bar &&
+	git config --default baz core.foo >actual &&
+	git config --unset core.foo &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=expiry-date --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "failed to format default config value" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.17.0

[PATCH v5 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--type=color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb..33366b52c 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac..0e060779d 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.17.0

[PATCH v5 3/3] 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 <me@ttaylorr.com>
---
 Documentation/git-config.txt |  7 +++++++
 builtin/config.c             | 22 ++++++++++++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 28567f75a..b49366a56 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -180,6 +180,10 @@ Valid `<type>`'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 writing to a caller, canonicalize by converting to an ANSI color
+  escape sequence. When writing to the configuration file, 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::
@@ -231,6 +235,9 @@ Valid `<type>`'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`.
++
+It is preferred to use `--type=color`, or `--type=color [--default=<default>]`
+instead of `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index e8da4009e..77f8cc25f 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
 
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		*type = TYPE_PATH;
 	else if (!strcmp(arg, "expiry-date"))
 		*type = TYPE_EXPIRY_DATE;
+	else if (!strcmp(arg, "color"))
+		*type = TYPE_COLOR;
 	else {
 		die(_("unrecognized --type argument, %s"), arg);
 		return 1;
@@ -203,6 +206,11 @@ 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 (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 {
@@ -348,6 +356,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 f5ae80e9a..c49473306 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.raw >actual &&
+	echo "<RED>" >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

Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 1:29 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > builtin/config.c: prefer `--type=bool` over `--bool`, etc.
>
> This patch isn't just preferring --type; it's actually introducing the
> functionality. Perhaps:
>
>     builtin/config.c: support --type=<type> as preferred alias for --<type>
>
> (Not worth a re-roll.)

Thanks; I like this more, even though it is lengthy.

> > `git config` has long allowed the ability for callers to provide a 'type
> > specifier', which instructs `git config` to (1) ensure that incoming
> > values are satisfiable under that type, and (2) that outgoing values are
> > canonicalized under that type.
> >
> > In another series, we propose to add extend this functionality with
>
> "add extend"?

Thanks for pointing this out -- I fixed it in v6.

> > `--type=color` and `--default` to replace `--get-color`.
> >
> > However, we traditionally use `--color` to mean "colorize this output",
> > instead of "this value should be treated as a color".
> >
> > Currently, `git config` does not support this kind of colorization, but
> > we should be careful to avoid inhabiting this option too soon, so that
> > `git config` can support `--type=color` (in the traditional sense) in
> > the future, if that is desired.
> >
> > In this patch, we prefer `--type=<int|bool|bool-or-int|...>` over
> > `--int`, `--bool`, and etc. This allows the aforementioned upcoming
> > patch to support querying a color value with a default via `--type=color
> > --default=....`, without squandering `--color`.
>
> Drop one of the periods in "...." if you happen to re-roll for some reason.

Great catch, thanks.

> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
> > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
>
> It's pretty confusing to see bare "--type" like this which makes it
> seem as if it doesn't take an argument at all. Better would be
> "[--type=<type>]".

Agreed; I have made this change in v6.

> > @@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If
> > +The `--type` option requests 'git config' to ensure that the configured values
> > +assosciated with the given variable(s) are of the given type. When given
>
> s/assosciated/associated/

Ditto.

> > +`--type`, 'git config' will convert the value to that type's canonical form.
> > +ensure that the variable(s) are of the given type and convert the value to the
> > +canonical form.
>
> Meh, looks like some sort of editing botch here. I doubt you meant to
> repeat "canonical form" like this.

Ditto.

> > If no type specifier is passed, no checks or transformations are
> > +performed on the value. Callers may unset any pre-existing type specifiers with
> > +`--no-type`.
> >
> > +--no-type::
> > +  Un-sets the previously set type specifier (if one was previously set). This
> > +  option requests that 'git config' not canonicalize the retrieved variable.
> > +  `--no-type` has no effect without `--type=<type>` or `--<type>`.
>
> The final sentence doesn't really add value; I'd probably drop it as
> extraneous. (Subjective, and not worth a re-roll.)

Hm. I like the completeness of it -- I think that I would prefer to
leave it in, unless others have strong feelings.

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -61,6 +61,33 @@ static int show_origin;
> > +static int option_parse_type(const struct option *opt, const char *arg,
> > +                            int unset)
> > +{
> > +       [...]
> > +       if (!strcmp(arg, "bool"))
> > +               *type = TYPE_BOOL;
> > +       else if (!strcmp(arg, "int"))
> > +               *type = TYPE_INT;
> > +       else if (!strcmp(arg, "bool-or-int"))
> > +               *type = TYPE_BOOL_OR_INT;
> > +       else if (!strcmp(arg, "path"))
> > +               *type = TYPE_PATH;
> > +       else if (!strcmp(arg, "expiry-date"))
> > +               *type = TYPE_EXPIRY_DATE;
> > +       else {
> > +               die(_("unrecognized --type argument, %s"), arg);
> > +               return 1;
>
> Pointless unreachable 'return'.

Removed -- this is a habit of mine from Git LFS. We have a function
similar to die(), but Go doesn't know that it will terminate the
program, so it forces me to write a "return" after it.


Thanks,
Taylor

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> This commit (and those following it in this series) aim to eventually
> replace `--get-color` with a consistent alternative. By introducing
> `--default`, we allow the `--get-color` action to be promoted to a
> `--type=color` type specifier, retaining the "fallback" behavior via the
> `--default` flag introduced in this commit.
>
> For example, we aim to replace:
>
>   $ git config --get-color variable [default] [...]
>
> with:
>
>   $ git config --default default --type=color variable [...]
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
> +       if (!values.nr && default_value) {
> +               struct strbuf *item;
> +               ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> +               item = &values.items[values.nr++];
> +               strbuf_init(item, 0);
> +               if (format_config(item, key_, default_value) < 0) {
> +                       die(_("failed to format default config value"));
> +               }

Unnecessary braces.

Also, an error message such as this is typically more helpful when it
shows the item which caused the problem:

    die(_("failed to format default config value: %s"), default_value);

However, since the message already says "default", it's pretty easy to
understand that it's talking about the argument to --default, so it's
perhaps not such a big deal in this case.

Anyhow, neither point is worth a re-roll.

> +       }
> @@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>                 usage_with_options(builtin_config_usage, builtin_config_options);
>         }
>
> +

Unnecessary new blank line.

> +       if (default_value && !(actions & ACTION_GET)) {
> +               error("--default is only applicable to --get");
> +               usage_with_options(builtin_config_usage,
> +                       builtin_config_options);
> +       }
> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
> @@ -0,0 +1,38 @@
> +test_expect_success 'uses --default when missing entry' '
> +       echo quux >expect &&
> +       git config -f config --default quux core.foo >actual &&
> +       test_cmp expect actual
> +'
>
> +test_expect_success 'canonicalizes --default with appropriate type' '
> +       echo true >expect &&
> +       git config -f config --default=true --bool core.foo >actual &&
> +       test_cmp expect actual
> +'

I would trust this canonicalization test a lot more if it used one of
the aliases for "true" since it doesn't presently prove that
canonicalization is taking place. For instance:

    git config -f config --default=yes --bool core.foo >actual &&

> +test_expect_success 'uses entry when available' '
> +       echo bar >expect &&
> +       git config --add core.foo bar &&
> +       git config --default baz core.foo >actual &&
> +       git config --unset core.foo &&
> +       test_cmp expect actual
> +'

If you happen to re-roll, can we move this test so it immediately
follows the "uses --default when missing entry" test? That's where I
had expected to find it and had even written a review comment saying
so (but deleted the comment once I got down to this spot in the
patch). Also, perhaps rename the test to make it clearer that it
complements the "uses --default when missing entry" test; perhaps
"does not fallback to --default when entry present" or something.

> +test_expect_success 'dies when --default cannot be parsed' '
> +       test_must_fail git config -f config --type=expiry-date --default=x --get \
> +               not.a.section 2>error &&
> +       test_i18ngrep "failed to format default config value" error
> +'
> +
> +test_expect_success 'does not allow --default without --get' '
> +       test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
> +       test_i18ngrep "\-\-default is only applicable to" output
> +'

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

[Eric Sunshine]

On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> [...]
> For consistency, let's introduce `--type=color` and encourage its use
> with `--default` together over `--get-color` alone.

A couple minor subjective comments below... neither worth a re-roll.

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -180,6 +180,10 @@ Valid `<type>`'s include:
> +- 'color': When writing to a caller, canonicalize by converting to an ANSI color
> +  escape sequence. When writing to the configuration file, a sanity-check is
> +  performed to ensure that the given value is canonicalize-able as an ANSI
> +  color, but it is written as-is.

"When writing to a caller" is a somewhat confusing way to say "When
getting a value".

Likewise, "When writing to the configuration file" could be stated
more concisely as "When setting a value".

>  --bool::
> @@ -231,6 +235,9 @@ Valid `<type>`'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`.
> ++
> +It is preferred to use `--type=color`, or `--type=color [--default=<default>]`
> +instead of `--get-color`.

Repetitious. More conscisely:

    It is preferred to use `--type=color [--default=<value>]`
    instead of `--get-color`.

Or, even:

    `--type=color [--default=<value>]` is preferred over `--get-color`.

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
>> +test_expect_success 'uses entry when available' '
>> +       echo bar >expect &&
>> +       git config --add core.foo bar &&
>> +       git config --default baz core.foo >actual &&
>> +       git config --unset core.foo &&
>> +       test_cmp expect actual
>> +'
>
> If you happen to re-roll, can we move this test so it immediately
> follows the "uses --default when missing entry" test?

One other issue. If "git config --default ..." fails, the --unset line
will never be invoked, thus cleanup won't happen.

To ensure cleanup, either use:

    test_when_finished "git config --unset core.foo" &&

earlier in the test (just after the --add line) or use the
test_config() function to both set the value and ensure its cleanup.

Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Jeff King]

On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:

> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options
> >  SYNOPSIS
> >  --------
> >  [verse]
> > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
> > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
> 
> It's pretty confusing to see bare "--type" like this which makes it
> seem as if it doesn't take an argument at all. Better would be
> "[--type=<type>]".

In the interest of brevity, I actually think it could remain "[type]".
IMHO the synopsis should be giving a summary and does not have to
mention every detail (as long as it is not _inaccurate_, which I agree
that "--type" is bordering on).

-Peff

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

[Taylor Blau]

On Fri, Apr 06, 2018 at 03:29:48AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > For consistency, let's introduce `--type=color` and encourage its use
> > with `--default` together over `--get-color` alone.
>
> A couple minor subjective comments below... neither worth a re-roll.
>
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -180,6 +180,10 @@ Valid `<type>`'s include:
> > +- 'color': When writing to a caller, canonicalize by converting to an ANSI color
> > +  escape sequence. When writing to the configuration file, a sanity-check is
> > +  performed to ensure that the given value is canonicalize-able as an ANSI
> > +  color, but it is written as-is.
>
> "When writing to a caller" is a somewhat confusing way to say "When
> getting a value".
>
> Likewise, "When writing to the configuration file" could be stated
> more concisely as "When setting a value".

Thanks, I think that these are much clearer, especially when used
together. I have updated this in my local copy, so it will be picked up
when I send the next re-roll.

> >  --bool::
> > @@ -231,6 +235,9 @@ Valid `<type>`'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`.
> > ++
> > +It is preferred to use `--type=color`, or `--type=color [--default=<default>]`
> > +instead of `--get-color`.
>
> Repetitious. More conscisely:
>
>     It is preferred to use `--type=color [--default=<value>]`
>     instead of `--get-color`.
>
> Or, even:
>
>     `--type=color [--default=<value>]` is preferred over `--get-color`.

Much clearer; I think that I prefer the second one. I have rolled this
into my local changes as above.


Thanks,
Taylor

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Fri, Apr 06, 2018 at 02:53:45AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > This commit (and those following it in this series) aim to eventually
> > replace `--get-color` with a consistent alternative. By introducing
> > `--default`, we allow the `--get-color` action to be promoted to a
> > `--type=color` type specifier, retaining the "fallback" behavior via the
> > `--default` flag introduced in this commit.
> >
> > For example, we aim to replace:
> >
> >   $ git config --get-color variable [default] [...]
> >
> > with:
> >
> >   $ git config --default default --type=color variable [...]
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
> > +       if (!values.nr && default_value) {
> > +               struct strbuf *item;
> > +               ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> > +               item = &values.items[values.nr++];
> > +               strbuf_init(item, 0);
> > +               if (format_config(item, key_, default_value) < 0) {
> > +                       die(_("failed to format default config value"));
> > +               }
>
> Unnecessary braces.

Ah, that's leftover from changing this around in your last round of
review. Cleaned up locally.

> Also, an error message such as this is typically more helpful when it
> shows the item which caused the problem:
>
>     die(_("failed to format default config value: %s"), default_value);
>
> However, since the message already says "default", it's pretty easy to
> understand that it's talking about the argument to --default, so it's
> perhaps not such a big deal in this case.
>
> Anyhow, neither point is worth a re-roll.

I like including the value of default_value. I've included it locally,

>
> > +       }
> > @@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> >                 usage_with_options(builtin_config_usage, builtin_config_options);
> >         }
> >
> > +
>
> Unnecessary new blank line.
>
> > +       if (default_value && !(actions & ACTION_GET)) {
> > +               error("--default is only applicable to --get");
> > +               usage_with_options(builtin_config_usage,
> > +                       builtin_config_options);
> > +       }
> > diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
> > @@ -0,0 +1,38 @@
> > +test_expect_success 'uses --default when missing entry' '
> > +       echo quux >expect &&
> > +       git config -f config --default quux core.foo >actual &&
> > +       test_cmp expect actual
> > +'
> >
> > +test_expect_success 'canonicalizes --default with appropriate type' '
> > +       echo true >expect &&
> > +       git config -f config --default=true --bool core.foo >actual &&
> > +       test_cmp expect actual
> > +'
>
> I would trust this canonicalization test a lot more if it used one of
> the aliases for "true" since it doesn't presently prove that
> canonicalization is taking place. For instance:
>
>     git config -f config --default=yes --bool core.foo >actual &&
>
> > +test_expect_success 'uses entry when available' '
> > +       echo bar >expect &&
> > +       git config --add core.foo bar &&
> > +       git config --default baz core.foo >actual &&
> > +       git config --unset core.foo &&
> > +       test_cmp expect actual
> > +'
>
> If you happen to re-roll, can we move this test so it immediately
> follows the "uses --default when missing entry" test? That's where I
> had expected to find it and had even written a review comment saying
> so (but deleted the comment once I got down to this spot in the
> patch). Also, perhaps rename the test to make it clearer that it
> complements the "uses --default when missing entry" test; perhaps
> "does not fallback to --default when entry present" or something.

That location makes much more sense, as does using --default=yes to
ensure that canonicalization is taking place. I've updated that locally,
as well as the preceding test to make it clearer that they are
contrasting tests:

	- 'falls back to --default when missing entry'
	- 'does not fallback to --default when entry present'

Though I am not sure about "falls back" to mean "uses --default". I am
not sure which to pick here... what are your thoughts?

Thanks,
Taylor

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Fri, Apr 06, 2018 at 03:40:56AM -0400, Eric Sunshine wrote:
> On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
> >> +test_expect_success 'uses entry when available' '
> >> +       echo bar >expect &&
> >> +       git config --add core.foo bar &&
> >> +       git config --default baz core.foo >actual &&
> >> +       git config --unset core.foo &&
> >> +       test_cmp expect actual
> >> +'
> >
> > If you happen to re-roll, can we move this test so it immediately
> > follows the "uses --default when missing entry" test?
>
> One other issue. If "git config --default ..." fails, the --unset line
> will never be invoked, thus cleanup won't happen.
>
> To ensure cleanup, either use:
>
>     test_when_finished "git config --unset core.foo" &&
>
> earlier in the test (just after the --add line) or use the
> test_config() function to both set the value and ensure its cleanup.

Neat. I wasn't aware of 'test_when_finished'. I think that I prefer the
explicitness of 'test_when_finished "git config ..."' over
'test_config()', but I am happy to use whichever is preferred. Since
t1310 is new, there's no historical precedent to follow. Please let me
know if you have a preference one way or another.

Thanks,
Taylor

Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.

[Taylor Blau]

On Fri, Apr 06, 2018 at 10:55:18AM -0400, Jeff King wrote:
> On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:
>
> > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options
> > >  SYNOPSIS
> > >  --------
> > >  [verse]
> > > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
> > > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]
> >
> > It's pretty confusing to see bare "--type" like this which makes it
> > seem as if it doesn't take an argument at all. Better would be
> > "[--type=<type>]".
>
> In the interest of brevity, I actually think it could remain "[type]".
> IMHO the synopsis should be giving a summary and does not have to
> mention every detail (as long as it is not _inaccurate_, which I agree
> that "--type" is bordering on).

I think I am OK with the "non-brevity" of "[--type=<type>]", since
"--type" alone does not make sense.

I agree that it would still be fairly clear even if that's the way that
it did end up being typeset, but I prefer the accuracy of its current
state.

That said, I am happy to bend towards any strong feelings about this
topic, but for now I will leave it as-is.


Thanks,
Taylor

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Fri, Apr 6, 2018 at 8:49 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Fri, Apr 06, 2018 at 02:53:45AM -0400, Eric Sunshine wrote:
>> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:
>> > +test_expect_success 'uses --default when missing entry' '
>> > +       echo quux >expect &&
>> > +       git config -f config --default quux core.foo >actual &&
>> > +       test_cmp expect actual
>> > +'
>> >
>> > +test_expect_success 'uses entry when available' '
>> > +       echo bar >expect &&
>> > +       git config --add core.foo bar &&
>> > +       git config --default baz core.foo >actual &&
>> > +       git config --unset core.foo &&
>> > +       test_cmp expect actual
>> > +'
>>
>> If you happen to re-roll, can we move this test so it immediately
>> follows the "uses --default when missing entry" test? That's where I
>> had expected to find it and had even written a review comment saying
>> so (but deleted the comment once I got down to this spot in the
>> patch). Also, perhaps rename the test to make it clearer that it
>> complements the "uses --default when missing entry" test; perhaps
>> "does not fallback to --default when entry present" or something.
>
> That location makes much more sense, as does using --default=yes to
> ensure that canonicalization is taking place. I've updated that locally,
> as well as the preceding test to make it clearer that they are
> contrasting tests:
>
>         - 'falls back to --default when missing entry'
>         - 'does not fallback to --default when entry present'
>
> Though I am not sure about "falls back" to mean "uses --default". I am
> not sure which to pick here... what are your thoughts?

It's nice for the titles to show that the two tests complement one
another but the exact wording is not terribly important. Taking your
original test title (slightly modified), perhaps:

    - 'uses --default when entry missing'
    - 'does not use --default when entry present'

Re: [PATCH v5 1/3] builtin/config: introduce `--default`

[Eric Sunshine]

On Fri, Apr 6, 2018 at 8:58 PM, Taylor Blau <me@ttaylorr.com> wrote:
> On Fri, Apr 06, 2018 at 03:40:56AM -0400, Eric Sunshine wrote:
>> On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> One other issue. If "git config --default ..." fails, the --unset line
>> will never be invoked, thus cleanup won't happen.
>>
>> To ensure cleanup, either use:
>>
>>     test_when_finished "git config --unset core.foo" &&
>>
>> earlier in the test (just after the --add line) or use the
>> test_config() function to both set the value and ensure its cleanup.
>
> Neat. I wasn't aware of 'test_when_finished'. I think that I prefer the
> explicitness of 'test_when_finished "git config ..."' over
> 'test_config()', but I am happy to use whichever is preferred. Since
> t1310 is new, there's no historical precedent to follow. Please let me
> know if you have a preference one way or another.

test_config() is preferred; not only is it shorter but, because it
automates cleanup, there's less opportunity to screw up (like
forgetting to use test_when_finished()).

However, since this config setting needs to be present for only a
single command, you can go even simpler by collapsing the entire test
to:

test_expect_success 'does not use --default when entry present' '
    echo bar >expect &&
    git -c core.foo=bar config --default baz core.foo >actual &&
    test_cmp expect actual
'

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
>
>> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
>>  	config_with_options(collect_config, &values,
>>  			    &given_config_source, &config_options);
>>  
>> +	if (!values.nr && default_value) {
>> +		struct strbuf *item;
>> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
>> +		item = &values.items[values.nr++];
>> +		strbuf_init(item, 0);
>> +		if (format_config(item, key_, default_value) < 0) {
>> +			exit(1);
>> +		}
>> +	}
> ...
>
> It's obvious in this toy example, but that config call may be buried
> deep in a script. It'd probably be nicer for that exit(1) to be
> something like:
>
>   die(_("failed to format default config value"));

Together with key_ and default_value, you mean?

>
>> +test_expect_success 'does not allow --default without --get' '
>> +	test_must_fail git config --default quux --unset a >output 2>&1 &&
>> +	test_i18ngrep "\-\-default is only applicable to" output
>> +'
>
> I think "a" here needs to be "a.section". I get:
>
>   error: key does not contain a section: a

"section.var"?  That aside, even with a properly formatted key, I
seem to get an empty output here, so this may need a bit more work.

[PATCH v6 0/3] Teach `--default` to `git-config(1)`

[Taylor Blau]

Hi,

Attached is the sixth re-roll of my series to add '--type=color' as a valid
option to 'git-config(1)'.

I have changed the following since v5 (an inter-diff is available below for
easier consumption):

  - Update prose in Documentation/git-config.txt to match Eric's
    suggestions.

  - Remove extraneous braces, newlines from builtin/config.c. (cc:
    Eric).

  - Rename, reorder tests in t1310 to make clear which tests are duals
    of one another. Use '-c' to introduce short-lived configuration
    values. (cc: Eric).


Thanks,
Taylor

Taylor Blau (3):
  builtin/config: introduce `--default`
  config.c: introduce 'git_config_color' to parse ANSI colors
  builtin/config: introduce `color` type specifier

 Documentation/git-config.txt | 10 +++++++++
 builtin/config.c             | 40 ++++++++++++++++++++++++++++++++++++
 config.c                     | 10 +++++++++
 config.h                     |  1 +
 t/t1300-repo-config.sh       | 30 +++++++++++++++++++++++++++
 t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++
 6 files changed, 127 insertions(+)
 create mode 100755 t/t1310-config-default.sh

Inter-diff (since v5):

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 05a555b568..7c8365e377 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,10 +177,10 @@ Valid `<type>`'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 writing to a caller, canonicalize by converting to an ANSI color
-  escape sequence. When writing to the configuration file, a sanity-check is
-  performed to ensure that the given value is canonicalize-able as an ANSI
-  color, but it is written as-is.
+- '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::
@@ -233,8 +233,7 @@ Valid `<type>`'s include:
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 +
-It is preferred to use `--type=color`, or `--type=color [--default=<default>]`
-instead of `--get-color`.
+`--type=color [--default=<default>]` is preferred over `--get-color`.

 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 0c7cfcf6c9..08016863ac 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -300,9 +300,9 @@ static int get_value(const char *key_, const char *regex_)
 		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
 		item = &values.items[values.nr++];
 		strbuf_init(item, 0);
-		if (format_config(item, key_, default_value) < 0) {
-			die(_("failed to format default config value"));
-		}
+		if (format_config(item, key_, default_value) < 0)
+			die(_("failed to format default config value: %s"),
+				default_value);
 	}

 	ret = !values.nr;
@@ -657,7 +657,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}

-
 	if (default_value && !(actions & ACTION_GET)) {
 		error("--default is only applicable to --get");
 		usage_with_options(builtin_config_usage,
@@ -814,3 +813,4 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}

 	return 0;
+}
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
index b74c932763..6049d91708 100755
--- a/t/t1310-config-default.sh
+++ b/t/t1310-config-default.sh
@@ -4,23 +4,21 @@ test_description='Test git config in different settings (with --default)'

 . ./test-lib.sh

-test_expect_success 'uses --default when missing entry' '
+test_expect_success 'uses --default when entry missing' '
 	echo quux >expect &&
-	git config -f config --default quux core.foo >actual &&
+	git config -f config --default=quux core.foo >actual &&
 	test_cmp expect actual
 '

-test_expect_success 'canonicalizes --default with appropriate type' '
-	echo true >expect &&
-	git config -f config --default=true --bool core.foo >actual &&
+test_expect_success 'does not use --default when entry present' '
+	echo bar >expect &&
+	git -c core.foo=bar config --default=baz core.foo >actual &&
 	test_cmp expect actual
 '

-test_expect_success 'uses entry when available' '
-	echo bar >expect &&
-	git config --add core.foo bar &&
-	git config --default baz core.foo >actual &&
-	git config --unset core.foo &&
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=yes --bool core.foo >actual &&
 	test_cmp expect actual
 '
--
2.17.0

[PATCH v6 1/3] builtin/config: introduce `--default`

[Taylor Blau]

For some use cases, callers of the `git-config(1)` builtin would like to
fallback to default values when the variable asked for does not exist.
In addition, users would like to use existing type specifiers to ensure
that values are parsed correctly when they do exist in the
configuration.

For example, to fetch a value without a type specifier and fallback to
`$fallback`, the following is required:

  $ git config core.foo || echo "$fallback"

This is fine for most values, but can be tricky for difficult-to-express
`$fallback`'s, like ANSI color codes.

This motivates `--get-color`, which is a one-off exception to the normal
type specifier rules wherein a user specifies both the configuration
variable and an optional fallback. Both are formatted according to their
type specifier, which eases the burden on the user to ensure that values
are correctly formatted.

This commit (and those following it in this series) aim to eventually
replace `--get-color` with a consistent alternative. By introducing
`--default`, we allow the `--get-color` action to be promoted to a
`--type=color` type specifier, retaining the "fallback" behavior via the
`--default` flag introduced in this commit.

For example, we aim to replace:

  $ git config --get-color variable [default] [...]

with:

  $ git config --default default --type=color variable [...]

Values filled by `--default` behave exactly as if they were present in
the affected configuration file; they will be parsed by type specifiers
without the knowledge that they are not themselves present in the
configuration.

Specifically, this means that the following will work:

  $ git config --int --default 1M does.not.exist
  1048576

In subsequent commits, we will offer `--type=color`, which (in
conjunction with `--default`) will be sufficient to replace
`--get-color`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 18 ++++++++++++++++++
 t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100755 t/t1310-config-default.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index a1e3ffe750..b644a44ae9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -240,6 +240,10 @@ Valid `<type>`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default <value>::
+  When using `--get`, and the requested variable is not found, behave as if
+  <value> were the value assigned to the that variable.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when
diff --git a/builtin/config.c b/builtin/config.c
index 5c8952a17c..014e3a0d53 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
@@ -121,6 +122,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };
 
@@ -285,6 +287,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0)
+			die(_("failed to format default config value: %s"),
+				default_value);
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {
@@ -623,6 +635,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage,
+			builtin_config_options);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh
new file mode 100755
index 0000000000..6049d91708
--- /dev/null
+++ b/t/t1310-config-default.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when entry missing' '
+	echo quux >expect &&
+	git config -f config --default=quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'does not use --default when entry present' '
+	echo bar >expect &&
+	git -c core.foo=bar config --default=baz core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=yes --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=expiry-date --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "failed to format default config value" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done
-- 
2.17.0

[PATCH v6 2/3] config.c: introduce 'git_config_color' to parse ANSI colors

[Taylor Blau]

In preparation for adding `--type=color` to the `git-config(1)` builtin,
let's introduce a color parsing utility, `git_config_color` in a similar
fashion to `git_config_<type>`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/config.c b/config.c
index b0c20e6cb8..33366b52c7 100644
--- a/config.c
+++ b/config.c
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */
diff --git a/config.h b/config.h
index ef70a9cac1..0e060779d9 100644
--- a/config.h
+++ b/config.h
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);
-- 
2.17.0

[PATCH v6 3/3] 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 <me@ttaylorr.com>
---
 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 b644a44ae9..7c8365e377 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,6 +177,10 @@ Valid `<type>`'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 `<type>`'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=<default>]` is preferred over `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 014e3a0d53..08016863ac 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
 
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		*type = TYPE_PATH;
 	else if (!strcmp(arg, "expiry-date"))
 		*type = TYPE_EXPIRY_DATE;
+	else if (!strcmp(arg, "color"))
+		*type = TYPE_COLOR;
 	else
 		die(_("unrecognized --type argument, %s"), arg);
 
@@ -202,6 +205,11 @@ 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 (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 {
@@ -347,6 +355,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 f5ae80e9ae..c494733061 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.raw >actual &&
+	echo "<RED>" >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

[PATCH v6 3/3] 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 <me@ttaylorr.com>
---
 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 b644a44ae9..7c8365e377 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -177,6 +177,10 @@ Valid `<type>`'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 `<type>`'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=<default>]` is preferred over `--get-color`.
 
 -e::
 --edit::
diff --git a/builtin/config.c b/builtin/config.c
index 014e3a0d53..08016863ac 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
 
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 		*type = TYPE_PATH;
 	else if (!strcmp(arg, "expiry-date"))
 		*type = TYPE_EXPIRY_DATE;
+	else if (!strcmp(arg, "color"))
+		*type = TYPE_COLOR;
 	else
 		die(_("unrecognized --type argument, %s"), arg);
 
@@ -202,6 +205,11 @@ 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 (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 {
@@ -347,6 +355,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 f5ae80e9ae..c494733061 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.raw >actual &&
+	echo "<RED>" >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

Re: [PATCH v4 1/3] builtin/config: introduce `--default`

[Taylor Blau]

On Mon, Apr 09, 2018 at 08:18:18AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:
> >
> >> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)
> >>  	config_with_options(collect_config, &values,
> >>  			    &given_config_source, &config_options);
> >>
> >> +	if (!values.nr && default_value) {
> >> +		struct strbuf *item;
> >> +		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
> >> +		item = &values.items[values.nr++];
> >> +		strbuf_init(item, 0);
> >> +		if (format_config(item, key_, default_value) < 0) {
> >> +			exit(1);
> >> +		}
> >> +	}
> > ...
> >
> > It's obvious in this toy example, but that config call may be buried
> > deep in a script. It'd probably be nicer for that exit(1) to be
> > something like:
> >
> >   die(_("failed to format default config value"));
>
> Together with key_ and default_value, you mean?
>
> >
> >> +test_expect_success 'does not allow --default without --get' '
> >> +	test_must_fail git config --default quux --unset a >output 2>&1 &&
> >> +	test_i18ngrep "\-\-default is only applicable to" output
> >> +'
> >
> > I think "a" here needs to be "a.section". I get:
> >
> >   error: key does not contain a section: a
>
> "section.var"?  That aside, even with a properly formatted key, I
> seem to get an empty output here, so this may need a bit more work.

This should be fixed in the latest re-roll, v6:

	expecting success:
					test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
					test_i18ngrep "\-\-default is only applicable to" output

	error: --default is only applicable to --get
	ok 5 - does not allow --default without --get


Thanks,
Taylor

Re: [PATCH v6 1/3] builtin/config: introduce `--default`

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> For some use cases, callers of the `git-config(1)` builtin would like to
> fallback to default values when the variable asked for does not exist.
> In addition, users would like to use existing type specifiers to ensure
> that values are parsed correctly when they do exist in the
> configuration.

Nicely done.

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index b644a44ae9..7c8365e377 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -177,6 +177,10 @@ Valid `<type>`'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.

This makes sense, in the same spirit as a value with "path" type
where tilde expansion is not done at all while setting.

Will queue.  Thanks.