[PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Martin Ågren]

To find all config items "foo.*" that are configured as the Boolean
value "true", you can try executing

  git config --type=bool --name-only --get-regexp '^foo\.' true

... and hope that you didn't spell "true" as "on", "True" or "1" back
when you populated your config. This shortcoming has been mentioned as a
left-over bit [1] [2].

This patch series teaches `git config` to canonicalize the incoming
"value_regex" ("true" in the example above), then canonicalize candidate
values as we go through the config. Or if you will, `git config` learns
a brand new type of regex, corresponding to the different ways there are
of spelling "true" and "false", respectively.

`--type=bool-or-int` gets the same treatment, except we need to to be
able to handle the ints and regexes matching particular ints that we
must expect. That said, even with `--type=bool` we can't move too
aggressively towards *requiring* that the incoming "value_regex"
canonializes as a Boolean value. The penultimate patch starts to warn on
non-canonicalizing values; the final patch makes us bail out entirely.

The last patch is not meant for immediate inclusion, but I post it
anyway. I can re-submit it at an appropriate time, or maybe it could
slumber on pu until the time is ripe for completing the switch.

[1] https://git-blame.blogspot.com/p/leftover-bits.html
[2] https://public-inbox.org/git/xmqq7frsh4tw.fsf@gitster.dls.corp.google.com/

Martin Ågren (8):
  config: make `git_parse_maybe_bool_text()` public
  t1300: modernize part of script
  builtin/config: extract `handle_value_regex()` from `get_value()`
  builtin/config: collect "value_regexp" data in a struct
  builtin/config: canonicalize "value_regex" with `--type=bool`
  builtin/config: canonicalize "value_regex" with `--type=bool-or-int`
  builtin/config: warn if "value_regex" doesn't canonicalize as boolean
  builtin/config: die if "value_regex" doesn't canonicalize as boolean

 Documentation/git-config.txt |   5 +
 builtin/config.c             |  84 +++++++++++----
 config.c                     |   2 +-
 config.h                     |   7 ++
 t/t1300-config.sh            | 199 ++++++++++++++++++++++-------------
 5 files changed, 203 insertions(+), 94 deletions(-)

-- 
2.24.0

[PATCH 1/8] config: make `git_parse_maybe_bool_text()` public

[Martin Ågren]

We will soon use it from outside config.c.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 config.c | 2 +-
 config.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index e7052b3977..318711f7a7 100644
--- a/config.c
+++ b/config.c
@@ -1047,7 +1047,7 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
 	return ret;
 }
 
