builtin/config.c: combined series '--type', '--default'

PATCH Superseded Taylor Blau 3 participants 20 messages Apr 26, 2018 – Apr 27, 2018 Documentation

Share & tools

Taylor Blau

author

2018-04-26 04:25 UTC #1 lore raw

PATCH 1/5 builtin/config.c: treat type specifiers singularly

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

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

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

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

  $ git config --bool some.section

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

  $ git config --int --type=int

Signed-off-by Taylor Blau <[email protected]>

---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

builtin/config.c +22 ~~-27~~

index 01169dd628..92fb8d56b1 100644
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;

@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),

@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
-	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+	OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+	OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+	OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+	OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),

@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;

@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store

@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)

@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;

@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}

t/t1300-repo-config.sh +11 -0

index e95b1e67da..ed9b9cd344 100755
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 04:25 UTC #2 lore raw

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

In reply to Taylor Blau

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

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

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

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

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

We retain the historic behavior of complaining when multiple, legacy-style --<type> flags are given, as well as extend this to conflicting new-style --type=<type> flags. --int --type=int (and its commutative pair) does not complain, but --bool --type=int (and its commutative pair) does.

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt | 71 ++++++++++++++++++++----------------
 builtin/config.c             | 66 ++++++++++++++++++++++++++++++---
 t/t1300-repo-config.sh       | 58 +++++++++++++++++++++++++++--
 3 files changed, 155 insertions(+), 40 deletions(-)

Documentation/git-config.txt +39 ~~-32~~

index e09ed5d7d5..b759009c75 100644
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name

@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options

@@ -160,30 +158,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::

builtin/config.c +61 -5

78 lines not on this page · raw message

t/t1300-repo-config.sh +55 -3

68 lines not on this page · raw message

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 04:25 UTC #6 lore raw

PATCH 3/5 builtin/config: introduce `--default`

In reply to Taylor Blau

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

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

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

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

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

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

For example, we aim to replace:

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

with:

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

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

Specifically, this means that the following will work:

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

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

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 18 ++++++++++++++++++
 t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100755 t/t1310-config-default.sh

Documentation/git-config.txt +4 -0

index b759009c75..c3adafd78a 100644
@@ -240,6 +240,10 @@ Valid `<type>`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default <value>::
+  When using `--get`, and the requested variable is not found, behave as if
+  <value> were the value assigned to the that variable.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when

builtin/config.c +18 -0

index 2f91ef15a4..d7acf912cd 100644
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;

@@ -150,6 +151,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };

@@ -314,6 +316,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0)
+			die(_("failed to format default config value: %s"),
+				default_value);
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {

@@ -652,6 +664,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage,
+			builtin_config_options);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);

t/t1310-config-default.sh new file +36 -0

new file mode 100755
index 0000000000..6049d91708
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when entry missing' '
+	echo quux >expect &&
+	git config -f config --default=quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'does not use --default when entry present' '
+	echo bar >expect &&
+	git -c core.foo=bar config --default=baz core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=yes --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=expiry-date --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "failed to format default config value" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 04:25 UTC #7 lore raw

PATCH 4/5 config.c: introduce 'git_config_color' to parse ANSI colors

In reply to Taylor Blau

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

Signed-off-by Taylor Blau <[email protected]>

---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

config.c +10 -0

index 62c56099bf..d14813bef7 100644
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */

config.h +1 -0

index ef70a9cac1..0e060779d9 100644
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 04:25 UTC #8 lore raw

PATCH 5/5 builtin/config: introduce `color` type specifier

In reply to Taylor Blau

As of this commit, the canonical way to retreive an ANSI-compatible color escape sequence from a configuration file is with the --get-color action.

This is to allow Git to "fall back" on a default value for the color should the given section not exist in the specified configuration(s).

With the addition of --default, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce --type=color and encourage its use with --default together over --get-color alone.

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt |  6 ++++++
 builtin/config.c             | 20 ++++++++++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

Documentation/git-config.txt +6 -0

index c3adafd78a..18ddc78f42 100644
@@ -177,6 +177,10 @@ Valid `<type>`'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative date-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': When getting a value, canonicalize by converting to an ANSI color
+  escape sequence. When setting a value, a sanity-check is performed to ensure
+  that the given value is canonicalize-able as an ANSI color, but it is written
+  as-is.
 +
 
 --bool::

@@ -228,6 +232,8 @@ Valid `<type>`'s include:
 	output it as the ANSI color escape sequence to the standard
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
++
+`--type=color [--default=<default>]` is preferred over `--get-color`.
 
 -e::
 --edit::

builtin/config.c +20 -0

index d7acf912cd..ec5c11293b 100644
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT	3
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
+#define TYPE_COLOR		6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
 	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \

