{"thread":{"id":"47986","subject":"[PATCH 0/4] Teach `--default` to `git-config(1)`","startedAt":"2018-03-06T02:17:42Z","lastAt":"2018-04-10T01:54:53Z","messageCount":83,"participants":["Taylor Blau","Jeff King","Eric Sunshine","Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"341089","messageId":"20180306021729.45813-1-me@ttaylorr.com","threadId":"47986","inReplyTo":null,"subject":"[PATCH 0/4] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:17:25Z","receivedAt":"2018-03-06T02:17:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is a patch series to introduce `--default` and `--color` to the\n`git-config(1)` builtin with the aim of introducing a consistent interface to\nreplace `--get-color`.\n\nThank you in advance for reviewing this series; I will be more than happy to\nrespond to any feedback seeing as I am still quite new to working on Git itself.\n\nThanks,\nTaylor\n\nTaylor Blau (4):\n  builtin/config: introduce `--default`\n  Documentation: list all type specifiers in config prose\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `--color` type specifier\n\n Documentation/git-config.txt |  21 +++++---\n builtin/config.c             |  36 +++++++++++++\n config.c                     |  10 ++++\n config.h                     |   1 +\n t/t1300-repo-config.sh       |  16 ++++++\n t/t1310-config-default.sh    | 119 +++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 197 insertions(+), 6 deletions(-)\n create mode 100755 t/t1310-config-default.sh\n\n--\n2.15.1.354.g95ec6b1b3\n\n"},{"id":"341090","messageId":"20180306021729.45813-2-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH 1/4] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:17:26Z","receivedAt":"2018-03-06T02:17:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In an aim to replace:\n\n  $ git config --get-color slot [default] [...]\n\nwith:\n\n  $ git config --default default --color slot [...]\n\nintroduce `--defualt` to behave as if the given default were present and\nassigned to slot in the case that that slot does not exist.\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguraion.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--color`, which (in conjunction\nwith `--default`) will be sufficient to replace `--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |   4 ++\n builtin/config.c             |  19 +++++++\n t/t1310-config-default.sh    | 119 +++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 142 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 14da5fc15..390b49831 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -233,6 +233,10 @@ See also <<FILES>>.\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+--default value::\n+  When using `--get`, `--get-all`, and `--get-regexp`, behave as\n+  if value were the value assigned to the given slot.\n+\n [[FILES]]\n FILES\n -----\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ab5f95476..76edefc07 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,6 +26,7 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -87,6 +88,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"use default value with missing entry\")),\n \tOPT_END(),\n };\n \n@@ -251,6 +253,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tif (format_config(item, key_, default_value) < 0) {\n+\t\t\tvalues.nr = 0;\n+\t\t}\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n@@ -594,6 +606,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (default_value && !(actions &\n+\t\t(ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP))) {\n+\t\terror(\"--default is only applicable to --get, --get-all, \"\n+\t\t\t\"and --get-regexp.\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n+\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (config_with_options(show_all_config, NULL,\ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 000000000..57fe63295\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,119 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'clear default config' '\n+\trm -f .git/config\n+'\n+\n+test_expect_success 'uses default when missing entry' '\n+\techo quux >expect &&\n+\tgit config --default quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'uses entry when available' '\n+\techo bar >expect &&\n+\tgit config --add core.foo bar &&\n+\tgit config --default baz core.foo >actual &&\n+\tgit config --unset core.foo &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as bool' '\n+\techo true >expect &&\n+\tgit config --default true --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as int' '\n+\techo 810 >expect &&\n+\tgit config --default 810 --int core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as int (storage unit)' '\n+\techo 1048576 >expect &&\n+\tgit config --default 1M --int core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as bool-or-int' '\n+\techo \"1\n+true\" >expect &&\n+\tgit config --default 1 --bool-or-int core.foo >actual &&\n+\tgit config --default true --bool-or-int core.foo >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshal default value as path' '\n+\techo /path/to/file >expect &&\n+\tgit config --default /path/to/file --path core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshal default value as expiry-date' '\n+\techo 0 >expect &&\n+\tgit config --default never --expiry-date core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshal default value as color' '\n+\techo \"\\033[31m\" >expect &&\n+\tgit config --default red --color core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'does not allow --default with --replace-all' '\n+\ttest_must_fail git config --default quux --replace-all a b >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --add' '\n+\ttest_must_fail git config --default quux --add a b >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --unset' '\n+\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --unset-all' '\n+\ttest_must_fail git config --default quux --unset-all a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --rename-section' '\n+\ttest_must_fail git config --default quux --rename-section a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --remove-section' '\n+\ttest_must_fail git config --default quux --remove-section a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --list' '\n+\ttest_must_fail git config --default quux --list >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --edit' '\n+\ttest_must_fail git config --default quux --edit >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --get-color' '\n+\ttest_must_fail git config --default quux --get-color >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --get-colorbool' '\n+\ttest_must_fail git config --default quux --get-colorbool >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.15.1.354.g95ec6b1b3\n\n"},{"id":"341091","messageId":"20180306021729.45813-4-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:17:28Z","receivedAt":"2018-03-06T02:17:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb..fb1f41ab3 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char **dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, *dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac..48f8e7684 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char **, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.15.1.354.g95ec6b1b3\n\n"},{"id":"341092","messageId":"20180306021729.45813-5-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH 4/4] builtin/config: introduce `--color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:17:29Z","receivedAt":"2018-03-06T02:17:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the cannonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--color` and encourage `--color`,\n`--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 10 +++++++---\n builtin/config.c             | 17 +++++++++++++++++\n t/t1300-repo-config.sh       | 16 ++++++++++++++++\n 3 files changed, 40 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 28d84ded9..0dfb20324 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -42,9 +42,9 @@ The type specifier can be either `--int` or `--bool`, to make 'git config'\n ensure that the variable(s) are of the given type and convert the value to the\n canonical form (simple decimal number for int, a \"true\" or \"false\" string for\n bool, either of for --bool-or-int), or `--path`, which does some path expansion\n-(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no\n-type specifier is passed, no checks or transformations are performed on the\n-value.\n+(see `--path` below), or `--expiry-date` (see `--expiry-date` below), or\n+`--color` (see `--color` below).  If no type specifier is passed, no checks or\n+transformations are performed on the value.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n@@ -186,6 +186,10 @@ See also <<FILES>>.\n \ta fixed or relative date-string to a timestamp. This option\n \thas no effect when setting the value.\n \n+--color::\n+  `git config` will ensure that the output is converted to an\n+  ANSI color escape sequence.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 76edefc07..05f97f2cb 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -54,6 +54,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n #define TYPE_EXPIRY_DATE (1<<4)\n+#define TYPE_COLOR (1<<5)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -83,6 +84,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n \tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n+\tOPT_BIT(0, \"color\", &types, N_(\"value is a color\"), TYPE_COLOR),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -168,6 +170,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (types == TYPE_COLOR) {\n+\t\t\tchar *v = xmalloc(COLOR_MAXLEN);\n+\t\t\tif (git_config_color(&v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n+\t\t\tfree((char *) v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -313,6 +321,15 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (types == TYPE_COLOR) {\n+\t\tchar *v = xmalloc(COLOR_MAXLEN);\n+\t\tif (!git_config_color(&v, key, value)) {\n+\t\t\tfree((char *) v);\n+\t\t\treturn xstrdup(value);\n+\t\t}\n+\t\tfree((char *) v);\n+\t\tdie(\"cannot parse color '%s'\", value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", types);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 4f8e6f5fd..c03f54fbe 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,22 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+cat >expect <<EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'get --color' '\n+\trm .git/config &&\n+\tgit config --color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --color foo.bar\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.15.1.354.g95ec6b1b3\n\n"},{"id":"341093","messageId":"20180306021729.45813-3-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH 2/4] Documentation: list all type specifiers in config prose","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:17:27Z","receivedAt":"2018-03-06T02:17:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The documentation for the `git-config(1)` builtin has not been recently\nupdated to include new types, such as `--bool-or-int`, and\n`--expiry-date`. To ensure completeness when adding a new type\nspecifier, let's update the existing documentation to include the new\ntypes.\n\nThis commit also prepares for the new type specifier `--color`, so that\nthis section will not lag behind when yet more new specifiers are added.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 390b49831..28d84ded9 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If\n you want to handle the lines that do *not* match the regex, just\n prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n \n-The type specifier can be either `--int` or `--bool`, to make\n-'git config' ensure that the variable(s) are of the given type and\n-convert the value to the canonical form (simple decimal number for int,\n-a \"true\" or \"false\" string for bool), or `--path`, which does some\n-path expansion (see `--path` below).  If no type specifier is passed, no\n-checks or transformations are performed on the value.\n+The type specifier can be either `--int` or `--bool`, to make 'git config'\n+ensure that the variable(s) are of the given type and convert the value to the\n+canonical form (simple decimal number for int, a \"true\" or \"false\" string for\n+bool, either of for --bool-or-int), or `--path`, which does some path expansion\n+(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no\n+type specifier is passed, no checks or transformations are performed on the\n+value.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n-- \n2.15.1.354.g95ec6b1b3\n\n"},{"id":"341094","messageId":"20180306022052.GA46077@syl.local","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"Re: [PATCH 0/4] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-06T02:20:52Z","receivedAt":"2018-03-06T02:21:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 05, 2018 at 06:17:25PM -0800, Taylor Blau wrote:\n> Attached is a patch series to introduce `--default` and `--color` to the\n> `git-config(1)` builtin with the aim of introducing a consistent interface to\n> replace `--get-color`.\n\nThis series draws significant inspiration from Soukaina Nait Hmid's work\nin:\n\n  https://public-inbox.org/git/0102015fb0bf2f74-cb456171-fe65-4d83-8784-b553c7c9e584-000000@eu-west-1.amazonses.com/\n\n--\n- Taylor\n"},{"id":"341099","messageId":"20180306065208.GB3522@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180306021729.45813-2-me@ttaylorr.com","subject":"Re: [PATCH 1/4] builtin/config: introduce `--default`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-06T06:52:08Z","receivedAt":"2018-03-06T06:52:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:\n\n> In an aim to replace:\n> \n>   $ git config --get-color slot [default] [...]\n> \n> with:\n> \n>   $ git config --default default --color slot [...]\n> \n> introduce `--defualt` to behave as if the given default were present and\n> assigned to slot in the case that that slot does not exist.\n\nI think this motivation skips over the beginning part of the story,\nwhich is why we want \"--color --default\". :)\n\nIMHO, the reason we want --default is two-fold:\n\n  1. Callers have to handle parsing defaults themselves, like:\n\n       foo=$(git config core.foo || echo 1234)\n\n     For an integer, that's not too bad, since you can write \"1048576\"\n     instead of \"1M\". For colors, it's abominable, which is why we added\n     \"--get-color\". But as we add more types that are hard to parse\n     (like --expiry-date), it would be nice for them to get the same\n     defaulting feature without adding --get-expiry-date, etc.\n\n  2. --get-color is a one-off unlike all of the other types. That's bad\n     interface design, but the inconsistency also makes it harder to add\n     features which treat the types uniformly (like, say, a --stdin\n     query mode).\n\nAnd perhaps minor, but it's also easier to correctly error-check\n--default, since the \"foo\" example above would do the wrong thing if\ngit-config encountered a fatal error.\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 14da5fc15..390b49831 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -233,6 +233,10 @@ See also <<FILES>>.\n>  \tusing `--file`, `--global`, etc) and `on` when searching all\n>  \tconfig files.\n>  \n> +--default value::\n> +  When using `--get`, `--get-all`, and `--get-regexp`, behave as\n> +  if value were the value assigned to the given slot.\n\nI had thought about this in the context of --get, where a single value\nmakes sense.\n\nFor --get-all, would we want to be able to specify a list of objects?\nE.g.:\n\n  git config --default foo --default bar --get-all core.slot\n\nand behave as if we found two entries, \"foo\" and \"bar\"?\n\nI'm not really sure what semantics would be most useful.\n\nDitto for --get-regexp.\n\nThis isn't necessarily an objection. I'm just not sure what people would\nexpect. So it might make sense to start more limited and wait for a real\nuse case to pop up. But I'm also open to arguments about plausible use\ncases. ;)\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index ab5f95476..76edefc07 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n\nThe general design of the implementation looks good. There's one funny\nthing:\n\n> +\tif (!values.nr && default_value) {\n> +\t\tstruct strbuf *item;\n> +\n> +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> +\t\titem = &values.items[values.nr++];\n> +\t\tif (format_config(item, key_, default_value) < 0) {\n> +\t\t\tvalues.nr = 0;\n> +\t\t}\n\nWe never initialize the strbuf data here, and we can't count on\nALLOC_GROW to even zero it (which I suspect would work, but still isn't\nhow strbufs are meant to be used).\n\nDo you need:\n\n  strbuf_init(item, 0);\n\nhere, similar to what collect_config does?\n\n(As an aside, it seems like this whole thing might be simpler with a\nstring_list, but that's certainly not a problem that you're introducing\nhere).\n\n> +test_expect_success 'marshals default value as bool-or-int' '\n> +\techo \"1\n> +true\" >expect &&\n> +\tgit config --default 1 --bool-or-int core.foo >actual &&\n> +\tgit config --default true --bool-or-int core.foo >>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nFunny indentation. Use:\n\n  {\n\techo 1 &&\n\techo true\n  } >expect &&\n\nor\n\n  cat >expect <<-\\EOF\n  1\n  true\n  EOF\n\n-Peff\n"},{"id":"341100","messageId":"20180306065253.GC3522@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180306021729.45813-3-me@ttaylorr.com","subject":"Re: [PATCH 2/4] Documentation: list all type specifiers in config prose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-06T06:52:54Z","receivedAt":"2018-03-06T06:53:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2018 at 06:17:27PM -0800, Taylor Blau wrote:\n\n> The documentation for the `git-config(1)` builtin has not been recently\n> updated to include new types, such as `--bool-or-int`, and\n> `--expiry-date`. To ensure completeness when adding a new type\n> specifier, let's update the existing documentation to include the new\n> types.\n> \n> This commit also prepares for the new type specifier `--color`, so that\n> this section will not lag behind when yet more new specifiers are added.\n\nGood catch. Thanks for cleaning this up.\n\n> -The type specifier can be either `--int` or `--bool`, to make\n> -'git config' ensure that the variable(s) are of the given type and\n> -convert the value to the canonical form (simple decimal number for int,\n> -a \"true\" or \"false\" string for bool), or `--path`, which does some\n> -path expansion (see `--path` below).  If no type specifier is passed, no\n> -checks or transformations are performed on the value.\n> +The type specifier can be either `--int` or `--bool`, to make 'git config'\n> +ensure that the variable(s) are of the given type and convert the value to the\n> +canonical form (simple decimal number for int, a \"true\" or \"false\" string for\n> +bool, either of for --bool-or-int), or `--path`, which does some path expansion\n> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no\n> +type specifier is passed, no checks or transformations are performed on the\n> +value.\n\nPerhaps it's time to switch to a list format for these?\n\n-Peff\n"},{"id":"341101","messageId":"20180306065349.GD3522@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180306021729.45813-4-me@ttaylorr.com","subject":"Re: [PATCH 3/4] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-06T06:53:50Z","receivedAt":"2018-03-06T06:53:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2018 at 06:17:28PM -0800, Taylor Blau wrote:\n\n> In preparation for adding `--color` to the `git-config(1)` builtin,\n> let's introduce a color parsing utility, `git_config_color` in a similar\n> fashion to `git_config_<type>`.\n\nSounds good...\n\n> @@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n>  \treturn 0;\n>  }\n>  \n> +int git_config_color(char **dest, const char *var, const char *value)\n> +{\n> +\tif (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\tif (color_parse(value, *dest) < 0)\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n\nWhy do we take a pointer-to-pointer here? We don't allocate like\ngit_config_string() does, but instead fill in the existing buffer.\n\n-Peff\n"},{"id":"341102","messageId":"20180306070057.GE3522@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180306021729.45813-5-me@ttaylorr.com","subject":"Re: [PATCH 4/4] builtin/config: introduce `--color` type specifier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-06T07:00:57Z","receivedAt":"2018-03-06T07:01:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 05, 2018 at 06:17:29PM -0800, Taylor Blau wrote:\n\n> As of this commit, the cannonical way to retreive an ANSI-compatible\n> color escape sequence from a configuration file is with the\n> `--get-color` action.\n> \n> This is to allow Git to \"fall back\" on a default value for the color\n> should the given section not exist in the specified configuration(s).\n> \n> With the addition of `--default`, this is no longer needed since:\n> \n>   $ git config --default red --color core.section\n> \n> will be have exactly as:\n> \n>   $ git config --get-color core.section red\n> \n> For consistency, let's introduce `--color` and encourage `--color`,\n> `--default` together over `--get-color` alone.\n\nSounds good. Do we want to explicitly mark \"--get-color\" as historical\nand/or deprecated in the git-config manpage? We won't remove it for a\nlong time, but this would start the cycle.\n\n> @@ -168,6 +170,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n>  \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n>  \t\t\t\treturn -1;\n>  \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n> +\t\t} else if (types == TYPE_COLOR) {\n> +\t\t\tchar *v = xmalloc(COLOR_MAXLEN);\n> +\t\t\tif (git_config_color(&v, key_, value_) < 0)\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addstr(buf, v);\n> +\t\t\tfree((char *) v);\n\nNo need to cast the argument to free, unless you're getting rid of a\n\"const\" (but here \"v\" already has type \"char *\").\n\nHowever, do we need heap allocation here at all? I think:\n\n  char v[COLOR_MAXLEN];\n  if (git_config_color(v, key_, value_) < 0)\n\treturn -1;\n  strbuf_addstr(buf, v);\n\nwould suffice (and would also fix the leak when we return on error).\n\nDitto for the call in normalize_value().\n\n> @@ -313,6 +321,15 @@ static char *normalize_value(const char *key, const char *value)\n>  \t\telse\n>  \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n>  \t}\n> +\tif (types == TYPE_COLOR) {\n> +\t\tchar *v = xmalloc(COLOR_MAXLEN);\n> +\t\tif (!git_config_color(&v, key, value)) {\n> +\t\t\tfree((char *) v);\n> +\t\t\treturn xstrdup(value);\n> +\t\t}\n> +\t\tfree((char *) v);\n> +\t\tdie(\"cannot parse color '%s'\", value);\n> +\t}\n>  \n>  \tdie(\"BUG: cannot normalize type %d\", types);\n>  }\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 4f8e6f5fd..c03f54fbe 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -931,6 +931,22 @@ test_expect_success 'get --expiry-date' '\n>  \ttest_must_fail git config --expiry-date date.invalid1\n>  '\n>  \n> +cat >expect <<EOF\n> +[foo]\n> +\tcolor = red\n> +EOF\n> +\n> +test_expect_success 'get --color' '\n> +\trm .git/config &&\n> +\tgit config --color foo.color \"red\" &&\n> +\ttest_cmp expect .git/config\n> +'\n> +\n> +test_expect_success 'get --color barfs on non-color' '\n> +\techo \"[foo]bar=not-a-color\" >.git/config &&\n> +\ttest_must_fail git config --get --color foo.bar\n> +'\n\nLooks good. The out-of-block setup of expect violates our modern style,\nbut matches the surrounding code.\n\n-Peff\n"},{"id":"341103","messageId":"CAPig+cTrXF_Hm8Uj8jFbBsNhB=J_6TP0wBzKEqfUZDDTwE-0tQ@mail.gmail.com","threadId":"47986","inReplyTo":"20180306021729.45813-2-me@ttaylorr.com","subject":"Re: [PATCH 1/4] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-06T07:08:43Z","receivedAt":"2018-03-06T07:08:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 5, 2018 at 9:17 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> In an aim to replace:\n>\n>   $ git config --get-color slot [default] [...]\n>\n> with:\n>\n>   $ git config --default default --color slot [...]\n>\n> introduce `--defualt` to behave as if the given default were present and\n\ns/--defualt/--default/\n\n> assigned to slot in the case that that slot does not exist.\n>\n> Values filled by `--default` behave exactly as if they were present in\n> the affected configuration file; they will be parsed by type specifiers\n> without the knowledge that they are not themselves present in the\n> configuraion.\n>\n> Specifically, this means that the following will work:\n>\n>   $ git config --int --default 1M does.not.exist\n>   1048576\n>\n> In subsequent commits, we will offer `--color`, which (in conjunction\n> with `--default`) will be sufficient to replace `--get-color`.\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n"},{"id":"341104","messageId":"CAPig+cSMx8oSWvT2dGgz-DwfLJkGDUFP-Gs19c=izK-=3422JA@mail.gmail.com","threadId":"47986","inReplyTo":"20180306065208.GB3522@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-06T07:14:12Z","receivedAt":"2018-03-06T07:14:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 6, 2018 at 1:52 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Mar 05, 2018 at 06:17:26PM -0800, Taylor Blau wrote:\n>> In an aim to replace:\n>>   $ git config --get-color slot [default] [...]\n>> with:\n>>   $ git config --default default --color slot [...]\n>> introduce `--defualt` to behave as if the given default were present and\n>> assigned to slot in the case that that slot does not exist.\n>\n> I think this motivation skips over the beginning part of the story,\n> which is why we want \"--color --default\". :)\n>\n> IMHO, the reason we want --default is two-fold:\n>\n>   1. Callers have to handle parsing defaults themselves, like:\n>\n>        foo=$(git config core.foo || echo 1234)\n>\n>      For an integer, that's not too bad, since you can write \"1048576\"\n>      instead of \"1M\". For colors, it's abominable, which is why we added\n>      \"--get-color\". But as we add more types that are hard to parse\n>      (like --expiry-date), it would be nice for them to get the same\n>      defaulting feature without adding --get-expiry-date, etc.\n>\n>   2. --get-color is a one-off unlike all of the other types. That's bad\n>      interface design, but the inconsistency also makes it harder to add\n>      features which treat the types uniformly (like, say, a --stdin\n>      query mode).\n>\n> And perhaps minor, but it's also easier to correctly error-check\n> --default, since the \"foo\" example above would do the wrong thing if\n> git-config encountered a fatal error.\n\nThanks. For someone (me) who didn't follow the earlier discussion\nclosely, this motivating explanation really helps; especially since\nthe commit message mentions only color, which seems to already allow\nfor a default value, so it wasn't clear what benefit the new --default\nprovides.\n\n>> +test_expect_success 'marshals default value as bool-or-int' '\n>> +     echo \"1\n>> +true\" >expect &&\n>> +     git config --default 1 --bool-or-int core.foo >actual &&\n>> +     git config --default true --bool-or-int core.foo >>actual &&\n>> +     test_cmp expect actual\n>> +'\n>\n> Funny indentation. Use:\n>\n>   {\n>         echo 1 &&\n>         echo true\n>   } >expect &&\n>\n> or\n>\n>   cat >expect <<-\\EOF\n>   1\n>   true\n>   EOF\n\nOr, simpler:\n\n    test_write_lines 1 true >expect\n"},{"id":"341105","messageId":"xmqqd10h7it8.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180306065253.GC3522@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] Documentation: list all type specifiers in config prose","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-06T07:40:51Z","receivedAt":"2018-03-06T07:41:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +The type specifier can be either `--int` or `--bool`, to make 'git config'\n>> +ensure that the variable(s) are of the given type and convert the value to the\n>> +canonical form (simple decimal number for int, a \"true\" or \"false\" string for\n>> +bool, either of for --bool-or-int), or `--path`, which does some path expansion\n>> +(see `--path` below), or `--expiry-date` (see `--expiry-date` below).  If no\n>> +type specifier is passed, no checks or transformations are performed on the\n>> +value.\n>\n> Perhaps it's time to switch to a list format for these?\n\nA very sensible suggestion.  The original was already bad enough but\nwith complete set, it does become quite hard to read.  Perhaps along\nthe lines of...\n\n    A type specifier option can be used to force interpretation of\n    values and conversion to canonical form.  Currently supported\n    type specifiers are:\n\n    `--int`::\n            The value is taken as an integer.\n\n    `--bool`::\n            The value is taken as a yes/no value, and are shown as\n            \"true\" or \"false\".\n\n    ...\n\n"},{"id":"342712","messageId":"20180324005556.8145-1-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH v2 0/4] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-24T00:55:52Z","receivedAt":"2018-03-24T00:56:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is 'v2' of my patch series to add a `--default` option to `git config`.\n\nI have addressed the following concerns since the first iteration:\n\n  1. Better motivation in 'builtin/config: introduce `--default`'. (cc: Peff,\n  Eric)\n\n  2. Fixed a typo in the message of 'builtin/config: introduce `--default`'.\n  (cc: Eric).\n\n  3. Changed the first mention of type specifiers in `git config`'s\n  documentation from a paragraph to a brief list (cc: Peff, Junio).\n\n  4. Changed the signatures of some new functions, particularly in 'config.c:\n  introduce 'git_config_color' to parse ANSI colors'.\n\nI have thus-far avoided mentioning any specific deprecation of `--get-color` and\n`--get-colorbool`, but would not be opposed to changing that in this series\nbefore queuing.\n\nThank you again for all of your feedback, and my apologies that it has taken me\nso long to respond. I was out of the office last week, and have been quite busy\nsince catching up on Git LFS-related issues.\n\n\nThanks,\nTaylor\n\nTaylor Blau (4):\n  builtin/config: introduce `--default`\n  Documentation: list all type specifiers in config prose\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `--color` type specifier\n\n Documentation/git-config.txt |  38 +++++++---\n builtin/config.c             |  30 ++++++++\n config.c                     |  10 +++\n config.h                     |   1 +\n t/t1300-repo-config.sh       |  24 +++++++\n t/t1310-config-default.sh    | 131 +++++++++++++++++++++++++++++++++++\n 6 files changed, 226 insertions(+), 8 deletions(-)\n create mode 100755 t/t1310-config-default.sh\n\n--\n2.16.2.440.gc6284da4f\n\n"},{"id":"342713","messageId":"20180324005556.8145-2-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"[PATCH v2 1/4] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-24T00:55:53Z","receivedAt":"2018-03-24T00:56:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For some use cases, callers of the `git-config(1)` builtin would like to\nfallback to default values when the slot asked for does not exist. In\naddition, users would like to use existing type specifiers to ensure\nthat values are parsed correctly when they do exist in the\nconfiguration.\n\nFor example, to fetch a value without a type specifier and fallback to\n`$fallback`, the following is required:\n\n  $ git config core.foo || echo \"$fallback\"\n\nThis is fine for most values, but can be tricky for difficult-to-express\n`$fallback`'s, like ANSI color codes.\n\nThis motivates `--get-color`, which is a one-off exception to the normal\ntype specifier rules wherein a user specifies both the configuration\nslot and an optional fallback. Both are formatted according to their\ntype specifier, which eases the burden on the user to ensure that values\nare correctly formatted.\n\nThis commit (and those following it in this series) aim to eventually\nreplace `--get-color` with a consistent alternative. By introducing\n`--default`, we allow the `--get-color` action to be promoted to a\n`--color` type specifier, retaining the \"fallback\" behavior via the\n`--default` flag introduced in this commit.\n\nFor example, we aim to replace:\n\n  $ git config --get-color slot [default] [...]\n\nwith:\n\n  $ git config --default default --color slot [...]\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguraion.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--color`, which (in conjunction\nwith `--default`) will be sufficient to replace `--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |   6 +-\n builtin/config.c             |  17 +++++\n t/t1310-config-default.sh    | 125 +++++++++++++++++++++++++++++++++++\n 3 files changed, 146 insertions(+), 2 deletions(-)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex e09ed5d7d..d9e389a33 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -233,8 +233,10 @@ See also <<FILES>>.\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n-CONFIGURATION\n--------------\n+--default value::\n+  When using `--get`, behave as if value were the value assigned to the given\n+  slot.\n+\n `pager.config` is only respected when listing configuration, i.e., when\n using `--list` or any of the `--get-*` which may return multiple results.\n The default is to use a pager.\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 01169dd62..1410be850 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,6 +26,7 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -94,6 +95,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n \tOPT_END(),\n };\n \n@@ -258,6 +260,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tstrbuf_init(item, 0);\n+\t\tif (format_config(item, key_, default_value) < 0) {\n+\t\t\tvalues.nr = 0;\n+\t\t}\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n@@ -601,6 +613,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (default_value && !(actions & ACTION_GET)) {\n+\t\terror(\"--default is only applicable to --get\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n+\n \tif (actions & PAGING_ACTIONS)\n \t\tsetup_auto_pager(\"config\", 1);\n \ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 000000000..0e464c206\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,125 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'clear default config' '\n+\trm -f .git/config\n+'\n+\n+test_expect_success 'uses default when missing entry' '\n+\techo quux >expect &&\n+\tgit config --default quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'uses entry when available' '\n+\techo bar >expect &&\n+\tgit config --add core.foo bar &&\n+\tgit config --default baz core.foo >actual &&\n+\tgit config --unset core.foo &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as bool' '\n+\techo true >expect &&\n+\tgit config --default true --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as int' '\n+\techo 810 >expect &&\n+\tgit config --default 810 --int core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as int (storage unit)' '\n+\techo 1048576 >expect &&\n+\tgit config --default 1M --int core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshals default value as bool-or-int' '\n+\t{\n+\t\techo 1 &&\n+\t\techo true\n+\t} >expect &&\n+\tgit config --default 1 --bool-or-int core.foo >actual &&\n+\tgit config --default true --bool-or-int core.foo >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshal default value as path' '\n+\techo /path/to/file >expect &&\n+\tgit config --default /path/to/file --path core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'marshal default value as expiry-date' '\n+\techo 0 >expect &&\n+\tgit config --default never --expiry-date core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'does not allow --default with --get-all' '\n+\ttest_must_fail git config --default quux --get-all a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --get-regexp' '\n+\ttest_must_fail git config --default quux --get-regexp a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --replace-all' '\n+\ttest_must_fail git config --default quux --replace-all a b >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --add' '\n+\ttest_must_fail git config --default quux --add a b >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --unset' '\n+\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --unset-all' '\n+\ttest_must_fail git config --default quux --unset-all a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --rename-section' '\n+\ttest_must_fail git config --default quux --rename-section a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --remove-section' '\n+\ttest_must_fail git config --default quux --remove-section a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --list' '\n+\ttest_must_fail git config --default quux --list >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --edit' '\n+\ttest_must_fail git config --default quux --edit >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --get-color' '\n+\ttest_must_fail git config --default quux --get-color >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_expect_success 'does not allow --default with --get-colorbool' '\n+\ttest_must_fail git config --default quux --get-colorbool >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"342714","messageId":"20180324005556.8145-3-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"[PATCH v2 2/4] Documentation: list all type specifiers in config prose","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-24T00:55:54Z","receivedAt":"2018-03-24T00:56:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The documentation for the `git-config(1)` builtin has not been recently\nupdated to include new types, such as `--bool-or-int`, and\n`--expiry-date`. To ensure completeness when adding a new type\nspecifier, let's update the existing documentation to include the new\ntypes.\n\nSince this paragraph is growing in length, let's also convert this to a\nlist so that it can be read with greater ease. We provide a minimal\ndescription of all valid type specifiers, and reference the <<OPTIONS>>\nsection where each specifier is described in full detail.\n\nThis commit also prepares for the new type specifier `--color`, so that\nthis section will not lag behind when yet more new specifiers are added.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 25 +++++++++++++++++++------\n 1 file changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex d9e389a33..d9d45aca3 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -38,12 +38,25 @@ existing values that match the regexp are updated or unset.  If\n you want to handle the lines that do *not* match the regex, just\n prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n \n-The type specifier can be either `--int` or `--bool`, to make\n-'git config' ensure that the variable(s) are of the given type and\n-convert the value to the canonical form (simple decimal number for int,\n-a \"true\" or \"false\" string for bool), or `--path`, which does some\n-path expansion (see `--path` below).  If no type specifier is passed, no\n-checks or transformations are performed on the value.\n+A type specifier option can be used to force interpretation of values and\n+conversion to canonical form.  Currently supported type specifiers are:\n+\n+`bool`::\n+    The value is taken as a boolean.\n+\n+`int`::\n+    The value is taken as an integer.\n+\n+`bool-or-int`::\n+    The value is taken as a boolean or integer, as above.\n+\n+`path`::\n+    The value is taken as a filepath.\n+\n+`expiry-date`::\n+    The value is taken as a timestamp.\n+\n+For more information about the above type specifiers, see <<OPTIONS>> below.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"342715","messageId":"20180324005556.8145-4-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"[PATCH v2 3/4] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-24T00:55:55Z","receivedAt":"2018-03-24T00:56:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb..33366b52c 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac..0e060779d 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"342716","messageId":"20180324005556.8145-5-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"[PATCH v2 4/4] builtin/config: introduce `--color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-24T00:55:56Z","receivedAt":"2018-03-24T00:56:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--color` and encourage `--color`,\n`--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  7 +++++++\n builtin/config.c             | 13 +++++++++++++\n t/t1300-repo-config.sh       | 24 ++++++++++++++++++++++++\n t/t1310-config-default.sh    |  6 ++++++\n 4 files changed, 50 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex d9d45aca3..e70c0a855 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -56,6 +56,9 @@ conversion to canonical form.  Currently supported type specifiers are:\n `expiry-date`::\n     The value is taken as a timestamp.\n \n+`color`::\n+    The value is taken as an ANSI color escape sequence.\n+\n For more information about the above type specifiers, see <<OPTIONS>> below.\n \n When reading, the values are read from the system, global and\n@@ -198,6 +201,10 @@ See also <<FILES>>.\n \ta fixed or relative date-string to a timestamp. This option\n \thas no effect when setting the value.\n \n+--color::\n+  `git config` will ensure that the output is converted to an\n+  ANSI color escape sequence.\n+\n -z::\n --null::\n \tFor all options that output values and/or keys, always\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 1410be850..e8661bc12 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n #define TYPE_EXPIRY_DATE (1<<4)\n+#define TYPE_COLOR (1<<5)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n \tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n+\tOPT_BIT(0, \"color\", &types, N_(\"value is a color\"), TYPE_COLOR),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -175,6 +177,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (types == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (types == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (!git_config_color(v, key, value))\n+\t\t\treturn xstrdup(value);\n+\t\tdie(\"cannot parse color '%s'\", value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", types);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 4f8e6f5fd..dab94d0c4 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,30 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --color foo.color | test_decode_color >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --color' '\n+\trm .git/config &&\n+\tgit config --color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --color foo.bar\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nindex 0e464c206..0ebece7d2 100755\n--- a/t/t1310-config-default.sh\n+++ b/t/t1310-config-default.sh\n@@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'marshal default value as color' '\n+\techo \"\\033[31m\" >expect &&\n+\tgit config --default red --color core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'does not allow --default with --get-all' '\n \ttest_must_fail git config --default quux --get-all a >output 2>&1 &&\n \ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"342978","messageId":"20180326083442.GB18714@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180324005556.8145-2-me@ttaylorr.com","subject":"Re: [PATCH v2 1/4] builtin/config: introduce `--default`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T08:34:42Z","receivedAt":"2018-03-26T08:34:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 23, 2018 at 08:55:53PM -0400, Taylor Blau wrote:\n\n> This commit (and those following it in this series) aim to eventually\n> replace `--get-color` with a consistent alternative. By introducing\n> `--default`, we allow the `--get-color` action to be promoted to a\n> `--color` type specifier, retaining the \"fallback\" behavior via the\n> `--default` flag introduced in this commit.\n> \n> For example, we aim to replace:\n> \n>   $ git config --get-color slot [default] [...]\n> \n> with:\n> \n>   $ git config --default default --color slot [...]\n> [...]\n> In subsequent commits, we will offer `--color`, which (in conjunction\n> with `--default`) will be sufficient to replace `--get-color`.\n\nNot a huge deal, but I suspect you could actually get away with less\ndetail here (I know, I know, I asked for more in the last review).\n\nThe motivation of \"--get-color is weirdly special, and other types\nshould match it\" is probably enough without laying out all of the future\nplans in this patch (which are going to end up repeated when we actually\ndo add --color).\n\nI don't think it's worth a re-roll by itself, but just a thought for\nfuture patches.\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index e09ed5d7d..d9e389a33 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -233,8 +233,10 @@ See also <<FILES>>.\n>  \tusing `--file`, `--global`, etc) and `on` when searching all\n>  \tconfig files.\n>  \n> -CONFIGURATION\n> --------------\n> +--default value::\n> +  When using `--get`, behave as if value were the value assigned to the given\n> +  slot.\n> +\n>  `pager.config` is only respected when listing configuration, i.e., when\n>  using `--list` or any of the `--get-*` which may return multiple results.\n>  The default is to use a pager.\n\nHmm, what's going on with the CONFIGURATION header here?\n\nFor the description of \"--default\" itself, do we want to say something\nlike:\n\n  When using `--get` and the requested slot is not found, behave as\n  if...\n\nThat is hopefully a given from the name \"--default\", but it seems like\nan important point to mention.\n\n> @@ -258,6 +260,16 @@ static int get_value(const char *key_, const char *regex_)\n>  \tconfig_with_options(collect_config, &values,\n>  \t\t\t    &given_config_source, &config_options);\n>  \n> +\tif (!values.nr && default_value) {\n> +\t\tstruct strbuf *item;\n> +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> +\t\titem = &values.items[values.nr++];\n> +\t\tstrbuf_init(item, 0);\n> +\t\tif (format_config(item, key_, default_value) < 0) {\n> +\t\t\tvalues.nr = 0;\n> +\t\t}\n> +\t}\n\nIf we see an error from format_config (which AFAICT is only for a bogus\nexpiry date; the other formats will just die() immediately), what\nhappens? I guess we'd eventually report \"I didn't find anything\". Which\nis true, but I suspect we may be better off explicitly saying \"your\ndefault value was junk\" (i.e., by dying right here).\n\n> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\n> new file mode 100755\n> index 000000000..0e464c206\n> --- /dev/null\n> +++ b/t/t1310-config-default.sh\n> @@ -0,0 +1,125 @@\n> +#!/bin/sh\n> +\n> +test_description='Test git config in different settings (with --default)'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'clear default config' '\n> +\trm -f .git/config\n> +'\n\nI know that you pulled this trick from t1300, but I think we should\navoid blowing away .git/config. With the current repo-setup code we'll\nstill blindly treat it as a git repository, but I'm not convinced that's\nnot a bug. And if we ever do tighten it, we may start rejecting the\ntrash directory as a repository, and this would set values in the\ngit.git repo's config file.\n\nInstead, doing \"git config -f file\" is probably a safer bet (unless you\nreally do want to check the system-user-repo lookup, but I don't think\nwe do that at all here).\n\n> +test_expect_success 'marshals default value as bool' '\n> +\techo true >expect &&\n> +\tgit config --default true --bool core.foo >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nMaybe \"--default 1\" would be a better test for --bool, since it would\nactually transform the value?\n\n> +test_expect_success 'marshals default value as int' '\n> +\techo 810 >expect &&\n> +\tgit config --default 810 --int core.foo >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'marshals default value as int (storage unit)' '\n> +\techo 1048576 >expect &&\n> +\tgit config --default 1M --int core.foo >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI'm not sure what we're really testing in the first one. The storage\nunit conversion is the interesting bit.\n\nTBH, I'm not sure we need to separately test each possible type\nconversion here. If we know that the type conversions are tested\nelsewhere (and I would not be surprised if they're not, but let's assume\nfor a moment...) and we check that type conversions are applied to\n\"--default\" values for some types, I don't think there's any reason to\nassume that the others would not work.\n\n> +test_expect_success 'does not allow --default with --get-all' '\n> +\ttest_must_fail git config --default quux --get-all a >output 2>&1 &&\n> +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n> +'\n> +\n> +test_expect_success 'does not allow --default with --get-regexp' '\n> +\ttest_must_fail git config --default quux --get-regexp a >output 2>&1 &&\n> +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n> +'\n\nFor these repeated tests, this _is_ the only place we'd test the\nbehavior. I do have mixed feelings on this kind of completionism,\nthough.  While of course any kind of bug is possible, it doesn't seem\nlike these are likely to catch a regression. The obvious regression IMHO\nwould be adding a new mode that isn't caught by the --default check. But\nthen, it's unlikely that somebody introducing that bug would find and\nupdate these tests to catch it.\n\nSo I dunno. They are written and they're not that complicated. But at\nsome point I wonder if we want to be more judicious in adding tests just\nto keep the test suite to a reasonable size.\n\n-Peff\n"},{"id":"342979","messageId":"20180326085556.GC18714@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180324005556.8145-3-me@ttaylorr.com","subject":"Re: [PATCH v2 2/4] Documentation: list all type specifiers in config prose","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T08:55:56Z","receivedAt":"2018-03-26T08:56:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 23, 2018 at 08:55:54PM -0400, Taylor Blau wrote:\n\n> The documentation for the `git-config(1)` builtin has not been recently\n> updated to include new types, such as `--bool-or-int`, and\n> `--expiry-date`. To ensure completeness when adding a new type\n> specifier, let's update the existing documentation to include the new\n> types.\n> \n> Since this paragraph is growing in length, let's also convert this to a\n> list so that it can be read with greater ease. We provide a minimal\n> description of all valid type specifiers, and reference the <<OPTIONS>>\n> section where each specifier is described in full detail.\n> \n> This commit also prepares for the new type specifier `--color`, so that\n> this section will not lag behind when yet more new specifiers are added.\n\nThanks for reformatting; the result is much easier to read. One nit:\n\n> -The type specifier can be either `--int` or `--bool`, to make\n> -'git config' ensure that the variable(s) are of the given type and\n> -convert the value to the canonical form (simple decimal number for int,\n> -a \"true\" or \"false\" string for bool), or `--path`, which does some\n> -path expansion (see `--path` below).  If no type specifier is passed, no\n> -checks or transformations are performed on the value.\n> +A type specifier option can be used to force interpretation of values and\n> +conversion to canonical form.  Currently supported type specifiers are:\n> +\n> +`bool`::\n> +    The value is taken as a boolean.\n> +\n> +`int`::\n> +    The value is taken as an integer.\n> +\n> +`bool-or-int`::\n> +    The value is taken as a boolean or integer, as above.\n> +\n> +`path`::\n> +    The value is taken as a filepath.\n> +\n> +`expiry-date`::\n> +    The value is taken as a timestamp.\n> +\n> +For more information about the above type specifiers, see <<OPTIONS>> below.\n\nThe connection between \"bool\" here and \"--bool\" below isn't made\nexplicit. We probably should mention it.\n\nSome of the details for the types are lost. E.g., what is a filepath?\nThe fact that we expand is not mentioned. I'm not sure how much that\nmatters, though. The full descriptions should be covered under \"--path\"\n(and \"--int\", etc).\n\nIn fact, that kind of makes me wonder if this \"type\" list should not\nexist at all. If we instead grouped the options and said \"these are the\ntype options\", then we'd only need one list.\n\nI'm OK to punt on that for now, though, in the interest of not holding\nup this patch series. I do think we should say something like:\n\n  Each type can be specified with the matching command-line option\n  (e.g., `--bool`, `--int`, etc); see <<OPTIONS>> below.\n\n-Peff\n"},{"id":"342980","messageId":"20180326091645.GD18714@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180324005556.8145-5-me@ttaylorr.com","subject":"Re: [PATCH v2 4/4] builtin/config: introduce `--color` type specifier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T09:16:45Z","receivedAt":"2018-03-26T09:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 23, 2018 at 08:55:56PM -0400, Taylor Blau wrote:\n\n> As of this commit, the canonical way to retreive an ANSI-compatible\n> color escape sequence from a configuration file is with the\n> `--get-color` action.\n\ns/retreive/retrieve/\n\n> This is to allow Git to \"fall back\" on a default value for the color\n> should the given section not exist in the specified configuration(s).\n> \n> With the addition of `--default`, this is no longer needed since:\n> \n>   $ git config --default red --color core.section\n> \n> will be have exactly as:\n> \n>   $ git config --get-color core.section red\n> \n> For consistency, let's introduce `--color` and encourage `--color`,\n> `--default` together over `--get-color` alone.\n\nI don't think we'll ever get rid of --get-color (at the very least, we'd\nneed a deprecation period). But it's probably worth adding a note under\nthe --get-color description to mention that it's a historical synonym,\nand that using \"--default\" should be preferred.\n\n> @@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {\n>  \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n>  \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n>  \tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n> +\tOPT_BIT(0, \"color\", &types, N_(\"value is a color\"), TYPE_COLOR),\n>  \tOPT_GROUP(N_(\"Other\")),\n>  \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n>  \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n\nI just had a funny thought. Normally in Git the \"--color\" option means\n\"colorize the output\". And we are diverging from that here. I wonder if\nanybody would be confused by that, or if we would ever want to later add\nan option to colorize git-config output. Would we regret squatting on\n--color?\n\nI'm not sure what else to name it. Anything _except_ \"--color\" would\ndiverge from the existing scheme of \"--<type>\".\n\nIf we were designing from scratch, I'd consider:\n\n  git config --type=int ...\n  git config --type=color ...\n\netc. I'm not sure if it's worth trying to switch now (on the other hand,\nit resolves the documentation issue I mentioned earlier, since that\nwould naturally group all of the types ;) ).\n\nIt would be pretty easy to declare \"--type\" as the Right Way, and list\n\"--int\" as a historical synonym for \"--type=int\".\n\n> @@ -175,6 +177,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n>  \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n>  \t\t\t\treturn -1;\n>  \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n> +\t\t} else if (types == TYPE_COLOR) {\n> +\t\t\tchar v[COLOR_MAXLEN];\n> +\t\t\tif (git_config_color(v, key_, value_) < 0)\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addstr(buf, v);\n\nLooks good.\n\n> @@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)\n>  \t\telse\n>  \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n>  \t}\n> +\tif (types == TYPE_COLOR) {\n> +\t\tchar v[COLOR_MAXLEN];\n> +\t\tif (!git_config_color(v, key, value))\n> +\t\t\treturn xstrdup(value);\n> +\t\tdie(\"cannot parse color '%s'\", value);\n> +\t}\n\nInteresting. This doesn't actually normalize anything, since we always\npass back the original value (or die). I think that's the right thing to\ndo, since otherwise you'd end up with ANSI codes in your config file.\n\nI wondered at first if this should go in the \"noop\" normalization that\nTYPE_PATH undergoes. But I like that it actually sanity-checks the\nvalue. We should probably have a comment here explaining that yes, we\nparse and then throw away the value (similar to the one near TYPE_PATH).\n\nI suspect that TYPE_EXPIRY_DATE should do the same thing (parse and\ncomplain if you fed nonsense, but always keep the original value).\n\n> +test_expect_success 'get --color' '\n> +\trm .git/config &&\n> +\tgit config foo.color \"red\" &&\n> +\tgit config --get --color foo.color | test_decode_color >actual &&\n> +\techo \"<RED>\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nWe should probably write this as:\n\n  git config --get --color foo.color >actual.raw &&\n  test_decode_color <actual.raw >actual\n\nto catch failures from git-config itself (there's a lot of old tests\nwhich pipe, but we've been trying to convert them to be more careful).\n\n> +test_expect_success 'set --color' '\n> +\trm .git/config &&\n> +\tgit config --color foo.color \"red\" &&\n> +\ttest_cmp expect .git/config\n> +'\n> +\n> +test_expect_success 'get --color barfs on non-color' '\n> +\techo \"[foo]bar=not-a-color\" >.git/config &&\n> +\ttest_must_fail git config --get --color foo.bar\n> +'\n\nAfter reading the normalize bits above, I think there's one more case to\ncover:\n\n  test_must_fail git config --color foo.color not-a-color\n\n> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\n> index 0e464c206..0ebece7d2 100755\n> --- a/t/t1310-config-default.sh\n> +++ b/t/t1310-config-default.sh\n> @@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'marshal default value as color' '\n> +\techo \"\\033[31m\" >expect &&\n> +\tgit config --default red --color core.foo >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI don't offhand recall whether octal escapes with \"echo\" are portable.\nIt _is_ in POSIX, but only for XSI. I think using \"printf\" would be\nportable.\n\n-Peff\n"},{"id":"342981","messageId":"20180326091845.GE18714@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"Re: [PATCH v2 0/4] Teach `--default` to `git-config(1)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-26T09:18:45Z","receivedAt":"2018-03-26T09:18:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 23, 2018 at 08:55:52PM -0400, Taylor Blau wrote:\n\n> Attached is 'v2' of my patch series to add a `--default` option to `git config`.\n\nThanks, this is looking much better. I did come up with a few\nsuggestions/fixes. Some minor which would make v3 easy, and some that\nwould require expanding the series quite a bit. I'm not sure if those\nbigger ones are worth doing or not. :)\n\n> Thank you again for all of your feedback, and my apologies that it has taken me\n> so long to respond. I was out of the office last week, and have been quite busy\n> since catching up on Git LFS-related issues.\n\nNo problem. Open source moves at the pace of patch submissions.\n\n-Peff\n"},{"id":"343301","messageId":"20180329011634.68582-1-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180324005556.8145-1-me@ttaylorr.com","subject":"[PATCH v3 0/4] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:16:31Z","receivedAt":"2018-03-29T01:16:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi all,\n\nAttached is 'v3' of my patch series to add a `--default` option to `git config`.\n\nThe most notable change since the last reroll is that this series is now based\nupon tb/config-type-specifier-option [1], with discussion about that change here\n[2]. This was motivated by my and Peff's shared concern that by adding `--color`\nas a top-level type specifier, that we'd be \"squatting\" on adding `--color` in\nthe traditional sense of \"colorize this output.\"\n\nAs such, `--color` is _not_ added as a top-level type specifier in this version,\nrather, it is accessible via `--type=color`. This leaves `--color` unused and\navailable for other uses in the future.\n\nOther changes since last time include:\n\n  1. Fixing an erroneous diff in Documentation/git-config.txt, where a header\n     was removed.\n\n  2. `git config` will now die immediately when the `--default` value is not\n      canonicalize able under the given type specifier.\n\n  3. Removing redundancies from t1300 and t1310 that are--as Peff wisely points\n     out--unlikely to catch any meaningful regressions in the future.\n\n  4. Documenting behavior in builtin/config.c that we \"throw away\" the\n     canonicalized value during normalization.\n\nAs always, thank you for the helpful feedback. I look forward to your thoughts\non this version of the series.\n\n\nThanks,\nTaylor\n\nTaylor Blau (3):\n  builtin/config: introduce `--default`\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `color` type specifier\n\n Documentation/git-config.txt | 15 ++++++++++----\n builtin/config.c             | 38 ++++++++++++++++++++++++++++++++++++\n config.c                     | 10 ++++++++++\n config.h                     |  1 +\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++\n t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 128 insertions(+), 4 deletions(-)\n create mode 100755 t/t1310-config-default.sh\n\n--\n2.16.2.440.gc6284da4f\n\n"},{"id":"343302","messageId":"20180329011634.68582-2-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v3 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:16:32Z","receivedAt":"2018-03-29T01:16:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For some use cases, callers of the `git-config(1)` builtin would like to\nfallback to default values when the slot asked for does not exist. In\naddition, users would like to use existing type specifiers to ensure\nthat values are parsed correctly when they do exist in the\nconfiguration.\n\nFor example, to fetch a value without a type specifier and fallback to\n`$fallback`, the following is required:\n\n  $ git config core.foo || echo \"$fallback\"\n\nThis is fine for most values, but can be tricky for difficult-to-express\n`$fallback`'s, like ANSI color codes.\n\nThis motivates `--get-color`, which is a one-off exception to the normal\ntype specifier rules wherein a user specifies both the configuration\nslot and an optional fallback. Both are formatted according to their\ntype specifier, which eases the burden on the user to ensure that values\nare correctly formatted.\n\nThis commit (and those following it in this series) aim to eventually\nreplace `--get-color` with a consistent alternative. By introducing\n`--default`, we allow the `--get-color` action to be promoted to a\n`--color` type specifier, retaining the \"fallback\" behavior via the\n`--default` flag introduced in this commit.\n\nFor example, we aim to replace:\n\n  $ git config --get-color slot [default] [...]\n\nwith:\n\n  $ git config --default default --color slot [...]\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguration.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--color`, which (in conjunction\nwith `--default`) will be sufficient to replace `--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  4 ++++\n builtin/config.c             | 17 ++++++++++++++++\n t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex a4a5ffb41..5aa528878 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -235,6 +235,10 @@ Valid `[type]`'s include:\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+--default value::\n+  When using `--get`, and the requested slot is not found, behave as if value\n+  were the value assigned to the that slot.\n+\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\ndiff --git a/builtin/config.c b/builtin/config.c\nindex ea7923493..4340f5f3d 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -27,6 +27,7 @@ static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, types;\n static char *type;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -96,6 +97,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n \tOPT_END(),\n };\n \n@@ -260,6 +262,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tstrbuf_init(item, 0);\n+\t\tif (format_config(item, key_, default_value) < 0) {\n+\t\t\texit(1);\n+\t\t}\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n@@ -618,6 +630,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (default_value && !(actions & ACTION_GET)) {\n+\t\terror(\"--default is only applicable to --get\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n+\n \tif (type) {\n \t\tif (types != 0) {\n \t\t\terror(\"usage of --type is ambiguous\");\ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 000000000..ab4e35f06\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'uses --default when missing entry' '\n+\techo quux >expect &&\n+\tgit config -f config --default quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'canonicalizes --default with appropriate type' '\n+\techo true >expect &&\n+\tgit config -f config --default=true --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'uses entry when available' '\n+\techo bar >expect &&\n+\tgit config --add core.foo bar &&\n+\tgit config --default baz core.foo >actual &&\n+\tgit config --unset core.foo &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'dies when --default cannot be parsed' '\n+\ttest_must_fail git config -f config --type=int --default=x --get \\\n+\t\tnot.a.section 2>error &&\n+\ttest_i18ngrep \"invalid unit\" error\n+'\n+\n+test_expect_success 'does not allow --default without --get' '\n+\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"343303","messageId":"20180329011634.68582-3-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:16:33Z","receivedAt":"2018-03-29T01:16:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb..33366b52c 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac..0e060779d 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"343304","messageId":"20180329011634.68582-4-me@ttaylorr.com","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v3 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:16:34Z","receivedAt":"2018-03-29T01:16:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --type=color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--color` and encourage `--type=color`,\n`--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 11 +++++++----\n builtin/config.c             | 21 +++++++++++++++++++++\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 5aa528878..aaba36615 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If\n you want to handle the lines that do *not* match the regex, just\n prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n \n-A type specifier may be given as an argument to `--type` to make 'git config'\n-ensure that the variable(s) are of the given type and convert the value to the\n-canonical form. If no type specifier is passed, no checks or transformations are\n-performed on the value.\n+`color`::\n+    The value is taken as an ANSI color escape sequence.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n@@ -177,6 +175,7 @@ Valid `[type]`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative ate-string\n   to a timestamp. This specifier has no effect when setting the value.\n+- 'color': canonicalize by converting to an ANSI color escape sequence.\n +\n \n --bool::\n@@ -184,6 +183,7 @@ Valid `[type]`'s include:\n --bool-or-int::\n --path::\n --expiry-date::\n+--color::\n   Historical options for selecting a type specifier. Prefer instead `--type`,\n   (see: above).\n \n@@ -223,6 +223,9 @@ Valid `[type]`'s include:\n \toutput it as the ANSI color escape sequence to the standard\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n++\n+It is preferred to use `--type=color`, or `--type=color --default=[default]`\n+instead of `--get-color`.\n \n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4340f5f3d..1aae228a6 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -62,6 +62,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT (1<<2)\n #define TYPE_PATH (1<<3)\n #define TYPE_EXPIRY_DATE (1<<4)\n+#define TYPE_COLOR (1<<5)\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -177,6 +178,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (types == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -322,6 +328,19 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (types == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (!git_config_color(v, key, value))\n+\t\t\t/*\n+\t\t\t * The contents of `v` now contain an ANSI escape\n+\t\t\t * sequence, not suitable for including within a\n+\t\t\t * configuration file. Treat the above as a\n+\t\t\t * \"sanity-check\", and return the given value, which we\n+\t\t\t * know is representable as valid color code.\n+\t\t\t */\n+\t\t\treturn xstrdup(value);\n+\t\tdie(\"cannot parse color '%s'\", value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", types);\n }\n@@ -519,6 +538,8 @@ static int type_name_to_specifier(char *name)\n \t\treturn TYPE_PATH;\n \telse if (!(strcmp(name, \"expiry-date\")))\n \t\treturn TYPE_EXPIRY_DATE;\n+\telse if (!(strcmp(name, \"color\")))\n+\t\treturn TYPE_COLOR;\n \tdie(_(\"unexpected --type argument, %s\"), name);\n }\n \ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 12dc94bd2..12e852a1d 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --type=color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --type=color foo.color >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --type=color' '\n+\trm .git/config &&\n+\tgit config --type=color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --type=color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --type=color foo.bar\n+'\n+\n+test_expect_success 'set --type=color barfs on non-color' '\n+\ttest_must_fail git config --type=color foo.color \"not-a-color\" 2>error &&\n+\ttest_i18ngrep \"cannot parse color\" error\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.16.2.440.gc6284da4f\n\n"},{"id":"343305","messageId":"20180329012905.GA71044@syl.local","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"Re: [PATCH v3 0/4] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:29:05Z","receivedAt":"2018-03-29T01:29:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 28, 2018 at 06:16:31PM -0700, Taylor Blau wrote:\n> Attached is 'v3' of my patch series to add a `--default` option to `git config`.\n\nMy apologies, this patch series is only 3 long, so the subject line of\nthis message is incorrect.\n\n\nThanks,\nTaylor\n"},{"id":"343306","messageId":"20180329013116.GB71044@syl.local","threadId":"47986","inReplyTo":"20180326083442.GB18714@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/4] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:31:16Z","receivedAt":"2018-03-29T01:31:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 26, 2018 at 04:34:42AM -0400, Jeff King wrote:\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > index e09ed5d7d..d9e389a33 100644\n> > --- a/Documentation/git-config.txt\n> > +++ b/Documentation/git-config.txt\n> > @@ -233,8 +233,10 @@ See also <<FILES>>.\n> >  \tusing `--file`, `--global`, etc) and `on` when searching all\n> >  \tconfig files.\n> >\n> > -CONFIGURATION\n> > --------------\n> > +--default value::\n> > +  When using `--get`, behave as if value were the value assigned to the given\n> > +  slot.\n> > +\n> >  `pager.config` is only respected when listing configuration, i.e., when\n> >  using `--list` or any of the `--get-*` which may return multiple results.\n> >  The default is to use a pager.\n>\n> Hmm, what's going on with the CONFIGURATION header here?\n\nMy mistake, I have correct this erroneous diff in v3. Thanks for\npointing it out!\n\n> For the description of \"--default\" itself, do we want to say something\n> like:\n>\n>   When using `--get` and the requested slot is not found, behave as\n>   if...\n>\n> That is hopefully a given from the name \"--default\", but it seems like\n> an important point to mention.\n\nDitto.\n\n> > +test_expect_success 'marshals default value as int' '\n> > +\techo 810 >expect &&\n> > +\tgit config --default 810 --int core.foo >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success 'marshals default value as int (storage unit)' '\n> > +\techo 1048576 >expect &&\n> > +\tgit config --default 1M --int core.foo >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n>\n> I'm not sure what we're really testing in the first one. The storage\n> unit conversion is the interesting bit.\n>\n> TBH, I'm not sure we need to separately test each possible type\n> conversion here. If we know that the type conversions are tested\n> elsewhere (and I would not be surprised if they're not, but let's assume\n> for a moment...) and we check that type conversions are applied to\n> \"--default\" values for some types, I don't think there's any reason to\n> assume that the others would not work.\n\nAgreed. I don't think that this test (and the ones that your comment\nbelow concerns) are testing anything meaningful, or that would catch any\ninteresting future regressions. I have removed them from this series in\nv3.\n\n\nThanks,\nTaylor\n"},{"id":"343307","messageId":"20180329013228.GC71044@syl.local","threadId":"47986","inReplyTo":"20180326085556.GC18714@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/4] Documentation: list all type specifiers in config prose","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:32:28Z","receivedAt":"2018-03-29T01:32:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 26, 2018 at 04:55:56AM -0400, Jeff King wrote:\n> In fact, that kind of makes me wonder if this \"type\" list should not\n> exist at all. If we instead grouped the options and said \"these are the\n> type options\", then we'd only need one list.\n>\n> I'm OK to punt on that for now, though, in the interest of not holding\n> up this patch series. I do think we should say something like:\n>\n>   Each type can be specified with the matching command-line option\n>   (e.g., `--bool`, `--int`, etc); see <<OPTIONS>> below.\n\nI punted on this for now, since rebasing on\ntb/config-type-specifier-option makes this commit a no-op.\n\n\nThanks,\nTaylor\n"},{"id":"343308","messageId":"20180329013646.GD71044@syl.local","threadId":"47986","inReplyTo":"20180326091645.GD18714@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] builtin/config: introduce `--color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-03-29T01:36:46Z","receivedAt":"2018-03-29T01:36:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Mar 26, 2018 at 05:16:45AM -0400, Jeff King wrote:\n> On Fri, Mar 23, 2018 at 08:55:56PM -0400, Taylor Blau wrote:\n>\n> > As of this commit, the canonical way to retreive an ANSI-compatible\n> > color escape sequence from a configuration file is with the\n> > `--get-color` action.\n>\n> s/retreive/retrieve/\n\nAck. I forgot to correct this in v3. I will remember to fix this in\nanticipation of v4 before this is queued.\n\n> > This is to allow Git to \"fall back\" on a default value for the color\n> > should the given section not exist in the specified configuration(s).\n> >\n> > With the addition of `--default`, this is no longer needed since:\n> >\n> >   $ git config --default red --color core.section\n> >\n> > will be have exactly as:\n> >\n> >   $ git config --get-color core.section red\n> >\n> > For consistency, let's introduce `--color` and encourage `--color`,\n> > `--default` together over `--get-color` alone.\n>\n> I don't think we'll ever get rid of --get-color (at the very least, we'd\n> need a deprecation period). But it's probably worth adding a note under\n> the --get-color description to mention that it's a historical synonym,\n> and that using \"--default\" should be preferred.\n\nI added a note in v3 under the `--get-color` section that it is now\nconsidered \"historical\", and it is instead preferred to use\n\n  $ git config --default=<default> --type=color <section>\n\n> > @@ -90,6 +91,7 @@ static struct option builtin_config_options[] = {\n> >  \tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n> >  \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n> >  \tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n> > +\tOPT_BIT(0, \"color\", &types, N_(\"value is a color\"), TYPE_COLOR),\n> >  \tOPT_GROUP(N_(\"Other\")),\n> >  \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n> >  \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n>\n> I just had a funny thought. Normally in Git the \"--color\" option means\n> \"colorize the output\". And we are diverging from that here. I wonder if\n> anybody would be confused by that, or if we would ever want to later add\n> an option to colorize git-config output. Would we regret squatting on\n> --color?\n>\n> I'm not sure what else to name it. Anything _except_ \"--color\" would\n> diverge from the existing scheme of \"--<type>\".\n>\n> If we were designing from scratch, I'd consider:\n>\n>   git config --type=int ...\n>   git config --type=color ...\n>\n> etc. I'm not sure if it's worth trying to switch now (on the other hand,\n> it resolves the documentation issue I mentioned earlier, since that\n> would naturally group all of the types ;) ).\n>\n> It would be pretty easy to declare \"--type\" as the Right Way, and list\n> \"--int\" as a historical synonym for \"--type=int\".\n\nAgreed. I have posted a topic to the list containing a patch to the\nabove effect.\n\n> > @@ -320,6 +327,12 @@ static char *normalize_value(const char *key, const char *value)\n> >  \t\telse\n> >  \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n> >  \t}\n> > +\tif (types == TYPE_COLOR) {\n> > +\t\tchar v[COLOR_MAXLEN];\n> > +\t\tif (!git_config_color(v, key, value))\n> > +\t\t\treturn xstrdup(value);\n> > +\t\tdie(\"cannot parse color '%s'\", value);\n> > +\t}\n>\n> Interesting. This doesn't actually normalize anything, since we always\n> pass back the original value (or die). I think that's the right thing to\n> do, since otherwise you'd end up with ANSI codes in your config file.\n>\n> I wondered at first if this should go in the \"noop\" normalization that\n> TYPE_PATH undergoes. But I like that it actually sanity-checks the\n> value. We should probably have a comment here explaining that yes, we\n> parse and then throw away the value (similar to the one near TYPE_PATH).\n\nThat was my original intent, but I agree that this is confusing without\na comment explaining so. I have added such a comment in v3.\n\n> I suspect that TYPE_EXPIRY_DATE should do the same thing (parse and\n> complain if you fed nonsense, but always keep the original value).\n\nWhere?\n\n> > +test_expect_success 'get --color' '\n> > +\trm .git/config &&\n> > +\tgit config foo.color \"red\" &&\n> > +\tgit config --get --color foo.color | test_decode_color >actual &&\n> > +\techo \"<RED>\" >expect &&\n> > +\ttest_cmp expect actual\n> > +'\n>\n> We should probably write this as:\n>\n>   git config --get --color foo.color >actual.raw &&\n>   test_decode_color <actual.raw >actual\n>\n> to catch failures from git-config itself (there's a lot of old tests\n> which pipe, but we've been trying to convert them to be more careful).\n\nSounds good; I have added this in v3.\n\n> > +test_expect_success 'set --color' '\n> > +\trm .git/config &&\n> > +\tgit config --color foo.color \"red\" &&\n> > +\ttest_cmp expect .git/config\n> > +'\n> > +\n> > +test_expect_success 'get --color barfs on non-color' '\n> > +\techo \"[foo]bar=not-a-color\" >.git/config &&\n> > +\ttest_must_fail git config --get --color foo.bar\n> > +'\n>\n> After reading the normalize bits above, I think there's one more case to\n> cover:\n>\n>   test_must_fail git config --color foo.color not-a-color\n\nGood idea, I've added this in v3.\n\n> > diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\n> > index 0e464c206..0ebece7d2 100755\n> > --- a/t/t1310-config-default.sh\n> > +++ b/t/t1310-config-default.sh\n> > @@ -62,6 +62,12 @@ test_expect_success 'marshal default value as expiry-date' '\n> >  \ttest_cmp expect actual\n> >  '\n> >\n> > +test_expect_success 'marshal default value as color' '\n> > +\techo \"\\033[31m\" >expect &&\n> > +\tgit config --default red --color core.foo >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n>\n> I don't offhand recall whether octal escapes with \"echo\" are portable.\n> It _is_ in POSIX, but only for XSI. I think using \"printf\" would be\n> portable.\n\nI removed this test in accordance with your earlier suggestions, so this\nchange is no longer required.\n\n\nThanks,\nTaylor\n"},{"id":"343443","messageId":"xmqqy3i930bl.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180329011634.68582-2-me@ttaylorr.com","subject":"Re: [PATCH v3 1/3] builtin/config: introduce `--default`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T18:06:22Z","receivedAt":"2018-03-30T18:06:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> For some use cases, callers of the `git-config(1)` builtin would like to\n> fallback to default values when the slot asked for does not exist. In\n> addition, users would like to use existing type specifiers to ensure\n> that values are parsed correctly when they do exist in the\n> configuration.\n> ...\n> +--default value::\n> +  When using `--get`, and the requested slot is not found, behave as if value\n> +  were the value assigned to the that slot.\n\nFor \"diff.<slot>.color\", the above is OK, but in general,\nconfiguration variables are not called \"slot\".  s/slot/variable/.\n"},{"id":"343444","messageId":"xmqqtvsx3060.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180329011634.68582-4-me@ttaylorr.com","subject":"Re: [PATCH v3 3/3] builtin/config: introduce `color` type specifier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-30T18:09:43Z","receivedAt":"2018-03-30T18:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> @@ -184,6 +183,7 @@ Valid `[type]`'s include:\n>  --bool-or-int::\n>  --path::\n>  --expiry-date::\n> +--color::\n>    Historical options for selecting a type specifier. Prefer instead `--type`,\n>    (see: above).\n>  \n> @@ -223,6 +223,9 @@ Valid `[type]`'s include:\n>  \toutput it as the ANSI color escape sequence to the standard\n>  \toutput.  The optional `default` parameter is used instead, if\n>  \tthere is no color configured for `name`.\n> ++\n> +It is preferred to use `--type=color`, or `--type=color --default=[default]`\n> +instead of `--get-color`.\n\nWasn't the whole point of the preliminary --type=<type> patch to\navoid having to add thse two hunks?\n"},{"id":"343476","messageId":"CAPig+cSk_H-Dh585JsynNfp1GSuXSkKay0Vo3Jhda6-vPdi6Uw@mail.gmail.com","threadId":"47986","inReplyTo":"20180329011634.68582-2-me@ttaylorr.com","subject":"Re: [PATCH v3 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-30T20:23:56Z","receivedAt":"2018-03-30T20:24:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> For some use cases, callers of the `git-config(1)` builtin would like to\n> fallback to default values when the slot asked for does not exist. In\n> addition, users would like to use existing type specifiers to ensure\n> that values are parsed correctly when they do exist in the\n> configuration.\n>\n> For example, to fetch a value without a type specifier and fallback to\n> `$fallback`, the following is required:\n>\n>   $ git config core.foo || echo \"$fallback\"\n>\n> This is fine for most values, but can be tricky for difficult-to-express\n> `$fallback`'s, like ANSI color codes.\n>\n> This motivates `--get-color`, which is a one-off exception to the normal\n> type specifier rules wherein a user specifies both the configuration\n> slot and an optional fallback. Both are formatted according to their\n> type specifier, which eases the burden on the user to ensure that values\n> are correctly formatted.\n>\n> This commit (and those following it in this series) aim to eventually\n> replace `--get-color` with a consistent alternative. By introducing\n> `--default`, we allow the `--get-color` action to be promoted to a\n> `--color` type specifier, retaining the \"fallback\" behavior via the\n> `--default` flag introduced in this commit.\n\nI'm confused. The cover letter said that this iteration no longer\nintroduces a --color option (favoring instead --type=color), but this\ncommit message still talks about --color. Did you mean\ns/--color/--type=color/ ?\n\n> For example, we aim to replace:\n>\n>   $ git config --get-color slot [default] [...]\n>\n> with:\n>\n>   $ git config --default default --color slot [...]\n\nDitto: s/--color/--type=color/\n\n> Values filled by `--default` behave exactly as if they were present in\n> the affected configuration file; they will be parsed by type specifiers\n> without the knowledge that they are not themselves present in the\n> configuration.\n>\n> Specifically, this means that the following will work:\n>\n>   $ git config --int --default 1M does.not.exist\n>   1048576\n>\n> In subsequent commits, we will offer `--color`, which (in conjunction\n> with `--default`) will be sufficient to replace `--get-color`.\n\nDitto: s/--color/--type=color/\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n"},{"id":"343477","messageId":"CAPig+cRbQbFLw_qarT0rcOoEgBg-7gW_pbFpiH5yuO=miLuWfA@mail.gmail.com","threadId":"47986","inReplyTo":"20180329011634.68582-3-me@ttaylorr.com","subject":"Re: [PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-30T20:26:09Z","receivedAt":"2018-03-30T20:26:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> In preparation for adding `--color` to the `git-config(1)` builtin,\n> let's introduce a color parsing utility, `git_config_color` in a similar\n> fashion to `git_config_<type>`.\n\nDid you mean s/--color/--type=color/ ?\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n"},{"id":"343839","messageId":"20180405024519.GA31883@syl.local","threadId":"47986","inReplyTo":"xmqqy3i930bl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:45:19Z","receivedAt":"2018-04-05T02:45:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 30, 2018 at 11:06:22AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > For some use cases, callers of the `git-config(1)` builtin would like to\n> > fallback to default values when the slot asked for does not exist. In\n> > addition, users would like to use existing type specifiers to ensure\n> > that values are parsed correctly when they do exist in the\n> > configuration.\n> > ...\n> > +--default value::\n> > +  When using `--get`, and the requested slot is not found, behave as if value\n> > +  were the value assigned to the that slot.\n>\n> For \"diff.<slot>.color\", the above is OK, but in general,\n> configuration variables are not called \"slot\".  s/slot/variable/.\n\nThanks; I was unaware of this convention. I have changed \"slot\" to\n\"variable\" as you suggested in the subsequent re-roll.\n\nThanks,\nTaylor\n"},{"id":"343840","messageId":"20180405024626.GB31883@syl.local","threadId":"47986","inReplyTo":"CAPig+cSk_H-Dh585JsynNfp1GSuXSkKay0Vo3Jhda6-vPdi6Uw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:46:26Z","receivedAt":"2018-04-05T02:46:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 30, 2018 at 04:23:56PM -0400, Eric Sunshine wrote:\n> On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> > This commit (and those following it in this series) aim to eventually\n> > replace `--get-color` with a consistent alternative. By introducing\n> > `--default`, we allow the `--get-color` action to be promoted to a\n> > `--color` type specifier, retaining the \"fallback\" behavior via the\n> > `--default` flag introduced in this commit.\n>\n> I'm confused. The cover letter said that this iteration no longer\n> introduces a --color option (favoring instead --type=color), but this\n> commit message still talks about --color. Did you mean\n> s/--color/--type=color/ ?\n\nMy mistake; I think I rebased this series off of the \"--type=\" series\nand forgot to amend this change. I have updated this and the below in\nthe subsequent re-roll.\n\n> > For example, we aim to replace:\n> >\n> >   $ git config --get-color slot [default] [...]\n> >\n> > with:\n> >\n> >   $ git config --default default --color slot [...]\n>\n> Ditto: s/--color/--type=color/\n\nAck.\n\n> > Values filled by `--default` behave exactly as if they were present in\n> > the affected configuration file; they will be parsed by type specifiers\n> > without the knowledge that they are not themselves present in the\n> > configuration.\n> >\n> > Specifically, this means that the following will work:\n> >\n> >   $ git config --int --default 1M does.not.exist\n> >   1048576\n> >\n> > In subsequent commits, we will offer `--color`, which (in conjunction\n> > with `--default`) will be sufficient to replace `--get-color`.\n>\n> Ditto: s/--color/--type=color/\n\nAck.\n\nThanks,\nTaylor\n"},{"id":"343841","messageId":"20180405024723.GC31883@syl.local","threadId":"47986","inReplyTo":"CAPig+cRbQbFLw_qarT0rcOoEgBg-7gW_pbFpiH5yuO=miLuWfA@mail.gmail.com","subject":"Re: [PATCH v3 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:47:23Z","receivedAt":"2018-04-05T02:47:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 30, 2018 at 04:26:09PM -0400, Eric Sunshine wrote:\n> On Wed, Mar 28, 2018 at 9:16 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> > In preparation for adding `--color` to the `git-config(1)` builtin,\n> > let's introduce a color parsing utility, `git_config_color` in a similar\n> > fashion to `git_config_<type>`.\n>\n> Did you mean s/--color/--type=color/ ?\n\nI did; thanks for pointing this out. I have fixed this to mention\n\"--type=color\" in the subsequent re-roll.\n\nThanks,\nTaylor\n"},{"id":"343842","messageId":"20180405024845.GD31883@syl.local","threadId":"47986","inReplyTo":"xmqqtvsx3060.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:48:45Z","receivedAt":"2018-04-05T02:48:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 30, 2018 at 11:09:43AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > @@ -184,6 +183,7 @@ Valid `[type]`'s include:\n> >  --bool-or-int::\n> >  --path::\n> >  --expiry-date::\n> > +--color::\n> >    Historical options for selecting a type specifier. Prefer instead `--type`,\n> >    (see: above).\n> >\n> > @@ -223,6 +223,9 @@ Valid `[type]`'s include:\n> >  \toutput it as the ANSI color escape sequence to the standard\n> >  \toutput.  The optional `default` parameter is used instead, if\n> >  \tthere is no color configured for `name`.\n> > ++\n> > +It is preferred to use `--type=color`, or `--type=color --default=[default]`\n> > +instead of `--get-color`.\n>\n> Wasn't the whole point of the preliminary --type=<type> patch to\n> avoid having to add thse two hunks?\n\nFor the first hunk, yes, but not for the second. The series that adds\n\"--type=<type>\" was meant to make it possible to say \"parse this as a\ncolor\" without squatting on the \"--color\" flag.\n\nSo, including \"--color\" in the list of historical options is a mistake.\nBut, using \"--type=color --default=...\" over \"--get-color\" is the\ndesired intention of this series.\n\n\nThanks,\nTaylor\n"},{"id":"343843","messageId":"20180405025845.GA49902@syl.local","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v4 0/3] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:58:45Z","receivedAt":"2018-04-05T02:58:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is a fourth re-roll of my series to add \"--default\" and\n\"--type=color\" to \"git config\" in order to replace:\n\n  $ git config --get-color <variable> [default]\n\nwith\n\n  $ git config [--default=<default>] --type=color <variable>\n\nSince the last revision, I have:\n\n  - Rebased it upon the latest changes from my series to add\n    \"--type=<type>\" in favor of \"--<type>\".\n\n  - Fixed various incorrect hunks mentioning \"--color\" (which does not\n    exist), cc: Eric, Junio.\n\n  - Fixed referencing \"slot\" when the correct spelling is \"variable\".\n    cc: Junio.\n\nAs always, thank you for your helpful feedback. This process is still\nnew to me, so I appreciate an extra degree of scrutiny :-).\n\nI am hopeful that this series is ready for queueing.\n\n\nThanks,\nTaylor\n\nTaylor Blau (3):\n  builtin/config: introduce `--default`\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `color` type specifier\n\n Documentation/git-config.txt | 14 +++++++++----\n builtin/config.c             | 33 +++++++++++++++++++++++++++++++\n config.c                     | 10 ++++++++++\n config.h                     |  1 +\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++\n t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 122 insertions(+), 4 deletions(-)\n create mode 100755 t/t1310-config-default.sh\n\n--\n2.17.0\n"},{"id":"343844","messageId":"20180405025912.GB49902@syl.local","threadId":"47986","inReplyTo":"cover.1522896713.git.me@ttaylorr.com","subject":"[PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:59:12Z","receivedAt":"2018-04-05T02:59:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For some use cases, callers of the `git-config(1)` builtin would like to\nfallback to default values when the variable asked for does not exist.\nIn addition, users would like to use existing type specifiers to ensure\nthat values are parsed correctly when they do exist in the\nconfiguration.\n\nFor example, to fetch a value without a type specifier and fallback to\n`$fallback`, the following is required:\n\n  $ git config core.foo || echo \"$fallback\"\n\nThis is fine for most values, but can be tricky for difficult-to-express\n`$fallback`'s, like ANSI color codes.\n\nThis motivates `--get-color`, which is a one-off exception to the normal\ntype specifier rules wherein a user specifies both the configuration\nvariable and an optional fallback. Both are formatted according to their\ntype specifier, which eases the burden on the user to ensure that values\nare correctly formatted.\n\nThis commit (and those following it in this series) aim to eventually\nreplace `--get-color` with a consistent alternative. By introducing\n`--default`, we allow the `--get-color` action to be promoted to a\n`--type=color` type specifier, retaining the \"fallback\" behavior via the\n`--default` flag introduced in this commit.\n\nFor example, we aim to replace:\n\n  $ git config --get-color variable [default] [...]\n\nwith:\n\n  $ git config --default default --type=color variable [...]\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguration.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--type=color`, which (in\nconjunction with `--default`) will be sufficient to replace\n`--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  4 ++++\n builtin/config.c             | 12 ++++++++++++\n t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 54 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex c7e56e3fd..620492d1e 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -235,6 +235,10 @@ Valid `<type>`'s include:\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+--default value::\n+  When using `--get`, and the requested slot is not found, behave as if value\n+  were the value assigned to the that slot.\n+\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 6e1495eac..1328b568b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,6 +26,7 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, type;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -122,6 +123,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n \tOPT_END(),\n };\n \n@@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tstrbuf_init(item, 0);\n+\t\tif (format_config(item, key_, default_value) < 0) {\n+\t\t\texit(1);\n+\t\t}\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 000000000..ab4e35f06\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'uses --default when missing entry' '\n+\techo quux >expect &&\n+\tgit config -f config --default quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'canonicalizes --default with appropriate type' '\n+\techo true >expect &&\n+\tgit config -f config --default=true --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'uses entry when available' '\n+\techo bar >expect &&\n+\tgit config --add core.foo bar &&\n+\tgit config --default baz core.foo >actual &&\n+\tgit config --unset core.foo &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'dies when --default cannot be parsed' '\n+\ttest_must_fail git config -f config --type=int --default=x --get \\\n+\t\tnot.a.section 2>error &&\n+\ttest_i18ngrep \"invalid unit\" error\n+'\n+\n+test_expect_success 'does not allow --default without --get' '\n+\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.17.0\n\n"},{"id":"343845","messageId":"20180405025914.GC49902@syl.local","threadId":"47986","inReplyTo":"cover.1522896713.git.me@ttaylorr.com","subject":"[PATCH v4 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:59:14Z","receivedAt":"2018-04-05T02:59:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--type=color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb..33366b52c 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac..0e060779d 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.17.0\n\n"},{"id":"343846","messageId":"20180405025917.GD49902@syl.local","threadId":"47986","inReplyTo":"cover.1522896713.git.me@ttaylorr.com","subject":"[PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-05T02:59:17Z","receivedAt":"2018-04-05T02:59:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --type=color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--color` and encourage `--type=color`,\n`--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 10 ++++++----\n builtin/config.c             | 21 +++++++++++++++++++++\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 620492d1e..bde702d2e 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If\n you want to handle the lines that do *not* match the regex, just\n prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n \n-A type specifier may be given as an argument to `--type` to make 'git config'\n-ensure that the variable(s) are of the given type and convert the value to the\n-canonical form. If no type specifier is passed, no checks or transformations are\n-performed on the value.\n+`color`::\n+    The value is taken as an ANSI color escape sequence.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n@@ -177,6 +175,7 @@ Valid `<type>`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n   to a timestamp. This specifier has no effect when setting the value.\n+- 'color': canonicalize by converting to an ANSI color escape sequence.\n +\n \n --bool::\n@@ -223,6 +222,9 @@ Valid `<type>`'s include:\n \toutput it as the ANSI color escape sequence to the standard\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n++\n+It is preferred to use `--type=color`, or `--type=color --default=[default]`\n+instead of `--get-color`.\n \n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 1328b568b..aa3fcabe9 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT\t3\n #define TYPE_PATH\t\t4\n #define TYPE_EXPIRY_DATE\t5\n+#define TYPE_COLOR\t\t6\n \n static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t*type = TYPE_PATH;\n \telse if (!strcmp(arg, \"expiry-date\"))\n \t\t*type = TYPE_EXPIRY_DATE;\n+\telse if (!strcmp(arg, \"color\"))\n+\t\t*type = TYPE_COLOR;\n \telse {\n \t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n \t\treturn 1;\n@@ -203,6 +206,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (type == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -348,6 +356,19 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (type == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (!git_config_color(v, key, value))\n+\t\t\t/*\n+\t\t\t * The contents of `v` now contain an ANSI escape\n+\t\t\t * sequence, not suitable for including within a\n+\t\t\t * configuration file. Treat the above as a\n+\t\t\t * \"sanity-check\", and return the given value, which we\n+\t\t\t * know is representable as valid color code.\n+\t\t\t */\n+\t\t\treturn xstrdup(value);\n+\t\tdie(\"cannot parse color '%s'\", value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", type);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex b25ab7b9e..c630bdc77 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --type=color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --type=color foo.color >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --type=color' '\n+\trm .git/config &&\n+\tgit config --type=color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --type=color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --type=color foo.bar\n+'\n+\n+test_expect_success 'set --type=color barfs on non-color' '\n+\ttest_must_fail git config --type=color foo.color \"not-a-color\" 2>error &&\n+\ttest_i18ngrep \"cannot parse color\" error\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.17.0\n"},{"id":"343906","messageId":"20180405222949.GC29117@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180405025912.GB49902@syl.local","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-05T22:29:49Z","receivedAt":"2018-04-05T22:29:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:\n\n> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n>  \tconfig_with_options(collect_config, &values,\n>  \t\t\t    &given_config_source, &config_options);\n>  \n> +\tif (!values.nr && default_value) {\n> +\t\tstruct strbuf *item;\n> +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> +\t\titem = &values.items[values.nr++];\n> +\t\tstrbuf_init(item, 0);\n> +\t\tif (format_config(item, key_, default_value) < 0) {\n> +\t\t\texit(1);\n> +\t\t}\n> +\t}\n\nCalling exit() explicitly is unusual for our code. Usually we would\neither die() or propagate the error. Most of the types in\nformat_config() would die on bogus input, but a few code paths will\nreturn an error.\n\nWhat happens if a non-default value has a bogus format? E.g.:\n\n  $ git config foo.bar '~NoSuchUser'\n  $ git config --path foo.bar\n  fatal: failed to expand user dir in: '~NoSuchUser'\n\nOops. Despite us checking for an error return from\ngit_config_pathname(), it will always either return 0 or die(). So\nthat's not a good example. ;)\n\nLet's try expiry-date:\n\n  $ git config foo.bar 'the first of octember'\n  $ git config --expiry-date foo.bar\n  error: 'the first of octember' for 'foo.bar' is not a valid timestamp\n  fatal: bad config line 7 in file .git/config\n\nOK. So we call format_config() there from the actual collect_config()\ncallback, and the error gets propagated back to the config parser, which\nthen gives us an informative die(). What happens with your new code:\n\n  $ ./git config --default 'the first of octember' --type=expiry-date no.such.key\n  error: 'the first of octember' for 'no.such.key' is not a valid timestamp\n\nIt's obvious in this toy example, but that config call may be buried\ndeep in a script. It'd probably be nicer for that exit(1) to be\nsomething like:\n\n  die(_(\"failed to format default config value\"));\n\n> +test_expect_success 'does not allow --default without --get' '\n> +\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n> +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n> +'\n\nI think \"a\" here needs to be \"a.section\". I get:\n\n  error: key does not contain a section: a\n\n-Peff\n"},{"id":"343907","messageId":"20180405223620.GD29117@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180405025917.GD49902@syl.local","subject":"Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-05T22:36:20Z","receivedAt":"2018-04-05T22:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:\n\n> As of this commit, the canonical way to retreive an ANSI-compatible\n> color escape sequence from a configuration file is with the\n> `--get-color` action.\n> \n> This is to allow Git to \"fall back\" on a default value for the color\n> should the given section not exist in the specified configuration(s).\n> \n> With the addition of `--default`, this is no longer needed since:\n> \n>   $ git config --default red --type=color core.section\n> \n> will be have exactly as:\n> \n>   $ git config --get-color core.section red\n> \n> For consistency, let's introduce `--color` and encourage `--type=color`,\n> `--default` together over `--get-color` alone.\n\nIn this last sentence, did you mean \"let's introduce --type=color and\nencourage its use with --default over --get-color alone\"?\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  Documentation/git-config.txt | 10 ++++++----\n>  builtin/config.c             | 21 +++++++++++++++++++++\n>  t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n>  3 files changed, 57 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 620492d1e..bde702d2e 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If\n>  you want to handle the lines that do *not* match the regex, just\n>  prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n>  \n> -A type specifier may be given as an argument to `--type` to make 'git config'\n> -ensure that the variable(s) are of the given type and convert the value to the\n> -canonical form. If no type specifier is passed, no checks or transformations are\n> -performed on the value.\n> +`color`::\n> +    The value is taken as an ANSI color escape sequence.\n\nWe'd want to keep that introductory paragraph, right? And there is no\n`--color`? So I think this hunk can go away (and is presumably a\nleftover mistake during rebasing).\n\n>  When reading, the values are read from the system, global and\n>  repository local configuration files by default, and options\n> @@ -177,6 +175,7 @@ Valid `<type>`'s include:\n>    ~/` from the command line to let your shell do the expansion.)\n>  - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n>    to a timestamp. This specifier has no effect when setting the value.\n> +- 'color': canonicalize by converting to an ANSI color escape sequence.\n>  +\n\nThis one is part of the --type list, so that's what we expect.\n\nYou may want to also cover the behavior when setting the value (we check\nthat it's sane, but store the original).\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 1328b568b..aa3fcabe9 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -61,6 +61,7 @@ static int show_origin;\n>  #define TYPE_BOOL_OR_INT\t3\n>  #define TYPE_PATH\t\t4\n>  #define TYPE_EXPIRY_DATE\t5\n> +#define TYPE_COLOR\t\t6\n\nNot strictly necessary for this series, but if this became an enum as\npart of the de-bitifying, you wouldn't have to write the numbers\nmanually. :)\n\n> @@ -203,6 +206,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n>  \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n>  \t\t\t\treturn -1;\n>  \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n> +\t\t} else if (type == TYPE_COLOR) {\n> +\t\t\tchar v[COLOR_MAXLEN];\n> +\t\t\tif (git_config_color(v, key_, value_) < 0)\n> +\t\t\t\treturn -1;\n> +\t\t\tstrbuf_addstr(buf, v);\n>  \t\t} else if (value_) {\n>  \t\t\tstrbuf_addstr(buf, value_);\n>  \t\t} else {\n\nOK, formatting shows the converted value. Good.\n\n> @@ -348,6 +356,19 @@ static char *normalize_value(const char *key, const char *value)\n>  \t\telse\n>  \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n>  \t}\n> +\tif (type == TYPE_COLOR) {\n> +\t\tchar v[COLOR_MAXLEN];\n> +\t\tif (!git_config_color(v, key, value))\n> +\t\t\t/*\n> +\t\t\t * The contents of `v` now contain an ANSI escape\n> +\t\t\t * sequence, not suitable for including within a\n> +\t\t\t * configuration file. Treat the above as a\n> +\t\t\t * \"sanity-check\", and return the given value, which we\n> +\t\t\t * know is representable as valid color code.\n> +\t\t\t */\n> +\t\t\treturn xstrdup(value);\n> +\t\tdie(\"cannot parse color '%s'\", value);\n> +\t}\n\nAnd this returns the original. Good.\n\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index b25ab7b9e..c630bdc77 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n\nThe tests look good.\n\n-Peff\n"},{"id":"343908","messageId":"20180405223707.GE29117@sigill.intra.peff.net","threadId":"47986","inReplyTo":"20180405025845.GA49902@syl.local","subject":"Re: [PATCH v4 0/3] Teach `--default` to `git-config(1)`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-05T22:37:07Z","receivedAt":"2018-04-05T22:37:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 04, 2018 at 07:58:45PM -0700, Taylor Blau wrote:\n\n> Hi,\n> \n> Attached is a fourth re-roll of my series to add \"--default\" and\n> \"--type=color\" to \"git config\" in order to replace:\n> \n>   $ git config --get-color <variable> [default]\n> \n> with\n> \n>   $ git config [--default=<default>] --type=color <variable>\n\nI think this is getting close, but I found a few minor problems to\naddress.\n\n-Peff\n"},{"id":"343909","messageId":"CAPig+cQwj8tqJToKU0_emHHC+mdtpPS666XJ7sqentMAFoLivw@mail.gmail.com","threadId":"47986","inReplyTo":"20180405025912.GB49902@syl.local","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-05T22:40:03Z","receivedAt":"2018-04-05T22:40:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> [...]\n> This commit (and those following it in this series) aim to eventually\n> replace `--get-color` with a consistent alternative. By introducing\n> `--default`, we allow the `--get-color` action to be promoted to a\n> `--type=color` type specifier, retaining the \"fallback\" behavior via the\n> `--default` flag introduced in this commit.\n> [...]\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> @@ -235,6 +235,10 @@ Valid `<type>`'s include:\n> +--default value::\n\nThis should be typeset as: --default <value>\n\n> +  When using `--get`, and the requested slot is not found, behave as if value\n> +  were the value assigned to the that slot.\n\nAnd you might use \"behaves as if <value>\" in the body as well (though\nI think Git documentation isn't terribly consistent about typesetting\nas \"<value>\" in the body, so I don't insist upon it).\n"},{"id":"343916","messageId":"CAPig+cQoTQ47n13q3t5MMS+UZQR6jbFDpVSP8Z=L4qckjrsxeQ@mail.gmail.com","threadId":"47986","inReplyTo":"20180405223620.GD29117@sigill.intra.peff.net","subject":"Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-05T22:52:29Z","receivedAt":"2018-04-05T22:52:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:\n>> +     if (type == TYPE_COLOR) {\n>> +             char v[COLOR_MAXLEN];\n>> +             if (!git_config_color(v, key, value))\n>> +                     /*\n>> +                      * The contents of `v` now contain an ANSI escape\n>> +                      * sequence, not suitable for including within a\n>> +                      * configuration file. Treat the above as a\n>> +                      * \"sanity-check\", and return the given value, which we\n>> +                      * know is representable as valid color code.\n>> +                      */\n>> +                     return xstrdup(value);\n>> +             die(\"cannot parse color '%s'\", value);\n>> +     }\n>\n> And this returns the original. Good.\n\nIt's entirely subjective, but I find this easier to read and\nunderstand when written like this:\n\n    char v[COLOR_MAXLEN];\n    if (git_config_color(v, key, value))\n        die(\"cannot parse color '%s'\", value);\n\n    /*\n     * The contents of `v` now contain an ANSI escape\n     * sequence, not suitable for including within a\n     * configuration file. Treat the above as a\n     * \"sanity-check\", and return the given value, which we\n     * know is representable as valid color code.\n    */\n    return xstrdup(value);\n"},{"id":"343917","messageId":"20180405225320.GA10913@sigill.intra.peff.net","threadId":"47986","inReplyTo":"CAPig+cQoTQ47n13q3t5MMS+UZQR6jbFDpVSP8Z=L4qckjrsxeQ@mail.gmail.com","subject":"Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-05T22:53:20Z","receivedAt":"2018-04-05T22:53:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:\n\n> On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:\n> > On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:\n> >> +     if (type == TYPE_COLOR) {\n> >> +             char v[COLOR_MAXLEN];\n> >> +             if (!git_config_color(v, key, value))\n> >> +                     /*\n> >> +                      * The contents of `v` now contain an ANSI escape\n> >> +                      * sequence, not suitable for including within a\n> >> +                      * configuration file. Treat the above as a\n> >> +                      * \"sanity-check\", and return the given value, which we\n> >> +                      * know is representable as valid color code.\n> >> +                      */\n> >> +                     return xstrdup(value);\n> >> +             die(\"cannot parse color '%s'\", value);\n> >> +     }\n> >\n> > And this returns the original. Good.\n> \n> It's entirely subjective, but I find this easier to read and\n> understand when written like this:\n> \n>     char v[COLOR_MAXLEN];\n>     if (git_config_color(v, key, value))\n>         die(\"cannot parse color '%s'\", value);\n> \n>     /*\n>      * The contents of `v` now contain an ANSI escape\n>      * sequence, not suitable for including within a\n>      * configuration file. Treat the above as a\n>      * \"sanity-check\", and return the given value, which we\n>      * know is representable as valid color code.\n>     */\n>     return xstrdup(value);\n\nIt may be subjective, but I happen to agree with you.\n\n-Peff\n"},{"id":"343927","messageId":"20180406052901.GB60769@syl.local","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v5 0/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:29:01Z","receivedAt":"2018-04-06T05:29:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\n(Apologies for the noise of accidentally sending an empty template cover\nletter. Here is the real one :-).)\n\nAttached is my fifth, and anticipated final re-roll of my series to add\n\"--type=<type>\" to \"git config\" in an effort to replace \"--bool\" with\n\"--type=bool\".\n\nI have changed the following since v4:\n\n  - Remove extra space when redirecting in t1300 (cc: Peff).\n\n  - Clarify the effect of using \"--type\" in Documentation (cc: Eric).\n\n  - Document \"--no-type\" in Documentation (cc: Eric).\n\n  - Fix an outstanding typo in Documentation, \"input output\" (cc: Peff).\n\nThanks as always for your feedback -- I look forward to having this queued.\n\n\nThanks,\nTaylor\n\nTaylor Blau (2):\n  builtin/config.c: treat type specifiers singularly\n  builtin/config.c: prefer `--type=bool` over `--bool`, etc.\n\n Documentation/git-config.txt | 74 +++++++++++++++++++---------------\n builtin/config.c             | 77 +++++++++++++++++++++++-------------\n t/t1300-repo-config.sh       | 29 ++++++++++++++\n 3 files changed, 121 insertions(+), 59 deletions(-)\n\n--\n2.17.0\n"},{"id":"343928","messageId":"20180406052724.GA60769@syl.local","threadId":"47986","inReplyTo":"20180329011634.68582-1-me@ttaylorr.com","subject":"[PATCH v5 0/2] *** SUBJECT HERE ***","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:27:24Z","receivedAt":"2018-04-06T05:29:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"*** BLURB HERE ***\n\nTaylor Blau (2):\n  builtin/config.c: treat type specifiers singularly\n  builtin/config.c: prefer `--type=bool` over `--bool`, etc.\n\n Documentation/git-config.txt | 74 +++++++++++++++++++---------------\n builtin/config.c             | 77 +++++++++++++++++++++++-------------\n t/t1300-repo-config.sh       | 29 ++++++++++++++\n 3 files changed, 121 insertions(+), 59 deletions(-)\n\n-- \n2.17.0\n"},{"id":"343929","messageId":"20180406052904.GC60769@syl.local","threadId":"47986","inReplyTo":"cover.1522992443.git.me@ttaylorr.com","subject":"[PATCH v5 1/2] builtin/config.c: treat type specifiers singularly","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:29:04Z","receivedAt":"2018-04-06T05:29:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Internally, we represent `git config`'s type specifiers as a bitset\nusing OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique\nallows for the representation of multiple type specifiers in the `int\ntypes` field, but this multi-representation is left unused.\n\nIn fact, `git config` will not accept multiple type specifiers at a\ntime, as indicated by:\n\n  $ git config --int --bool some.section\n  error: only one type at a time.\n\nThis patch uses `OPT_SET_INT` to prefer the _last_ mentioned type\nspecifier, so that the above command would instead be valid, and a\nsynonym of:\n\n  $ git config --bool some.section\n\nThis change is motivated by two urges: (1) it does not make sense to\nrepresent a singular type specifier internally as a bitset, only to\ncomplain when there are multiple bits in the set. `OPT_SET_INT` is more\nwell-suited to this task than `OPT_BIT` is. (2) a future patch will\nintroduce `--type=<type>`, and we would like not to complain in the\nfollowing situation:\n\n  $ git config --int --type=int\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n builtin/config.c       | 49 +++++++++++++++++++-----------------------\n t/t1300-repo-config.sh | 11 ++++++++++\n 2 files changed, 33 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 01169dd62..92fb8d56b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -25,7 +25,7 @@ static char term = '\\n';\n \n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n-static int actions, types;\n+static int actions, type;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -55,11 +55,11 @@ static int show_origin;\n #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \\\n \t\t\tACTION_GET_REGEXP | ACTION_GET_URLMATCH)\n \n-#define TYPE_BOOL (1<<0)\n-#define TYPE_INT (1<<1)\n-#define TYPE_BOOL_OR_INT (1<<2)\n-#define TYPE_PATH (1<<3)\n-#define TYPE_EXPIRY_DATE (1<<4)\n+#define TYPE_BOOL\t\t1\n+#define TYPE_INT\t\t2\n+#define TYPE_BOOL_OR_INT\t3\n+#define TYPE_PATH\t\t4\n+#define TYPE_EXPIRY_DATE\t5\n \n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"get-color\", &actions, N_(\"find the color configured: slot [default]\"), ACTION_GET_COLOR),\n \tOPT_BIT(0, \"get-colorbool\", &actions, N_(\"find the color setting: slot [stdout-is-tty]\"), ACTION_GET_COLORBOOL),\n \tOPT_GROUP(N_(\"Type\")),\n-\tOPT_BIT(0, \"bool\", &types, N_(\"value is \\\"true\\\" or \\\"false\\\"\"), TYPE_BOOL),\n-\tOPT_BIT(0, \"int\", &types, N_(\"value is decimal number\"), TYPE_INT),\n-\tOPT_BIT(0, \"bool-or-int\", &types, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n-\tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n-\tOPT_BIT(0, \"expiry-date\", &types, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n+\tOPT_SET_INT(0, \"bool\", &type, N_(\"value is \\\"true\\\" or \\\"false\\\"\"), TYPE_BOOL),\n+\tOPT_SET_INT(0, \"int\", &type, N_(\"value is decimal number\"), TYPE_INT),\n+\tOPT_SET_INT(0, \"bool-or-int\", &type, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\n+\tOPT_SET_INT(0, \"path\", &type, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n+\tOPT_SET_INT(0, \"expiry-date\", &type, N_(\"value is an expiry date\"), TYPE_EXPIRY_DATE),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\tif (show_keys)\n \t\t\tstrbuf_addch(buf, key_delim);\n \n-\t\tif (types == TYPE_INT)\n+\t\tif (type == TYPE_INT)\n \t\t\tstrbuf_addf(buf, \"%\"PRId64,\n \t\t\t\t    git_config_int64(key_, value_ ? value_ : \"\"));\n-\t\telse if (types == TYPE_BOOL)\n+\t\telse if (type == TYPE_BOOL)\n \t\t\tstrbuf_addstr(buf, git_config_bool(key_, value_) ?\n \t\t\t\t      \"true\" : \"false\");\n-\t\telse if (types == TYPE_BOOL_OR_INT) {\n+\t\telse if (type == TYPE_BOOL_OR_INT) {\n \t\t\tint is_bool, v;\n \t\t\tv = git_config_bool_or_int(key_, value_, &is_bool);\n \t\t\tif (is_bool)\n \t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t\telse\n \t\t\t\tstrbuf_addf(buf, \"%d\", v);\n-\t\t} else if (types == TYPE_PATH) {\n+\t\t} else if (type == TYPE_PATH) {\n \t\t\tconst char *v;\n \t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addstr(buf, v);\n \t\t\tfree((char *)v);\n-\t\t} else if (types == TYPE_EXPIRY_DATE) {\n+\t\t} else if (type == TYPE_EXPIRY_DATE) {\n \t\t\ttimestamp_t t;\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)\n \tif (!value)\n \t\treturn NULL;\n \n-\tif (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)\n+\tif (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)\n \t\t/*\n \t\t * We don't do normalization for TYPE_PATH here: If\n \t\t * the path is like ~/foobar/, we prefer to store\n@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)\n \t\t * Also don't do normalization for expiry dates.\n \t\t */\n \t\treturn xstrdup(value);\n-\tif (types == TYPE_INT)\n+\tif (type == TYPE_INT)\n \t\treturn xstrfmt(\"%\"PRId64, git_config_int64(key, value));\n-\tif (types == TYPE_BOOL)\n+\tif (type == TYPE_BOOL)\n \t\treturn xstrdup(git_config_bool(key, value) ?  \"true\" : \"false\");\n-\tif (types == TYPE_BOOL_OR_INT) {\n+\tif (type == TYPE_BOOL_OR_INT) {\n \t\tint is_bool, v;\n \t\tv = git_config_bool_or_int(key, value, &is_bool);\n \t\tif (!is_bool)\n@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n \n-\tdie(\"BUG: cannot normalize type %d\", types);\n+\tdie(\"BUG: cannot normalize type %d\", type);\n }\n \n static int get_color_found;\n@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tkey_delim = '\\n';\n \t}\n \n-\tif (HAS_MULTI_BITS(types)) {\n-\t\terror(\"only one type at a time.\");\n-\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n-\t}\n-\n-\tif ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {\n+\tif ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {\n \t\terror(\"--get-color and variable type are incoherent\");\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 4f8e6f5fd..24de37d54 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '\n \ttest_expect_code 128 nongit git config --local foo.bar\n '\n \n+cat >.git/config <<-\\EOF &&\n+[core]\n+number = 10\n+EOF\n+\n+test_expect_success 'later legacy specifiers are given precedence' '\n+\tgit config --bool --int core.number >actual &&\n+\techo 10 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.17.0\n\n"},{"id":"343930","messageId":"20180406052907.GD60769@syl.local","threadId":"47986","inReplyTo":"cover.1522992443.git.me@ttaylorr.com","subject":"[PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:29:07Z","receivedAt":"2018-04-06T05:29:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"`git config` has long allowed the ability for callers to provide a 'type\nspecifier', which instructs `git config` to (1) ensure that incoming\nvalues are satisfiable under that type, and (2) that outgoing values are\ncanonicalized under that type.\n\nIn another series, we propose to add extend this functionality with\n`--type=color` and `--default` to replace `--get-color`.\n\nHowever, we traditionally use `--color` to mean \"colorize this output\",\ninstead of \"this value should be treated as a color\".\n\nCurrently, `git config` does not support this kind of colorization, but\nwe should be careful to avoid inhabiting this option too soon, so that\n`git config` can support `--type=color` (in the traditional sense) in\nthe future, if that is desired.\n\nIn this patch, we prefer `--type=<int|bool|bool-or-int|...>` over\n`--int`, `--bool`, and etc. This allows the aforementioned upcoming\npatch to support querying a color value with a default via `--type=color\n--default=....`, without squandering `--color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt | 74 ++++++++++++++++++++----------------\n builtin/config.c             | 28 ++++++++++++++\n t/t1300-repo-config.sh       | 18 +++++++++\n 3 files changed, 88 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex e09ed5d7d..31931ae00 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,13 +9,13 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n-'git config' [<file-option>] [type] --add name value\n-'git config' [<file-option>] [type] --replace-all name value [value_regex]\n-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]\n-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]\n-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]\n-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL\n+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]\n+'git config' [<file-option>] [--type] --add name value\n+'git config' [<file-option>] [--type] --replace-all name value [value_regex]\n+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get name [value_regex]\n+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] --get-all name [value_regex]\n+'git config' [<file-option>] [--type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]\n+'git config' [<file-option>] [--type] [-z|--null] --get-urlmatch name URL\n 'git config' [<file-option>] --unset name [value_regex]\n 'git config' [<file-option>] --unset-all name [value_regex]\n 'git config' [<file-option>] --rename-section old_name new_name\n@@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If\n you want to handle the lines that do *not* match the regex, just\n prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n \n-The type specifier can be either `--int` or `--bool`, to make\n-'git config' ensure that the variable(s) are of the given type and\n-convert the value to the canonical form (simple decimal number for int,\n-a \"true\" or \"false\" string for bool), or `--path`, which does some\n-path expansion (see `--path` below).  If no type specifier is passed, no\n-checks or transformations are performed on the value.\n+The `--type` option requests 'git config' to ensure that the configured values\n+assosciated with the given variable(s) are of the given type. When given\n+`--type`, 'git config' will convert the value to that type's canonical form.\n+ensure that the variable(s) are of the given type and convert the value to the\n+canonical form. If no type specifier is passed, no checks or transformations are\n+performed on the value. Callers may unset any pre-existing type specifiers with\n+`--no-type`.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n@@ -160,30 +161,39 @@ See also <<FILES>>.\n --list::\n \tList all variables set in config file, along with their values.\n \n---bool::\n-\t'git config' will ensure that the output is \"true\" or \"false\"\n+--type <type>::\n+  'git config' will ensure that any input or output is valid under the given\n+  type constraint(s), and will canonicalize outgoing values in `<type>`'s\n+  canonical form.\n++\n+Valid `<type>`'s include:\n++\n+- 'bool': canonicalize values as either \"true\" or \"false\".\n+- 'int': canonicalize values as simple decimal numbers. An optional suffix of\n+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or\n+  1073741824 prior to output.\n+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described\n+  above.\n+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and\n+  `~user` to the home directory for the specified user. This specifier has no\n+  effect when setting the value (but you can use `git config section.variable\n+  ~/` from the command line to let your shell do the expansion.)\n+- 'expiry-date': canonicalize by converting from a fixed or relative date-string\n+  to a timestamp. This specifier has no effect when setting the value.\n++\n \n+--bool::\n --int::\n-\t'git config' will ensure that the output is a simple\n-\tdecimal number.  An optional value suffix of 'k', 'm', or 'g'\n-\tin the config file will cause the value to be multiplied\n-\tby 1024, 1048576, or 1073741824 prior to output.\n-\n --bool-or-int::\n-\t'git config' will ensure that the output matches the format of\n-\teither --bool or --int, as described above.\n-\n --path::\n-\t`git config` will expand a leading `~` to the value of\n-\t`$HOME`, and `~user` to the home directory for the\n-\tspecified user.  This option has no effect when setting the\n-\tvalue (but you can use `git config section.variable ~/`\n-\tfrom the command line to let your shell do the expansion).\n-\n --expiry-date::\n-\t`git config` will ensure that the output is converted from\n-\ta fixed or relative date-string to a timestamp. This option\n-\thas no effect when setting the value.\n+  Historical options for selecting a type specifier. Prefer instead `--type`,\n+  (see: above).\n+\n+--no-type::\n+  Un-sets the previously set type specifier (if one was previously set). This\n+  option requests that 'git config' not canonicalize the retrieved variable.\n+  `--no-type` has no effect without `--type=<type>` or `--<type>`.\n \n -z::\n --null::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 92fb8d56b..6e1495eac 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,33 @@ static int show_origin;\n #define TYPE_PATH\t\t4\n #define TYPE_EXPIRY_DATE\t5\n \n+static int option_parse_type(const struct option *opt, const char *arg,\n+\t\t\t     int unset)\n+{\n+\tint *type = opt->value;\n+\n+\tif (unset) {\n+\t\t*type = 0;\n+\t\treturn 0;\n+\t}\n+\n+\tif (!strcmp(arg, \"bool\"))\n+\t\t*type = TYPE_BOOL;\n+\telse if (!strcmp(arg, \"int\"))\n+\t\t*type = TYPE_INT;\n+\telse if (!strcmp(arg, \"bool-or-int\"))\n+\t\t*type = TYPE_BOOL_OR_INT;\n+\telse if (!strcmp(arg, \"path\"))\n+\t\t*type = TYPE_PATH;\n+\telse if (!strcmp(arg, \"expiry-date\"))\n+\t\t*type = TYPE_EXPIRY_DATE;\n+\telse {\n+\t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static struct option builtin_config_options[] = {\n \tOPT_GROUP(N_(\"Config file location\")),\n \tOPT_BOOL(0, \"global\", &use_global_config, N_(\"use global config file\")),\n@@ -84,6 +111,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"get-color\", &actions, N_(\"find the color configured: slot [default]\"), ACTION_GET_COLOR),\n \tOPT_BIT(0, \"get-colorbool\", &actions, N_(\"find the color setting: slot [stdout-is-tty]\"), ACTION_GET_COLORBOOL),\n \tOPT_GROUP(N_(\"Type\")),\n+\tOPT_CALLBACK('t', \"type\", &type, N_(\"type\"), N_(\"value is given this type\"), option_parse_type),\n \tOPT_SET_INT(0, \"bool\", &type, N_(\"value is \\\"true\\\" or \\\"false\\\"\"), TYPE_BOOL),\n \tOPT_SET_INT(0, \"int\", &type, N_(\"value is decimal number\"), TYPE_INT),\n \tOPT_SET_INT(0, \"bool-or-int\", &type, N_(\"value is --bool or --int\"), TYPE_BOOL_OR_INT),\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 24de37d54..f5ae80e9a 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1613,6 +1613,7 @@ test_expect_success '--local requires a repo' '\n \n cat >.git/config <<-\\EOF &&\n [core]\n+foo = true\n number = 10\n EOF\n \n@@ -1622,4 +1623,21 @@ test_expect_success 'later legacy specifiers are given precedence' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--type allows valid type specifiers' '\n+\techo \"true\" >expect &&\n+\tgit config --type=bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--no-type unsets type specifiers' '\n+\techo \"10\" >expect &&\n+\tgit config --type=bool --no-type core.number >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--type rejects unknown specifiers' '\n+\ttest_must_fail git config --type=nonsense core.foo 2>error &&\n+\ttest_i18ngrep \"unrecognized --type argument\" error\n+'\n+\n test_done\n-- \n2.17.0\n"},{"id":"343931","messageId":"20180406053252.GA61207@syl.local","threadId":"47986","inReplyTo":"20180406052901.GB60769@syl.local","subject":"Re: [PATCH v5 0/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:32:52Z","receivedAt":"2018-04-06T05:33:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 05, 2018 at 10:29:01PM -0700, Taylor Blau wrote:\n> Hi,\n\nAck. Clearly I am still getting used to things outside of Git LFS. I have sent\nthis topic in response to my series to add \"--default\" instead of the\nappropriate series.\n\nI'll avoid re-sending it in an effort to reduce further noise. In the meantime,\nthanks for understanding my silly mistakes.\n\n\nThanks,\nTaylor\n"},{"id":"343932","messageId":"20180406054015.GA62593@syl.local","threadId":"47986","inReplyTo":"20180405222949.GC29117@sigill.intra.peff.net","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:40:15Z","receivedAt":"2018-04-06T05:40:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 05, 2018 at 06:29:49PM -0400, Jeff King wrote:\n> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:\n>\n> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n> >  \tconfig_with_options(collect_config, &values,\n> >  \t\t\t    &given_config_source, &config_options);\n> >\n> > +\tif (!values.nr && default_value) {\n> > +\t\tstruct strbuf *item;\n> > +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> > +\t\titem = &values.items[values.nr++];\n> > +\t\tstrbuf_init(item, 0);\n> > +\t\tif (format_config(item, key_, default_value) < 0) {\n> > +\t\t\texit(1);\n> > +\t\t}\n> > +\t}\n>\n> Calling exit() explicitly is unusual for our code. Usually we would\n> either die() or propagate the error. Most of the types in\n> format_config() would die on bogus input, but a few code paths will\n> return an error.\n>\n> What happens if a non-default value has a bogus format? E.g.:\n>\n>   $ git config foo.bar '~NoSuchUser'\n>   $ git config --path foo.bar\n>   fatal: failed to expand user dir in: '~NoSuchUser'\n>\n> Oops. Despite us checking for an error return from\n> git_config_pathname(), it will always either return 0 or die(). So\n> that's not a good example. ;)\n>\n> Let's try expiry-date:\n>\n>   $ git config foo.bar 'the first of octember'\n>   $ git config --expiry-date foo.bar\n>   error: 'the first of octember' for 'foo.bar' is not a valid timestamp\n>   fatal: bad config line 7 in file .git/config\n>\n> OK. So we call format_config() there from the actual collect_config()\n> callback, and the error gets propagated back to the config parser, which\n> then gives us an informative die(). What happens with your new code:\n>\n>   $ ./git config --default 'the first of octember' --type=expiry-date no.such.key\n>   error: 'the first of octember' for 'no.such.key' is not a valid timestamp\n>\n> It's obvious in this toy example, but that config call may be buried\n> deep in a script. It'd probably be nicer for that exit(1) to be\n> something like:\n>\n>   die(_(\"failed to format default config value\"));\n\nAha. Thanks for the explanation :-). I've removed the exit() and changed\nit to the die() that you suggested above. The test in t1310 is updated\nto grep for the new message, so we know that it is being reported\nappropriately.\n\n> > +test_expect_success 'does not allow --default without --get' '\n> > +\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n> > +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n> > +'\n>\n> I think \"a\" here needs to be \"a.section\". I get:\n>\n>   error: key does not contain a section: a\n\nAha, thanks again. I have updated this in the forthcoming re-roll.\n\n\nThanks,\nTaylor\n"},{"id":"343933","messageId":"CA+P7+xpjsmuuDBBgUDswBkMbUONEmPh8Nd4hnu5j52hYyhTjmw@mail.gmail.com","threadId":"47986","inReplyTo":"20180406052724.GA60769@syl.local","subject":"Re: [PATCH v5 0/2] *** SUBJECT HERE ***","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-04-06T05:40:12Z","receivedAt":"2018-04-06T05:40:38Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 5, 2018 at 10:27 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> *** BLURB HERE ***\n>\n\nMissing subject and cover letter stuff.. did you submit before you\nwere ready? or did you not mean to include the cover letter? :)\n\n-Jake\n\n> Taylor Blau (2):\n>   builtin/config.c: treat type specifiers singularly\n>   builtin/config.c: prefer `--type=bool` over `--bool`, etc.\n>\n>  Documentation/git-config.txt | 74 +++++++++++++++++++---------------\n>  builtin/config.c             | 77 +++++++++++++++++++++++-------------\n>  t/t1300-repo-config.sh       | 29 ++++++++++++++\n>  3 files changed, 121 insertions(+), 59 deletions(-)\n>\n> --\n> 2.17.0\n"},{"id":"343934","messageId":"20180406055037.GB62593@syl.local","threadId":"47986","inReplyTo":"CAPig+cQwj8tqJToKU0_emHHC+mdtpPS666XJ7sqentMAFoLivw@mail.gmail.com","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T05:50:37Z","receivedAt":"2018-04-06T05:50:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 05, 2018 at 06:40:03PM -0400, Eric Sunshine wrote:\n> On Wed, Apr 4, 2018 at 10:59 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> > [...]\n> > This commit (and those following it in this series) aim to eventually\n> > replace `--get-color` with a consistent alternative. By introducing\n> > `--default`, we allow the `--get-color` action to be promoted to a\n> > `--type=color` type specifier, retaining the \"fallback\" behavior via the\n> > `--default` flag introduced in this commit.\n> > [...]\n> >\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > @@ -235,6 +235,10 @@ Valid `<type>`'s include:\n> > +--default value::\n>\n> This should be typeset as: --default <value>\n\nI see; thank you for letting me know. I have updated this in the\nforthcoming re-roll.\n\n> > +  When using `--get`, and the requested slot is not found, behave as if value\n> > +  were the value assigned to the that slot.\n>\n> And you might use \"behaves as if <value>\" in the body as well (though\n> I think Git documentation isn't terribly consistent about typesetting\n> as \"<value>\" in the body, so I don't insist upon it).\n\nSince I'm here, and re-rolling anyway, I have included this change as\nwell.\n\n\nThanks,\nTaylor\n"},{"id":"343935","messageId":"20180406060216.GC62593@syl.local","threadId":"47986","inReplyTo":"20180405223620.GD29117@sigill.intra.peff.net","subject":"Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:02:16Z","receivedAt":"2018-04-06T06:02:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 05, 2018 at 06:36:20PM -0400, Jeff King wrote:\n> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:\n>\n> > As of this commit, the canonical way to retreive an ANSI-compatible\n> > color escape sequence from a configuration file is with the\n> > `--get-color` action.\n> >\n> > This is to allow Git to \"fall back\" on a default value for the color\n> > should the given section not exist in the specified configuration(s).\n> >\n> > With the addition of `--default`, this is no longer needed since:\n> >\n> >   $ git config --default red --type=color core.section\n> >\n> > will be have exactly as:\n> >\n> >   $ git config --get-color core.section red\n> >\n> > For consistency, let's introduce `--color` and encourage `--type=color`,\n> > `--default` together over `--get-color` alone.\n>\n> In this last sentence, did you mean \"let's introduce --type=color and\n> encourage its use with --default over --get-color alone\"?\n\nI did; thank you :-). I have updated this in the forthcoming re-roll.\n\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> >  Documentation/git-config.txt | 10 ++++++----\n> >  builtin/config.c             | 21 +++++++++++++++++++++\n> >  t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n> >  3 files changed, 57 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > index 620492d1e..bde702d2e 100644\n> > --- a/Documentation/git-config.txt\n> > +++ b/Documentation/git-config.txt\n> > @@ -38,10 +38,8 @@ existing values that match the regexp are updated or unset.  If\n> >  you want to handle the lines that do *not* match the regex, just\n> >  prepend a single exclamation mark in front (see also <<EXAMPLES>>).\n> >\n> > -A type specifier may be given as an argument to `--type` to make 'git config'\n> > -ensure that the variable(s) are of the given type and convert the value to the\n> > -canonical form. If no type specifier is passed, no checks or transformations are\n> > -performed on the value.\n> > +`color`::\n> > +    The value is taken as an ANSI color escape sequence.\n>\n> We'd want to keep that introductory paragraph, right? And there is no\n> `--color`? So I think this hunk can go away (and is presumably a\n> leftover mistake during rebasing).\n\nYes; this entire hunk should not have been staged. I think that it was a\nleftover from rebasing. I have removed it in the forthcoming re-roll.\n\n> >  When reading, the values are read from the system, global and\n> >  repository local configuration files by default, and options\n> > @@ -177,6 +175,7 @@ Valid `<type>`'s include:\n> >    ~/` from the command line to let your shell do the expansion.)\n> >  - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n> >    to a timestamp. This specifier has no effect when setting the value.\n> > +- 'color': canonicalize by converting to an ANSI color escape sequence.\n> >  +\n>\n> This one is part of the --type list, so that's what we expect.\n>\n> You may want to also cover the behavior when setting the value (we check\n> that it's sane, but store the original).\n\nGood idea; thank you.\n\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > index 1328b568b..aa3fcabe9 100644\n> > --- a/builtin/config.c\n> > +++ b/builtin/config.c\n> > @@ -61,6 +61,7 @@ static int show_origin;\n> >  #define TYPE_BOOL_OR_INT\t3\n> >  #define TYPE_PATH\t\t4\n> >  #define TYPE_EXPIRY_DATE\t5\n> > +#define TYPE_COLOR\t\t6\n>\n> Not strictly necessary for this series, but if this became an enum as\n> part of the de-bitifying, you wouldn't have to write the numbers\n> manually. :)\n\nCertainly agree that this would be useful. I think that I'd prefer to\naccomplish this in a future series, since this one is starting to become\nlong-lived :-).\n\n> > @@ -203,6 +206,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> >  \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n> >  \t\t\t\treturn -1;\n> >  \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n> > +\t\t} else if (type == TYPE_COLOR) {\n> > +\t\t\tchar v[COLOR_MAXLEN];\n> > +\t\t\tif (git_config_color(v, key_, value_) < 0)\n> > +\t\t\t\treturn -1;\n> > +\t\t\tstrbuf_addstr(buf, v);\n> >  \t\t} else if (value_) {\n> >  \t\t\tstrbuf_addstr(buf, value_);\n> >  \t\t} else {\n>\n> OK, formatting shows the converted value. Good.\n>\n> > @@ -348,6 +356,19 @@ static char *normalize_value(const char *key, const char *value)\n> >  \t\telse\n> >  \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n> >  \t}\n> > +\tif (type == TYPE_COLOR) {\n> > +\t\tchar v[COLOR_MAXLEN];\n> > +\t\tif (!git_config_color(v, key, value))\n> > +\t\t\t/*\n> > +\t\t\t * The contents of `v` now contain an ANSI escape\n> > +\t\t\t * sequence, not suitable for including within a\n> > +\t\t\t * configuration file. Treat the above as a\n> > +\t\t\t * \"sanity-check\", and return the given value, which we\n> > +\t\t\t * know is representable as valid color code.\n> > +\t\t\t */\n> > +\t\t\treturn xstrdup(value);\n> > +\t\tdie(\"cannot parse color '%s'\", value);\n> > +\t}\n>\n> And this returns the original. Good.\n>\n> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> > index b25ab7b9e..c630bdc77 100755\n> > --- a/t/t1300-repo-config.sh\n> > +++ b/t/t1300-repo-config.sh\n>\n> The tests look good.\n\nThank you.\n\n\nThanks,\nTaylor\n"},{"id":"343936","messageId":"20180406060504.GD62593@syl.local","threadId":"47986","inReplyTo":"20180405225320.GA10913@sigill.intra.peff.net","subject":"Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:05:04Z","receivedAt":"2018-04-06T06:05:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Apr 05, 2018 at 06:53:20PM -0400, Jeff King wrote:\n> On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:\n>\n> > On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:\n> > > On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:\n> > >> +     if (type == TYPE_COLOR) {\n> > >> +             char v[COLOR_MAXLEN];\n> > >> +             if (!git_config_color(v, key, value))\n> > >> +                     /*\n> > >> +                      * The contents of `v` now contain an ANSI escape\n> > >> +                      * sequence, not suitable for including within a\n> > >> +                      * configuration file. Treat the above as a\n> > >> +                      * \"sanity-check\", and return the given value, which we\n> > >> +                      * know is representable as valid color code.\n> > >> +                      */\n> > >> +                     return xstrdup(value);\n> > >> +             die(\"cannot parse color '%s'\", value);\n> > >> +     }\n> > >\n> > > And this returns the original. Good.\n> >\n> > It's entirely subjective, but I find this easier to read and\n> > understand when written like this:\n> >\n> >     char v[COLOR_MAXLEN];\n> >     if (git_config_color(v, key, value))\n> >         die(\"cannot parse color '%s'\", value);\n> >\n> >     /*\n> >      * The contents of `v` now contain an ANSI escape\n> >      * sequence, not suitable for including within a\n> >      * configuration file. Treat the above as a\n> >      * \"sanity-check\", and return the given value, which we\n> >      * know is representable as valid color code.\n> >     */\n> >     return xstrdup(value);\n>\n> It may be subjective, but I happen to agree with you.\n\nI agree, too :-). I have changed this in the forthcoming re-roll.\n\nThanks,\nTaylor\n"},{"id":"343937","messageId":"CAPig+cSYuWReTFfnN8Wg2VGKcOk5SPHHWpTdW6yks67mkOjY0Q@mail.gmail.com","threadId":"47986","inReplyTo":"20180406052907.GD60769@syl.local","subject":"Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-06T06:14:34Z","receivedAt":"2018-04-06T06:14:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 1:29 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> builtin/config.c: prefer `--type=bool` over `--bool`, etc.\n\nThis patch isn't just preferring --type; it's actually introducing the\nfunctionality. Perhaps:\n\n    builtin/config.c: support --type=<type> as preferred alias for --<type>\n\n(Not worth a re-roll.)\n\n> `git config` has long allowed the ability for callers to provide a 'type\n> specifier', which instructs `git config` to (1) ensure that incoming\n> values are satisfiable under that type, and (2) that outgoing values are\n> canonicalized under that type.\n>\n> In another series, we propose to add extend this functionality with\n\n\"add extend\"?\n\n> `--type=color` and `--default` to replace `--get-color`.\n>\n> However, we traditionally use `--color` to mean \"colorize this output\",\n> instead of \"this value should be treated as a color\".\n>\n> Currently, `git config` does not support this kind of colorization, but\n> we should be careful to avoid inhabiting this option too soon, so that\n> `git config` can support `--type=color` (in the traditional sense) in\n> the future, if that is desired.\n>\n> In this patch, we prefer `--type=<int|bool|bool-or-int|...>` over\n> `--int`, `--bool`, and etc. This allows the aforementioned upcoming\n> patch to support querying a color value with a default via `--type=color\n> --default=....`, without squandering `--color`.\n\nDrop one of the periods in \"....\" if you happen to re-roll for some reason.\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> @@ -9,13 +9,13 @@ git-config - Get and set repository or global options\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n> +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]\n\nIt's pretty confusing to see bare \"--type\" like this which makes it\nseem as if it doesn't take an argument at all. Better would be\n\"[--type=<type>]\".\n\n> @@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If\n> +The `--type` option requests 'git config' to ensure that the configured values\n> +assosciated with the given variable(s) are of the given type. When given\n\ns/assosciated/associated/\n\n> +`--type`, 'git config' will convert the value to that type's canonical form.\n> +ensure that the variable(s) are of the given type and convert the value to the\n> +canonical form.\n\nMeh, looks like some sort of editing botch here. I doubt you meant to\nrepeat \"canonical form\" like this.\n\n> If no type specifier is passed, no checks or transformations are\n> +performed on the value. Callers may unset any pre-existing type specifiers with\n> +`--no-type`.\n>\n> +--no-type::\n> +  Un-sets the previously set type specifier (if one was previously set). This\n> +  option requests that 'git config' not canonicalize the retrieved variable.\n> +  `--no-type` has no effect without `--type=<type>` or `--<type>`.\n\nThe final sentence doesn't really add value; I'd probably drop it as\nextraneous. (Subjective, and not worth a re-roll.)\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> @@ -61,6 +61,33 @@ static int show_origin;\n> +static int option_parse_type(const struct option *opt, const char *arg,\n> +                            int unset)\n> +{\n> +       [...]\n> +       if (!strcmp(arg, \"bool\"))\n> +               *type = TYPE_BOOL;\n> +       else if (!strcmp(arg, \"int\"))\n> +               *type = TYPE_INT;\n> +       else if (!strcmp(arg, \"bool-or-int\"))\n> +               *type = TYPE_BOOL_OR_INT;\n> +       else if (!strcmp(arg, \"path\"))\n> +               *type = TYPE_PATH;\n> +       else if (!strcmp(arg, \"expiry-date\"))\n> +               *type = TYPE_EXPIRY_DATE;\n> +       else {\n> +               die(_(\"unrecognized --type argument, %s\"), arg);\n> +               return 1;\n\nPointless unreachable 'return'.\n\n> +       }\n> +       return 0;\n> +}\n"},{"id":"343938","messageId":"20180406063009.GA662@syl.local","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH v5 0/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:30:09Z","receivedAt":"2018-04-06T06:30:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is the fifth re-roll of my series to add \"--default\" and\n\"--type=color\" to \"git config\" in order to introduce a consistency\nbetween type-specifiers and the historic \"--get-color\".\n\nI have changed the following since the last series:\n\n  - Replace exit() with die() in the case where the given \"--default\"\n    cannot be parsed. (cc: Peff).\n\n  - Fixed incorrectly calling \"git config\" in t1310. (cc: Peff).\n\n  - Correct typesetting in Documentation (cc: Eric).\n\n  - Perform guard checking in a negative context (cc: Eric & Peff).\n\n  - Add notes to Documentation stating when we do and do not sanitize\n    values into a \".gitconfig\". (cc: Peff).\n\n  - Remove a hunk left over during rebasing (cc: Peff).\n\n  - Replace one last outdated reference to \"--color\" (cc: Peff).\n\nThanks as always for your review :-).\n\n\nThanks,\nTaylor\n\nTaylor Blau (3):\n  builtin/config: introduce `--default`\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `color` type specifier\n\n Documentation/git-config.txt | 11 ++++++++++\n builtin/config.c             | 41 ++++++++++++++++++++++++++++++++++++\n config.c                     | 10 +++++++++\n config.h                     |  1 +\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++\n t/t1310-config-default.sh    | 38 +++++++++++++++++++++++++++++++++\n 6 files changed, 131 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\n--\n2.17.0\n"},{"id":"343939","messageId":"20180406063013.GB662@syl.local","threadId":"47986","inReplyTo":"cover.1522996150.git.me@ttaylorr.com","subject":"[PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:30:13Z","receivedAt":"2018-04-06T06:30:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For some use cases, callers of the `git-config(1)` builtin would like to\nfallback to default values when the variable asked for does not exist.\nIn addition, users would like to use existing type specifiers to ensure\nthat values are parsed correctly when they do exist in the\nconfiguration.\n\nFor example, to fetch a value without a type specifier and fallback to\n`$fallback`, the following is required:\n\n  $ git config core.foo || echo \"$fallback\"\n\nThis is fine for most values, but can be tricky for difficult-to-express\n`$fallback`'s, like ANSI color codes.\n\nThis motivates `--get-color`, which is a one-off exception to the normal\ntype specifier rules wherein a user specifies both the configuration\nvariable and an optional fallback. Both are formatted according to their\ntype specifier, which eases the burden on the user to ensure that values\nare correctly formatted.\n\nThis commit (and those following it in this series) aim to eventually\nreplace `--get-color` with a consistent alternative. By introducing\n`--default`, we allow the `--get-color` action to be promoted to a\n`--type=color` type specifier, retaining the \"fallback\" behavior via the\n`--default` flag introduced in this commit.\n\nFor example, we aim to replace:\n\n  $ git config --get-color variable [default] [...]\n\nwith:\n\n  $ git config --default default --type=color variable [...]\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguration.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--type=color`, which (in\nconjunction with `--default`) will be sufficient to replace\n`--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  4 ++++\n builtin/config.c             | 19 ++++++++++++++++++\n t/t1310-config-default.sh    | 38 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 61 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 31931ae00..28567f75a 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -243,6 +243,10 @@ Valid `<type>`'s include:\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+--default <value>::\n+  When using `--get`, and the requested variable is not found, behave as if\n+  <value> were the value assigned to the that variable.\n+\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 6e1495eac..e8da4009e 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,6 +26,7 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, type;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -122,6 +123,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n \tOPT_END(),\n };\n \n@@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tstrbuf_init(item, 0);\n+\t\tif (format_config(item, key_, default_value) < 0) {\n+\t\t\tdie(_(\"failed to format default config value\"));\n+\t\t}\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n@@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\n+\tif (default_value && !(actions & ACTION_GET)) {\n+\t\terror(\"--default is only applicable to --get\");\n+\t\tusage_with_options(builtin_config_usage,\n+\t\t\tbuiltin_config_options);\n+\t}\n+\n \tif (actions & PAGING_ACTIONS)\n \t\tsetup_auto_pager(\"config\", 1);\n \ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 000000000..b74c93276\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'uses --default when missing entry' '\n+\techo quux >expect &&\n+\tgit config -f config --default quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'canonicalizes --default with appropriate type' '\n+\techo true >expect &&\n+\tgit config -f config --default=true --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'uses entry when available' '\n+\techo bar >expect &&\n+\tgit config --add core.foo bar &&\n+\tgit config --default baz core.foo >actual &&\n+\tgit config --unset core.foo &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'dies when --default cannot be parsed' '\n+\ttest_must_fail git config -f config --type=expiry-date --default=x --get \\\n+\t\tnot.a.section 2>error &&\n+\ttest_i18ngrep \"failed to format default config value\" error\n+'\n+\n+test_expect_success 'does not allow --default without --get' '\n+\ttest_must_fail git config --default=quux --unset a.section >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.17.0\n\n"},{"id":"343940","messageId":"20180406063015.GC662@syl.local","threadId":"47986","inReplyTo":"cover.1522996150.git.me@ttaylorr.com","subject":"[PATCH v5 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:30:15Z","receivedAt":"2018-04-06T06:30:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--type=color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb..33366b52c 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac..0e060779d 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.17.0\n\n"},{"id":"343941","messageId":"20180406063017.GD662@syl.local","threadId":"47986","inReplyTo":"cover.1522996150.git.me@ttaylorr.com","subject":"[PATCH v5 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:30:17Z","receivedAt":"2018-04-06T06:30:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --type=color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--type=color` and encourage its use\nwith `--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  7 +++++++\n builtin/config.c             | 22 ++++++++++++++++++++++\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 59 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 28567f75a..b49366a56 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -180,6 +180,10 @@ Valid `<type>`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n   to a timestamp. This specifier has no effect when setting the value.\n+- 'color': When writing to a caller, canonicalize by converting to an ANSI color\n+  escape sequence. When writing to the configuration file, a sanity-check is\n+  performed to ensure that the given value is canonicalize-able as an ANSI\n+  color, but it is written as-is.\n +\n \n --bool::\n@@ -231,6 +235,9 @@ Valid `<type>`'s include:\n \toutput it as the ANSI color escape sequence to the standard\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n++\n+It is preferred to use `--type=color`, or `--type=color [--default=<default>]`\n+instead of `--get-color`.\n \n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex e8da4009e..77f8cc25f 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT\t3\n #define TYPE_PATH\t\t4\n #define TYPE_EXPIRY_DATE\t5\n+#define TYPE_COLOR\t\t6\n \n static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t*type = TYPE_PATH;\n \telse if (!strcmp(arg, \"expiry-date\"))\n \t\t*type = TYPE_EXPIRY_DATE;\n+\telse if (!strcmp(arg, \"color\"))\n+\t\t*type = TYPE_COLOR;\n \telse {\n \t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n \t\treturn 1;\n@@ -203,6 +206,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (type == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -348,6 +356,20 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (type == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (git_config_color(v, key, value))\n+\t\t\tdie(\"cannot parse color '%s'\", value);\n+\n+\t\t/*\n+\t\t * The contents of `v` now contain an ANSI escape\n+\t\t * sequence, not suitable for including within a\n+\t\t * configuration file. Treat the above as a\n+\t\t * \"sanity-check\", and return the given value, which we\n+\t\t * know is representable as valid color code.\n+\t\t */\n+\t\treturn xstrdup(value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", type);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex f5ae80e9a..c49473306 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --type=color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --type=color foo.color >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --type=color' '\n+\trm .git/config &&\n+\tgit config --type=color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --type=color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --type=color foo.bar\n+'\n+\n+test_expect_success 'set --type=color barfs on non-color' '\n+\ttest_must_fail git config --type=color foo.color \"not-a-color\" 2>error &&\n+\ttest_i18ngrep \"cannot parse color\" error\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.17.0\n"},{"id":"343945","messageId":"20180406064157.GA3500@syl.local","threadId":"47986","inReplyTo":"CAPig+cSYuWReTFfnN8Wg2VGKcOk5SPHHWpTdW6yks67mkOjY0Q@mail.gmail.com","subject":"Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-06T06:41:57Z","receivedAt":"2018-04-06T06:42:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:\n> On Fri, Apr 6, 2018 at 1:29 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> > builtin/config.c: prefer `--type=bool` over `--bool`, etc.\n>\n> This patch isn't just preferring --type; it's actually introducing the\n> functionality. Perhaps:\n>\n>     builtin/config.c: support --type=<type> as preferred alias for --<type>\n>\n> (Not worth a re-roll.)\n\nThanks; I like this more, even though it is lengthy.\n\n> > `git config` has long allowed the ability for callers to provide a 'type\n> > specifier', which instructs `git config` to (1) ensure that incoming\n> > values are satisfiable under that type, and (2) that outgoing values are\n> > canonicalized under that type.\n> >\n> > In another series, we propose to add extend this functionality with\n>\n> \"add extend\"?\n\nThanks for pointing this out -- I fixed it in v6.\n\n> > `--type=color` and `--default` to replace `--get-color`.\n> >\n> > However, we traditionally use `--color` to mean \"colorize this output\",\n> > instead of \"this value should be treated as a color\".\n> >\n> > Currently, `git config` does not support this kind of colorization, but\n> > we should be careful to avoid inhabiting this option too soon, so that\n> > `git config` can support `--type=color` (in the traditional sense) in\n> > the future, if that is desired.\n> >\n> > In this patch, we prefer `--type=<int|bool|bool-or-int|...>` over\n> > `--int`, `--bool`, and etc. This allows the aforementioned upcoming\n> > patch to support querying a color value with a default via `--type=color\n> > --default=....`, without squandering `--color`.\n>\n> Drop one of the periods in \"....\" if you happen to re-roll for some reason.\n\nGreat catch, thanks.\n\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options\n> >  SYNOPSIS\n> >  --------\n> >  [verse]\n> > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n> > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]\n>\n> It's pretty confusing to see bare \"--type\" like this which makes it\n> seem as if it doesn't take an argument at all. Better would be\n> \"[--type=<type>]\".\n\nAgreed; I have made this change in v6.\n\n> > @@ -38,12 +38,13 @@ existing values that match the regexp are updated or unset.  If\n> > +The `--type` option requests 'git config' to ensure that the configured values\n> > +assosciated with the given variable(s) are of the given type. When given\n>\n> s/assosciated/associated/\n\nDitto.\n\n> > +`--type`, 'git config' will convert the value to that type's canonical form.\n> > +ensure that the variable(s) are of the given type and convert the value to the\n> > +canonical form.\n>\n> Meh, looks like some sort of editing botch here. I doubt you meant to\n> repeat \"canonical form\" like this.\n\nDitto.\n\n> > If no type specifier is passed, no checks or transformations are\n> > +performed on the value. Callers may unset any pre-existing type specifiers with\n> > +`--no-type`.\n> >\n> > +--no-type::\n> > +  Un-sets the previously set type specifier (if one was previously set). This\n> > +  option requests that 'git config' not canonicalize the retrieved variable.\n> > +  `--no-type` has no effect without `--type=<type>` or `--<type>`.\n>\n> The final sentence doesn't really add value; I'd probably drop it as\n> extraneous. (Subjective, and not worth a re-roll.)\n\nHm. I like the completeness of it -- I think that I would prefer to\nleave it in, unless others have strong feelings.\n\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > @@ -61,6 +61,33 @@ static int show_origin;\n> > +static int option_parse_type(const struct option *opt, const char *arg,\n> > +                            int unset)\n> > +{\n> > +       [...]\n> > +       if (!strcmp(arg, \"bool\"))\n> > +               *type = TYPE_BOOL;\n> > +       else if (!strcmp(arg, \"int\"))\n> > +               *type = TYPE_INT;\n> > +       else if (!strcmp(arg, \"bool-or-int\"))\n> > +               *type = TYPE_BOOL_OR_INT;\n> > +       else if (!strcmp(arg, \"path\"))\n> > +               *type = TYPE_PATH;\n> > +       else if (!strcmp(arg, \"expiry-date\"))\n> > +               *type = TYPE_EXPIRY_DATE;\n> > +       else {\n> > +               die(_(\"unrecognized --type argument, %s\"), arg);\n> > +               return 1;\n>\n> Pointless unreachable 'return'.\n\nRemoved -- this is a habit of mine from Git LFS. We have a function\nsimilar to die(), but Go doesn't know that it will terminate the\nprogram, so it forces me to write a \"return\" after it.\n\n\nThanks,\nTaylor\n"},{"id":"343946","messageId":"CAPig+cRKaxECLHb1id6Mcd0O3uOiDzdGB4ZxPt1UpwUDi9Xb+g@mail.gmail.com","threadId":"47986","inReplyTo":"20180406063013.GB662@syl.local","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-06T06:53:45Z","receivedAt":"2018-04-06T06:53:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> [...]\n> This commit (and those following it in this series) aim to eventually\n> replace `--get-color` with a consistent alternative. By introducing\n> `--default`, we allow the `--get-color` action to be promoted to a\n> `--type=color` type specifier, retaining the \"fallback\" behavior via the\n> `--default` flag introduced in this commit.\n>\n> For example, we aim to replace:\n>\n>   $ git config --get-color variable [default] [...]\n>\n> with:\n>\n>   $ git config --default default --type=color variable [...]\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> diff --git a/builtin/config.c b/builtin/config.c\n> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n> +       if (!values.nr && default_value) {\n> +               struct strbuf *item;\n> +               ALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> +               item = &values.items[values.nr++];\n> +               strbuf_init(item, 0);\n> +               if (format_config(item, key_, default_value) < 0) {\n> +                       die(_(\"failed to format default config value\"));\n> +               }\n\nUnnecessary braces.\n\nAlso, an error message such as this is typically more helpful when it\nshows the item which caused the problem:\n\n    die(_(\"failed to format default config value: %s\"), default_value);\n\nHowever, since the message already says \"default\", it's pretty easy to\nunderstand that it's talking about the argument to --default, so it's\nperhaps not such a big deal in this case.\n\nAnyhow, neither point is worth a re-roll.\n\n> +       }\n> @@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n>                 usage_with_options(builtin_config_usage, builtin_config_options);\n>         }\n>\n> +\n\nUnnecessary new blank line.\n\n> +       if (default_value && !(actions & ACTION_GET)) {\n> +               error(\"--default is only applicable to --get\");\n> +               usage_with_options(builtin_config_usage,\n> +                       builtin_config_options);\n> +       }\n> diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\n> @@ -0,0 +1,38 @@\n> +test_expect_success 'uses --default when missing entry' '\n> +       echo quux >expect &&\n> +       git config -f config --default quux core.foo >actual &&\n> +       test_cmp expect actual\n> +'\n>\n> +test_expect_success 'canonicalizes --default with appropriate type' '\n> +       echo true >expect &&\n> +       git config -f config --default=true --bool core.foo >actual &&\n> +       test_cmp expect actual\n> +'\n\nI would trust this canonicalization test a lot more if it used one of\nthe aliases for \"true\" since it doesn't presently prove that\ncanonicalization is taking place. For instance:\n\n    git config -f config --default=yes --bool core.foo >actual &&\n\n> +test_expect_success 'uses entry when available' '\n> +       echo bar >expect &&\n> +       git config --add core.foo bar &&\n> +       git config --default baz core.foo >actual &&\n> +       git config --unset core.foo &&\n> +       test_cmp expect actual\n> +'\n\nIf you happen to re-roll, can we move this test so it immediately\nfollows the \"uses --default when missing entry\" test? That's where I\nhad expected to find it and had even written a review comment saying\nso (but deleted the comment once I got down to this spot in the\npatch). Also, perhaps rename the test to make it clearer that it\ncomplements the \"uses --default when missing entry\" test; perhaps\n\"does not fallback to --default when entry present\" or something.\n\n> +test_expect_success 'dies when --default cannot be parsed' '\n> +       test_must_fail git config -f config --type=expiry-date --default=x --get \\\n> +               not.a.section 2>error &&\n> +       test_i18ngrep \"failed to format default config value\" error\n> +'\n> +\n> +test_expect_success 'does not allow --default without --get' '\n> +       test_must_fail git config --default=quux --unset a.section >output 2>&1 &&\n> +       test_i18ngrep \"\\-\\-default is only applicable to\" output\n> +'\n"},{"id":"343948","messageId":"CAPig+cT2T8r9mefv-7GEU4khEDVrthK45X5kbVXU5c-MXU39dg@mail.gmail.com","threadId":"47986","inReplyTo":"20180406063017.GD662@syl.local","subject":"Re: [PATCH v5 3/3] builtin/config: introduce `color` type specifier","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-06T07:29:48Z","receivedAt":"2018-04-06T07:29:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> [...]\n> For consistency, let's introduce `--type=color` and encourage its use\n> with `--default` together over `--get-color` alone.\n\nA couple minor subjective comments below... neither worth a re-roll.\n\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> @@ -180,6 +180,10 @@ Valid `<type>`'s include:\n> +- 'color': When writing to a caller, canonicalize by converting to an ANSI color\n> +  escape sequence. When writing to the configuration file, a sanity-check is\n> +  performed to ensure that the given value is canonicalize-able as an ANSI\n> +  color, but it is written as-is.\n\n\"When writing to a caller\" is a somewhat confusing way to say \"When\ngetting a value\".\n\nLikewise, \"When writing to the configuration file\" could be stated\nmore concisely as \"When setting a value\".\n\n>  --bool::\n> @@ -231,6 +235,9 @@ Valid `<type>`'s include:\n>         output it as the ANSI color escape sequence to the standard\n>         output.  The optional `default` parameter is used instead, if\n>         there is no color configured for `name`.\n> ++\n> +It is preferred to use `--type=color`, or `--type=color [--default=<default>]`\n> +instead of `--get-color`.\n\nRepetitious. More conscisely:\n\n    It is preferred to use `--type=color [--default=<value>]`\n    instead of `--get-color`.\n\nOr, even:\n\n    `--type=color [--default=<value>]` is preferred over `--get-color`.\n"},{"id":"343949","messageId":"CAPig+cRrz_Reb1CJN0u1LSQ38LjED+tuBAT=RT5rESfJ05absg@mail.gmail.com","threadId":"47986","inReplyTo":"CAPig+cRKaxECLHb1id6Mcd0O3uOiDzdGB4ZxPt1UpwUDi9Xb+g@mail.gmail.com","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-06T07:40:56Z","receivedAt":"2018-04-06T07:41:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n>> +test_expect_success 'uses entry when available' '\n>> +       echo bar >expect &&\n>> +       git config --add core.foo bar &&\n>> +       git config --default baz core.foo >actual &&\n>> +       git config --unset core.foo &&\n>> +       test_cmp expect actual\n>> +'\n>\n> If you happen to re-roll, can we move this test so it immediately\n> follows the \"uses --default when missing entry\" test?\n\nOne other issue. If \"git config --default ...\" fails, the --unset line\nwill never be invoked, thus cleanup won't happen.\n\nTo ensure cleanup, either use:\n\n    test_when_finished \"git config --unset core.foo\" &&\n\nearlier in the test (just after the --add line) or use the\ntest_config() function to both set the value and ensure its cleanup.\n"},{"id":"343963","messageId":"20180406145517.GB25041@sigill.intra.peff.net","threadId":"47986","inReplyTo":"CAPig+cSYuWReTFfnN8Wg2VGKcOk5SPHHWpTdW6yks67mkOjY0Q@mail.gmail.com","subject":"Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T14:55:18Z","receivedAt":"2018-04-06T14:55:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:\n\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options\n> >  SYNOPSIS\n> >  --------\n> >  [verse]\n> > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n> > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]\n> \n> It's pretty confusing to see bare \"--type\" like this which makes it\n> seem as if it doesn't take an argument at all. Better would be\n> \"[--type=<type>]\".\n\nIn the interest of brevity, I actually think it could remain \"[type]\".\nIMHO the synopsis should be giving a summary and does not have to\nmention every detail (as long as it is not _inaccurate_, which I agree\nthat \"--type\" is bordering on).\n\n-Peff\n"},{"id":"344045","messageId":"20180407004241.GB78042@syl.local","threadId":"47986","inReplyTo":"CAPig+cT2T8r9mefv-7GEU4khEDVrthK45X5kbVXU5c-MXU39dg@mail.gmail.com","subject":"Re: [PATCH v5 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-07T00:42:41Z","receivedAt":"2018-04-07T00:42:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 06, 2018 at 03:29:48AM -0400, Eric Sunshine wrote:\n> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> > [...]\n> > For consistency, let's introduce `--type=color` and encourage its use\n> > with `--default` together over `--get-color` alone.\n>\n> A couple minor subjective comments below... neither worth a re-roll.\n>\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > @@ -180,6 +180,10 @@ Valid `<type>`'s include:\n> > +- 'color': When writing to a caller, canonicalize by converting to an ANSI color\n> > +  escape sequence. When writing to the configuration file, a sanity-check is\n> > +  performed to ensure that the given value is canonicalize-able as an ANSI\n> > +  color, but it is written as-is.\n>\n> \"When writing to a caller\" is a somewhat confusing way to say \"When\n> getting a value\".\n>\n> Likewise, \"When writing to the configuration file\" could be stated\n> more concisely as \"When setting a value\".\n\nThanks, I think that these are much clearer, especially when used\ntogether. I have updated this in my local copy, so it will be picked up\nwhen I send the next re-roll.\n\n> >  --bool::\n> > @@ -231,6 +235,9 @@ Valid `<type>`'s include:\n> >         output it as the ANSI color escape sequence to the standard\n> >         output.  The optional `default` parameter is used instead, if\n> >         there is no color configured for `name`.\n> > ++\n> > +It is preferred to use `--type=color`, or `--type=color [--default=<default>]`\n> > +instead of `--get-color`.\n>\n> Repetitious. More conscisely:\n>\n>     It is preferred to use `--type=color [--default=<value>]`\n>     instead of `--get-color`.\n>\n> Or, even:\n>\n>     `--type=color [--default=<value>]` is preferred over `--get-color`.\n\nMuch clearer; I think that I prefer the second one. I have rolled this\ninto my local changes as above.\n\n\nThanks,\nTaylor\n"},{"id":"344046","messageId":"20180407004916.GC78042@syl.local","threadId":"47986","inReplyTo":"CAPig+cRKaxECLHb1id6Mcd0O3uOiDzdGB4ZxPt1UpwUDi9Xb+g@mail.gmail.com","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-07T00:49:16Z","receivedAt":"2018-04-07T00:49:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 06, 2018 at 02:53:45AM -0400, Eric Sunshine wrote:\n> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> > [...]\n> > This commit (and those following it in this series) aim to eventually\n> > replace `--get-color` with a consistent alternative. By introducing\n> > `--default`, we allow the `--get-color` action to be promoted to a\n> > `--type=color` type specifier, retaining the \"fallback\" behavior via the\n> > `--default` flag introduced in this commit.\n> >\n> > For example, we aim to replace:\n> >\n> >   $ git config --get-color variable [default] [...]\n> >\n> > with:\n> >\n> >   $ git config --default default --type=color variable [...]\n> >\n> > Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> > ---\n> > diff --git a/builtin/config.c b/builtin/config.c\n> > @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n> > +       if (!values.nr && default_value) {\n> > +               struct strbuf *item;\n> > +               ALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> > +               item = &values.items[values.nr++];\n> > +               strbuf_init(item, 0);\n> > +               if (format_config(item, key_, default_value) < 0) {\n> > +                       die(_(\"failed to format default config value\"));\n> > +               }\n>\n> Unnecessary braces.\n\nAh, that's leftover from changing this around in your last round of\nreview. Cleaned up locally.\n\n> Also, an error message such as this is typically more helpful when it\n> shows the item which caused the problem:\n>\n>     die(_(\"failed to format default config value: %s\"), default_value);\n>\n> However, since the message already says \"default\", it's pretty easy to\n> understand that it's talking about the argument to --default, so it's\n> perhaps not such a big deal in this case.\n>\n> Anyhow, neither point is worth a re-roll.\n\nI like including the value of default_value. I've included it locally,\n\n>\n> > +       }\n> > @@ -624,6 +636,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> >                 usage_with_options(builtin_config_usage, builtin_config_options);\n> >         }\n> >\n> > +\n>\n> Unnecessary new blank line.\n>\n> > +       if (default_value && !(actions & ACTION_GET)) {\n> > +               error(\"--default is only applicable to --get\");\n> > +               usage_with_options(builtin_config_usage,\n> > +                       builtin_config_options);\n> > +       }\n> > diff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\n> > @@ -0,0 +1,38 @@\n> > +test_expect_success 'uses --default when missing entry' '\n> > +       echo quux >expect &&\n> > +       git config -f config --default quux core.foo >actual &&\n> > +       test_cmp expect actual\n> > +'\n> >\n> > +test_expect_success 'canonicalizes --default with appropriate type' '\n> > +       echo true >expect &&\n> > +       git config -f config --default=true --bool core.foo >actual &&\n> > +       test_cmp expect actual\n> > +'\n>\n> I would trust this canonicalization test a lot more if it used one of\n> the aliases for \"true\" since it doesn't presently prove that\n> canonicalization is taking place. For instance:\n>\n>     git config -f config --default=yes --bool core.foo >actual &&\n>\n> > +test_expect_success 'uses entry when available' '\n> > +       echo bar >expect &&\n> > +       git config --add core.foo bar &&\n> > +       git config --default baz core.foo >actual &&\n> > +       git config --unset core.foo &&\n> > +       test_cmp expect actual\n> > +'\n>\n> If you happen to re-roll, can we move this test so it immediately\n> follows the \"uses --default when missing entry\" test? That's where I\n> had expected to find it and had even written a review comment saying\n> so (but deleted the comment once I got down to this spot in the\n> patch). Also, perhaps rename the test to make it clearer that it\n> complements the \"uses --default when missing entry\" test; perhaps\n> \"does not fallback to --default when entry present\" or something.\n\nThat location makes much more sense, as does using --default=yes to\nensure that canonicalization is taking place. I've updated that locally,\nas well as the preceding test to make it clearer that they are\ncontrasting tests:\n\n\t- 'falls back to --default when missing entry'\n\t- 'does not fallback to --default when entry present'\n\nThough I am not sure about \"falls back\" to mean \"uses --default\". I am\nnot sure which to pick here... what are your thoughts?\n\nThanks,\nTaylor\n"},{"id":"344047","messageId":"20180407005837.GD78042@syl.local","threadId":"47986","inReplyTo":"CAPig+cRrz_Reb1CJN0u1LSQ38LjED+tuBAT=RT5rESfJ05absg@mail.gmail.com","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-07T00:58:37Z","receivedAt":"2018-04-07T00:58:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 06, 2018 at 03:40:56AM -0400, Eric Sunshine wrote:\n> On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n> >> +test_expect_success 'uses entry when available' '\n> >> +       echo bar >expect &&\n> >> +       git config --add core.foo bar &&\n> >> +       git config --default baz core.foo >actual &&\n> >> +       git config --unset core.foo &&\n> >> +       test_cmp expect actual\n> >> +'\n> >\n> > If you happen to re-roll, can we move this test so it immediately\n> > follows the \"uses --default when missing entry\" test?\n>\n> One other issue. If \"git config --default ...\" fails, the --unset line\n> will never be invoked, thus cleanup won't happen.\n>\n> To ensure cleanup, either use:\n>\n>     test_when_finished \"git config --unset core.foo\" &&\n>\n> earlier in the test (just after the --add line) or use the\n> test_config() function to both set the value and ensure its cleanup.\n\nNeat. I wasn't aware of 'test_when_finished'. I think that I prefer the\nexplicitness of 'test_when_finished \"git config ...\"' over\n'test_config()', but I am happy to use whichever is preferred. Since\nt1310 is new, there's no historical precedent to follow. Please let me\nknow if you have a preference one way or another.\n\nThanks,\nTaylor\n"},{"id":"344048","messageId":"20180407010003.GE78042@syl.local","threadId":"47986","inReplyTo":"20180406145517.GB25041@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc.","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-07T01:00:03Z","receivedAt":"2018-04-07T01:00:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 06, 2018 at 10:55:18AM -0400, Jeff King wrote:\n> On Fri, Apr 06, 2018 at 02:14:34AM -0400, Eric Sunshine wrote:\n>\n> > > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> > > @@ -9,13 +9,13 @@ git-config - Get and set repository or global options\n> > >  SYNOPSIS\n> > >  --------\n> > >  [verse]\n> > > -'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]\n> > > +'git config' [<file-option>] [--type] [--show-origin] [-z|--null] name [value [value_regex]]\n> >\n> > It's pretty confusing to see bare \"--type\" like this which makes it\n> > seem as if it doesn't take an argument at all. Better would be\n> > \"[--type=<type>]\".\n>\n> In the interest of brevity, I actually think it could remain \"[type]\".\n> IMHO the synopsis should be giving a summary and does not have to\n> mention every detail (as long as it is not _inaccurate_, which I agree\n> that \"--type\" is bordering on).\n\nI think I am OK with the \"non-brevity\" of \"[--type=<type>]\", since\n\"--type\" alone does not make sense.\n\nI agree that it would still be fairly clear even if that's the way that\nit did end up being typeset, but I prefer the accuracy of its current\nstate.\n\nThat said, I am happy to bend towards any strong feelings about this\ntopic, but for now I will leave it as-is.\n\n\nThanks,\nTaylor\n"},{"id":"344056","messageId":"CAPig+cR5szdsE79-S4So1kGhe5EVHFDFJnPK7R59kBwn79KtJg@mail.gmail.com","threadId":"47986","inReplyTo":"20180407004916.GC78042@syl.local","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-07T08:38:21Z","receivedAt":"2018-04-07T08:40:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 8:49 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> On Fri, Apr 06, 2018 at 02:53:45AM -0400, Eric Sunshine wrote:\n>> On Fri, Apr 6, 2018 at 2:30 AM, Taylor Blau <me@ttaylorr.com> wrote:\n>> > +test_expect_success 'uses --default when missing entry' '\n>> > +       echo quux >expect &&\n>> > +       git config -f config --default quux core.foo >actual &&\n>> > +       test_cmp expect actual\n>> > +'\n>> >\n>> > +test_expect_success 'uses entry when available' '\n>> > +       echo bar >expect &&\n>> > +       git config --add core.foo bar &&\n>> > +       git config --default baz core.foo >actual &&\n>> > +       git config --unset core.foo &&\n>> > +       test_cmp expect actual\n>> > +'\n>>\n>> If you happen to re-roll, can we move this test so it immediately\n>> follows the \"uses --default when missing entry\" test? That's where I\n>> had expected to find it and had even written a review comment saying\n>> so (but deleted the comment once I got down to this spot in the\n>> patch). Also, perhaps rename the test to make it clearer that it\n>> complements the \"uses --default when missing entry\" test; perhaps\n>> \"does not fallback to --default when entry present\" or something.\n>\n> That location makes much more sense, as does using --default=yes to\n> ensure that canonicalization is taking place. I've updated that locally,\n> as well as the preceding test to make it clearer that they are\n> contrasting tests:\n>\n>         - 'falls back to --default when missing entry'\n>         - 'does not fallback to --default when entry present'\n>\n> Though I am not sure about \"falls back\" to mean \"uses --default\". I am\n> not sure which to pick here... what are your thoughts?\n\nIt's nice for the titles to show that the two tests complement one\nanother but the exact wording is not terribly important. Taking your\noriginal test title (slightly modified), perhaps:\n\n    - 'uses --default when entry missing'\n    - 'does not use --default when entry present'\n"},{"id":"344057","messageId":"CAPig+cSZPrnCuqZ0JG2hKqQEqYT6qYoGEzy4Une3GOo6hX_LCg@mail.gmail.com","threadId":"47986","inReplyTo":"20180407005837.GD78042@syl.local","subject":"Re: [PATCH v5 1/3] builtin/config: introduce `--default`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-07T08:44:41Z","receivedAt":"2018-04-07T08:44:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 6, 2018 at 8:58 PM, Taylor Blau <me@ttaylorr.com> wrote:\n> On Fri, Apr 06, 2018 at 03:40:56AM -0400, Eric Sunshine wrote:\n>> On Fri, Apr 6, 2018 at 2:53 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> One other issue. If \"git config --default ...\" fails, the --unset line\n>> will never be invoked, thus cleanup won't happen.\n>>\n>> To ensure cleanup, either use:\n>>\n>>     test_when_finished \"git config --unset core.foo\" &&\n>>\n>> earlier in the test (just after the --add line) or use the\n>> test_config() function to both set the value and ensure its cleanup.\n>\n> Neat. I wasn't aware of 'test_when_finished'. I think that I prefer the\n> explicitness of 'test_when_finished \"git config ...\"' over\n> 'test_config()', but I am happy to use whichever is preferred. Since\n> t1310 is new, there's no historical precedent to follow. Please let me\n> know if you have a preference one way or another.\n\ntest_config() is preferred; not only is it shorter but, because it\nautomates cleanup, there's less opportunity to screw up (like\nforgetting to use test_when_finished()).\n\nHowever, since this config setting needs to be present for only a\nsingle command, you can go even simpler by collapsing the entire test\nto:\n\ntest_expect_success 'does not use --default when entry present' '\n    echo bar >expect &&\n    git -c core.foo=bar config --default baz core.foo >actual &&\n    test_cmp expect actual\n'\n"},{"id":"344124","messageId":"xmqq60518eyt.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180405222949.GC29117@sigill.intra.peff.net","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-08T23:18:18Z","receivedAt":"2018-04-08T23:18:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:\n>\n>> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n>>  \tconfig_with_options(collect_config, &values,\n>>  \t\t\t    &given_config_source, &config_options);\n>>  \n>> +\tif (!values.nr && default_value) {\n>> +\t\tstruct strbuf *item;\n>> +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n>> +\t\titem = &values.items[values.nr++];\n>> +\t\tstrbuf_init(item, 0);\n>> +\t\tif (format_config(item, key_, default_value) < 0) {\n>> +\t\t\texit(1);\n>> +\t\t}\n>> +\t}\n> ...\n>\n> It's obvious in this toy example, but that config call may be buried\n> deep in a script. It'd probably be nicer for that exit(1) to be\n> something like:\n>\n>   die(_(\"failed to format default config value\"));\n\nTogether with key_ and default_value, you mean?\n\n>\n>> +test_expect_success 'does not allow --default without --get' '\n>> +\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n>> +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n>> +'\n>\n> I think \"a\" here needs to be \"a.section\". I get:\n>\n>   error: key does not contain a section: a\n\n\"section.var\"?  That aside, even with a properly formatted key, I\nseem to get an empty output here, so this may need a bit more work.\n\n\n"},{"id":"344279","messageId":"20180410001822.GA67209@syl.local","threadId":"47986","inReplyTo":"20180306021729.45813-1-me@ttaylorr.com","subject":"[PATCH v6 0/3] Teach `--default` to `git-config(1)`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:18:22Z","receivedAt":"2018-04-10T00:18:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nAttached is the sixth re-roll of my series to add '--type=color' as a valid\noption to 'git-config(1)'.\n\nI have changed the following since v5 (an inter-diff is available below for\neasier consumption):\n\n  - Update prose in Documentation/git-config.txt to match Eric's\n    suggestions.\n\n  - Remove extraneous braces, newlines from builtin/config.c. (cc:\n    Eric).\n\n  - Rename, reorder tests in t1310 to make clear which tests are duals\n    of one another. Use '-c' to introduce short-lived configuration\n    values. (cc: Eric).\n\n\nThanks,\nTaylor\n\nTaylor Blau (3):\n  builtin/config: introduce `--default`\n  config.c: introduce 'git_config_color' to parse ANSI colors\n  builtin/config: introduce `color` type specifier\n\n Documentation/git-config.txt | 10 +++++++++\n builtin/config.c             | 40 ++++++++++++++++++++++++++++++++++++\n config.c                     | 10 +++++++++\n config.h                     |  1 +\n t/t1300-repo-config.sh       | 30 +++++++++++++++++++++++++++\n t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++\n 6 files changed, 127 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\nInter-diff (since v5):\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 05a555b568..7c8365e377 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -177,10 +177,10 @@ Valid `<type>`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n   to a timestamp. This specifier has no effect when setting the value.\n-- 'color': When writing to a caller, canonicalize by converting to an ANSI color\n-  escape sequence. When writing to the configuration file, a sanity-check is\n-  performed to ensure that the given value is canonicalize-able as an ANSI\n-  color, but it is written as-is.\n+- 'color': When getting a value, canonicalize by converting to an ANSI color\n+  escape sequence. When setting a value, a sanity-check is performed to ensure\n+  that the given value is canonicalize-able as an ANSI color, but it is written\n+  as-is.\n +\n\n --bool::\n@@ -233,8 +233,7 @@ Valid `<type>`'s include:\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n +\n-It is preferred to use `--type=color`, or `--type=color [--default=<default>]`\n-instead of `--get-color`.\n+`--type=color [--default=<default>]` is preferred over `--get-color`.\n\n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0c7cfcf6c9..08016863ac 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -300,9 +300,9 @@ static int get_value(const char *key_, const char *regex_)\n \t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n \t\titem = &values.items[values.nr++];\n \t\tstrbuf_init(item, 0);\n-\t\tif (format_config(item, key_, default_value) < 0) {\n-\t\t\tdie(_(\"failed to format default config value\"));\n-\t\t}\n+\t\tif (format_config(item, key_, default_value) < 0)\n+\t\t\tdie(_(\"failed to format default config value: %s\"),\n+\t\t\t\tdefault_value);\n \t}\n\n \tret = !values.nr;\n@@ -657,7 +657,6 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n\n-\n \tif (default_value && !(actions & ACTION_GET)) {\n \t\terror(\"--default is only applicable to --get\");\n \t\tusage_with_options(builtin_config_usage,\n@@ -814,3 +813,4 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n\n \treturn 0;\n+}\ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nindex b74c932763..6049d91708 100755\n--- a/t/t1310-config-default.sh\n+++ b/t/t1310-config-default.sh\n@@ -4,23 +4,21 @@ test_description='Test git config in different settings (with --default)'\n\n . ./test-lib.sh\n\n-test_expect_success 'uses --default when missing entry' '\n+test_expect_success 'uses --default when entry missing' '\n \techo quux >expect &&\n-\tgit config -f config --default quux core.foo >actual &&\n+\tgit config -f config --default=quux core.foo >actual &&\n \ttest_cmp expect actual\n '\n\n-test_expect_success 'canonicalizes --default with appropriate type' '\n-\techo true >expect &&\n-\tgit config -f config --default=true --bool core.foo >actual &&\n+test_expect_success 'does not use --default when entry present' '\n+\techo bar >expect &&\n+\tgit -c core.foo=bar config --default=baz core.foo >actual &&\n \ttest_cmp expect actual\n '\n\n-test_expect_success 'uses entry when available' '\n-\techo bar >expect &&\n-\tgit config --add core.foo bar &&\n-\tgit config --default baz core.foo >actual &&\n-\tgit config --unset core.foo &&\n+test_expect_success 'canonicalizes --default with appropriate type' '\n+\techo true >expect &&\n+\tgit config -f config --default=yes --bool core.foo >actual &&\n \ttest_cmp expect actual\n '\n--\n2.17.0\n"},{"id":"344280","messageId":"20180410001828.GC67209@syl.local","threadId":"47986","inReplyTo":"cover.1523319159.git.me@ttaylorr.com","subject":"[PATCH v6 2/3] config.c: introduce 'git_config_color' to parse ANSI colors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:18:28Z","receivedAt":"2018-04-10T00:18:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In preparation for adding `--type=color` to the `git-config(1)` builtin,\nlet's introduce a color parsing utility, `git_config_color` in a similar\nfashion to `git_config_<type>`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n config.c | 10 ++++++++++\n config.h |  1 +\n 2 files changed, 11 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex b0c20e6cb8..33366b52c7 100644\n--- a/config.c\n+++ b/config.c\n@@ -16,6 +16,7 @@\n #include \"string-list.h\"\n #include \"utf8.h\"\n #include \"dir.h\"\n+#include \"color.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *\n \treturn 0;\n }\n \n+int git_config_color(char *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tif (color_parse(value, dest) < 0)\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_default_core_config(const char *var, const char *value)\n {\n \t/* This needs a better name */\ndiff --git a/config.h b/config.h\nindex ef70a9cac1..0e060779d9 100644\n--- a/config.h\n+++ b/config.h\n@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);\n extern int git_config_string(const char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_expiry_date(timestamp_t *, const char *, const char *);\n+extern int git_config_color(char *, const char *, const char *);\n extern int git_config_set_in_file_gently(const char *, const char *, const char *);\n extern void git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set_gently(const char *, const char *);\n-- \n2.17.0\n\n"},{"id":"344281","messageId":"20180410001831.GD67209@syl.local","threadId":"47986","inReplyTo":"cover.1523319159.git.me@ttaylorr.com","subject":"[PATCH v6 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:18:31Z","receivedAt":"2018-04-10T00:18:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --type=color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--type=color` and encourage its use\nwith `--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  6 ++++++\n builtin/config.c             | 22 ++++++++++++++++++++++\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex b644a44ae9..7c8365e377 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -177,6 +177,10 @@ Valid `<type>`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n   to a timestamp. This specifier has no effect when setting the value.\n+- 'color': When getting a value, canonicalize by converting to an ANSI color\n+  escape sequence. When setting a value, a sanity-check is performed to ensure\n+  that the given value is canonicalize-able as an ANSI color, but it is written\n+  as-is.\n +\n \n --bool::\n@@ -228,6 +232,8 @@ Valid `<type>`'s include:\n \toutput it as the ANSI color escape sequence to the standard\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n++\n+`--type=color [--default=<default>]` is preferred over `--get-color`.\n \n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 014e3a0d53..08016863ac 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT\t3\n #define TYPE_PATH\t\t4\n #define TYPE_EXPIRY_DATE\t5\n+#define TYPE_COLOR\t\t6\n \n static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t*type = TYPE_PATH;\n \telse if (!strcmp(arg, \"expiry-date\"))\n \t\t*type = TYPE_EXPIRY_DATE;\n+\telse if (!strcmp(arg, \"color\"))\n+\t\t*type = TYPE_COLOR;\n \telse\n \t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n \n@@ -202,6 +205,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (type == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -347,6 +355,20 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (type == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (git_config_color(v, key, value))\n+\t\t\tdie(\"cannot parse color '%s'\", value);\n+\n+\t\t/*\n+\t\t * The contents of `v` now contain an ANSI escape\n+\t\t * sequence, not suitable for including within a\n+\t\t * configuration file. Treat the above as a\n+\t\t * \"sanity-check\", and return the given value, which we\n+\t\t * know is representable as valid color code.\n+\t\t */\n+\t\treturn xstrdup(value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", type);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex f5ae80e9ae..c494733061 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --type=color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --type=color foo.color >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --type=color' '\n+\trm .git/config &&\n+\tgit config --type=color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --type=color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --type=color foo.bar\n+'\n+\n+test_expect_success 'set --type=color barfs on non-color' '\n+\ttest_must_fail git config --type=color foo.color \"not-a-color\" 2>error &&\n+\ttest_i18ngrep \"cannot parse color\" error\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.17.0\n"},{"id":"344282","messageId":"20180410001834.GE67209@syl.local","threadId":"47986","inReplyTo":"cover.1523319159.git.me@ttaylorr.com","subject":"[PATCH v6 3/3] builtin/config: introduce `color` type specifier","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:18:34Z","receivedAt":"2018-04-10T00:18:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As of this commit, the canonical way to retreive an ANSI-compatible\ncolor escape sequence from a configuration file is with the\n`--get-color` action.\n\nThis is to allow Git to \"fall back\" on a default value for the color\nshould the given section not exist in the specified configuration(s).\n\nWith the addition of `--default`, this is no longer needed since:\n\n  $ git config --default red --type=color core.section\n\nwill be have exactly as:\n\n  $ git config --get-color core.section red\n\nFor consistency, let's introduce `--type=color` and encourage its use\nwith `--default` together over `--get-color` alone.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  6 ++++++\n builtin/config.c             | 22 ++++++++++++++++++++++\n t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex b644a44ae9..7c8365e377 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -177,6 +177,10 @@ Valid `<type>`'s include:\n   ~/` from the command line to let your shell do the expansion.)\n - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n   to a timestamp. This specifier has no effect when setting the value.\n+- 'color': When getting a value, canonicalize by converting to an ANSI color\n+  escape sequence. When setting a value, a sanity-check is performed to ensure\n+  that the given value is canonicalize-able as an ANSI color, but it is written\n+  as-is.\n +\n \n --bool::\n@@ -228,6 +232,8 @@ Valid `<type>`'s include:\n \toutput it as the ANSI color escape sequence to the standard\n \toutput.  The optional `default` parameter is used instead, if\n \tthere is no color configured for `name`.\n++\n+`--type=color [--default=<default>]` is preferred over `--get-color`.\n \n -e::\n --edit::\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 014e3a0d53..08016863ac 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -61,6 +61,7 @@ static int show_origin;\n #define TYPE_BOOL_OR_INT\t3\n #define TYPE_PATH\t\t4\n #define TYPE_EXPIRY_DATE\t5\n+#define TYPE_COLOR\t\t6\n \n static int option_parse_type(const struct option *opt, const char *arg,\n \t\t\t     int unset)\n@@ -82,6 +83,8 @@ static int option_parse_type(const struct option *opt, const char *arg,\n \t\t*type = TYPE_PATH;\n \telse if (!strcmp(arg, \"expiry-date\"))\n \t\t*type = TYPE_EXPIRY_DATE;\n+\telse if (!strcmp(arg, \"color\"))\n+\t\t*type = TYPE_COLOR;\n \telse\n \t\tdie(_(\"unrecognized --type argument, %s\"), arg);\n \n@@ -202,6 +205,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t\tif (git_config_expiry_date(&t, key_, value_) < 0)\n \t\t\t\treturn -1;\n \t\t\tstrbuf_addf(buf, \"%\"PRItime, t);\n+\t\t} else if (type == TYPE_COLOR) {\n+\t\t\tchar v[COLOR_MAXLEN];\n+\t\t\tif (git_config_color(v, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tstrbuf_addstr(buf, v);\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n@@ -347,6 +355,20 @@ static char *normalize_value(const char *key, const char *value)\n \t\telse\n \t\t\treturn xstrdup(v ? \"true\" : \"false\");\n \t}\n+\tif (type == TYPE_COLOR) {\n+\t\tchar v[COLOR_MAXLEN];\n+\t\tif (git_config_color(v, key, value))\n+\t\t\tdie(\"cannot parse color '%s'\", value);\n+\n+\t\t/*\n+\t\t * The contents of `v` now contain an ANSI escape\n+\t\t * sequence, not suitable for including within a\n+\t\t * configuration file. Treat the above as a\n+\t\t * \"sanity-check\", and return the given value, which we\n+\t\t * know is representable as valid color code.\n+\t\t */\n+\t\treturn xstrdup(value);\n+\t}\n \n \tdie(\"BUG: cannot normalize type %d\", type);\n }\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex f5ae80e9ae..c494733061 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '\n \ttest_must_fail git config --expiry-date date.invalid1\n '\n \n+test_expect_success 'get --type=color' '\n+\trm .git/config &&\n+\tgit config foo.color \"red\" &&\n+\tgit config --get --type=color foo.color >actual.raw &&\n+\ttest_decode_color <actual.raw >actual &&\n+\techo \"<RED>\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect << EOF\n+[foo]\n+\tcolor = red\n+EOF\n+\n+test_expect_success 'set --type=color' '\n+\trm .git/config &&\n+\tgit config --type=color foo.color \"red\" &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'get --type=color barfs on non-color' '\n+\techo \"[foo]bar=not-a-color\" >.git/config &&\n+\ttest_must_fail git config --get --type=color foo.bar\n+'\n+\n+test_expect_success 'set --type=color barfs on non-color' '\n+\ttest_must_fail git config --type=color foo.color \"not-a-color\" 2>error &&\n+\ttest_i18ngrep \"cannot parse color\" error\n+'\n+\n cat > expect << EOF\n [quote]\n \tleading = \" test\"\n-- \n2.17.0\n"},{"id":"344283","messageId":"20180410001826.GB67209@syl.local","threadId":"47986","inReplyTo":"cover.1523319159.git.me@ttaylorr.com","subject":"[PATCH v6 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:18:26Z","receivedAt":"2018-04-10T00:18:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"For some use cases, callers of the `git-config(1)` builtin would like to\nfallback to default values when the variable asked for does not exist.\nIn addition, users would like to use existing type specifiers to ensure\nthat values are parsed correctly when they do exist in the\nconfiguration.\n\nFor example, to fetch a value without a type specifier and fallback to\n`$fallback`, the following is required:\n\n  $ git config core.foo || echo \"$fallback\"\n\nThis is fine for most values, but can be tricky for difficult-to-express\n`$fallback`'s, like ANSI color codes.\n\nThis motivates `--get-color`, which is a one-off exception to the normal\ntype specifier rules wherein a user specifies both the configuration\nvariable and an optional fallback. Both are formatted according to their\ntype specifier, which eases the burden on the user to ensure that values\nare correctly formatted.\n\nThis commit (and those following it in this series) aim to eventually\nreplace `--get-color` with a consistent alternative. By introducing\n`--default`, we allow the `--get-color` action to be promoted to a\n`--type=color` type specifier, retaining the \"fallback\" behavior via the\n`--default` flag introduced in this commit.\n\nFor example, we aim to replace:\n\n  $ git config --get-color variable [default] [...]\n\nwith:\n\n  $ git config --default default --type=color variable [...]\n\nValues filled by `--default` behave exactly as if they were present in\nthe affected configuration file; they will be parsed by type specifiers\nwithout the knowledge that they are not themselves present in the\nconfiguration.\n\nSpecifically, this means that the following will work:\n\n  $ git config --int --default 1M does.not.exist\n  1048576\n\nIn subsequent commits, we will offer `--type=color`, which (in\nconjunction with `--default`) will be sufficient to replace\n`--get-color`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n Documentation/git-config.txt |  4 ++++\n builtin/config.c             | 18 ++++++++++++++++++\n t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 58 insertions(+)\n create mode 100755 t/t1310-config-default.sh\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex a1e3ffe750..b644a44ae9 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -240,6 +240,10 @@ Valid `<type>`'s include:\n \tusing `--file`, `--global`, etc) and `on` when searching all\n \tconfig files.\n \n+--default <value>::\n+  When using `--get`, and the requested variable is not found, behave as if\n+  <value> were the value assigned to the that variable.\n+\n CONFIGURATION\n -------------\n `pager.config` is only respected when listing configuration, i.e., when\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5c8952a17c..014e3a0d53 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -26,6 +26,7 @@ static char term = '\\n';\n static int use_global_config, use_system_config, use_local_config;\n static struct git_config_source given_config_source;\n static int actions, type;\n+static char *default_value;\n static int end_null;\n static int respect_includes_opt = -1;\n static struct config_options config_options;\n@@ -121,6 +122,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"name-only\", &omit_values, N_(\"show variable names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes_opt, N_(\"respect include directives on lookup\")),\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n+\tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n \tOPT_END(),\n };\n \n@@ -285,6 +287,16 @@ static int get_value(const char *key_, const char *regex_)\n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n \n+\tif (!values.nr && default_value) {\n+\t\tstruct strbuf *item;\n+\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n+\t\titem = &values.items[values.nr++];\n+\t\tstrbuf_init(item, 0);\n+\t\tif (format_config(item, key_, default_value) < 0)\n+\t\t\tdie(_(\"failed to format default config value: %s\"),\n+\t\t\t\tdefault_value);\n+\t}\n+\n \tret = !values.nr;\n \n \tfor (i = 0; i < values.nr; i++) {\n@@ -623,6 +635,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t}\n \n+\tif (default_value && !(actions & ACTION_GET)) {\n+\t\terror(\"--default is only applicable to --get\");\n+\t\tusage_with_options(builtin_config_usage,\n+\t\t\tbuiltin_config_options);\n+\t}\n+\n \tif (actions & PAGING_ACTIONS)\n \t\tsetup_auto_pager(\"config\", 1);\n \ndiff --git a/t/t1310-config-default.sh b/t/t1310-config-default.sh\nnew file mode 100755\nindex 0000000000..6049d91708\n--- /dev/null\n+++ b/t/t1310-config-default.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+\n+test_description='Test git config in different settings (with --default)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'uses --default when entry missing' '\n+\techo quux >expect &&\n+\tgit config -f config --default=quux core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'does not use --default when entry present' '\n+\techo bar >expect &&\n+\tgit -c core.foo=bar config --default=baz core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'canonicalizes --default with appropriate type' '\n+\techo true >expect &&\n+\tgit config -f config --default=yes --bool core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'dies when --default cannot be parsed' '\n+\ttest_must_fail git config -f config --type=expiry-date --default=x --get \\\n+\t\tnot.a.section 2>error &&\n+\ttest_i18ngrep \"failed to format default config value\" error\n+'\n+\n+test_expect_success 'does not allow --default without --get' '\n+\ttest_must_fail git config --default=quux --unset a.section >output 2>&1 &&\n+\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n+'\n+\n+test_done\n-- \n2.17.0\n\n"},{"id":"344284","messageId":"20180410002024.GA48196@syl.local","threadId":"47986","inReplyTo":"xmqq60518eyt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 1/3] builtin/config: introduce `--default`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-04-10T00:20:24Z","receivedAt":"2018-04-10T00:20:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Apr 09, 2018 at 08:18:18AM +0900, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n>\n> > On Wed, Apr 04, 2018 at 07:59:12PM -0700, Taylor Blau wrote:\n> >\n> >> @@ -286,6 +288,16 @@ static int get_value(const char *key_, const char *regex_)\n> >>  \tconfig_with_options(collect_config, &values,\n> >>  \t\t\t    &given_config_source, &config_options);\n> >>\n> >> +\tif (!values.nr && default_value) {\n> >> +\t\tstruct strbuf *item;\n> >> +\t\tALLOC_GROW(values.items, values.nr + 1, values.alloc);\n> >> +\t\titem = &values.items[values.nr++];\n> >> +\t\tstrbuf_init(item, 0);\n> >> +\t\tif (format_config(item, key_, default_value) < 0) {\n> >> +\t\t\texit(1);\n> >> +\t\t}\n> >> +\t}\n> > ...\n> >\n> > It's obvious in this toy example, but that config call may be buried\n> > deep in a script. It'd probably be nicer for that exit(1) to be\n> > something like:\n> >\n> >   die(_(\"failed to format default config value\"));\n>\n> Together with key_ and default_value, you mean?\n>\n> >\n> >> +test_expect_success 'does not allow --default without --get' '\n> >> +\ttest_must_fail git config --default quux --unset a >output 2>&1 &&\n> >> +\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n> >> +'\n> >\n> > I think \"a\" here needs to be \"a.section\". I get:\n> >\n> >   error: key does not contain a section: a\n>\n> \"section.var\"?  That aside, even with a properly formatted key, I\n> seem to get an empty output here, so this may need a bit more work.\n\nThis should be fixed in the latest re-roll, v6:\n\n\texpecting success:\n\t\t\t\t\ttest_must_fail git config --default=quux --unset a.section >output 2>&1 &&\n\t\t\t\t\ttest_i18ngrep \"\\-\\-default is only applicable to\" output\n\n\terror: --default is only applicable to --get\n\tok 5 - does not allow --default without --get\n\n\nThanks,\nTaylor\n"},{"id":"344288","messageId":"xmqqo9ir4yof.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180410001826.GB67209@syl.local","subject":"Re: [PATCH v6 1/3] builtin/config: introduce `--default`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-10T01:50:40Z","receivedAt":"2018-04-10T01:50:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> For some use cases, callers of the `git-config(1)` builtin would like to\n> fallback to default values when the variable asked for does not exist.\n> In addition, users would like to use existing type specifiers to ensure\n> that values are parsed correctly when they do exist in the\n> configuration.\n\nNicely done.\n"},{"id":"344289","messageId":"xmqqk1tf4yhl.fsf@gitster-ct.c.googlers.com","threadId":"47986","inReplyTo":"20180410001834.GE67209@syl.local","subject":"Re: [PATCH v6 3/3] builtin/config: introduce `color` type specifier","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-04-10T01:54:46Z","receivedAt":"2018-04-10T01:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index b644a44ae9..7c8365e377 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -177,6 +177,10 @@ Valid `<type>`'s include:\n>    ~/` from the command line to let your shell do the expansion.)\n>  - 'expiry-date': canonicalize by converting from a fixed or relative date-string\n>    to a timestamp. This specifier has no effect when setting the value.\n> +- 'color': When getting a value, canonicalize by converting to an ANSI color\n> +  escape sequence. When setting a value, a sanity-check is performed to ensure\n> +  that the given value is canonicalize-able as an ANSI color, but it is written\n> +  as-is.\n\nThis makes sense, in the same spirit as a value with \"path\" type\nwhere tilde expansion is not done at all while setting.\n\nWill queue.  Thanks.\n"}]}