-static int git_parse_maybe_bool_text(const char *value)
+int git_parse_maybe_bool_text(const char *value)
 {
 	if (!value)
 		return 1;
diff --git a/config.h b/config.h
index f0ed464004..343f24c408 100644
--- a/config.h
+++ b/config.h
@@ -95,6 +95,13 @@ int config_with_options(config_fn_t fn, void *,
 int git_parse_ssize_t(const char *, ssize_t *);
 int git_parse_ulong(const char *, unsigned long *);
 int git_parse_maybe_bool(const char *);
+
+/**
+ * Same as `git_parse_maybe_bool`, except that it does not handle
+ * integer values, i.e., those cause this function to return -1.
+ */
+int git_parse_maybe_bool_text(const char *);
+
 int git_config_int(const char *, const char *);
 int64_t git_config_int64(const char *, const char *);
 unsigned long git_config_ulong(const char *, const char *);
-- 
2.24.0

[PATCH 2/8] t1300: modernize part of script

[Martin Ågren]

Create `.git/config` and `expect` files inside `test_expect_success`,
either inside the one existing test that uses the file, or in a new
"setup" step before several tests that use it.

Redirect using `>output` rather than `> output`. Use `<<-\EOF" with
heredocs rather than just `<< EOF` and use `q_to_tab` to create properly
indented input and output files.

This commit does not modernize the whole script, but just some of it,
around the point where a later commit will add new content.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 t/t1300-config.sh | 138 ++++++++++++++++++++++------------------------
 1 file changed, 65 insertions(+), 73 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 983a0a1583..a38cc143a1 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -321,15 +321,14 @@ test_expect_success 'hierarchical section value' '
 	test_cmp expect .git/config
 '
 
-cat > expect << EOF
-beta.noindent=sillyValue
-nextsection.nonewline=wow2 for me
-123456.a123=987
-version.1.2.3eX.alpha=beta
-EOF
-
 test_expect_success 'working --list' '
-	git config --list > output &&
+	cat >expect <<-\EOF &&
+	beta.noindent=sillyValue
+	nextsection.nonewline=wow2 for me
+	123456.a123=987
+	version.1.2.3eX.alpha=beta
+	EOF
+	git config --list >output &&
 	test_cmp expect output
 '
 test_expect_success '--list without repo produces empty output' '
@@ -337,55 +336,53 @@ test_expect_success '--list without repo produces empty output' '
 	test_must_be_empty output
 '
 
-cat > expect << EOF
-beta.noindent
-nextsection.nonewline
-123456.a123
-version.1.2.3eX.alpha
-EOF
-
 test_expect_success '--name-only --list' '
+	cat >expect <<-\EOF &&
+	beta.noindent
+	nextsection.nonewline
+	123456.a123
+	version.1.2.3eX.alpha
+	EOF
 	git config --name-only --list >output &&
 	test_cmp expect output
 '
 
-cat > expect << EOF
-beta.noindent sillyValue
-nextsection.nonewline wow2 for me
-EOF
-
 test_expect_success '--get-regexp' '
+	cat >expect <<-\EOF &&
+	beta.noindent sillyValue
+	nextsection.nonewline wow2 for me
+	EOF
 	git config --get-regexp in >output &&
 	test_cmp expect output
 '
 
-cat > expect << EOF
-beta.noindent
-nextsection.nonewline
-EOF
-
 test_expect_success '--name-only --get-regexp' '
+	cat >expect <<-\EOF &&
+	beta.noindent
+	nextsection.nonewline
+	EOF
 	git config --name-only --get-regexp in >output &&
 	test_cmp expect output
 '
 
-cat > expect << EOF
-wow2 for me
-wow4 for you
-EOF
-
 test_expect_success '--add' '
+	cat >expect <<-\EOF &&
+	wow2 for me
+	wow4 for you
+	EOF
 	git config --add nextsection.nonewline "wow4 for you" &&
-	git config --get-all nextsection.nonewline > output &&
+	git config --get-all nextsection.nonewline >output &&
 	test_cmp expect output
 '
 
-cat > .git/config << EOF
-[novalue]
-	variable
-[emptyvalue]
-	variable =
-EOF
+test_expect_success 'setup config file with no/empty values' '
+	q_to_tab >.git/config <<-\EOF
+	[novalue]
+		Qvariable
+	[emptyvalue]
+		Qvariable =
+	EOF
+'
 
 test_expect_success 'get variable with no value' '
 	git config --get novalue.variable ^$
@@ -395,38 +392,33 @@ test_expect_success 'get variable with empty value' '
 	git config --get emptyvalue.variable ^$
 '
 
-echo novalue.variable > expect
-
 test_expect_success 'get-regexp variable with no value' '
-	git config --get-regexp novalue > output &&
+	echo novalue.variable >expect &&
+	git config --get-regexp novalue >output &&
 	test_cmp expect output
 '
 
-echo 'novalue.variable true' > expect
-
 test_expect_success 'get-regexp --bool variable with no value' '
-	git config --bool --get-regexp novalue > output &&
+	echo "novalue.variable true" >expect &&
+	git config --bool --get-regexp novalue >output &&
 	test_cmp expect output
 '
 
-echo 'emptyvalue.variable ' > expect
-
 test_expect_success 'get-regexp variable with empty value' '
-	git config --get-regexp emptyvalue > output &&
+	echo "emptyvalue.variable " >expect &&
+	git config --get-regexp emptyvalue >output &&
 	test_cmp expect output
 '
 
-echo true > expect
-
 test_expect_success 'get bool variable with no value' '
-	git config --bool novalue.variable > output &&
+	echo true >expect &&
+	git config --bool novalue.variable >output &&
 	test_cmp expect output
 '
 
-echo false > expect
-
 test_expect_success 'get bool variable with empty value' '
-	git config --bool emptyvalue.variable > output &&
+	echo false >expect &&
+	git config --bool emptyvalue.variable >output &&
 	test_cmp expect output
 '
 
@@ -435,34 +427,34 @@ test_expect_success 'no arguments, but no crash' '
 	test_i18ngrep usage output
 '
 
-cat > .git/config << EOF
-[a.b]
-	c = d
-EOF
-
-cat > expect << EOF
-[a.b]
-	c = d
-[a]
-	x = y
-EOF
+test_expect_success 'setup simple config file' '
+	q_to_tab >.git/config <<-\EOF
+	[a.b]
+		Qc = d
+	EOF
+'
 
 test_expect_success 'new section is partial match of another' '
+	q_to_tab >expect <<-\EOF &&
+	[a.b]
+		Qc = d
+	[a]
+		Qx = y
+	EOF
 	git config a.x y &&
 	test_cmp expect .git/config
 '
 
-cat > expect << EOF
-[a.b]
-	c = d
-[a]
-	x = y
-	b = c
-[b]
-	x = y
-EOF
-
 test_expect_success 'new variable inserts into proper section' '
+	q_to_tab >expect <<-\EOF &&
+	[a.b]
+		Qc = d
+	[a]
+		Qx = y
+		Qb = c
+	[b]
+		Qx = y
+	EOF
 	git config b.x y &&
 	git config a.b c &&
 	test_cmp expect .git/config
-- 
2.24.0

[PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`

[Martin Ågren]

This is a self-contained and fairly large chunk of code which will soon
gain a few more lines. Prepare by extracting it into a separate
function.

This whole chunk is wrapped in "if (regex_)" -- rewrite it into an early
return path in the new helper function to reduce indentation.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 I copy the "xmalloc(sizeof(regex_t))" anti-pattern verbatim. We will
 lose it in the next patch.

 builtin/config.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 98d65bc0ad..e675ae0640 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -280,6 +280,28 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	return format_config(&values->items[values->nr++], key_, value_);
 }
 
+static int handle_value_regex(const char *regex_)
+{
+	if (!regex_) {
+		regexp = NULL;
+		return 0;
+	}
+
+	if (regex_[0] == '!') {
+		do_not_match = 1;
+		regex_++;
+	}
+
+	regexp = (regex_t*)xmalloc(sizeof(regex_t));
+	if (regcomp(regexp, regex_, REG_EXTENDED)) {
+		error(_("invalid pattern: %s"), regex_);
+		FREE_AND_NULL(regexp);
+		return CONFIG_INVALID_PATTERN;
+	}
+
+	return 0;
+}
+
 static int get_value(const char *key_, const char *regex_)
 {
 	int ret = CONFIG_GENERIC_ERROR;
@@ -317,20 +339,9 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	if (regex_) {
-		if (regex_[0] == '!') {
-			do_not_match = 1;
-			regex_++;
-		}
-
-		regexp = (regex_t*)xmalloc(sizeof(regex_t));
-		if (regcomp(regexp, regex_, REG_EXTENDED)) {
-			error(_("invalid pattern: %s"), regex_);
-			FREE_AND_NULL(regexp);
-			ret = CONFIG_INVALID_PATTERN;
-			goto free_strings;
-		}
-	}
+	ret = handle_value_regex(regex_);
+	if (ret)
+		goto free_strings;
 
 	config_with_options(collect_config, &values,
 			    &given_config_source, &config_options);
-- 
2.24.0

[PATCH 4/8] builtin/config: collect "value_regexp" data in a struct

[Martin Ågren]

`git config` can take an optional "value_regexp". Collect the
`regex_t`-pointer and the `do_not_match` flag into a new `struct
cmd_line_value`.

Rather than signalling/judging presence of a regexp by the NULL-ness of
the pointer, introduce a `mode` enum. After this commit, we just have
two modes, "none" and "regexp", but we will gain another one in the next
commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/config.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index e675ae0640..d812e73e2b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -14,12 +14,15 @@ static const char *const builtin_config_usage[] = {
 
 static char *key;
 static regex_t *key_regexp;
-static regex_t *regexp;
+static struct {
+	enum { none, regexp } mode;
+	regex_t *regexp;
+	int do_not_match; /* used with `regexp` */
+} cmd_line_value;
 static int show_keys;
 static int omit_values;
 static int use_key_regexp;
 static int do_all;
-static int do_not_match;
 static char delim = '=';
 static char key_delim = ' ';
 static char term = '\n';
@@ -270,8 +273,10 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 		return 0;
 	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
 		return 0;
-	if (regexp != NULL &&
-	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
+	if (cmd_line_value.mode == regexp &&
+	    (cmd_line_value.do_not_match ^
+	     !!regexec(cmd_line_value.regexp, value_ ? value_ : "",
+		       0, NULL, 0)))
 		return 0;
 
 	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
@@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 static int handle_value_regex(const char *regex_)
 {
 	if (!regex_) {
-		regexp = NULL;
+		cmd_line_value.mode = none;
 		return 0;
 	}
 
+	cmd_line_value.mode = regexp;
+
 	if (regex_[0] == '!') {
-		do_not_match = 1;
+		cmd_line_value.do_not_match = 1;
 		regex_++;
 	}
 
-	regexp = (regex_t*)xmalloc(sizeof(regex_t));
-	if (regcomp(regexp, regex_, REG_EXTENDED)) {
+	cmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));
+	if (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {
 		error(_("invalid pattern: %s"), regex_);
-		FREE_AND_NULL(regexp);
+		FREE_AND_NULL(cmd_line_value.regexp);
 		return CONFIG_INVALID_PATTERN;
 	}
 
@@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)
 		regfree(key_regexp);
 		free(key_regexp);
 	}
-	if (regexp) {
-		regfree(regexp);
-		free(regexp);
+	if (cmd_line_value.regexp) {
+		regfree(cmd_line_value.regexp);
+		free(cmd_line_value.regexp);
 	}
 
 	return ret;
-- 
2.24.0

[PATCH 5/8] builtin/config: canonicalize "value_regex" with `--type=bool`

[Martin Ågren]

With `--type=bool`, we use the "value_regex" as a normal regex and do
not use our knowledge about the different well-defined ways we have of
spelling the boolean values. Let's consider a few examples.

These output "true":
	git -c foo.bar=true config --type=bool --get foo.bar true
	git -c foo.bar=true config --type=bool --get foo.bar t

These produce no output:
	git -c foo.bar=true config --type=bool --get foo.bar True
	git -c foo.bar=true config --type=bool --get foo.bar 1

Canonicalize the provided "value_regex", then canonicalize candidate
values as we go through the config and compare the canonicalized values.
If the parameter doesn't canonicalize, fall back to handling it as a
regex, as before. This would happen in the second example above, but
also if someone has hand-rolled their own canonicalisation with, e.g.,
something like "^(on|On|ON|true|1|42)$".

This commit affects all modes that take a "value_regex", e.g.,
`--get-regexp` which can be used in a more useful way than the examples
above might at first suggest:

	git config --type=bool --name-only --get-regexp '^foo\.' true

This commit does change the behavior for certain usages, but it almost
certainly does so for the better: We don't exclude config items based on
how the config happens to spell "true" or "false". This commit would
cause a regression if someone uses different synonyms for "true",
knowing that Git handles them all the same in day-to-day operation, but
relying on the encoding (using, say, integers 1-100) to signal some sort
of confidence and using `git config` to query for various such "levels".
I'm tempted to bet no-one will be hurt by this change.

Do not rename the "value_regex" in the documentation. This commit can be
seen as teaching `git config --type=bool` about a particular type of
regex, where "true" matches "yes", but not "no". So arguably,
"value_regex" still describes quite well what is going on.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt |  4 ++++
 builtin/config.c             | 15 +++++++++++-
 t/t1300-config.sh            | 45 ++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 899e92a1c9..139750bbda 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -43,6 +43,10 @@ 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`.
 
+With `--type=bool`, if `value_regex` is given
+and canonicalizes to a boolean value, it matches all entries
+that canonicalize to the same boolean value.
+
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
 `--system`, `--global`, `--local`, `--worktree` and
diff --git a/builtin/config.c b/builtin/config.c
index d812e73e2b..c9fe0c5752 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -15,9 +15,10 @@ static const char *const builtin_config_usage[] = {
 static char *key;
 static regex_t *key_regexp;
 static struct {
-	enum { none, regexp } mode;
+	enum { none, regexp, boolean } mode;
 	regex_t *regexp;
 	int do_not_match; /* used with `regexp` */
+	int boolean;
 } cmd_line_value;
 static int show_keys;
 static int omit_values;
@@ -278,6 +279,9 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	     !!regexec(cmd_line_value.regexp, value_ ? value_ : "",
 		       0, NULL, 0)))
 		return 0;
+	if (cmd_line_value.mode == boolean &&
+	    git_parse_maybe_bool(value_) != cmd_line_value.boolean)
+		return 0;
 
 	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
 	strbuf_init(&values->items[values->nr], 0);
@@ -292,6 +296,15 @@ static int handle_value_regex(const char *regex_)
 		return 0;
 	}
 
+	if (type == TYPE_BOOL) {
+		int boolval = git_parse_maybe_bool(regex_);
+		if (boolval >= 0) {
+			cmd_line_value.mode = boolean;
+			cmd_line_value.boolean = boolval;
+			return 0;
+		}
+	}
+
 	cmd_line_value.mode = regexp;
 
 	if (regex_[0] == '!') {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index a38cc143a1..e4906a893e 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -427,6 +427,51 @@ test_expect_success 'no arguments, but no crash' '
 	test_i18ngrep usage output
 '
 
+test_expect_success 'setup config file with several boolean values' '
+	cat >.git/config <<-\EOF
+	[foo]
+		n1 = no
+		n2 = NO
+		n3 = off
+		n4 = false
+		n5 = 0
+		n6 =
+		y1 = yes
+		y2 = YES
+		y3 = on
+		y4 = true
+		y5 = 1
+		y6 = 42
+		y7
+	EOF
+'
+
+test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (false)' '
+	git config --type=bool --get-regexp "foo\..*" OFF >output &&
+	test_line_count = 6 output &&
+	! grep -v "^foo.n" output
+'
+
+test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (true)' '
+	git config --type=bool --get-regexp "foo\..*" ON >output &&
+	test_line_count = 7 output &&
+	! grep -v "^foo.y" output
+'
+
+test_expect_success '--get canonicalizes integer value_regex with --type=bool' '
+	echo true >expect &&
+	git config --type=bool --get foo.y2 1 >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--type=bool with "non-bool" value_regex' '
+	echo true >expect &&
+	git config --type=bool --get foo.y4 "t.*" >output &&
+	test_cmp expect output &&
+	test_must_fail git config --type=bool --get foo.y4 "T.*" >output &&
+	test_must_be_empty output
+'
+
 test_expect_success 'setup simple config file' '
 	q_to_tab >.git/config <<-\EOF
 	[a.b]
-- 
2.24.0

[PATCH 6/8] builtin/config: canonicalize "value_regex" with `--type=bool-or-int`

[Martin Ågren]

As an obvious follow-up to the previous commit, also canonicalize the
"value_regex" when the type is "bool-or-int".

Observe that in this case, falling back to handling the "value_regex" as
a normal regex is not just to cater to old scripts and habits. It is
necessary to handle the numerical inputs (or regexes matching some
specific numerical values!) that we must expect.

Future commits will expand on the code for `--type=bool`. Rather than
trying to shoehorn both these cases into a single chunk of code, let's
just duplicate some of the code from the previous commit.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt |  2 +-
 builtin/config.c             |  9 +++++++++
 t/t1300-config.sh            | 20 ++++++++++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 139750bbda..864375b1ec 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -43,7 +43,7 @@ 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`.
 
-With `--type=bool`, if `value_regex` is given
+With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given
 and canonicalizes to a boolean value, it matches all entries
 that canonicalize to the same boolean value.
 
diff --git a/builtin/config.c b/builtin/config.c
index c9fe0c5752..4e274d4867 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -305,6 +305,15 @@ static int handle_value_regex(const char *regex_)
 		}
 	}
 
+	if (type == TYPE_BOOL_OR_INT) {
+		int boolval = git_parse_maybe_bool_text(regex_);
+		if (boolval >= 0) {
+			cmd_line_value.mode = boolean;
+			cmd_line_value.boolean = boolval;
+			return 0;
+		}
+	}
+
 	cmd_line_value.mode = regexp;
 
 	if (regex_[0] == '!') {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index e4906a893e..f0e9a21dc4 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -472,6 +472,26 @@ test_expect_success '--type=bool with "non-bool" value_regex' '
 	test_must_be_empty output
 '
 
+test_expect_success '--type=bool-or-int with boolean value_regex' '
+	echo true >expect &&
+	git config --type=bool-or-int --get foo.y2 true >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--type=bool-or-int with integer value_regex' '
+	test_must_fail git config --type=bool-or-int --get foo.y2 1 >output &&
+	test_must_be_empty output &&
+	echo 1 >expect &&
+	git config --type=bool-or-int --get foo.y5 1 >output &&
+	test_cmp expect output
+'
+
+test_expect_success '--type=bool-or-int with regex value_regex' '
+	echo true >expect &&
+	git config --type=bool-or-int --get foo.y4 "t.*" >output &&
+	test_cmp expect output
+'
+
 test_expect_success 'setup simple config file' '
 	q_to_tab >.git/config <<-\EOF
 	[a.b]
-- 
2.24.0

[PATCH 7/8] builtin/config: warn if "value_regex" doesn't canonicalize as boolean

[Martin Ågren]

With `--type=bool`, we try to canonicalize the "value_regex". If it
doesn't canonicalize, we continue and handle the "value_regex" as an
ordinary regex. This is deliberate -- we do not want to cause existing
scripts to fail.

This does mean that users might be at risk of missing out on config
values depending on which representation they use in their config file:

	$ git config foo.bar on
	$ git config foo.baz true
	$ git config --type=bool --get-regex "foo\.*" tru
	foo.baz true
	$ # oops! missing foo.bar

Let's start warning when the "value_regex" doesn't look like a boolean.
Document our intention of eventually requiring the canonicalization to
pass.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt | 2 ++
 builtin/config.c             | 2 ++
 t/t1300-config.sh            | 3 ++-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 864375b1ec..43310ca3c0 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -46,6 +46,8 @@ unset an existing `--type` specifier with `--no-type`.
 With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given
 and canonicalizes to a boolean value, it matches all entries
 that canonicalize to the same boolean value.
+The support for non-canonicalizing values of `value_regex` with
+`--type=bool` is deprecated.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
diff --git a/builtin/config.c b/builtin/config.c
index 4e274d4867..2af62b95f8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -303,6 +303,8 @@ static int handle_value_regex(const char *regex_)
 			cmd_line_value.boolean = boolval;
 			return 0;
 		}
+		warning(_("value_regex '%s' cannot be canonicalized "
+			  "to a boolean value"), regex_);
 	}
 
 	if (type == TYPE_BOOL_OR_INT) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f0e9a21dc4..3e067c211d 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -466,8 +466,9 @@ test_expect_success '--get canonicalizes integer value_regex with --type=bool' '
 
 test_expect_success '--type=bool with "non-bool" value_regex' '
 	echo true >expect &&
-	git config --type=bool --get foo.y4 "t.*" >output &&
+	git config --type=bool --get foo.y4 "t.*" >output 2>err &&
 	test_cmp expect output &&
+	test_i18ngrep "cannot be canonicalized" err &&
 	test_must_fail git config --type=bool --get foo.y4 "T.*" >output &&
 	test_must_be_empty output
 '
-- 
2.24.0

[PATCH 8/8] builtin/config: die if "value_regex" doesn't canonicalize as boolean

[Martin Ågren]

This completes the transition from handling a "value_regexp" with
`--type=bool` as a regex, to handling it on the assumption that it
canonicalizes to a boolean value.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/git-config.txt | 5 ++---
 builtin/config.c             | 4 ++--
 t/t1300-config.sh            | 5 +----
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 43310ca3c0..598915eac6 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -43,11 +43,10 @@ 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`.
 
-With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given
+With `--type=bool-or-int`, if `value_regex` is given
 and canonicalizes to a boolean value, it matches all entries
 that canonicalize to the same boolean value.
-The support for non-canonicalizing values of `value_regex` with
-`--type=bool` is deprecated.
+With `--type=bool`, `value_regex` (if given) must canonicalize.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
diff --git a/builtin/config.c b/builtin/config.c
index 2af62b95f8..837766cfb3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -303,8 +303,8 @@ static int handle_value_regex(const char *regex_)
 			cmd_line_value.boolean = boolval;
 			return 0;
 		}
-		warning(_("value_regex '%s' cannot be canonicalized "
-			  "to a boolean value"), regex_);
+		die(_("value_regex '%s' cannot be canonicalized "
+		      "to a boolean value"), regex_);
 	}
 
 	if (type == TYPE_BOOL_OR_INT) {
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 3e067c211d..9eccc255db 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -465,11 +465,8 @@ test_expect_success '--get canonicalizes integer value_regex with --type=bool' '
 '
 
 test_expect_success '--type=bool with "non-bool" value_regex' '
-	echo true >expect &&
-	git config --type=bool --get foo.y4 "t.*" >output 2>err &&
-	test_cmp expect output &&
+	test_must_fail git config --type=bool --get foo.y4 t >output 2>err &&
 	test_i18ngrep "cannot be canonicalized" err &&
-	test_must_fail git config --type=bool --get foo.y4 "T.*" >output &&
 	test_must_be_empty output
 '
 
-- 
2.24.0

Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>   git config --type=bool --name-only --get-regexp '^foo\.' true
> ...
> This patch series teaches `git config` to canonicalize the incoming
> "value_regex" ("true" in the example above), then canonicalize candidate
> values as we go through the config. Or if you will, `git config` learns
> a brand new type of regex, corresponding to the different ways there are
> of spelling "true" and "false", respectively.

Nice ;-)

> `--type=bool-or-int` gets the same treatment, except we need to to be
> able to handle the ints and regexes matching particular ints that we
> must expect.

Hmm, so I can say 1024k or 1m and that would match 1048576?  

Doubly nice.

Looking forward to reading it thru.

Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Jeff King]

On Wed, Nov 13, 2019 at 07:54:59PM +0100, Martin Ågren wrote:

> To find all config items "foo.*" that are configured as the Boolean
> value "true", you can try executing
> 
>   git config --type=bool --name-only --get-regexp '^foo\.' true
> 
> ... and hope that you didn't spell "true" as "on", "True" or "1" back
> when you populated your config. This shortcoming has been mentioned as a
> left-over bit [1] [2].

This seems like a good idea, but I wonder why we'd limit it to bools?
It seems like any type would probably be better off matching a
normalized version.

We already have two functions in builtin/config.c which handle types:

  - format_config() is for actually interpreting an existing value and
    writing it to stdout

  - normalize_value() is used when writing a new value into a config
    file, and normalizing what the user provided on the command-line

I don't think you'd want to use format_config() here. For example, if I
say:

  git config --type=color --get-regexp ^foo red

I want to match the original "red" color, but _output_ the ANSI code.
But normalize_value(), by contrast, leaves colors intact. So maybe it's
a good match?

OTOH, I'd probably expect to match expanded pathnames or expiration
dates there, too, and it doesn't expand those. Those ones are less clear
to me. The whole premise of this series is making an assumption that
"--type" is about how you want to match, and not just about what you
want to output. In your example above it's clear that you don't care
about the output type (because you're using --name-only), but for:

  git config --type=path --get-regexp ^foo /home/peff

it's unclear if you want to match a value of "~peff/foo", or if you
simply want to output the expansion.

I wonder if we'd want to allow specifying the output type and the
matching type separately? Maybe that just makes it more awkward to use
for little benefit (I admit that it's hard to imagine somebody wanting
to normalize bools on output but _not_ for matching).

Anyway. It would be nice if we could build on the existing logic in some
way, rather than making a third place that has to handle every type we
know about.

> `--type=bool-or-int` gets the same treatment, except we need to to be
> able to handle the ints and regexes matching particular ints that we
> must expect. That said, even with `--type=bool` we can't move too
> aggressively towards *requiring* that the incoming "value_regex"
> canonializes as a Boolean value. The penultimate patch starts to warn on
> non-canonicalizing values; the final patch makes us bail out entirely.
> 
> The last patch is not meant for immediate inclusion, but I post it
> anyway. I can re-submit it at an appropriate time, or maybe it could
> slumber on pu until the time is ripe for completing the switch.

I think bailing on values that can't be converted is normal for other
code paths. E.g., just trying to print:

  $ git -c foo.bar=abc config --type=bool foo.abr
  fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit

-Peff

Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Martin Ågren]

Hi Junio

On Thu, 14 Nov 2019 at 03:19, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > `--type=bool-or-int` gets the same treatment, except we need to to be
> > able to handle the ints and regexes matching particular ints that we
> > must expect.
>
> Hmm, so I can say 1024k or 1m and that would match 1048576?
>
> Doubly nice.
>
> Looking forward to reading it thru.

Maybe you already noticed, but no, I didn't get to canonicalizing
integers like that. What I meant was, type=bool-or-int learns to handle
booleans similar to what I did to type=bool.

I don't feel entirely satisfied by some of my commit message oneliners.
They could make that a bit clearer, I think.

Not directly related to your question, but I realize now that with
type=bool-or-int, I only added the first of these example usages below
as a test. The second one is perhaps just as interesting.

$ ./git -c an.int=1 config --get --type=bool-or-int an.int 1
1
$ ./git -c an.int=1 config --get --type=bool-or-int an.int on
1

This last one emits "1". That's because by the time we've decided to
output the value, `format_config()` has some logic around
type=bool-or-int, but doesn't know about why exactly we selected this
an.int=1 in the first place
  (git_parse_maybe_bool("1") == git_parse_maybe_bool_TEXT("on")).

Just after thinking about this for a short while, I can't immediately
say whether this second one should emit "1" or "true". My added
documentation is actually vague enough to allow both of these to
happen... I'll ponder this.


Martin

Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Martin Ågren]

On Thu, 14 Nov 2019 at 07:29, Jeff King <peff@peff.net> wrote:
> This seems like a good idea, but I wonder why we'd limit it to bools?

Basically because that's what the existing left-over-bits mentioned and
it felt like a good place to start. But you're right in asking about the
bigger picture up front.

> It seems like any type would probably be better off matching a
> normalized version.
>
> We already have two functions in builtin/config.c which handle types:
>
>   - format_config() is for actually interpreting an existing value and
>     writing it to stdout
>
>   - normalize_value() is used when writing a new value into a config
>     file, and normalizing what the user provided on the command-line
>
> I don't think you'd want to use format_config() here.

I just speculated a little around format_config() in a reply to Junio.
Already with my patch series and with type=bool-or-int, there's a
visible funny-funky corner case where one hand doesn't know what the
other is doing.

> For example, if I
> say:
>
>   git config --type=color --get-regexp ^foo red
>
> I want to match the original "red" color, but _output_ the ANSI code.
> But normalize_value(), by contrast, leaves colors intact. So maybe it's
> a good match?
>
> OTOH, I'd probably expect to match expanded pathnames or expiration
> dates there, too, and it doesn't expand those. Those ones are less clear
> to me. The whole premise of this series is making an assumption that
> "--type" is about how you want to match,

Right.

> and not just about what you
> want to output. In your example above it's clear that you don't care
> about the output type (because you're using --name-only), but for:
>
>   git config --type=path --get-regexp ^foo /home/peff
>
> it's unclear if you want to match a value of "~peff/foo", or if you
> simply want to output the expansion.

Hmm, I feel like we're opening a can of worms here.

> I wonder if we'd want to allow specifying the output type and the
> matching type separately? Maybe that just makes it more awkward to use
> for little benefit (I admit that it's hard to imagine somebody wanting
> to normalize bools on output but _not_ for matching).
>
> Anyway. It would be nice if we could build on the existing logic in some
> way, rather than making a third place that has to handle every type we
> know about.

Understood. Thanks a lot for sharing your thoughts.

> > The last patch is not meant for immediate inclusion, but I post it
> > anyway. I can re-submit it at an appropriate time, or maybe it could
> > slumber on pu until the time is ripe for completing the switch.
>
> I think bailing on values that can't be converted is normal for other
> code paths. E.g., just trying to print:
>
>   $ git -c foo.bar=abc config --type=bool foo.abr
>   fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit

I'm not sure if you mean "... so we could be a lot more aggressive
here"?

I'm running now and I feel like I'll need to read your mail again
tonight and get back to you in more detail.

Thanks
Martin

Re: [PATCH 0/8] builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]`

[Jeff King]

On Thu, Nov 14, 2019 at 07:54:35AM +0100, Martin Ågren wrote:

> > > The last patch is not meant for immediate inclusion, but I post it
> > > anyway. I can re-submit it at an appropriate time, or maybe it could
> > > slumber on pu until the time is ripe for completing the switch.
> >
> > I think bailing on values that can't be converted is normal for other
> > code paths. E.g., just trying to print:
> >
> >   $ git -c foo.bar=abc config --type=bool foo.abr
> >   fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit
> 
> I'm not sure if you mean "... so we could be a lot more aggressive
> here"?

Yeah, I think it's OK to be aggressive with bailing when the user gave
us a --type, but the value doesn't match it.

> I'm running now and I feel like I'll need to read your mail again
> tonight and get back to you in more detail.

Sure, no rush and thanks for working on it. :)