@@ -231,6 +232,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
 			strbuf_addf(buf, "%"PRItime, t);
+		} else if (type == TYPE_COLOR) {
+			char v[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {

@@ -376,6 +382,20 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (type == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (git_config_color(v, key, value))
+			die("cannot parse color '%s'", value);
+
+		/*
+		 * The contents of `v` now contain an ANSI escape
+		 * sequence, not suitable for including within a
+		 * configuration file. Treat the above as a
+		 * "sanity-check", and return the given value, which we
+		 * know is representable as valid color code.
+		 */
+		return xstrdup(value);
+	}
 
 	die("BUG: cannot normalize type %d", type);
 }

t/t1300-repo-config.sh +30 -0

index 1e3a1ace14..2acfd513f1 100755
@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
 	test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --type=color' '
+	rm .git/config &&
+	git config foo.color "red" &&
+	git config --get --type=color foo.color >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	echo "<RED>" >expect &&
+	test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+	color = red
+EOF
+
+test_expect_success 'set --type=color' '
+	rm .git/config &&
+	git config --type=color foo.color "red" &&
+	test_cmp expect .git/config
+'
+
+test_expect_success 'get --type=color barfs on non-color' '
+	echo "[foo]bar=not-a-color" >.git/config &&
+	test_must_fail git config --get --type=color foo.bar
+'
+
+test_expect_success 'set --type=color barfs on non-color' '
+	test_must_fail git config --type=color foo.color "not-a-color" 2>error &&
+	test_i18ngrep "cannot parse color" error
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"

Signature · 1 line

2.17.0

Junio C Hamano

2018-04-26 05:25 UTC #3 lore raw

Re: [PATCH 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--type`

In reply to Taylor Blau

Quoted 1 line · Taylor Blau

Subject: Re: [PATCH 2/5] builtin/config.c: support --type=<type> as preferred alias for --type

I'd retitle while queuing, as the last 'type' is a placeholder for concrete types like <type> above.

builtin/config.c static int show_origin; · line 74

Quoted 5 lines

builtin/config.c +5 -0

+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	int new_type;
+	int *to_type;

Splitting these into two lines (unlike placing on a single same line, which was how the previous round was queued) like this is good.

builtin/config.c static int show_origin; · line 101

Quoted 7 lines

+...

builtin/config.c +6 -0

+	new_type = opt->defval;
+	if (!new_type) {
+...
+	}
+
+	*to_type = opt->value;

But this is wrong, no? You meant opt->value points at an integer variable that receives the type we discover by parsing, i.e.

	to_type = opt->value;

Junio C Hamano

2018-04-26 05:33 UTC #9 lore raw

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

In reply to Taylor Blau

Quoted 35 lines · Taylor Blau

builtin/config.c +13 -0

index d7acf912cd..ec5c11293b 100644
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT	3
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
+#define TYPE_COLOR		6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
 	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \

@@ -231,6 +232,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
 			strbuf_addf(buf, "%"PRItime, t);
+		} else if (type == TYPE_COLOR) {
+			char v[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {

@@ -376,6 +382,20 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (type == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (git_config_color(v, key, value))
+			die("cannot parse color '%s'", value);
+
+		/*
+		 * The contents of `v` now contain an ANSI escape

builtin/config.c static char *normalize_value(const char *key, const char *value) · line 401

Quoted 10 lines

builtin/config.c +7 -0

+		 * sequence, not suitable for including within a
+		 * configuration file. Treat the above as a
+		 * "sanity-check", and return the given value, which we
+		 * know is representable as valid color code.
+		 */
+		return xstrdup(value);
+	}
 
 	die("BUG: cannot normalize type %d", type);
 }

Hmmm, option_parse_type() introduced in [PATCH 2/5] used to learn to parse "color" in this step, but it no longer does.

Here is the difference between what has been queued and the result of applying these latest patches I found.

builtin/config.c +3 -4

index 69e7270356..ec5c11293b 100644
@@ -72,7 +72,8 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
-	int new_type, *to_type;
+	int new_type;
+	int *to_type;
 
 	if (unset) {
 		*((int *) opt->value) = 0;

@@ -95,13 +96,11 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			new_type = TYPE_PATH;
 		else if (!strcmp(arg, "expiry-date"))
 			new_type = TYPE_EXPIRY_DATE;
-		else if (!strcmp(arg, "color"))
-			new_type = TYPE_COLOR;
 		else
 			die(_("unrecognized --type argument, %s"), arg);
 	}
 
-	to_type = opt->value;
+	*to_type = opt->value;
 	if (*to_type && *to_type != new_type) {
 		/*
 		 * Complain when there is a new type not equal to the old type.

Taylor Blau

author

2018-04-26 05:58 UTC #11 lore raw

v2 cover builtin/config.c: combined series '--type', '--default'

In reply to Taylor Blau

Hi,

Here is an amended version of my combined series to add '--type' and '--default'. My apologies for the re-roll, I thought that I had looked everything over closely enough :-).

Since last time:

* Correct an obviously-wrong assignment into '*to_type' [1]. I have moved both of these assignments into the top-line declaration of those variables.

* Re-add a removed hunk to support '--type=color' correctly [2].

Thanks,
Taylor

[1]: https://public-inbox.org/git/[email protected]
[2]: https://public-inbox.org/git/[email protected]

Taylor Blau (5):
builtin/config.c: treat type specifiers singularly builtin/config.c: support --type=<type> as preferred alias for

    `--<type>`

builtin/config: introduce --default
config.c: introduce 'git_config_color' to parse ANSI colors builtin/config: introduce color type specifier

 Documentation/git-config.txt |  81 ++++++++++++--------
 builtin/config.c             | 143 ++++++++++++++++++++++++++++-------
 config.c                     |  10 +++
 config.h                     |   1 +
 t/t1300-repo-config.sh       |  93 +++++++++++++++++++++++
 t/t1310-config-default.sh    |  36 +++++++++
 6 files changed, 305 insertions(+), 59 deletions(-)
 create mode 100755 t/t1310-config-default.sh

Inter-diff (since v1):

builtin/config.c +8 -8

index ec5c11293b..bb62816bba 100644
@@ -72,19 +72,18 @@ static struct option builtin_config_options[];
 static int option_parse_type(const struct option *opt, const char *arg,
 			     int unset)
 {
-	int new_type;
-	int *to_type;
+	/*
+	 * To support '--<type>' style flags, begin with new_type equal to
+	 * opt->defval.
+	 */
+	int new_type = opt->defval;
+	int *to_type = opt->value;
 
 	if (unset) {
 		*((int *) opt->value) = 0;
 		return 0;
 	}
 
-	/*
-	 * To support '--<type>' style flags, begin with new_type equal to
-	 * opt->defval.
-	 */
-	new_type = opt->defval;
 	if (!new_type) {
 		if (!strcmp(arg, "bool"))
 			new_type = TYPE_BOOL;

@@ -96,11 +95,12 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			new_type = TYPE_PATH;
 		else if (!strcmp(arg, "expiry-date"))
 			new_type = TYPE_EXPIRY_DATE;
+		else if (!strcmp(arg, "color"))
+			new_type = TYPE_COLOR;
 		else
 			die(_("unrecognized --type argument, %s"), arg);
 	}
 
-	*to_type = opt->value;
 	if (*to_type && *to_type != new_type) {
 		/*
 		 * Complain when there is a new type not equal to the old type.

--
2.17.0

Taylor Blau

author

2018-04-26 05:58 UTC #12 lore raw

PATCH v2 1/5 builtin/config.c: treat type specifiers singularly

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

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

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

  $ git config --bool some.section

  $ git config --int --type=int

Signed-off-by Taylor Blau <[email protected]>

---
 builtin/config.c       | 49 +++++++++++++++++++-----------------------
 t/t1300-repo-config.sh | 11 ++++++++++
 2 files changed, 33 insertions(+), 27 deletions(-)

builtin/config.c +22 ~~-27~~

index 01169dd628..92fb8d56b1 100644
@@ -25,7 +25,7 @@ static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
-static int actions, types;
+static int actions, type;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;

@@ -55,11 +55,11 @@ static int show_origin;
 #define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
 			ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
 
-#define TYPE_BOOL (1<<0)
-#define TYPE_INT (1<<1)
-#define TYPE_BOOL_OR_INT (1<<2)
-#define TYPE_PATH (1<<3)
-#define TYPE_EXPIRY_DATE (1<<4)
+#define TYPE_BOOL		1
+#define TYPE_INT		2
+#define TYPE_BOOL_OR_INT	3
+#define TYPE_PATH		4
+#define TYPE_EXPIRY_DATE	5
 
 static struct option builtin_config_options[] = {
 	OPT_GROUP(N_("Config file location")),

@@ -84,11 +84,11 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
 	OPT_GROUP(N_("Type")),
-	OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
-	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
-	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
-	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
-	OPT_BIT(0, "expiry-date", &types, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+	OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+	OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+	OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+	OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+	OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),

@@ -149,26 +149,26 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		if (show_keys)
 			strbuf_addch(buf, key_delim);
 
-		if (types == TYPE_INT)
+		if (type == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
 				    git_config_int64(key_, value_ ? value_ : ""));
-		else if (types == TYPE_BOOL)
+		else if (type == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
-		else if (types == TYPE_BOOL_OR_INT) {
+		else if (type == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
 				strbuf_addstr(buf, v ? "true" : "false");
 			else
 				strbuf_addf(buf, "%d", v);
-		} else if (types == TYPE_PATH) {
+		} else if (type == TYPE_PATH) {
 			const char *v;
 			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
 			strbuf_addstr(buf, v);
 			free((char *)v);
-		} else if (types == TYPE_EXPIRY_DATE) {
+		} else if (type == TYPE_EXPIRY_DATE) {
 			timestamp_t t;
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;

@@ -287,7 +287,7 @@ static char *normalize_value(const char *key, const char *value)
 	if (!value)
 		return NULL;
 
-	if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE)
+	if (type == 0 || type == TYPE_PATH || type == TYPE_EXPIRY_DATE)
 		/*
 		 * We don't do normalization for TYPE_PATH here: If
 		 * the path is like ~/foobar/, we prefer to store

@@ -296,11 +296,11 @@ static char *normalize_value(const char *key, const char *value)
 		 * Also don't do normalization for expiry dates.
 		 */
 		return xstrdup(value);
-	if (types == TYPE_INT)
+	if (type == TYPE_INT)
 		return xstrfmt("%"PRId64, git_config_int64(key, value));
-	if (types == TYPE_BOOL)
+	if (type == TYPE_BOOL)
 		return xstrdup(git_config_bool(key, value) ?  "true" : "false");
-	if (types == TYPE_BOOL_OR_INT) {
+	if (type == TYPE_BOOL_OR_INT) {
 		int is_bool, v;
 		v = git_config_bool_or_int(key, value, &is_bool);
 		if (!is_bool)

@@ -309,7 +309,7 @@ static char *normalize_value(const char *key, const char *value)
 			return xstrdup(v ? "true" : "false");
 	}
 
-	die("BUG: cannot normalize type %d", types);
+	die("BUG: cannot normalize type %d", type);
 }
 
 static int get_color_found;

@@ -566,12 +566,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		key_delim = '\n';
 	}
 
-	if (HAS_MULTI_BITS(types)) {
-		error("only one type at a time.");
-		usage_with_options(builtin_config_usage, builtin_config_options);
-	}
-
-	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
+	if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && type) {
 		error("--get-color and variable type are incoherent");
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}

t/t1300-repo-config.sh +11 -0

index e95b1e67da..ed9b9cd344 100755
@@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
 	test_expect_code 128 nongit git config --local foo.bar
 '
 
+cat >.git/config <<-\EOF &&
+[core]
+number = 10
+EOF
+
+test_expect_success 'later legacy specifiers are given precedence' '
+	git config --bool --int core.number >actual &&
+	echo 10 >expect &&
+	test_cmp expect actual
+'
+
 test_done

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 05:58 UTC #13 lore raw

PATCH v2 2/5 builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`

In reply to Taylor Blau

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

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

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt | 71 ++++++++++++++++++++----------------
 builtin/config.c             | 64 +++++++++++++++++++++++++++++---
 t/t1300-repo-config.sh       | 58 +++++++++++++++++++++++++++--
 3 files changed, 153 insertions(+), 40 deletions(-)

Documentation/git-config.txt +39 ~~-32~~

index e09ed5d7d5..b759009c75 100644
@@ -9,13 +9,13 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [type] --add name value
-'git config' [<file-option>] [type] --replace-all name value [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [type] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
-'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name

@@ -38,12 +38,10 @@ existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
 
-The type specifier can be either `--int` or `--bool`, to make
-'git config' ensure that the variable(s) are of the given type and
-convert the value to the canonical form (simple decimal number for int,
-a "true" or "false" string for bool), or `--path`, which does some
-path expansion (see `--path` below).  If no type specifier is passed, no
-checks or transformations are performed on the value.
+The `--type=<type>` option instructs 'git config' to ensure that incoming and
+outgoing values are canonicalize-able under the given <type>.  If no
+`--type=<type>` is given, no canonicalization will be performed. Callers may
+unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options

@@ -160,30 +158,39 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
---bool::
-	'git config' will ensure that the output is "true" or "false"
+--type <type>::
+  'git config' will ensure that any input or output is valid under the given
+  type constraint(s), and will canonicalize outgoing values in `<type>`'s
+  canonical form.
++
+Valid `<type>`'s include:
++
+- 'bool': canonicalize values as either "true" or "false".
+- 'int': canonicalize values as simple decimal numbers. An optional suffix of
+  'k', 'm', or 'g' will cause the value to be multiplied by 1024, 1048576, or
+  1073741824 upon input.
+- 'bool-or-int': canonicalize according to either 'bool' or 'int', as described
+  above.
+- 'path': canonicalize by adding a leading `~` to the value of `$HOME` and
+  `~user` to the home directory for the specified user. This specifier has no
+  effect when setting the value (but you can use `git config section.variable
+  ~/` from the command line to let your shell do the expansion.)
+- 'expiry-date': canonicalize by converting from a fixed or relative date-string
+  to a timestamp. This specifier has no effect when setting the value.
++
 
+--bool::
 --int::
-	'git config' will ensure that the output is a simple
-	decimal number.  An optional value suffix of 'k', 'm', or 'g'
-	in the config file will cause the value to be multiplied
-	by 1024, 1048576, or 1073741824 prior to output.
-
 --bool-or-int::
-	'git config' will ensure that the output matches the format of
-	either --bool or --int, as described above.
-
 --path::
-	`git config` will expand a leading `~` to the value of
-	`$HOME`, and `~user` to the home directory for the
-	specified user.  This option has no effect when setting the
-	value (but you can use `git config section.variable ~/`
-	from the command line to let your shell do the expansion).
-
 --expiry-date::
-	`git config` will ensure that the output is converted from
-	a fixed or relative date-string to a timestamp. This option
-	has no effect when setting the value.
+  Historical options for selecting a type specifier. Prefer instead `--type`,
+  (see: above).
+
+--no-type::
+  Un-sets the previously set type specifier (if one was previously set). This
+  option requests that 'git config' not canonicalize the retrieved variable.
+  `--no-type` has no effect without `--type=<type>` or `--<type>`.
 
 -z::
 --null::

builtin/config.c +59 -5

76 lines not on this page · raw message

t/t1300-repo-config.sh +55 -3

68 lines not on this page · raw message

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 05:58 UTC #16 lore raw

PATCH v2 3/5 builtin/config: introduce `--default`

In reply to Taylor Blau

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

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

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

For example, we aim to replace:

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

with:

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

Specifically, this means that the following will work:

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

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

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 18 ++++++++++++++++++
 t/t1310-config-default.sh    | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100755 t/t1310-config-default.sh

Documentation/git-config.txt +4 -0

index b759009c75..c3adafd78a 100644
@@ -240,6 +240,10 @@ Valid `<type>`'s include:
 	using `--file`, `--global`, etc) and `on` when searching all
 	config files.
 
+--default <value>::
+  When using `--get`, and the requested variable is not found, behave as if
+  <value> were the value assigned to the that variable.
+
 CONFIGURATION
 -------------
 `pager.config` is only respected when listing configuration, i.e., when

builtin/config.c +18 -0

index 9eda01615b..4f222a0ca9 100644
@@ -26,6 +26,7 @@ static char term = '\n';
 static int use_global_config, use_system_config, use_local_config;
 static struct git_config_source given_config_source;
 static int actions, type;
+static char *default_value;
 static int end_null;
 static int respect_includes_opt = -1;
 static struct config_options config_options;

@@ -148,6 +149,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
+	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
 	OPT_END(),
 };

@@ -312,6 +314,16 @@ static int get_value(const char *key_, const char *regex_)
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
 
+	if (!values.nr && default_value) {
+		struct strbuf *item;
+		ALLOC_GROW(values.items, values.nr + 1, values.alloc);
+		item = &values.items[values.nr++];
+		strbuf_init(item, 0);
+		if (format_config(item, key_, default_value) < 0)
+			die(_("failed to format default config value: %s"),
+				default_value);
+	}
+
 	ret = !values.nr;
 
 	for (i = 0; i < values.nr; i++) {

@@ -650,6 +662,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_config_usage, builtin_config_options);
 	}
 
+	if (default_value && !(actions & ACTION_GET)) {
+		error("--default is only applicable to --get");
+		usage_with_options(builtin_config_usage,
+			builtin_config_options);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);

t/t1310-config-default.sh new file +36 -0

new file mode 100755
index 0000000000..6049d91708
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='Test git config in different settings (with --default)'
+
+. ./test-lib.sh
+
+test_expect_success 'uses --default when entry missing' '
+	echo quux >expect &&
+	git config -f config --default=quux core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'does not use --default when entry present' '
+	echo bar >expect &&
+	git -c core.foo=bar config --default=baz core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'canonicalizes --default with appropriate type' '
+	echo true >expect &&
+	git config -f config --default=yes --bool core.foo >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'dies when --default cannot be parsed' '
+	test_must_fail git config -f config --type=expiry-date --default=x --get \
+		not.a.section 2>error &&
+	test_i18ngrep "failed to format default config value" error
+'
+
+test_expect_success 'does not allow --default without --get' '
+	test_must_fail git config --default=quux --unset a.section >output 2>&1 &&
+	test_i18ngrep "\-\-default is only applicable to" output
+'
+
+test_done

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 05:58 UTC #17 lore raw

PATCH v2 4/5 config.c: introduce 'git_config_color' to parse ANSI colors

In reply to Taylor Blau

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

Signed-off-by Taylor Blau <[email protected]>

---
 config.c | 10 ++++++++++
 config.h |  1 +
 2 files changed, 11 insertions(+)

config.c +10 -0

index 62c56099bf..d14813bef7 100644
@@ -16,6 +16,7 @@
 #include "string-list.h"
 #include "utf8.h"
 #include "dir.h"
+#include "color.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -1000,6 +1001,15 @@ int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *
 	return 0;
 }
 
