{"thread":{"id":"52260","subject":"[PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","startedAt":"2019-11-13T18:55:32Z","lastAt":"2019-11-22T06:30:50Z","messageCount":23,"participants":["Martin Ågren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"386129","messageId":"cover.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":null,"subject":"[PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:54:59Z","receivedAt":"2019-11-13T18:55:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"To find all config items \"foo.*\" that are configured as the Boolean\nvalue \"true\", you can try executing\n\n  git config --type=bool --name-only --get-regexp '^foo\\.' true\n\n... and hope that you didn't spell \"true\" as \"on\", \"True\" or \"1\" back\nwhen you populated your config. This shortcoming has been mentioned as a\nleft-over bit [1] [2].\n\nThis patch series teaches `git config` to canonicalize the incoming\n\"value_regex\" (\"true\" in the example above), then canonicalize candidate\nvalues as we go through the config. Or if you will, `git config` learns\na brand new type of regex, corresponding to the different ways there are\nof spelling \"true\" and \"false\", respectively.\n\n`--type=bool-or-int` gets the same treatment, except we need to to be\nable to handle the ints and regexes matching particular ints that we\nmust expect. That said, even with `--type=bool` we can't move too\naggressively towards *requiring* that the incoming \"value_regex\"\ncanonializes as a Boolean value. The penultimate patch starts to warn on\nnon-canonicalizing values; the final patch makes us bail out entirely.\n\nThe last patch is not meant for immediate inclusion, but I post it\nanyway. I can re-submit it at an appropriate time, or maybe it could\nslumber on pu until the time is ripe for completing the switch.\n\n[1] https://git-blame.blogspot.com/p/leftover-bits.html\n[2] https://public-inbox.org/git/xmqq7frsh4tw.fsf@gitster.dls.corp.google.com/\n\nMartin Ågren (8):\n  config: make `git_parse_maybe_bool_text()` public\n  t1300: modernize part of script\n  builtin/config: extract `handle_value_regex()` from `get_value()`\n  builtin/config: collect \"value_regexp\" data in a struct\n  builtin/config: canonicalize \"value_regex\" with `--type=bool`\n  builtin/config: canonicalize \"value_regex\" with `--type=bool-or-int`\n  builtin/config: warn if \"value_regex\" doesn't canonicalize as boolean\n  builtin/config: die if \"value_regex\" doesn't canonicalize as boolean\n\n Documentation/git-config.txt |   5 +\n builtin/config.c             |  84 +++++++++++----\n config.c                     |   2 +-\n config.h                     |   7 ++\n t/t1300-config.sh            | 199 ++++++++++++++++++++++-------------\n 5 files changed, 203 insertions(+), 94 deletions(-)\n\n-- \n2.24.0\n\n"},{"id":"386130","messageId":"ff9bddbbb31361c372ed7b5942fe8e83a68ac415.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 1/8] config: make `git_parse_maybe_bool_text()` public","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:00Z","receivedAt":"2019-11-13T18:55:38Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We will soon use it from outside config.c.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n config.c | 2 +-\n config.h | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex e7052b3977..318711f7a7 100644\n--- a/config.c\n+++ b/config.c\n@@ -1047,7 +1047,7 @@ ssize_t git_config_ssize_t(const char *name, const char *value)\n \treturn ret;\n }\n \n-static int git_parse_maybe_bool_text(const char *value)\n+int git_parse_maybe_bool_text(const char *value)\n {\n \tif (!value)\n \t\treturn 1;\ndiff --git a/config.h b/config.h\nindex f0ed464004..343f24c408 100644\n--- a/config.h\n+++ b/config.h\n@@ -95,6 +95,13 @@ int config_with_options(config_fn_t fn, void *,\n int git_parse_ssize_t(const char *, ssize_t *);\n int git_parse_ulong(const char *, unsigned long *);\n int git_parse_maybe_bool(const char *);\n+\n+/**\n+ * Same as `git_parse_maybe_bool`, except that it does not handle\n+ * integer values, i.e., those cause this function to return -1.\n+ */\n+int git_parse_maybe_bool_text(const char *);\n+\n int git_config_int(const char *, const char *);\n int64_t git_config_int64(const char *, const char *);\n unsigned long git_config_ulong(const char *, const char *);\n-- \n2.24.0\n\n"},{"id":"386131","messageId":"37e2b121d5946cfacb3fc4d2721770e496f7b752.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 2/8] t1300: modernize part of script","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:01Z","receivedAt":"2019-11-13T18:55:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Create `.git/config` and `expect` files inside `test_expect_success`,\neither inside the one existing test that uses the file, or in a new\n\"setup\" step before several tests that use it.\n\nRedirect using `>output` rather than `> output`. Use `<<-\\EOF\" with\nheredocs rather than just `<< EOF` and use `q_to_tab` to create properly\nindented input and output files.\n\nThis commit does not modernize the whole script, but just some of it,\naround the point where a later commit will add new content.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n t/t1300-config.sh | 138 ++++++++++++++++++++++------------------------\n 1 file changed, 65 insertions(+), 73 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 983a0a1583..a38cc143a1 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -321,15 +321,14 @@ test_expect_success 'hierarchical section value' '\n \ttest_cmp expect .git/config\n '\n \n-cat > expect << EOF\n-beta.noindent=sillyValue\n-nextsection.nonewline=wow2 for me\n-123456.a123=987\n-version.1.2.3eX.alpha=beta\n-EOF\n-\n test_expect_success 'working --list' '\n-\tgit config --list > output &&\n+\tcat >expect <<-\\EOF &&\n+\tbeta.noindent=sillyValue\n+\tnextsection.nonewline=wow2 for me\n+\t123456.a123=987\n+\tversion.1.2.3eX.alpha=beta\n+\tEOF\n+\tgit config --list >output &&\n \ttest_cmp expect output\n '\n test_expect_success '--list without repo produces empty output' '\n@@ -337,55 +336,53 @@ test_expect_success '--list without repo produces empty output' '\n \ttest_must_be_empty output\n '\n \n-cat > expect << EOF\n-beta.noindent\n-nextsection.nonewline\n-123456.a123\n-version.1.2.3eX.alpha\n-EOF\n-\n test_expect_success '--name-only --list' '\n+\tcat >expect <<-\\EOF &&\n+\tbeta.noindent\n+\tnextsection.nonewline\n+\t123456.a123\n+\tversion.1.2.3eX.alpha\n+\tEOF\n \tgit config --name-only --list >output &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n-beta.noindent sillyValue\n-nextsection.nonewline wow2 for me\n-EOF\n-\n test_expect_success '--get-regexp' '\n+\tcat >expect <<-\\EOF &&\n+\tbeta.noindent sillyValue\n+\tnextsection.nonewline wow2 for me\n+\tEOF\n \tgit config --get-regexp in >output &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n-beta.noindent\n-nextsection.nonewline\n-EOF\n-\n test_expect_success '--name-only --get-regexp' '\n+\tcat >expect <<-\\EOF &&\n+\tbeta.noindent\n+\tnextsection.nonewline\n+\tEOF\n \tgit config --name-only --get-regexp in >output &&\n \ttest_cmp expect output\n '\n \n-cat > expect << EOF\n-wow2 for me\n-wow4 for you\n-EOF\n-\n test_expect_success '--add' '\n+\tcat >expect <<-\\EOF &&\n+\twow2 for me\n+\twow4 for you\n+\tEOF\n \tgit config --add nextsection.nonewline \"wow4 for you\" &&\n-\tgit config --get-all nextsection.nonewline > output &&\n+\tgit config --get-all nextsection.nonewline >output &&\n \ttest_cmp expect output\n '\n \n-cat > .git/config << EOF\n-[novalue]\n-\tvariable\n-[emptyvalue]\n-\tvariable =\n-EOF\n+test_expect_success 'setup config file with no/empty values' '\n+\tq_to_tab >.git/config <<-\\EOF\n+\t[novalue]\n+\t\tQvariable\n+\t[emptyvalue]\n+\t\tQvariable =\n+\tEOF\n+'\n \n test_expect_success 'get variable with no value' '\n \tgit config --get novalue.variable ^$\n@@ -395,38 +392,33 @@ test_expect_success 'get variable with empty value' '\n \tgit config --get emptyvalue.variable ^$\n '\n \n-echo novalue.variable > expect\n-\n test_expect_success 'get-regexp variable with no value' '\n-\tgit config --get-regexp novalue > output &&\n+\techo novalue.variable >expect &&\n+\tgit config --get-regexp novalue >output &&\n \ttest_cmp expect output\n '\n \n-echo 'novalue.variable true' > expect\n-\n test_expect_success 'get-regexp --bool variable with no value' '\n-\tgit config --bool --get-regexp novalue > output &&\n+\techo \"novalue.variable true\" >expect &&\n+\tgit config --bool --get-regexp novalue >output &&\n \ttest_cmp expect output\n '\n \n-echo 'emptyvalue.variable ' > expect\n-\n test_expect_success 'get-regexp variable with empty value' '\n-\tgit config --get-regexp emptyvalue > output &&\n+\techo \"emptyvalue.variable \" >expect &&\n+\tgit config --get-regexp emptyvalue >output &&\n \ttest_cmp expect output\n '\n \n-echo true > expect\n-\n test_expect_success 'get bool variable with no value' '\n-\tgit config --bool novalue.variable > output &&\n+\techo true >expect &&\n+\tgit config --bool novalue.variable >output &&\n \ttest_cmp expect output\n '\n \n-echo false > expect\n-\n test_expect_success 'get bool variable with empty value' '\n-\tgit config --bool emptyvalue.variable > output &&\n+\techo false >expect &&\n+\tgit config --bool emptyvalue.variable >output &&\n \ttest_cmp expect output\n '\n \n@@ -435,34 +427,34 @@ test_expect_success 'no arguments, but no crash' '\n \ttest_i18ngrep usage output\n '\n \n-cat > .git/config << EOF\n-[a.b]\n-\tc = d\n-EOF\n-\n-cat > expect << EOF\n-[a.b]\n-\tc = d\n-[a]\n-\tx = y\n-EOF\n+test_expect_success 'setup simple config file' '\n+\tq_to_tab >.git/config <<-\\EOF\n+\t[a.b]\n+\t\tQc = d\n+\tEOF\n+'\n \n test_expect_success 'new section is partial match of another' '\n+\tq_to_tab >expect <<-\\EOF &&\n+\t[a.b]\n+\t\tQc = d\n+\t[a]\n+\t\tQx = y\n+\tEOF\n \tgit config a.x y &&\n \ttest_cmp expect .git/config\n '\n \n-cat > expect << EOF\n-[a.b]\n-\tc = d\n-[a]\n-\tx = y\n-\tb = c\n-[b]\n-\tx = y\n-EOF\n-\n test_expect_success 'new variable inserts into proper section' '\n+\tq_to_tab >expect <<-\\EOF &&\n+\t[a.b]\n+\t\tQc = d\n+\t[a]\n+\t\tQx = y\n+\t\tQb = c\n+\t[b]\n+\t\tQx = y\n+\tEOF\n \tgit config b.x y &&\n \tgit config a.b c &&\n \ttest_cmp expect .git/config\n-- \n2.24.0\n\n"},{"id":"386132","messageId":"c4bcb32299291549d82c0544937a647c5000ad64.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:02Z","receivedAt":"2019-11-13T18:55:43Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is a self-contained and fairly large chunk of code which will soon\ngain a few more lines. Prepare by extracting it into a separate\nfunction.\n\nThis whole chunk is wrapped in \"if (regex_)\" -- rewrite it into an early\nreturn path in the new helper function to reduce indentation.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n I copy the \"xmalloc(sizeof(regex_t))\" anti-pattern verbatim. We will\n lose it in the next patch.\n\n builtin/config.c | 39 +++++++++++++++++++++++++--------------\n 1 file changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 98d65bc0ad..e675ae0640 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -280,6 +280,28 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \treturn format_config(&values->items[values->nr++], key_, value_);\n }\n \n+static int handle_value_regex(const char *regex_)\n+{\n+\tif (!regex_) {\n+\t\tregexp = NULL;\n+\t\treturn 0;\n+\t}\n+\n+\tif (regex_[0] == '!') {\n+\t\tdo_not_match = 1;\n+\t\tregex_++;\n+\t}\n+\n+\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n+\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n+\t\terror(_(\"invalid pattern: %s\"), regex_);\n+\t\tFREE_AND_NULL(regexp);\n+\t\treturn CONFIG_INVALID_PATTERN;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int get_value(const char *key_, const char *regex_)\n {\n \tint ret = CONFIG_GENERIC_ERROR;\n@@ -317,20 +339,9 @@ static int get_value(const char *key_, const char *regex_)\n \t\t}\n \t}\n \n-\tif (regex_) {\n-\t\tif (regex_[0] == '!') {\n-\t\t\tdo_not_match = 1;\n-\t\t\tregex_++;\n-\t\t}\n-\n-\t\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n-\t\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n-\t\t\terror(_(\"invalid pattern: %s\"), regex_);\n-\t\t\tFREE_AND_NULL(regexp);\n-\t\t\tret = CONFIG_INVALID_PATTERN;\n-\t\t\tgoto free_strings;\n-\t\t}\n-\t}\n+\tret = handle_value_regex(regex_);\n+\tif (ret)\n+\t\tgoto free_strings;\n \n \tconfig_with_options(collect_config, &values,\n \t\t\t    &given_config_source, &config_options);\n-- \n2.24.0\n\n"},{"id":"386133","messageId":"336eaa77e4974f84ea1eef473672e1d300f3a43d.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 4/8] builtin/config: collect \"value_regexp\" data in a struct","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:03Z","receivedAt":"2019-11-13T18:55:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`git config` can take an optional \"value_regexp\". Collect the\n`regex_t`-pointer and the `do_not_match` flag into a new `struct\ncmd_line_value`.\n\nRather than signalling/judging presence of a regexp by the NULL-ness of\nthe pointer, introduce a `mode` enum. After this commit, we just have\ntwo modes, \"none\" and \"regexp\", but we will gain another one in the next\ncommit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/config.c | 31 +++++++++++++++++++------------\n 1 file changed, 19 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex e675ae0640..d812e73e2b 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -14,12 +14,15 @@ static const char *const builtin_config_usage[] = {\n \n static char *key;\n static regex_t *key_regexp;\n-static regex_t *regexp;\n+static struct {\n+\tenum { none, regexp } mode;\n+\tregex_t *regexp;\n+\tint do_not_match; /* used with `regexp` */\n+} cmd_line_value;\n static int show_keys;\n static int omit_values;\n static int use_key_regexp;\n static int do_all;\n-static int do_not_match;\n static char delim = '=';\n static char key_delim = ' ';\n static char term = '\\n';\n@@ -270,8 +273,10 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t\treturn 0;\n \tif (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))\n \t\treturn 0;\n-\tif (regexp != NULL &&\n-\t    (do_not_match ^ !!regexec(regexp, (value_?value_:\"\"), 0, NULL, 0)))\n+\tif (cmd_line_value.mode == regexp &&\n+\t    (cmd_line_value.do_not_match ^\n+\t     !!regexec(cmd_line_value.regexp, value_ ? value_ : \"\",\n+\t\t       0, NULL, 0)))\n \t\treturn 0;\n \n \tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n@@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n static int handle_value_regex(const char *regex_)\n {\n \tif (!regex_) {\n-\t\tregexp = NULL;\n+\t\tcmd_line_value.mode = none;\n \t\treturn 0;\n \t}\n \n+\tcmd_line_value.mode = regexp;\n+\n \tif (regex_[0] == '!') {\n-\t\tdo_not_match = 1;\n+\t\tcmd_line_value.do_not_match = 1;\n \t\tregex_++;\n \t}\n \n-\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n-\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n+\tcmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));\n+\tif (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {\n \t\terror(_(\"invalid pattern: %s\"), regex_);\n-\t\tFREE_AND_NULL(regexp);\n+\t\tFREE_AND_NULL(cmd_line_value.regexp);\n \t\treturn CONFIG_INVALID_PATTERN;\n \t}\n \n@@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)\n \t\tregfree(key_regexp);\n \t\tfree(key_regexp);\n \t}\n-\tif (regexp) {\n-\t\tregfree(regexp);\n-\t\tfree(regexp);\n+\tif (cmd_line_value.regexp) {\n+\t\tregfree(cmd_line_value.regexp);\n+\t\tfree(cmd_line_value.regexp);\n \t}\n \n \treturn ret;\n-- \n2.24.0\n\n"},{"id":"386134","messageId":"beb0cfbb78e3ae69e64ff2ed57c8e628ccb187ce.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 5/8] builtin/config: canonicalize \"value_regex\" with `--type=bool`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:04Z","receivedAt":"2019-11-13T18:55:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"With `--type=bool`, we use the \"value_regex\" as a normal regex and do\nnot use our knowledge about the different well-defined ways we have of\nspelling the boolean values. Let's consider a few examples.\n\nThese output \"true\":\n\tgit -c foo.bar=true config --type=bool --get foo.bar true\n\tgit -c foo.bar=true config --type=bool --get foo.bar t\n\nThese produce no output:\n\tgit -c foo.bar=true config --type=bool --get foo.bar True\n\tgit -c foo.bar=true config --type=bool --get foo.bar 1\n\nCanonicalize the provided \"value_regex\", then canonicalize candidate\nvalues as we go through the config and compare the canonicalized values.\nIf the parameter doesn't canonicalize, fall back to handling it as a\nregex, as before. This would happen in the second example above, but\nalso if someone has hand-rolled their own canonicalisation with, e.g.,\nsomething like \"^(on|On|ON|true|1|42)$\".\n\nThis commit affects all modes that take a \"value_regex\", e.g.,\n`--get-regexp` which can be used in a more useful way than the examples\nabove might at first suggest:\n\n\tgit config --type=bool --name-only --get-regexp '^foo\\.' true\n\nThis commit does change the behavior for certain usages, but it almost\ncertainly does so for the better: We don't exclude config items based on\nhow the config happens to spell \"true\" or \"false\". This commit would\ncause a regression if someone uses different synonyms for \"true\",\nknowing that Git handles them all the same in day-to-day operation, but\nrelying on the encoding (using, say, integers 1-100) to signal some sort\nof confidence and using `git config` to query for various such \"levels\".\nI'm tempted to bet no-one will be hurt by this change.\n\nDo not rename the \"value_regex\" in the documentation. This commit can be\nseen as teaching `git config --type=bool` about a particular type of\nregex, where \"true\" matches \"yes\", but not \"no\". So arguably,\n\"value_regex\" still describes quite well what is going on.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt |  4 ++++\n builtin/config.c             | 15 +++++++++++-\n t/t1300-config.sh            | 45 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 899e92a1c9..139750bbda 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -43,6 +43,10 @@ outgoing values are canonicalize-able under the given <type>.  If no\n `--type=<type>` is given, no canonicalization will be performed. Callers may\n unset an existing `--type` specifier with `--no-type`.\n \n+With `--type=bool`, if `value_regex` is given\n+and canonicalizes to a boolean value, it matches all entries\n+that canonicalize to the same boolean value.\n+\n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\n `--system`, `--global`, `--local`, `--worktree` and\ndiff --git a/builtin/config.c b/builtin/config.c\nindex d812e73e2b..c9fe0c5752 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -15,9 +15,10 @@ static const char *const builtin_config_usage[] = {\n static char *key;\n static regex_t *key_regexp;\n static struct {\n-\tenum { none, regexp } mode;\n+\tenum { none, regexp, boolean } mode;\n \tregex_t *regexp;\n \tint do_not_match; /* used with `regexp` */\n+\tint boolean;\n } cmd_line_value;\n static int show_keys;\n static int omit_values;\n@@ -278,6 +279,9 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t     !!regexec(cmd_line_value.regexp, value_ ? value_ : \"\",\n \t\t       0, NULL, 0)))\n \t\treturn 0;\n+\tif (cmd_line_value.mode == boolean &&\n+\t    git_parse_maybe_bool(value_) != cmd_line_value.boolean)\n+\t\treturn 0;\n \n \tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n \tstrbuf_init(&values->items[values->nr], 0);\n@@ -292,6 +296,15 @@ static int handle_value_regex(const char *regex_)\n \t\treturn 0;\n \t}\n \n+\tif (type == TYPE_BOOL) {\n+\t\tint boolval = git_parse_maybe_bool(regex_);\n+\t\tif (boolval >= 0) {\n+\t\t\tcmd_line_value.mode = boolean;\n+\t\t\tcmd_line_value.boolean = boolval;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n \tcmd_line_value.mode = regexp;\n \n \tif (regex_[0] == '!') {\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex a38cc143a1..e4906a893e 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -427,6 +427,51 @@ test_expect_success 'no arguments, but no crash' '\n \ttest_i18ngrep usage output\n '\n \n+test_expect_success 'setup config file with several boolean values' '\n+\tcat >.git/config <<-\\EOF\n+\t[foo]\n+\t\tn1 = no\n+\t\tn2 = NO\n+\t\tn3 = off\n+\t\tn4 = false\n+\t\tn5 = 0\n+\t\tn6 =\n+\t\ty1 = yes\n+\t\ty2 = YES\n+\t\ty3 = on\n+\t\ty4 = true\n+\t\ty5 = 1\n+\t\ty6 = 42\n+\t\ty7\n+\tEOF\n+'\n+\n+test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (false)' '\n+\tgit config --type=bool --get-regexp \"foo\\..*\" OFF >output &&\n+\ttest_line_count = 6 output &&\n+\t! grep -v \"^foo.n\" output\n+'\n+\n+test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (true)' '\n+\tgit config --type=bool --get-regexp \"foo\\..*\" ON >output &&\n+\ttest_line_count = 7 output &&\n+\t! grep -v \"^foo.y\" output\n+'\n+\n+test_expect_success '--get canonicalizes integer value_regex with --type=bool' '\n+\techo true >expect &&\n+\tgit config --type=bool --get foo.y2 1 >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--type=bool with \"non-bool\" value_regex' '\n+\techo true >expect &&\n+\tgit config --type=bool --get foo.y4 \"t.*\" >output &&\n+\ttest_cmp expect output &&\n+\ttest_must_fail git config --type=bool --get foo.y4 \"T.*\" >output &&\n+\ttest_must_be_empty output\n+'\n+\n test_expect_success 'setup simple config file' '\n \tq_to_tab >.git/config <<-\\EOF\n \t[a.b]\n-- \n2.24.0\n\n"},{"id":"386135","messageId":"5d6bf76f2e9bccf38f566524f6133252be0603c9.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 6/8] builtin/config: canonicalize \"value_regex\" with `--type=bool-or-int`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:05Z","receivedAt":"2019-11-13T18:55:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"As an obvious follow-up to the previous commit, also canonicalize the\n\"value_regex\" when the type is \"bool-or-int\".\n\nObserve that in this case, falling back to handling the \"value_regex\" as\na normal regex is not just to cater to old scripts and habits. It is\nnecessary to handle the numerical inputs (or regexes matching some\nspecific numerical values!) that we must expect.\n\nFuture commits will expand on the code for `--type=bool`. Rather than\ntrying to shoehorn both these cases into a single chunk of code, let's\njust duplicate some of the code from the previous commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt |  2 +-\n builtin/config.c             |  9 +++++++++\n t/t1300-config.sh            | 20 ++++++++++++++++++++\n 3 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 139750bbda..864375b1ec 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -43,7 +43,7 @@ outgoing values are canonicalize-able under the given <type>.  If no\n `--type=<type>` is given, no canonicalization will be performed. Callers may\n unset an existing `--type` specifier with `--no-type`.\n \n-With `--type=bool`, if `value_regex` is given\n+With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given\n and canonicalizes to a boolean value, it matches all entries\n that canonicalize to the same boolean value.\n \ndiff --git a/builtin/config.c b/builtin/config.c\nindex c9fe0c5752..4e274d4867 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -305,6 +305,15 @@ static int handle_value_regex(const char *regex_)\n \t\t}\n \t}\n \n+\tif (type == TYPE_BOOL_OR_INT) {\n+\t\tint boolval = git_parse_maybe_bool_text(regex_);\n+\t\tif (boolval >= 0) {\n+\t\t\tcmd_line_value.mode = boolean;\n+\t\t\tcmd_line_value.boolean = boolval;\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n \tcmd_line_value.mode = regexp;\n \n \tif (regex_[0] == '!') {\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex e4906a893e..f0e9a21dc4 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -472,6 +472,26 @@ test_expect_success '--type=bool with \"non-bool\" value_regex' '\n \ttest_must_be_empty output\n '\n \n+test_expect_success '--type=bool-or-int with boolean value_regex' '\n+\techo true >expect &&\n+\tgit config --type=bool-or-int --get foo.y2 true >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--type=bool-or-int with integer value_regex' '\n+\ttest_must_fail git config --type=bool-or-int --get foo.y2 1 >output &&\n+\ttest_must_be_empty output &&\n+\techo 1 >expect &&\n+\tgit config --type=bool-or-int --get foo.y5 1 >output &&\n+\ttest_cmp expect output\n+'\n+\n+test_expect_success '--type=bool-or-int with regex value_regex' '\n+\techo true >expect &&\n+\tgit config --type=bool-or-int --get foo.y4 \"t.*\" >output &&\n+\ttest_cmp expect output\n+'\n+\n test_expect_success 'setup simple config file' '\n \tq_to_tab >.git/config <<-\\EOF\n \t[a.b]\n-- \n2.24.0\n\n"},{"id":"386136","messageId":"0f48ab4fc344b3cc226d0a45d13530022208ff3e.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 7/8] builtin/config: warn if \"value_regex\" doesn't canonicalize as boolean","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:06Z","receivedAt":"2019-11-13T18:55:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"With `--type=bool`, we try to canonicalize the \"value_regex\". If it\ndoesn't canonicalize, we continue and handle the \"value_regex\" as an\nordinary regex. This is deliberate -- we do not want to cause existing\nscripts to fail.\n\nThis does mean that users might be at risk of missing out on config\nvalues depending on which representation they use in their config file:\n\n\t$ git config foo.bar on\n\t$ git config foo.baz true\n\t$ git config --type=bool --get-regex \"foo\\.*\" tru\n\tfoo.baz true\n\t$ # oops! missing foo.bar\n\nLet's start warning when the \"value_regex\" doesn't look like a boolean.\nDocument our intention of eventually requiring the canonicalization to\npass.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt | 2 ++\n builtin/config.c             | 2 ++\n t/t1300-config.sh            | 3 ++-\n 3 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 864375b1ec..43310ca3c0 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -46,6 +46,8 @@ unset an existing `--type` specifier with `--no-type`.\n With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given\n and canonicalizes to a boolean value, it matches all entries\n that canonicalize to the same boolean value.\n+The support for non-canonicalizing values of `value_regex` with\n+`--type=bool` is deprecated.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4e274d4867..2af62b95f8 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -303,6 +303,8 @@ static int handle_value_regex(const char *regex_)\n \t\t\tcmd_line_value.boolean = boolval;\n \t\t\treturn 0;\n \t\t}\n+\t\twarning(_(\"value_regex '%s' cannot be canonicalized \"\n+\t\t\t  \"to a boolean value\"), regex_);\n \t}\n \n \tif (type == TYPE_BOOL_OR_INT) {\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex f0e9a21dc4..3e067c211d 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -466,8 +466,9 @@ test_expect_success '--get canonicalizes integer value_regex with --type=bool' '\n \n test_expect_success '--type=bool with \"non-bool\" value_regex' '\n \techo true >expect &&\n-\tgit config --type=bool --get foo.y4 \"t.*\" >output &&\n+\tgit config --type=bool --get foo.y4 \"t.*\" >output 2>err &&\n \ttest_cmp expect output &&\n+\ttest_i18ngrep \"cannot be canonicalized\" err &&\n \ttest_must_fail git config --type=bool --get foo.y4 \"T.*\" >output &&\n \ttest_must_be_empty output\n '\n-- \n2.24.0\n\n"},{"id":"386137","messageId":"309960f7d4bd790a11c31b727475134368eb6a10.1573670565.git.martin.agren@gmail.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"[PATCH 8/8] builtin/config: die if \"value_regex\" doesn't canonicalize as boolean","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:55:07Z","receivedAt":"2019-11-13T18:56:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This completes the transition from handling a \"value_regexp\" with\n`--type=bool` as a regex, to handling it on the assumption that it\ncanonicalizes to a boolean value.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/git-config.txt | 5 ++---\n builtin/config.c             | 4 ++--\n t/t1300-config.sh            | 5 +----\n 3 files changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 43310ca3c0..598915eac6 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -43,11 +43,10 @@ outgoing values are canonicalize-able under the given <type>.  If no\n `--type=<type>` is given, no canonicalization will be performed. Callers may\n unset an existing `--type` specifier with `--no-type`.\n \n-With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given\n+With `--type=bool-or-int`, if `value_regex` is given\n and canonicalizes to a boolean value, it matches all entries\n that canonicalize to the same boolean value.\n-The support for non-canonicalizing values of `value_regex` with\n-`--type=bool` is deprecated.\n+With `--type=bool`, `value_regex` (if given) must canonicalize.\n \n When reading, the values are read from the system, global and\n repository local configuration files by default, and options\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 2af62b95f8..837766cfb3 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -303,8 +303,8 @@ static int handle_value_regex(const char *regex_)\n \t\t\tcmd_line_value.boolean = boolval;\n \t\t\treturn 0;\n \t\t}\n-\t\twarning(_(\"value_regex '%s' cannot be canonicalized \"\n-\t\t\t  \"to a boolean value\"), regex_);\n+\t\tdie(_(\"value_regex '%s' cannot be canonicalized \"\n+\t\t      \"to a boolean value\"), regex_);\n \t}\n \n \tif (type == TYPE_BOOL_OR_INT) {\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 3e067c211d..9eccc255db 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -465,11 +465,8 @@ test_expect_success '--get canonicalizes integer value_regex with --type=bool' '\n '\n \n test_expect_success '--type=bool with \"non-bool\" value_regex' '\n-\techo true >expect &&\n-\tgit config --type=bool --get foo.y4 \"t.*\" >output 2>err &&\n-\ttest_cmp expect output &&\n+\ttest_must_fail git config --type=bool --get foo.y4 t >output 2>err &&\n \ttest_i18ngrep \"cannot be canonicalized\" err &&\n-\ttest_must_fail git config --type=bool --get foo.y4 \"T.*\" >output &&\n \ttest_must_be_empty output\n '\n \n-- \n2.24.0\n\n"},{"id":"386153","messageId":"xmqqeeyb9p4i.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-14T02:18:53Z","receivedAt":"2019-11-14T02:19:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n>   git config --type=bool --name-only --get-regexp '^foo\\.' true\n> ...\n> This patch series teaches `git config` to canonicalize the incoming\n> \"value_regex\" (\"true\" in the example above), then canonicalize candidate\n> values as we go through the config. Or if you will, `git config` learns\n> a brand new type of regex, corresponding to the different ways there are\n> of spelling \"true\" and \"false\", respectively.\n\nNice ;-)\n\n> `--type=bool-or-int` gets the same treatment, except we need to to be\n> able to handle the ints and regexes matching particular ints that we\n> must expect.\n\nHmm, so I can say 1024k or 1m and that would match 1048576?  \n\nDoubly nice.\n\nLooking forward to reading it thru.\n"},{"id":"386166","messageId":"20191114062900.GD10643@sigill.intra.peff.net","threadId":"52260","inReplyTo":"cover.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-14T06:29:00Z","receivedAt":"2019-11-14T06:29:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2019 at 07:54:59PM +0100, Martin Ågren wrote:\n\n> To find all config items \"foo.*\" that are configured as the Boolean\n> value \"true\", you can try executing\n> \n>   git config --type=bool --name-only --get-regexp '^foo\\.' true\n> \n> ... and hope that you didn't spell \"true\" as \"on\", \"True\" or \"1\" back\n> when you populated your config. This shortcoming has been mentioned as a\n> left-over bit [1] [2].\n\nThis seems like a good idea, but I wonder why we'd limit it to bools?\nIt seems like any type would probably be better off matching a\nnormalized version.\n\nWe already have two functions in builtin/config.c which handle types:\n\n  - format_config() is for actually interpreting an existing value and\n    writing it to stdout\n\n  - normalize_value() is used when writing a new value into a config\n    file, and normalizing what the user provided on the command-line\n\nI don't think you'd want to use format_config() here. For example, if I\nsay:\n\n  git config --type=color --get-regexp ^foo red\n\nI want to match the original \"red\" color, but _output_ the ANSI code.\nBut normalize_value(), by contrast, leaves colors intact. So maybe it's\na good match?\n\nOTOH, I'd probably expect to match expanded pathnames or expiration\ndates there, too, and it doesn't expand those. Those ones are less clear\nto me. The whole premise of this series is making an assumption that\n\"--type\" is about how you want to match, and not just about what you\nwant to output. In your example above it's clear that you don't care\nabout the output type (because you're using --name-only), but for:\n\n  git config --type=path --get-regexp ^foo /home/peff\n\nit's unclear if you want to match a value of \"~peff/foo\", or if you\nsimply want to output the expansion.\n\nI wonder if we'd want to allow specifying the output type and the\nmatching type separately? Maybe that just makes it more awkward to use\nfor little benefit (I admit that it's hard to imagine somebody wanting\nto normalize bools on output but _not_ for matching).\n\nAnyway. It would be nice if we could build on the existing logic in some\nway, rather than making a third place that has to handle every type we\nknow about.\n\n> `--type=bool-or-int` gets the same treatment, except we need to to be\n> able to handle the ints and regexes matching particular ints that we\n> must expect. That said, even with `--type=bool` we can't move too\n> aggressively towards *requiring* that the incoming \"value_regex\"\n> canonializes as a Boolean value. The penultimate patch starts to warn on\n> non-canonicalizing values; the final patch makes us bail out entirely.\n> \n> The last patch is not meant for immediate inclusion, but I post it\n> anyway. I can re-submit it at an appropriate time, or maybe it could\n> slumber on pu until the time is ripe for completing the switch.\n\nI think bailing on values that can't be converted is normal for other\ncode paths. E.g., just trying to print:\n\n  $ git -c foo.bar=abc config --type=bool foo.abr\n  fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit\n\n-Peff\n"},{"id":"386170","messageId":"CAN0heSrTtEHM7bEGMbokf1RJDGPobD=C73Wq1Ry5fgE69+QV-g@mail.gmail.com","threadId":"52260","inReplyTo":"xmqqeeyb9p4i.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-14T06:40:16Z","receivedAt":"2019-11-14T06:40:30Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Junio\n\nOn Thu, 14 Nov 2019 at 03:19, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > `--type=bool-or-int` gets the same treatment, except we need to to be\n> > able to handle the ints and regexes matching particular ints that we\n> > must expect.\n>\n> Hmm, so I can say 1024k or 1m and that would match 1048576?\n>\n> Doubly nice.\n>\n> Looking forward to reading it thru.\n\nMaybe you already noticed, but no, I didn't get to canonicalizing\nintegers like that. What I meant was, type=bool-or-int learns to handle\nbooleans similar to what I did to type=bool.\n\nI don't feel entirely satisfied by some of my commit message oneliners.\nThey could make that a bit clearer, I think.\n\nNot directly related to your question, but I realize now that with\ntype=bool-or-int, I only added the first of these example usages below\nas a test. The second one is perhaps just as interesting.\n\n$ ./git -c an.int=1 config --get --type=bool-or-int an.int 1\n1\n$ ./git -c an.int=1 config --get --type=bool-or-int an.int on\n1\n\nThis last one emits \"1\". That's because by the time we've decided to\noutput the value, `format_config()` has some logic around\ntype=bool-or-int, but doesn't know about why exactly we selected this\nan.int=1 in the first place\n  (git_parse_maybe_bool(\"1\") == git_parse_maybe_bool_TEXT(\"on\")).\n\nJust after thinking about this for a short while, I can't immediately\nsay whether this second one should emit \"1\" or \"true\". My added\ndocumentation is actually vague enough to allow both of these to\nhappen... I'll ponder this.\n\n\nMartin\n"},{"id":"386172","messageId":"CAN0heSrtwi9V607vBX9PMSfNLQ8iGcno6_iGuR4Fs8ndGxqh8A@mail.gmail.com","threadId":"52260","inReplyTo":"20191114062900.GD10643@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-14T06:54:35Z","receivedAt":"2019-11-14T06:54:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 14 Nov 2019 at 07:29, Jeff King <peff@peff.net> wrote:\n> This seems like a good idea, but I wonder why we'd limit it to bools?\n\nBasically because that's what the existing left-over-bits mentioned and\nit felt like a good place to start. But you're right in asking about the\nbigger picture up front.\n\n> It seems like any type would probably be better off matching a\n> normalized version.\n>\n> We already have two functions in builtin/config.c which handle types:\n>\n>   - format_config() is for actually interpreting an existing value and\n>     writing it to stdout\n>\n>   - normalize_value() is used when writing a new value into a config\n>     file, and normalizing what the user provided on the command-line\n>\n> I don't think you'd want to use format_config() here.\n\nI just speculated a little around format_config() in a reply to Junio.\nAlready with my patch series and with type=bool-or-int, there's a\nvisible funny-funky corner case where one hand doesn't know what the\nother is doing.\n\n> For example, if I\n> say:\n>\n>   git config --type=color --get-regexp ^foo red\n>\n> I want to match the original \"red\" color, but _output_ the ANSI code.\n> But normalize_value(), by contrast, leaves colors intact. So maybe it's\n> a good match?\n>\n> OTOH, I'd probably expect to match expanded pathnames or expiration\n> dates there, too, and it doesn't expand those. Those ones are less clear\n> to me. The whole premise of this series is making an assumption that\n> \"--type\" is about how you want to match,\n\nRight.\n\n> and not just about what you\n> want to output. In your example above it's clear that you don't care\n> about the output type (because you're using --name-only), but for:\n>\n>   git config --type=path --get-regexp ^foo /home/peff\n>\n> it's unclear if you want to match a value of \"~peff/foo\", or if you\n> simply want to output the expansion.\n\nHmm, I feel like we're opening a can of worms here.\n\n> I wonder if we'd want to allow specifying the output type and the\n> matching type separately? Maybe that just makes it more awkward to use\n> for little benefit (I admit that it's hard to imagine somebody wanting\n> to normalize bools on output but _not_ for matching).\n>\n> Anyway. It would be nice if we could build on the existing logic in some\n> way, rather than making a third place that has to handle every type we\n> know about.\n\nUnderstood. Thanks a lot for sharing your thoughts.\n\n> > The last patch is not meant for immediate inclusion, but I post it\n> > anyway. I can re-submit it at an appropriate time, or maybe it could\n> > slumber on pu until the time is ripe for completing the switch.\n>\n> I think bailing on values that can't be converted is normal for other\n> code paths. E.g., just trying to print:\n>\n>   $ git -c foo.bar=abc config --type=bool foo.abr\n>   fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit\n\nI'm not sure if you mean \"... so we could be a lot more aggressive\nhere\"?\n\nI'm running now and I feel like I'll need to read your mail again\ntonight and get back to you in more detail.\n\nThanks\nMartin\n"},{"id":"386173","messageId":"20191114073721.GA17186@sigill.intra.peff.net","threadId":"52260","inReplyTo":"CAN0heSrtwi9V607vBX9PMSfNLQ8iGcno6_iGuR4Fs8ndGxqh8A@mail.gmail.com","subject":"Re: [PATCH 0/8] builtin/config: canonicalize \"value_regex\" with `--type=bool[-or-int]`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-14T07:37:21Z","receivedAt":"2019-11-14T07:37:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2019 at 07:54:35AM +0100, Martin Ågren wrote:\n\n> > > The last patch is not meant for immediate inclusion, but I post it\n> > > anyway. I can re-submit it at an appropriate time, or maybe it could\n> > > slumber on pu until the time is ripe for completing the switch.\n> >\n> > I think bailing on values that can't be converted is normal for other\n> > code paths. E.g., just trying to print:\n> >\n> >   $ git -c foo.bar=abc config --type=bool foo.abr\n> >   fatal: bad numeric config value 'abc' for 'foo.bar': invalid unit\n> \n> I'm not sure if you mean \"... so we could be a lot more aggressive\n> here\"?\n\nYeah, I think it's OK to be aggressive with bailing when the user gave\nus a --type, but the value doesn't match it.\n\n> I'm running now and I feel like I'll need to read your mail again\n> tonight and get back to you in more detail.\n\nSure, no rush and thanks for working on it. :)\n\n-Peff\n"},{"id":"386697","messageId":"xmqq8so9akx5.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"37e2b121d5946cfacb3fc4d2721770e496f7b752.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 2/8] t1300: modernize part of script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T04:54:46Z","receivedAt":"2019-11-21T04:54:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> Create `.git/config` and `expect` files inside `test_expect_success`,\n> either inside the one existing test that uses the file, or in a new\n> \"setup\" step before several tests that use it.\n>\n> Redirect using `>output` rather than `> output`. Use `<<-\\EOF\" with\n> heredocs rather than just `<< EOF` and use `q_to_tab` to create properly\n> indented input and output files.\n>\n> This commit does not modernize the whole script, but just some of it,\n> around the point where a later commit will add new content.\n>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  t/t1300-config.sh | 138 ++++++++++++++++++++++------------------------\n>  1 file changed, 65 insertions(+), 73 deletions(-)\n\nThanks.  All conversions looked OK.\n\n"},{"id":"386698","messageId":"xmqq4kyxakjs.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"c4bcb32299291549d82c0544937a647c5000ad64.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T05:02:47Z","receivedAt":"2019-11-21T05:02:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> This is a self-contained and fairly large chunk of code which will soon\n> gain a few more lines. Prepare by extracting it into a separate\n> function.\n>\n> This whole chunk is wrapped in \"if (regex_)\" -- rewrite it into an early\n> return path in the new helper function to reduce indentation.\n\nIt is not clear if regexp were cleared to NULL when !regex_ in the\noriginal code, so if that were the case, this refactoring is a\nworthy clean-up from that point of view, too.\n\n>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  I copy the \"xmalloc(sizeof(regex_t))\" anti-pattern verbatim. We will\n>  lose it in the next patch.\n\nIt also spreads the use of file-scope global variables like\ndo_not_match and regexp, which also is existing anti-pattern that we\nmay want to fix by enclosing them in a struct and pass a pointer to\nit around the callchain.  We can clean it up later.\n\n"},{"id":"386699","messageId":"xmqqy2w9951v.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"336eaa77e4974f84ea1eef473672e1d300f3a43d.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 4/8] builtin/config: collect \"value_regexp\" data in a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T05:22:52Z","receivedAt":"2019-11-21T05:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> `git config` can take an optional \"value_regexp\". Collect the\n> `regex_t`-pointer and the `do_not_match` flag into a new `struct\n> cmd_line_value`.\n\nA \"struct cmd_line_value\" sounded, to me at least during my first\nreading, as if it is about all command line options, but that is not\nat all what you meant to imply.  Is this only about the optional\nvalue-regexp (if so perhaps calling it \"value_regexp_option\" would\nhave helped me avoid such a misunderstanding)?\n\n> Rather than signalling/judging presence of a regexp by the NULL-ness of\n> the pointer, introduce a `mode` enum.\n\nOK.  Tangentially this makes readers wonder why the existing code\nfor key_regexp does not follow the same \"NULL-ness\" pattern but has\na separate use_key_regexp boolean.  It appears that the original\ncode is quite confused---it is totally outside the scope of this\nseries to clean it up and inject sanity into it though ;-)\n\n>  static regex_t *key_regexp;\n> -static regex_t *regexp;\n> +static struct {\n> +\tenum { none, regexp } mode;\n\nWe often use the same identifier for a struct and an instance of the\nstruct, taking advantage of the fact that they live in separate\nnamespaces, but lowercase enumerated values like 'regexp' that\ncollides with the field name (and possibly a variable name used\nelsewhere) smells a bit too much.\n\n> +\tregex_t *regexp;\n> +\tint do_not_match; /* used with `regexp` */\n> +} cmd_line_value;\n>  static int show_keys;\n>  static int omit_values;\n>  static int use_key_regexp;\n\n> @@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n>  static int handle_value_regex(const char *regex_)\n>  {\n>  \tif (!regex_) {\n> -\t\tregexp = NULL;\n> +\t\tcmd_line_value.mode = none;\n>  \t\treturn 0;\n\nNow we are back to relying on cmd_line_value.regexp staying to be\nNULL after initialized, which is the state before the previous\npatch.  If the end result is correct, then it is OK, I think, but\nthen the previous step shouldn't have added the NULL assignment here\nin the first place.\n\n>  \t}\n>  \n> +\tcmd_line_value.mode = regexp;\n> +\n>  \tif (regex_[0] == '!') {\n> -\t\tdo_not_match = 1;\n> +\t\tcmd_line_value.do_not_match = 1;\n>  \t\tregex_++;\n>  \t}\n>  \n> -\tregexp = (regex_t*)xmalloc(sizeof(regex_t));\n> -\tif (regcomp(regexp, regex_, REG_EXTENDED)) {\n> +\tcmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));\n> +\tif (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {\n>  \t\terror(_(\"invalid pattern: %s\"), regex_);\n> -\t\tFREE_AND_NULL(regexp);\n> +\t\tFREE_AND_NULL(cmd_line_value.regexp);\n\nHmph.  !regexp in old code should mean cmd_line_value.mode==regexp\nin the new world order after this patch is applied, no?  Should we\nbe treaking the mode field here before we leave?  I think it should\nnot matter, but thought it wouldn't hurt to ask.\n\nIn collect_config(), cmd_line_value.regexp is blindly passed to\nregexec(3) as long as cmd_line_value.mode==regexp, so the invariant\n\"when .mode is regexp, .regexp must be valid, or collect_config() would\nnever be called for such cmd_line_value\" is rather important to\navoid crashing ;-)\n\n>  \t\treturn CONFIG_INVALID_PATTERN;\n>  \t}\n>  \n> @@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)\n>  \t\tregfree(key_regexp);\n>  \t\tfree(key_regexp);\n>  \t}\n> -\tif (regexp) {\n> -\t\tregfree(regexp);\n> -\t\tfree(regexp);\n> +\tif (cmd_line_value.regexp) {\n> +\t\tregfree(cmd_line_value.regexp);\n> +\t\tfree(cmd_line_value.regexp);\n\nLikewise.\n\n>  \t}\n>  \n>  \treturn ret;\n"},{"id":"386700","messageId":"xmqqtv6x94d0.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"beb0cfbb78e3ae69e64ff2ed57c8e628ccb187ce.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 5/8] builtin/config: canonicalize \"value_regex\" with `--type=bool`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T05:37:47Z","receivedAt":"2019-11-21T05:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> With `--type=bool`, we use the \"value_regex\" as a normal regex and do\n> not use our knowledge about the different well-defined ways we have of\n> spelling the boolean values. Let's consider a few examples.\n>\n> These output \"true\":\n> \tgit -c foo.bar=true config --type=bool --get foo.bar true\n> \tgit -c foo.bar=true config --type=bool --get foo.bar t\n\nI wonder what\n\n    git -c foo.bar=true config --type=bool --get foo.bar 't.*'\n\nshould say.  I think you and Peff discussed in the downthread to\nmake it more strict to require <value_regex> *NOT* regexp at all,\nbut one of the string representations of boolean we would take,\nwhich means 't' (in your exmaple) and 't.*' would stop yielding\ncorrect result, which I think is fine.\n\n> Canonicalize the provided \"value_regex\", then canonicalize candidate\n> values as we go through the config and compare the canonicalized values.\n\nI think (for the reason stated earlier) this is OK, but we should\nmake sure that we are clear in the documentation that in this mode\n<value_regex> is no longer a regex but a string.  We might even want\nto have a separate SYNOPSIS entry that does not say regex at all,\nsomething along the lines of ...\n\n-'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]\n+'git config' [<file-option>] [--show-origin] [-z|--null] --get name [value_regex]\n+'git config' [<file-option>] --type=<type> [--show-origin] [-z|--null] --get name [value]\n\n> If the parameter doesn't canonicalize, fall back to handling it as a\n> regex, as before. This would happen in the second example above, but\n> also if someone has hand-rolled their own canonicalisation with, e.g.,\n> something like \"^(on|On|ON|true|1|42)$\".\n\nI actually am OK with this fallback, too.  That would also mean the\nadditional SYNOPSIS entry unnecessary ;-).\n\n> Do not rename the \"value_regex\" in the documentation. This commit can be\n> seen as teaching `git config --type=bool` about a particular type of\n> regex, where \"true\" matches \"yes\", but not \"no\". So arguably,\n> \"value_regex\" still describes quite well what is going on.\n\nHeh ;-) I probably should learn to blindly read thru the proposed\nlog message to the end before starting to think about ramifications\nof the proposed changes myself.  Some of the reasoning in this\nparagraph should be reflected to the explanation of the argument in\nthe documentation, so that the readers will know this is not a usual\nregex at all.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  Documentation/git-config.txt |  4 ++++\n>  builtin/config.c             | 15 +++++++++++-\n>  t/t1300-config.sh            | 45 ++++++++++++++++++++++++++++++++++++\n>  3 files changed, 63 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index 899e92a1c9..139750bbda 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -43,6 +43,10 @@ outgoing values are canonicalize-able under the given <type>.  If no\n>  `--type=<type>` is given, no canonicalization will be performed. Callers may\n>  unset an existing `--type` specifier with `--no-type`.\n>  \n> +With `--type=bool`, if `value_regex` is given\n> +and canonicalizes to a boolean value, it matches all entries\n> +that canonicalize to the same boolean value.\n\n... otherwise `value_regex` is used as a string, i.e. as if no\n'--type=bool' is given.\n\n> +\tif (cmd_line_value.mode == boolean &&\n> +\t    git_parse_maybe_bool(value_) != cmd_line_value.boolean)\n> +\t\treturn 0;\n>  \n>  \tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n>  \tstrbuf_init(&values->items[values->nr], 0);\n> @@ -292,6 +296,15 @@ static int handle_value_regex(const char *regex_)\n>  \t\treturn 0;\n>  \t}\n>  \n> +\tif (type == TYPE_BOOL) {\n> +\t\tint boolval = git_parse_maybe_bool(regex_);\n> +\t\tif (boolval >= 0) {\n> +\t\t\tcmd_line_value.mode = boolean;\n> +\t\t\tcmd_line_value.boolean = boolval;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n>  \tcmd_line_value.mode = regexp;\n\n... which can be seen here.  OK.\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index a38cc143a1..e4906a893e 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -427,6 +427,51 @@ test_expect_success 'no arguments, but no crash' '\n>  \ttest_i18ngrep usage output\n>  '\n>  \n> +test_expect_success 'setup config file with several boolean values' '\n> +\tcat >.git/config <<-\\EOF\n> +\t[foo]\n> +\t\tn1 = no\n> +\t\tn2 = NO\n> +\t\tn3 = off\n> +\t\tn4 = false\n> +\t\tn5 = 0\n> +\t\tn6 =\n> +\t\ty1 = yes\n> +\t\ty2 = YES\n> +\t\ty3 = on\n> +\t\ty4 = true\n> +\t\ty5 = 1\n> +\t\ty6 = 42\n> +\t\ty7\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (false)' '\n> +\tgit config --type=bool --get-regexp \"foo\\..*\" OFF >output &&\n> +\ttest_line_count = 6 output &&\n> +\t! grep -v \"^foo.n\" output\n> +'\n> +\n> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (true)' '\n> +\tgit config --type=bool --get-regexp \"foo\\..*\" ON >output &&\n> +\ttest_line_count = 7 output &&\n> +\t! grep -v \"^foo.y\" output\n> +'\n> +\n> +test_expect_success '--get canonicalizes integer value_regex with --type=bool' '\n> +\techo true >expect &&\n> +\tgit config --type=bool --get foo.y2 1 >output &&\n> +\ttest_cmp expect output\n> +'\n> +\n> +test_expect_success '--type=bool with \"non-bool\" value_regex' '\n> +\techo true >expect &&\n> +\tgit config --type=bool --get foo.y4 \"t.*\" >output &&\n> +\ttest_cmp expect output &&\n> +\ttest_must_fail git config --type=bool --get foo.y4 \"T.*\" >output &&\n> +\ttest_must_be_empty output\n> +'\n> +\n>  test_expect_success 'setup simple config file' '\n>  \tq_to_tab >.git/config <<-\\EOF\n>  \t[a.b]\n"},{"id":"386701","messageId":"xmqqpnhl943d.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"0f48ab4fc344b3cc226d0a45d13530022208ff3e.1573670565.git.martin.agren@gmail.com","subject":"Re: [PATCH 7/8] builtin/config: warn if \"value_regex\" doesn't canonicalize as boolean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-21T05:43:34Z","receivedAt":"2019-11-21T05:43:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> With `--type=bool`, we try to canonicalize the \"value_regex\". If it\n> doesn't canonicalize, we continue and handle the \"value_regex\" as an\n> ordinary regex. This is deliberate -- we do not want to cause existing\n> scripts to fail.\n>\n> This does mean that users might be at risk of missing out on config\n> values depending on which representation they use in their config file:\n>\n> \t$ git config foo.bar on\n> \t$ git config foo.baz true\n> \t$ git config --type=bool --get-regex \"foo\\.*\" tru\n> \tfoo.baz true\n> \t$ # oops! missing foo.bar\n>\n> Let's start warning when the \"value_regex\" doesn't look like a boolean.\n> Document our intention of eventually requiring the canonicalization to\n> pass.\n\nWhile I actually think this warning is counter-productive, if we\nwere to do so, value-regex given for a bool-or-int type should also\nbe warned if it does not name a boolean value and is not an integer.\n\nWith the way you laid out the \"use enum to tell what kind of token\nvalue_regex argument has\" pattern, I think support for \"--type=int\" \nto normalize human-readable numbers would also fall out naturally,\nwhich is nice.\n"},{"id":"386742","messageId":"CAN0heSrn07_dSTFB3ru3UZ14rxZvRD0mEAh5AMBFEDMgLKFLtA@mail.gmail.com","threadId":"52260","inReplyTo":"xmqq4kyxakjs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-21T19:53:01Z","receivedAt":"2019-11-21T19:53:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 21 Nov 2019 at 06:02, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > This is a self-contained and fairly large chunk of code which will soon\n> > gain a few more lines. Prepare by extracting it into a separate\n> > function.\n> >\n> > This whole chunk is wrapped in \"if (regex_)\" -- rewrite it into an early\n> > return path in the new helper function to reduce indentation.\n>\n> It is not clear if regexp were cleared to NULL when !regex_ in the\n> original code, so if that were the case, this refactoring is a\n> worthy clean-up from that point of view, too.\n\nHmmm, this is something I added which makes it not a true refactoring\nas such. I don't even recall doing this, but it does make sure we always\nset this pointer to something sane. If we've already initialized this,\nwe'll risk leaking, but that should be better than running amok due to\nbailing out early here and leaving the pointer pointing \"somewhere\".\n\nThat said, this is the only function where we set this, and we're\ncalling this function at most once (directly from `cmd_config()`),\nso right now this NULL assignment is a no-op.\n\n> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> > ---\n> >  I copy the \"xmalloc(sizeof(regex_t))\" anti-pattern verbatim. We will\n> >  lose it in the next patch.\n>\n> It also spreads the use of file-scope global variables like\n> do_not_match and regexp, which also is existing anti-pattern that we\n> may want to fix by enclosing them in a struct and pass a pointer to\n> it around the callchain.  We can clean it up later.\n\nRight, in the next patch I collect them into a struct, but leave it\nfile-scope global. I didn't feel good adding another such global later\nin the series, so decided to take a smallish step towards the end goal\nyou outline...\n\n\nMartin\n"},{"id":"386743","messageId":"CAN0heSo3E0g_ZhHb64cisZQu9hfF31sQEyyZB4X_eRyiFtv4mQ@mail.gmail.com","threadId":"52260","inReplyTo":"xmqqy2w9951v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 4/8] builtin/config: collect \"value_regexp\" data in a struct","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-21T19:55:53Z","receivedAt":"2019-11-21T19:56:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 21 Nov 2019 at 06:22, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > `git config` can take an optional \"value_regexp\". Collect the\n> > `regex_t`-pointer and the `do_not_match` flag into a new `struct\n> > cmd_line_value`.\n>\n> A \"struct cmd_line_value\" sounded, to me at least during my first\n> reading, as if it is about all command line options, but that is not\n> at all what you meant to imply.  Is this only about the optional\n> value-regexp (if so perhaps calling it \"value_regexp_option\" would\n> have helped me avoid such a misunderstanding)?\n\nYes, that's right. Your suggested name is better. Thanks.\n\n> > Rather than signalling/judging presence of a regexp by the NULL-ness of\n> > the pointer, introduce a `mode` enum.\n>\n> OK.  Tangentially this makes readers wonder why the existing code\n> for key_regexp does not follow the same \"NULL-ness\" pattern but has\n> a separate use_key_regexp boolean.  It appears that the original\n> code is quite confused---it is totally outside the scope of this\n> series to clean it up and inject sanity into it though ;-)\n\nYeah, I considered doing such a cleanup, but opted to try and stay\nfocused.\n\n> >  static regex_t *key_regexp;\n> > -static regex_t *regexp;\n> > +static struct {\n> > +     enum { none, regexp } mode;\n>\n> We often use the same identifier for a struct and an instance of the\n> struct, taking advantage of the fact that they live in separate\n> namespaces, but lowercase enumerated values like 'regexp' that\n> collides with the field name (and possibly a variable name used\n> elsewhere) smells a bit too much.\n\nOk, thanks for sanity-checking.\n\n> > +     regex_t *regexp;\n> > +     int do_not_match; /* used with `regexp` */\n> > +} cmd_line_value;\n> >  static int show_keys;\n> >  static int omit_values;\n> >  static int use_key_regexp;\n>\n> > @@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n> >  static int handle_value_regex(const char *regex_)\n> >  {\n> >       if (!regex_) {\n> > -             regexp = NULL;\n> > +             cmd_line_value.mode = none;\n> >               return 0;\n>\n> Now we are back to relying on cmd_line_value.regexp staying to be\n> NULL after initialized, which is the state before the previous\n> patch.  If the end result is correct, then it is OK, I think, but\n> then the previous step shouldn't have added the NULL assignment here\n> in the first place.\n\nOk, noted.\n\nAs I wrote in my reply there, that made the whole thing not a 100%\nrefactoring anyway. I'll drop that one.\n\n> > +     cmd_line_value.mode = regexp;\n> > +\n> >       if (regex_[0] == '!') {\n> > -             do_not_match = 1;\n> > +             cmd_line_value.do_not_match = 1;\n> >               regex_++;\n> >       }\n> >\n> > -     regexp = (regex_t*)xmalloc(sizeof(regex_t));\n> > -     if (regcomp(regexp, regex_, REG_EXTENDED)) {\n> > +     cmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));\n> > +     if (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {\n> >               error(_(\"invalid pattern: %s\"), regex_);\n> > -             FREE_AND_NULL(regexp);\n> > +             FREE_AND_NULL(cmd_line_value.regexp);\n>\n> Hmph.  !regexp in old code should mean cmd_line_value.mode==regexp\n> in the new world order after this patch is applied, no?  Should we\n> be treaking the mode field here before we leave?  I think it should\n> not matter, but thought it wouldn't hurt to ask.\n>\n> In collect_config(), cmd_line_value.regexp is blindly passed to\n> regexec(3) as long as cmd_line_value.mode==regexp, so the invariant\n> \"when .mode is regexp, .regexp must be valid, or collect_config() would\n> never be called for such cmd_line_value\" is rather important to\n> avoid crashing ;-)\n\nGood point. No-one will be looking at the struct when we bail out here,\nbut we're just one missing \"if\" away from that changing... Might as well\ntry to leave things in a sane state to reduce the possibility of this\nbiting us in the future.\n\n> > @@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)\n> >               regfree(key_regexp);\n> >               free(key_regexp);\n> >       }\n> > -     if (regexp) {\n> > -             regfree(regexp);\n> > -             free(regexp);\n> > +     if (cmd_line_value.regexp) {\n> > +             regfree(cmd_line_value.regexp);\n> > +             free(cmd_line_value.regexp);\n>\n> Likewise.\n\nThanks.\n\n\nMartin\n"},{"id":"386744","messageId":"CAN0heSpGYi7WWT+o+qvyPEaJpbVahGxNhx_2z9d=OF+V2wmzBw@mail.gmail.com","threadId":"52260","inReplyTo":"xmqqpnhl943d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 7/8] builtin/config: warn if \"value_regex\" doesn't canonicalize as boolean","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-21T19:58:01Z","receivedAt":"2019-11-21T19:58:15Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 21 Nov 2019 at 06:43, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > With `--type=bool`, we try to canonicalize the \"value_regex\". If it\n> > doesn't canonicalize, we continue and handle the \"value_regex\" as an\n> > ordinary regex. This is deliberate -- we do not want to cause existing\n> > scripts to fail.\n> >\n> > This does mean that users might be at risk of missing out on config\n> > values depending on which representation they use in their config file:\n> >\n> >       $ git config foo.bar on\n> >       $ git config foo.baz true\n> >       $ git config --type=bool --get-regex \"foo\\.*\" tru\n> >       foo.baz true\n> >       $ # oops! missing foo.bar\n> >\n> > Let's start warning when the \"value_regex\" doesn't look like a boolean.\n> > Document our intention of eventually requiring the canonicalization to\n> > pass.\n>\n> While I actually think this warning is counter-productive, if we\n> were to do so, value-regex given for a bool-or-int type should also\n> be warned if it does not name a boolean value and is not an integer.\n\nOk, noted.\n\n> With the way you laid out the \"use enum to tell what kind of token\n> value_regex argument has\" pattern, I think support for \"--type=int\"\n> to normalize human-readable numbers would also fall out naturally,\n> which is nice.\n\nWell, that's what I was hoping for at least... ;-)\n\n\nMartin\n"},{"id":"386799","messageId":"xmqqpnhk4e3x.fsf@gitster-ct.c.googlers.com","threadId":"52260","inReplyTo":"CAN0heSo3E0g_ZhHb64cisZQu9hfF31sQEyyZB4X_eRyiFtv4mQ@mail.gmail.com","subject":"Re: [PATCH 4/8] builtin/config: collect \"value_regexp\" data in a struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T06:30:42Z","receivedAt":"2019-11-22T06:30:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n>> > +static struct {\n>> > +     enum { none, regexp } mode;\n>>\n>> We often use the same identifier for a struct and an instance of the\n>> struct, taking advantage of the fact that they live in separate\n>> namespaces, but lowercase enumerated values like 'regexp' that\n>> collides with the field name (and possibly a variable name used\n>> elsewhere) smells a bit too much.\n>\n> Ok, thanks for sanity-checking.\n>\n>> > +     regex_t *regexp;\n>> > +     int do_not_match; /* used with `regexp` */\n>> > +} cmd_line_value;\n\nI _might_ want to take this back.  A pattern that uses the \"mode\" to\nswitch among the possibilities in a union, i.e.\n\n\tstruct {\n\t\tenum {\n\t\t\t<something>_none,\n\t\t\t<something>_regexp,\n\t\t\t<something>_bool,\n\t\t\t<something>_int,\n\t\t} mode;\n                union {\n\t\t\t<type-used-when-it-is-regexp> regexp;\n\t\t\t<type-used-when-it-is-bool> bool;\n\t\t\t<type-used-when-it-is-int> int;\n\t\t} u;\n\t};\n\nmay not be too bad.  So I do not strongly mind the lowercase.\n\nBut I still do mind an overly bland names for identifiers in an\nenum, as enum is not quite a type on its own ('regexp' in one enum\nmay collide with the same identifier in another enum)..\n\nThanks.\n"}]}