-Peff

Re: [PATCH 2/8] t1300: modernize part of script

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> Create `.git/config` and `expect` files inside `test_expect_success`,
> either inside the one existing test that uses the file, or in a new
> "setup" step before several tests that use it.
>
> Redirect using `>output` rather than `> output`. Use `<<-\EOF" with
> heredocs rather than just `<< EOF` and use `q_to_tab` to create properly
> indented input and output files.
>
> This commit does not modernize the whole script, but just some of it,
> around the point where a later commit will add new content.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  t/t1300-config.sh | 138 ++++++++++++++++++++++------------------------
>  1 file changed, 65 insertions(+), 73 deletions(-)

Thanks.  All conversions looked OK.

Re: [PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> This is a self-contained and fairly large chunk of code which will soon
> gain a few more lines. Prepare by extracting it into a separate
> function.
>
> This whole chunk is wrapped in "if (regex_)" -- rewrite it into an early
> return path in the new helper function to reduce indentation.

It is not clear if regexp were cleared to NULL when !regex_ in the
original code, so if that were the case, this refactoring is a
worthy clean-up from that point of view, too.

>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  I copy the "xmalloc(sizeof(regex_t))" anti-pattern verbatim. We will
>  lose it in the next patch.

It also spreads the use of file-scope global variables like
do_not_match and regexp, which also is existing anti-pattern that we
may want to fix by enclosing them in a struct and pass a pointer to
it around the callchain.  We can clean it up later.

Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> `git config` can take an optional "value_regexp". Collect the
> `regex_t`-pointer and the `do_not_match` flag into a new `struct
> cmd_line_value`.

A "struct cmd_line_value" sounded, to me at least during my first
reading, as if it is about all command line options, but that is not
at all what you meant to imply.  Is this only about the optional
value-regexp (if so perhaps calling it "value_regexp_option" would
have helped me avoid such a misunderstanding)?

> Rather than signalling/judging presence of a regexp by the NULL-ness of
> the pointer, introduce a `mode` enum.

OK.  Tangentially this makes readers wonder why the existing code
for key_regexp does not follow the same "NULL-ness" pattern but has
a separate use_key_regexp boolean.  It appears that the original
code is quite confused---it is totally outside the scope of this
series to clean it up and inject sanity into it though ;-)

>  static regex_t *key_regexp;
> -static regex_t *regexp;
> +static struct {
> +	enum { none, regexp } mode;

We often use the same identifier for a struct and an instance of the
struct, taking advantage of the fact that they live in separate
namespaces, but lowercase enumerated values like 'regexp' that
collides with the field name (and possibly a variable name used
elsewhere) smells a bit too much.

> +	regex_t *regexp;
> +	int do_not_match; /* used with `regexp` */
> +} cmd_line_value;
>  static int show_keys;
>  static int omit_values;
>  static int use_key_regexp;

> @@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)
>  static int handle_value_regex(const char *regex_)
>  {
>  	if (!regex_) {
> -		regexp = NULL;
> +		cmd_line_value.mode = none;
>  		return 0;

Now we are back to relying on cmd_line_value.regexp staying to be
NULL after initialized, which is the state before the previous
patch.  If the end result is correct, then it is OK, I think, but
then the previous step shouldn't have added the NULL assignment here
in the first place.

>  	}
>  
> +	cmd_line_value.mode = regexp;
> +
>  	if (regex_[0] == '!') {
> -		do_not_match = 1;
> +		cmd_line_value.do_not_match = 1;
>  		regex_++;
>  	}
>  
> -	regexp = (regex_t*)xmalloc(sizeof(regex_t));
> -	if (regcomp(regexp, regex_, REG_EXTENDED)) {
> +	cmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));
> +	if (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {
>  		error(_("invalid pattern: %s"), regex_);
> -		FREE_AND_NULL(regexp);
> +		FREE_AND_NULL(cmd_line_value.regexp);

Hmph.  !regexp in old code should mean cmd_line_value.mode==regexp
in the new world order after this patch is applied, no?  Should we
be treaking the mode field here before we leave?  I think it should
not matter, but thought it wouldn't hurt to ask.

In collect_config(), cmd_line_value.regexp is blindly passed to
regexec(3) as long as cmd_line_value.mode==regexp, so the invariant
"when .mode is regexp, .regexp must be valid, or collect_config() would
never be called for such cmd_line_value" is rather important to
avoid crashing ;-)

>  		return CONFIG_INVALID_PATTERN;
>  	}
>  
> @@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)
>  		regfree(key_regexp);
>  		free(key_regexp);
>  	}
> -	if (regexp) {
> -		regfree(regexp);
> -		free(regexp);
> +	if (cmd_line_value.regexp) {
> +		regfree(cmd_line_value.regexp);
> +		free(cmd_line_value.regexp);

Likewise.

>  	}
>  
>  	return ret;

Re: [PATCH 5/8] builtin/config: canonicalize "value_regex" with `--type=bool`

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> With `--type=bool`, we use the "value_regex" as a normal regex and do
> not use our knowledge about the different well-defined ways we have of
> spelling the boolean values. Let's consider a few examples.
>
> These output "true":
> 	git -c foo.bar=true config --type=bool --get foo.bar true
> 	git -c foo.bar=true config --type=bool --get foo.bar t

I wonder what

    git -c foo.bar=true config --type=bool --get foo.bar 't.*'

should say.  I think you and Peff discussed in the downthread to
make it more strict to require <value_regex> *NOT* regexp at all,
but one of the string representations of boolean we would take,
which means 't' (in your exmaple) and 't.*' would stop yielding
correct result, which I think is fine.

> Canonicalize the provided "value_regex", then canonicalize candidate
> values as we go through the config and compare the canonicalized values.

I think (for the reason stated earlier) this is OK, but we should
make sure that we are clear in the documentation that in this mode
<value_regex> is no longer a regex but a string.  We might even want
to have a separate SYNOPSIS entry that does not say regex at all,
something along the lines of ...

-'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] --type=<type> [--show-origin] [-z|--null] --get name [value]

> If the parameter doesn't canonicalize, fall back to handling it as a
> regex, as before. This would happen in the second example above, but
> also if someone has hand-rolled their own canonicalisation with, e.g.,
> something like "^(on|On|ON|true|1|42)$".

I actually am OK with this fallback, too.  That would also mean the
additional SYNOPSIS entry unnecessary ;-).

> Do not rename the "value_regex" in the documentation. This commit can be
> seen as teaching `git config --type=bool` about a particular type of
> regex, where "true" matches "yes", but not "no". So arguably,
> "value_regex" still describes quite well what is going on.

Heh ;-) I probably should learn to blindly read thru the proposed
log message to the end before starting to think about ramifications
of the proposed changes myself.  Some of the reasoning in this
paragraph should be reflected to the explanation of the argument in
the documentation, so that the readers will know this is not a usual
regex at all.

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Documentation/git-config.txt |  4 ++++
>  builtin/config.c             | 15 +++++++++++-
>  t/t1300-config.sh            | 45 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 899e92a1c9..139750bbda 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -43,6 +43,10 @@ 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`.
>  
> +With `--type=bool`, if `value_regex` is given
> +and canonicalizes to a boolean value, it matches all entries
> +that canonicalize to the same boolean value.