+int git_config_color(char *dest, const char *var, const char *value)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (color_parse(value, dest) < 0)
+		return -1;
+	return 0;
+}
+
 static int git_default_core_config(const char *var, const char *value)
 {
 	/* This needs a better name */

config.h +1 -0

index ef70a9cac1..0e060779d9 100644
@@ -59,6 +59,7 @@ extern int git_config_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_expiry_date(timestamp_t *, const char *, const char *);
+extern int git_config_color(char *, const char *, const char *);
 extern int git_config_set_in_file_gently(const char *, const char *, const char *);
 extern void git_config_set_in_file(const char *, const char *, const char *);
 extern int git_config_set_gently(const char *, const char *);

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 05:59 UTC #18 lore raw

PATCH v2 5/5 builtin/config: introduce `color` type specifier

In reply to Taylor Blau

As of this commit, the canonical way to retreive an ANSI-compatible color escape sequence from a configuration file is with the --get-color action.

This is to allow Git to "fall back" on a default value for the color should the given section not exist in the specified configuration(s).

With the addition of --default, this is no longer needed since:

  $ git config --default red --type=color core.section

will be have exactly as:

  $ git config --get-color core.section red

For consistency, let's introduce --type=color and encourage its use with --default together over --get-color alone.

Signed-off-by Taylor Blau <[email protected]>

---
 Documentation/git-config.txt |  6 ++++++
 builtin/config.c             | 22 ++++++++++++++++++++++
 t/t1300-repo-config.sh       | 30 ++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+)

Documentation/git-config.txt +6 -0

index c3adafd78a..18ddc78f42 100644
@@ -177,6 +177,10 @@ Valid `<type>`'s include:
   ~/` from the command line to let your shell do the expansion.)
 - 'expiry-date': canonicalize by converting from a fixed or relative date-string
   to a timestamp. This specifier has no effect when setting the value.
+- 'color': When getting a value, canonicalize by converting to an ANSI color
+  escape sequence. When setting a value, a sanity-check is performed to ensure
+  that the given value is canonicalize-able as an ANSI color, but it is written
+  as-is.
 +
 
 --bool::

@@ -228,6 +232,8 @@ Valid `<type>`'s include:
 	output it as the ANSI color escape sequence to the standard
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
++
+`--type=color [--default=<default>]` is preferred over `--get-color`.
 
 -e::
 --edit::

builtin/config.c +22 -0

index 4f222a0ca9..bb62816bba 100644
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT	3
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
+#define TYPE_COLOR		6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
 	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \

@@ -94,6 +95,8 @@ static int option_parse_type(const struct option *opt, const char *arg,
 			new_type = TYPE_PATH;
 		else if (!strcmp(arg, "expiry-date"))
 			new_type = TYPE_EXPIRY_DATE;
+		else if (!strcmp(arg, "color"))
+			new_type = TYPE_COLOR;
 		else
 			die(_("unrecognized --type argument, %s"), arg);
 	}

@@ -229,6 +232,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
 			strbuf_addf(buf, "%"PRItime, t);
+		} else if (type == TYPE_COLOR) {
+			char v[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {

@@ -374,6 +382,20 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (type == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (git_config_color(v, key, value))
+			die("cannot parse color '%s'", value);
+
+		/*
+		 * The contents of `v` now contain an ANSI escape
+		 * sequence, not suitable for including within a
+		 * configuration file. Treat the above as a
+		 * "sanity-check", and return the given value, which we
+		 * know is representable as valid color code.
+		 */
+		return xstrdup(value);
+	}
 
 	die("BUG: cannot normalize type %d", type);
 }

t/t1300-repo-config.sh +30 -0

index 1e3a1ace14..2acfd513f1 100755
@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
 	test_must_fail git config --expiry-date date.invalid1
 '
 
+test_expect_success 'get --type=color' '
+	rm .git/config &&
+	git config foo.color "red" &&
+	git config --get --type=color foo.color >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	echo "<RED>" >expect &&
+	test_cmp expect actual
+'
+
+cat >expect << EOF
+[foo]
+	color = red
+EOF
+
+test_expect_success 'set --type=color' '
+	rm .git/config &&
+	git config --type=color foo.color "red" &&
+	test_cmp expect .git/config
+'
+
+test_expect_success 'get --type=color barfs on non-color' '
+	echo "[foo]bar=not-a-color" >.git/config &&
+	test_must_fail git config --get --type=color foo.bar
+'
+
+test_expect_success 'set --type=color barfs on non-color' '
+	test_must_fail git config --type=color foo.color "not-a-color" 2>error &&
+	test_i18ngrep "cannot parse color" error
+'
+
 cat > expect << EOF
 [quote]
 	leading = " test"

Signature · 1 line

2.17.0

Taylor Blau

author

2018-04-26 06:00 UTC #4 lore raw

Re: [PATCH 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--type`