... otherwise `value_regex` is used as a string, i.e. as if no
'--type=bool' is given.

> +	if (cmd_line_value.mode == boolean &&
> +	    git_parse_maybe_bool(value_) != cmd_line_value.boolean)
> +		return 0;
>  
>  	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
>  	strbuf_init(&values->items[values->nr], 0);
> @@ -292,6 +296,15 @@ static int handle_value_regex(const char *regex_)
>  		return 0;
>  	}
>  
> +	if (type == TYPE_BOOL) {
> +		int boolval = git_parse_maybe_bool(regex_);
> +		if (boolval >= 0) {
> +			cmd_line_value.mode = boolean;
> +			cmd_line_value.boolean = boolval;
> +			return 0;
> +		}
> +	}
> +
>  	cmd_line_value.mode = regexp;

... which can be seen here.  OK.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index a38cc143a1..e4906a893e 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -427,6 +427,51 @@ test_expect_success 'no arguments, but no crash' '
>  	test_i18ngrep usage output
>  '
>  
> +test_expect_success 'setup config file with several boolean values' '
> +	cat >.git/config <<-\EOF
> +	[foo]
> +		n1 = no
> +		n2 = NO
> +		n3 = off
> +		n4 = false
> +		n5 = 0
> +		n6 =
> +		y1 = yes
> +		y2 = YES
> +		y3 = on
> +		y4 = true
> +		y5 = 1
> +		y6 = 42
> +		y7
> +	EOF
> +'
> +
> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (false)' '
> +	git config --type=bool --get-regexp "foo\..*" OFF >output &&
> +	test_line_count = 6 output &&
> +	! grep -v "^foo.n" output
> +'
> +
> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (true)' '
> +	git config --type=bool --get-regexp "foo\..*" ON >output &&
> +	test_line_count = 7 output &&
> +	! grep -v "^foo.y" output
> +'
> +
> +test_expect_success '--get canonicalizes integer value_regex with --type=bool' '
> +	echo true >expect &&
> +	git config --type=bool --get foo.y2 1 >output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success '--type=bool with "non-bool" value_regex' '
> +	echo true >expect &&
> +	git config --type=bool --get foo.y4 "t.*" >output &&
> +	test_cmp expect output &&
> +	test_must_fail git config --type=bool --get foo.y4 "T.*" >output &&
> +	test_must_be_empty output
> +'
> +
>  test_expect_success 'setup simple config file' '
>  	q_to_tab >.git/config <<-\EOF
>  	[a.b]