In reply to Junio C Hamano

Quoted 6 lines · Junio C Hamano

Quoted 1 line · Taylor Blau
Subject: Re: [PATCH 2/5] builtin/config.c: support --type=<type> as preferred alias for --type
I'd retitle while queuing, as the last 'type' is a placeholder for concrete types like <type> above.

Good idea. I amended v2 in this fashion.

Quoted 11 lines

builtin/config.c static int show_origin; · line 101
Quoted 7 lines
+...
builtin/config.c +6 -0
+	new_type = opt->defval;
+	if (!new_type) {
+...
+	}
+
+	*to_type = opt->value;
But this is wrong, no? You meant opt->value points at an integer variable that receives the type we discover by parsing, i.e.

Quoted 1 line

	to_type = opt->value;

Oof. You're absolutely right. I fixed this and moved the assignment to the declaration at the top of this function.

Thanks,
Taylor

Taylor Blau

author

2018-04-26 06:01 UTC #10 lore raw

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

In reply to Junio C Hamano

Quoted 45 lines · Junio C Hamano

Quoted 43 lines · Taylor Blau

builtin/config.c +20 -0

index d7acf912cd..ec5c11293b 100644
@@ -61,6 +61,7 @@ static int show_origin;
 #define TYPE_BOOL_OR_INT	3
 #define TYPE_PATH		4
 #define TYPE_EXPIRY_DATE	5
+#define TYPE_COLOR		6
 
 #define OPT_CALLBACK_VALUE(s, l, v, h, i) \
 	{ OPTION_CALLBACK, (s), (l), (v), NULL, (h), PARSE_OPT_NOARG | \

@@ -231,6 +232,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 			if (git_config_expiry_date(&t, key_, value_) < 0)
 				return -1;
 			strbuf_addf(buf, "%"PRItime, t);
+		} else if (type == TYPE_COLOR) {
+			char v[COLOR_MAXLEN];
+			if (git_config_color(v, key_, value_) < 0)
+				return -1;
+			strbuf_addstr(buf, v);
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {

@@ -376,6 +382,20 @@ static char *normalize_value(const char *key, const char *value)
 		else
 			return xstrdup(v ? "true" : "false");
 	}
+	if (type == TYPE_COLOR) {
+		char v[COLOR_MAXLEN];
+		if (git_config_color(v, key, value))
+			die("cannot parse color '%s'", value);
+
+		/*
+		 * The contents of `v` now contain an ANSI escape
+		 * sequence, not suitable for including within a
+		 * configuration file. Treat the above as a
+		 * "sanity-check", and return the given value, which we
+		 * know is representable as valid color code.
+		 */
+		return xstrdup(value);
+	}

Quoted 5 lines

builtin/config.c static char *normalize_value(const char *key, const char *value) · line 401
Quoted 2 lines
builtin/config.c +0 -0
 	die("BUG: cannot normalize type %d", type);
 }