Re: [PATCH 7/8] builtin/config: warn if "value_regex" doesn't canonicalize as boolean

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> With `--type=bool`, we try to canonicalize the "value_regex". If it
> doesn't canonicalize, we continue and handle the "value_regex" as an
> ordinary regex. This is deliberate -- we do not want to cause existing
> scripts to fail.
>
> This does mean that users might be at risk of missing out on config
> values depending on which representation they use in their config file:
>
> 	$ git config foo.bar on
> 	$ git config foo.baz true
> 	$ git config --type=bool --get-regex "foo\.*" tru
> 	foo.baz true
> 	$ # oops! missing foo.bar
>
> Let's start warning when the "value_regex" doesn't look like a boolean.
> Document our intention of eventually requiring the canonicalization to
> pass.

While I actually think this warning is counter-productive, if we
were to do so, value-regex given for a bool-or-int type should also
be warned if it does not name a boolean value and is not an integer.

With the way you laid out the "use enum to tell what kind of token
value_regex argument has" pattern, I think support for "--type=int" 
to normalize human-readable numbers would also fall out naturally,
which is nice.

Re: [PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`

[Martin Ågren]

On Thu, 21 Nov 2019 at 06:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > This is a self-contained and fairly large chunk of code which will soon
> > gain a few more lines. Prepare by extracting it into a separate
> > function.
> >
> > This whole chunk is wrapped in "if (regex_)" -- rewrite it into an early
> > return path in the new helper function to reduce indentation.
>
> It is not clear if regexp were cleared to NULL when !regex_ in the
> original code, so if that were the case, this refactoring is a
> worthy clean-up from that point of view, too.

Hmmm, this is something I added which makes it not a true refactoring
as such. I don't even recall doing this, but it does make sure we always
set this pointer to something sane. If we've already initialized this,
we'll risk leaking, but that should be better than running amok due to
bailing out early here and leaving the pointer pointing "somewhere".

That said, this is the only function where we set this, and we're
calling this function at most once (directly from `cmd_config()`),
so right now this NULL assignment is a no-op.

> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> > ---
> >  I copy the "xmalloc(sizeof(regex_t))" anti-pattern verbatim. We will
> >  lose it in the next patch.
>
> It also spreads the use of file-scope global variables like
> do_not_match and regexp, which also is existing anti-pattern that we
> may want to fix by enclosing them in a struct and pass a pointer to
> it around the callchain.  We can clean it up later.

Right, in the next patch I collect them into a struct, but leave it
file-scope global. I didn't feel good adding another such global later
in the series, so decided to take a smallish step towards the end goal
you outline...


Martin

Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct

[Martin Ågren]

On Thu, 21 Nov 2019 at 06:22, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > `git config` can take an optional "value_regexp". Collect the
> > `regex_t`-pointer and the `do_not_match` flag into a new `struct
> > cmd_line_value`.
>
> A "struct cmd_line_value" sounded, to me at least during my first
> reading, as if it is about all command line options, but that is not
> at all what you meant to imply.  Is this only about the optional
> value-regexp (if so perhaps calling it "value_regexp_option" would
> have helped me avoid such a misunderstanding)?

Yes, that's right. Your suggested name is better. Thanks.

> > Rather than signalling/judging presence of a regexp by the NULL-ness of
> > the pointer, introduce a `mode` enum.
>
> OK.  Tangentially this makes readers wonder why the existing code
> for key_regexp does not follow the same "NULL-ness" pattern but has
> a separate use_key_regexp boolean.  It appears that the original
> code is quite confused---it is totally outside the scope of this
> series to clean it up and inject sanity into it though ;-)

Yeah, I considered doing such a cleanup, but opted to try and stay
focused.

> >  static regex_t *key_regexp;
> > -static regex_t *regexp;
> > +static struct {
> > +     enum { none, regexp } mode;
>
> We often use the same identifier for a struct and an instance of the
> struct, taking advantage of the fact that they live in separate
> namespaces, but lowercase enumerated values like 'regexp' that
> collides with the field name (and possibly a variable name used
> elsewhere) smells a bit too much.

Ok, thanks for sanity-checking.

> > +     regex_t *regexp;
> > +     int do_not_match; /* used with `regexp` */
> > +} cmd_line_value;
> >  static int show_keys;
> >  static int omit_values;
> >  static int use_key_regexp;
>
> > @@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)
> >  static int handle_value_regex(const char *regex_)
> >  {
> >       if (!regex_) {
> > -             regexp = NULL;
> > +             cmd_line_value.mode = none;
> >               return 0;
>
> Now we are back to relying on cmd_line_value.regexp staying to be
> NULL after initialized, which is the state before the previous
> patch.  If the end result is correct, then it is OK, I think, but
> then the previous step shouldn't have added the NULL assignment here
> in the first place.

Ok, noted.

As I wrote in my reply there, that made the whole thing not a 100%
refactoring anyway. I'll drop that one.

> > +     cmd_line_value.mode = regexp;
> > +
> >       if (regex_[0] == '!') {
> > -             do_not_match = 1;
> > +             cmd_line_value.do_not_match = 1;
> >               regex_++;
> >       }
> >
> > -     regexp = (regex_t*)xmalloc(sizeof(regex_t));
> > -     if (regcomp(regexp, regex_, REG_EXTENDED)) {
> > +     cmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));
> > +     if (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {
> >               error(_("invalid pattern: %s"), regex_);
> > -             FREE_AND_NULL(regexp);
> > +             FREE_AND_NULL(cmd_line_value.regexp);
>
> Hmph.  !regexp in old code should mean cmd_line_value.mode==regexp
> in the new world order after this patch is applied, no?  Should we
> be treaking the mode field here before we leave?  I think it should
> not matter, but thought it wouldn't hurt to ask.
>
> In collect_config(), cmd_line_value.regexp is blindly passed to
> regexec(3) as long as cmd_line_value.mode==regexp, so the invariant
> "when .mode is regexp, .regexp must be valid, or collect_config() would
> never be called for such cmd_line_value" is rather important to
> avoid crashing ;-)

Good point. No-one will be looking at the struct when we bail out here,
but we're just one missing "if" away from that changing... Might as well
try to leave things in a sane state to reduce the possibility of this
biting us in the future.

> > @@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)
> >               regfree(key_regexp);
> >               free(key_regexp);
> >       }
> > -     if (regexp) {
> > -             regfree(regexp);
> > -             free(regexp);
> > +     if (cmd_line_value.regexp) {
> > +             regfree(cmd_line_value.regexp);
> > +             free(cmd_line_value.regexp);
>
> Likewise.

Thanks.


Martin

Re: [PATCH 7/8] builtin/config: warn if "value_regex" doesn't canonicalize as boolean

[Martin Ågren]

On Thu, 21 Nov 2019 at 06:43, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > With `--type=bool`, we try to canonicalize the "value_regex". If it
> > doesn't canonicalize, we continue and handle the "value_regex" as an
> > ordinary regex. This is deliberate -- we do not want to cause existing
> > scripts to fail.
> >
> > This does mean that users might be at risk of missing out on config
> > values depending on which representation they use in their config file:
> >
> >       $ git config foo.bar on
> >       $ git config foo.baz true
> >       $ git config --type=bool --get-regex "foo\.*" tru
> >       foo.baz true
> >       $ # oops! missing foo.bar
> >
> > Let's start warning when the "value_regex" doesn't look like a boolean.
> > Document our intention of eventually requiring the canonicalization to
> > pass.
>
> While I actually think this warning is counter-productive, if we
> were to do so, value-regex given for a bool-or-int type should also
> be warned if it does not name a boolean value and is not an integer.

Ok, noted.

> With the way you laid out the "use enum to tell what kind of token
> value_regex argument has" pattern, I think support for "--type=int"
> to normalize human-readable numbers would also fall out naturally,
> which is nice.

Well, that's what I was hoping for at least... ;-)


Martin

Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>> > +static struct {
>> > +     enum { none, regexp } mode;
>>
>> We often use the same identifier for a struct and an instance of the
>> struct, taking advantage of the fact that they live in separate
>> namespaces, but lowercase enumerated values like 'regexp' that
>> collides with the field name (and possibly a variable name used
>> elsewhere) smells a bit too much.
>
> Ok, thanks for sanity-checking.
>
>> > +     regex_t *regexp;
>> > +     int do_not_match; /* used with `regexp` */
>> > +} cmd_line_value;

I _might_ want to take this back.  A pattern that uses the "mode" to
switch among the possibilities in a union, i.e.

	struct {
		enum {
			<something>_none,
			<something>_regexp,
			<something>_bool,
			<something>_int,
		} mode;
                union {
			<type-used-when-it-is-regexp> regexp;
			<type-used-when-it-is-bool> bool;
			<type-used-when-it-is-int> int;
		} u;
	};

may not be too bad.  So I do not strongly mind the lowercase.

But I still do mind an overly bland names for identifiers in an
enum, as enum is not quite a type on its own ('regexp' in one enum
may collide with the same identifier in another enum)..

Thanks.