Hmmm, option_parse_type() introduced in [PATCH 2/5] used to learn to parse "color" in this step, but it no longer does.

Oof, again. I dropped this hunk on the floor when integrating. I put it back in v2.

Thanks,
Taylor

Junio C Hamano

2018-04-26 09:55 UTC #14 lore raw

Re: [PATCH v2 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`

In reply to Taylor Blau

builtin/config.c static int show_origin; · line 81

Quoted 12 lines · Taylor Blau

builtin/config.c +12 -0

+static int option_parse_type(const struct option *opt, const char *arg,
+			     int unset)
+{
+	/*
+	 * To support '--<type>' style flags, begin with new_type equal to
+	 * opt->defval.
+	 */
+	int new_type = opt->defval;
+	int *to_type = opt->value;
+
+	if (unset) {
+		*((int *) opt->value) = 0;

As you moved the definition of to_type higher than the previous rounds, you can already use it here to avoid cryptic casting, i.e.

		*to_type = 0;

no?

Quoted 2 lines

(quoted diff) +2 -0
+		return 0;
+	}

Eric Sunshine

2018-04-27 05:57 UTC #15 lore raw

Re: [PATCH v2 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--<type>`

In reply to Taylor Blau

Quoted 7 lines · Taylor Blau

git config has long allowed the ability for callers to provide a 'type specifier', which instructs git config to (1) ensure that incoming values can be interpreted as that type, and (2) that outgoing values are canonicalized under that type.
In another series, we propose to extend this functionality with --type=color and --default to replace --get-color.

Now that you've combined the two series, this sentence no longer makes sense as written. Perhaps say:

Later patches will extend this functionality...

Quoted 8 lines

However, we traditionally use --color to mean "colorize this output", instead of "this value should be treated as a color".
Currently, git config does not support this kind of colorization, but we should be careful to avoid squatting on this option too soon, so that git config can support --color (in the traditional sense) in the future, if that is desired.

Quoted 6 lines

In this patch, we support --type=<int|bool|bool-or-int|...> in addition to --int, --bool, and etc. This allows the aforementioned upcoming patch to support querying a color value with a default via --type=color --default=..., without squandering --color.
We retain the historic behavior of complaining when multiple,

Drop the comma and be more specific:
s/multiple,/multiple conflicting/

Quoted 4 lines

legacy-style --<type> flags are given, as well as extend this to conflicting new-style --type=<type> flags. --int --type=int (and its commutative pair) does not complain, but --bool --type=int (and its commutative pair) does.

Confusing. Part of the selling point of the commit message of patch 1/5 is the removal of this complaint/restriction, claiming that it intentionally treats "git config --int --bool" simply as "git config --bool", and that that loosening is required to support "git config --int --type=int" without complaining, thus is a good thing. But this commit message (2/5) backpedals and reinstates the original complaint/restriction.

Perhaps I could have understood if 1/5 said that the loosening of the restriction was only temporary and that it would be restored by a later patch rather than using the restriction-removal as a selling point. However, this patch series doesn't need to be crafted such that a feature is temporarily lost and later restored, so I'm having trouble buying the way this series is architected.

What could make more sense would be for 1/5 to introduce option_parse_type() for --<type>, thus retaining the restriction, and for 2/5 simply to augment option_parse_type() to also understand --type=<type>.

Quoted 1 line

Signed-off-by Taylor Blau <[email protected]>

Eric Sunshine

2018-04-27 06:06 UTC #5 lore raw

Re: [PATCH 2/5] builtin/config.c: support `--type=<type>` as preferred alias for `--type`

In reply to Taylor Blau

Quoted 9 lines · Taylor Blau

Quoted 6 lines · Junio C Hamano
Quoted 1 line · Taylor Blau
Subject: Re: [PATCH 2/5] builtin/config.c: support --type=<type> as preferred alias for --type
I'd retitle while queuing, as the last 'type' is a placeholder for concrete types like <type> above.
Good idea. I amended v2 in this fashion.

Suggested previously[1], as well.

[1]: https://public-inbox.org/git/CAPig+cTw5ZXKGOcnombwiM4P+9iRpbYUvOagQ0XujLB0ttXsOg@mail.gmail.com/

Eric Sunshine

2018-04-27 06:22 UTC #19 lore raw

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

In reply to Taylor Blau

Documentation/git-config.txt Valid `<type>`'s include: · line 236

Quoted 13 lines · Taylor Blau

[...]
For consistency, let's introduce --type=color and encourage its use with --default together over --get-color alone.
Signed-off-by Taylor Blau <[email protected]>
---
Documentation/git-config.txt +2 -0
@@ -228,6 +232,8 @@ Valid `<type>`'s include:
        output it as the ANSI color escape sequence to the standard
        output.  The optional `default` parameter is used instead, if
        there is no color configured for `name`.
++
+`--type=color [--default=<default>]` is preferred over `--get-color`.

Rather than augmenting the documentation of --get-color like this, I wonder if it would instead make more sense to replace it entirely, like this:

--get-color name [<default>]
Deprecated alias for --type=color [--default=<default>].

Not necessarily worth a re-roll; could be done as a follow-up if someone cares enough.

t/t1300-repo-config.sh test_expect_success 'get --expiry-date' ' · line 932

Quoted 4 lines

t/t1300-repo-config.sh +2 -0

@@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
+test_expect_success 'get --type=color' '
+       rm .git/config &&

I would feel more confident about the robustness of this test if it instead used 'rm -f .git/config', as is already done elsewhere in this test script.

t/t1300-repo-config.sh test_expect_success 'get --expiry-date' ' · line 949

Quoted 9 lines

t/t1300-repo-config.sh +9 -0

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

Ditto.

Quoted 3 lines

t/t1300-repo-config.sh +3 -0

+       git config --type=color foo.color "red" &&
+       test_cmp expect .git/config
+'