[PATCH 0/7] config: add --literal-value option

[PATCH 0/7] config: add --literal-value option

[Derrick Stolee via GitGitGadget]

As reported [1], 'git maintenance unregister' fails when a repository is
located in a directory with regex glob characters.

[1] 
https://lore.kernel.org/git/2c2db228-069a-947d-8446-89f4d3f6181a@gmail.com/T/#mb96fa4187a0d6aeda097cd95804a8aafc0273022

The discussed solution was to modify 'git config' to specify that the
'value_regex' argument should be treated as an exact string match. This is
the primary change in this series, with an additional patch at the end to
make 'git maintenance [un]register' use this option, when necessary.

Thanks, -Stolee

Derrick Stolee (7):
  t1300: test "set all" mode with value_regex
  t1300: add test for --replace-all with value_regex
  config: convert multi_replace to flags
  config: add --literal-value option, un-implemented
  config: plumb --literal-value into config API
  config: implement --literal-value with --get*
  maintenance: use 'git config --literal-value'

 Documentation/git-config.txt |  20 ++++--
 builtin/branch.c             |   4 +-
 builtin/config.c             |  50 ++++++++++---
 builtin/gc.c                 |   5 +-
 builtin/remote.c             |   8 ++-
 config.c                     |  29 ++++----
 config.h                     |  24 +++++--
 t/t1300-config.sh            | 136 +++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh       |  12 ++++
 9 files changed, 248 insertions(+), 40 deletions(-)


base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-796%2Fderrickstolee%2Fmaintenance%2Fconfig-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-796/derrickstolee/maintenance/config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/796
-- 
gitgitgadget

[PATCH 1/7] t1300: test "set all" mode with value_regex

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Without additional modifiers, 'git config' attempts to set a single
value in the .git/config file. When the value_regex parameter is
supplied, this command behaves in a non-trivial manner.

Consider 'git config key value value_regex'. The expected behavior
is as follows:

1. If there are multiple existing values that match 'value_regex',
   then the command fails. Users should use --replace-all instead.

2. If there is one existing value that matches 'value_regex', then
   the new config has one entry where 'key=value'.

3. If there is no existing values match 'value_regex', then the
   'key=value' pair is appended, making this 'key' a multi-valued
   config setting.

Add a test that demonstrates these options.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..ef56b08070 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1914,4 +1914,39 @@ test_expect_success '--replace-all does not invent newlines' '
 	test_cmp expect .git/config
 '
 
+test_expect_success 'set all config with value_regex' '
+	q_to_tab >initial <<-\EOF &&
+	[abc]
+	Qkey = one
+	EOF
+
+	cp initial .git/config &&
+	git config abc.key two a+ &&
+	q_to_tab >expect <<-\EOF &&
+	[abc]
+	Qkey = one
+	Qkey = two
+	EOF
+	test_cmp expect .git/config &&
+
+	test_must_fail git config abc.key three o+ 2>err &&
+	test_i18ngrep "has multiple values" err &&
+	git config abc.key three a+ &&
+	q_to_tab >expect <<-\EOF &&
+	[abc]
+	Qkey = one
+	Qkey = two
+	Qkey = three
+	EOF
+	test_cmp expect .git/config &&
+
+	cp initial .git/config &&
+	git config abc.key three o+ &&
+	q_to_tab >expect <<-\EOF &&
+	[abc]
+	Qkey = three
+	EOF
+	test_cmp expect .git/config
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_expect_success 'set all config with value_regex' '
> +	q_to_tab >initial <<-\EOF &&
> +	[abc]
> +	Qkey = one
> +	EOF
> +
> +	cp initial .git/config &&

Not a new problem with this patch, but does the above pattern
introduce potential problems?  I am wondering if overwriting the
config file with a little piece that has only the stuff the test is
interested in, while wiping the parts that may be essential for
repository integrity (e.g. "extensions.objectFormat"), is OK in the
long run (brian cc'ed for his sha256 work).  There also are
autodetected crlf settings etc. that are in the .git/config when a
test repository is created, and we probably would want to keep them
intact.

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On 2020-11-19 at 22:24:33, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > +test_expect_success 'set all config with value_regex' '
> > +	q_to_tab >initial <<-\EOF &&
> > +	[abc]
> > +	Qkey = one
> > +	EOF
> > +
> > +	cp initial .git/config &&
> 
> Not a new problem with this patch, but does the above pattern
> introduce potential problems?  I am wondering if overwriting the
> config file with a little piece that has only the stuff the test is
> interested in, while wiping the parts that may be essential for
> repository integrity (e.g. "extensions.objectFormat"), is OK in the
> long run (brian cc'ed for his sha256 work).  There also are
> autodetected crlf settings etc. that are in the .git/config when a
> test repository is created, and we probably would want to keep them
> intact.

I haven't looked at the code, but if you're just using git config in a
test, then overwriting the config file shouldn't be a problem with
SHA-256.  If you're trying to read or write objects or the index, then
that's definitely a problem, and you'll definitely notice exciting
failures if you do that.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2020-11-19 at 22:24:33, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>> > +test_expect_success 'set all config with value_regex' '
>> > +	q_to_tab >initial <<-\EOF &&
>> > +	[abc]
>> > +	Qkey = one
>> > +	EOF
>> > +
>> > +	cp initial .git/config &&
>> 
>> Not a new problem with this patch, but does the above pattern
>> introduce potential problems?  I am wondering if overwriting the
>> config file with a little piece that has only the stuff the test is
>> interested in, while wiping the parts that may be essential for
>> repository integrity (e.g. "extensions.objectFormat"), is OK in the
>> long run (brian cc'ed for his sha256 work).  There also are
>> autodetected crlf settings etc. that are in the .git/config when a
>> test repository is created, and we probably would want to keep them
>> intact.
>
> I haven't looked at the code, but if you're just using git config in a
> test, then overwriting the config file shouldn't be a problem with
> SHA-256.  If you're trying to read or write objects or the index, then
> that's definitely a problem, and you'll definitely notice exciting
> failures if you do that.

Yes, that is exactly what worries me.

And I was trolling for ideas from those on cc: list to come up with
a better convention to test the behaviour of "git config".  I think
taking "git config --list [--file FILE]" before and after the ops of
interest and basing the good/bad decision on the difference would be
a saner approach.  It obviously relies on "git config --list" to be
working correctly, which the current approach that uses hardcoded
"initial" state and expects result that is byte-for-byte identical
to a hardcoded file contents after the ops does not have to rely on,
but at the same time, the current approach assumes too much on the
actual file format (e.g. no user or script would care how "key =
val" line is indented, but the current approach insists that they
are indented exactly with one tab), so overall it may even be an
improvement.

Thanks.

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Jeff King]

On Thu, Nov 19, 2020 at 02:24:33PM -0800, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > +test_expect_success 'set all config with value_regex' '
> > +	q_to_tab >initial <<-\EOF &&
> > +	[abc]
> > +	Qkey = one
> > +	EOF
> > +
> > +	cp initial .git/config &&
> 
> Not a new problem with this patch, but does the above pattern
> introduce potential problems?  I am wondering if overwriting the
> config file with a little piece that has only the stuff the test is
> interested in, while wiping the parts that may be essential for
> repository integrity (e.g. "extensions.objectFormat"), is OK in the
> long run (brian cc'ed for his sha256 work).  There also are
> autodetected crlf settings etc. that are in the .git/config when a
> test repository is created, and we probably would want to keep them
> intact.

t1300 is full of this kind of junk. Several years ago, while working on
some of the repositoryformatversion code, I noticed that we will accept
a repository that does not have core.repositoryformatversion set at all,
nor even has a .git/config present!

It's easy to fix in the code, but it causes failures all over t1300. So
then I started converting t1300 to use "config --file" (which
almost certainly didn't exist back when most of those tests were
originally written).  I don't remember how or why it got hairy, but it
was enough that I eventually dropped it (unlike many of my other stale
topics, I don't think I've even kept rebasing it forward as a WIP).

Possibly I was concerned that people in the wild were relying on a blank
or missing config being the same as repositoryformatversion=0. That will
definitely stop working in a sha256 world anyway, though, because
they'll need the objectFormat extension.

So that got a bit off-track, but I think:

  - t1300 already is very much like this, so it's not a new thing

  - but I would be happy not to see it go further in that direction,
    even if it means inconsistency with the rest of the script

-Peff

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So that got a bit off-track, but I think:
>
>   - t1300 already is very much like this, so it's not a new thing
>
>   - but I would be happy not to see it go further in that direction,
>     even if it means inconsistency with the rest of the script

Yeah, I especially like the latter half ;-)

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

On 2020-11-20 at 18:39:03, Jeff King wrote:
> t1300 is full of this kind of junk. Several years ago, while working on
> some of the repositoryformatversion code, I noticed that we will accept
> a repository that does not have core.repositoryformatversion set at all,
> nor even has a .git/config present!

Yup.  We test this in t3200 as well.  I don't love it, but it exists.

> It's easy to fix in the code, but it causes failures all over t1300. So
> then I started converting t1300 to use "config --file" (which
> almost certainly didn't exist back when most of those tests were
> originally written).  I don't remember how or why it got hairy, but it
> was enough that I eventually dropped it (unlike many of my other stale
> topics, I don't think I've even kept rebasing it forward as a WIP).
> 
> Possibly I was concerned that people in the wild were relying on a blank
> or missing config being the same as repositoryformatversion=0. That will
> definitely stop working in a sha256 world anyway, though, because
> they'll need the objectFormat extension.

Which is exactly why that test in t3200 has a SHA1 prerequisite.  I'm
sure we'll hear someone complain about the fact that SHA-256
repositories have to have a config file, but I'm fine with us not
supporting it.

I should point out that lacking a config file also only works on Unix
systems with a POSIX file system (including case-sensitive macOS), since
otherwise core.ignorecase (and core.symlinks, if appropriate) aren't set
correctly.  It also doesn't work for bare repositories on any OS.  So
hopefully the number of people doing this is quite small.

> So that got a bit off-track, but I think:
> 
>   - t1300 already is very much like this, so it's not a new thing
> 
>   - but I would be happy not to see it go further in that direction,
>     even if it means inconsistency with the rest of the script

I agree we shouldn't make things worse.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> So that got a bit off-track, but I think:
>> 
>>   - t1300 already is very much like this, so it's not a new thing
>> 
>>   - but I would be happy not to see it go further in that direction,
>>     even if it means inconsistency with the rest of the script
>
> I agree we shouldn't make things worse.

I started looking at early parts of t1300 and here is how far I
managed to get before I can no longer keep staring the existing
tests without vomitting.

I am reasonably happy with the "let's keep the vanilla untouched one
in .git/config-initial, refrain from using [core] and other sections
that MUST be in the initial configuration for testing, and use a
wrapper that reads expected addition to the initial one from the
standard input for validation" approach I came up with, but I am not
happy with the name 'compare_expect'; 'validate_config_result' might
be a better name.

In any case, the reason I am sending this out early is if people
find this approach to clean things up a sensible one.  If we can
find concensus, perhaps I (or somebody else---hint, hint) can find
time to do the #leftoverbits following the approach after the
ds/config-literal-value and ds/maintenance-part-3 topics graduate
to 'master'.



 t/t1300-config.sh | 139 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 71 insertions(+), 68 deletions(-)

diff --git c/t/t1300-config.sh w/t/t1300-config.sh
index df13afaffd..c33520d7fa 100755
--- c/t/t1300-config.sh
+++ w/t/t1300-config.sh
@@ -7,80 +7,84 @@ test_description='Test git config in different settings'
 
 . ./test-lib.sh
 
-test_expect_success 'clear default config' '
-	rm -f .git/config
+test_expect_success 'save away default config' '
+	cp .git/config .git/config-initial
 '
 
-cat > expect << EOF
-[core]
-	penguin = little blue
-EOF
-test_expect_success 'initial' '
-	git config core.penguin "little blue" &&
+compare_expect () {
+	{
+		cat .git/config-initial &&
+		sed -e 's/^[|]//'
+	} >expect &&
 	test_cmp expect .git/config
+}
+
+test_expect_success 'initial' '
+	git config configtest.penguin "little blue" &&
+	compare_expect <<-\EOF
+	[configtest]
+	|	penguin = little blue
+	EOF
 '
 
-cat > expect << EOF
-[core]
-	penguin = little blue
-	Movie = BadPhysics
-EOF
 test_expect_success 'mixed case' '
-	git config Core.Movie BadPhysics &&
-	test_cmp expect .git/config
+	git config ConfigTest.Movie BadPhysics &&
+	compare_expect <<-\EOF
+	[configtest]
+	|	penguin = little blue
+	|	Movie = BadPhysics
+	EOF
 '
 
-cat > expect << EOF
-[core]
-	penguin = little blue
-	Movie = BadPhysics
-[Cores]
-	WhatEver = Second
-EOF
 test_expect_success 'similar section' '
-	git config Cores.WhatEver Second &&
-	test_cmp expect .git/config
+	git config ConfigTests.WhatEver Second &&
+	compare_expect <<-\EOF
+	[configtest]
+	|	penguin = little blue
+	|	Movie = BadPhysics
+	[ConfigTests]
+	|	WhatEver = Second
+	EOF
 '
 
-cat > expect << EOF
-[core]
-	penguin = little blue
-	Movie = BadPhysics
-	UPPERCASE = true
-[Cores]
-	WhatEver = Second
-EOF
 test_expect_success 'uppercase section' '
-	git config CORE.UPPERCASE true &&
-	test_cmp expect .git/config
+	git config CONFIGTEST.UPPERCASE true &&
+	compare_expect <<-\EOF
+	[configtest]
+	|	penguin = little blue
+	|	Movie = BadPhysics
+	|	UPPERCASE = true
+	[ConfigTests]
+	|	WhatEver = Second
+	EOF
 '
 
 test_expect_success 'replace with non-match' '
-	git config core.penguin kingpin !blue
+	git config configtest.penguin kingpin !blue
 '
 
 test_expect_success 'replace with non-match (actually matching)' '
-	git config core.penguin "very blue" !kingpin
+	git config configtest.penguin "very blue" !kingpin
 '
 
-cat > expect << EOF
-[core]
-	penguin = very blue
-	Movie = BadPhysics
-	UPPERCASE = true
-	penguin = kingpin
-[Cores]
-	WhatEver = Second
-EOF
-
-test_expect_success 'non-match result' 'test_cmp expect .git/config'
+test_expect_success 'non-match result' '
+	compare_expect <<-\EOF
+	[configtest]
+	|	penguin = very blue
+	|	Movie = BadPhysics
+	|	UPPERCASE = true
+	|	penguin = kingpin
+	[ConfigTests]
+	|	WhatEver = Second
+	EOF
+'
 
 test_expect_success 'find mixed-case key by canonical name' '
-	test_cmp_config Second cores.whatever
+	test_cmp_config Second configtests.whatever
 '
 
 test_expect_success 'find mixed-case key by non-canonical name' '
-	test_cmp_config Second CoReS.WhAtEvEr
+	test_cmp_config Second CoNfIgTeSts.WhAtEvEr
 '
 
 test_expect_success 'subsections are not canonicalized by git-config' '
@@ -94,28 +98,27 @@ test_expect_success 'subsections are not canonicalized by git-config' '
 	test_cmp_config two section.SubSection.key
 '
 
-cat > .git/config <<\EOF
-[alpha]
-bar = foo
-[beta]
-baz = multiple \
-lines
-foo = bar
-EOF
-
 test_expect_success 'unset with cont. lines' '
-	git config --unset beta.baz
+	{
+		cat .git/config-initial &&
+		cat <<-\EOF
+		[alpha]
+		bar = foo
+		[beta]
+		baz = multiple \
+		lines
+		foo = bar
+		EOF
+	} >.git/config &&
+	git config --unset beta.baz &&
+	compare_expect <<-\EOF
+	[alpha]
+	bar = foo
+	[beta]
+	foo = bar
+	EOF
 '
 
-cat > expect <<\EOF
-[alpha]
-bar = foo
-[beta]
-foo = bar
-EOF
-
-test_expect_success 'unset with cont. lines is correct' 'test_cmp expect .git/config'
-
 cat > .git/config << EOF
 [beta] ; silly comment # another comment
 noIndent= sillyValue ; 'nother silly comment

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Jeff King]

On Sat, Nov 21, 2020 at 07:31:14PM -0800, Junio C Hamano wrote:

> >> So that got a bit off-track, but I think:
> >> 
> >>   - t1300 already is very much like this, so it's not a new thing
> >> 
> >>   - but I would be happy not to see it go further in that direction,
> >>     even if it means inconsistency with the rest of the script
> >
> > I agree we shouldn't make things worse.
> 
> I started looking at early parts of t1300 and here is how far I
> managed to get before I can no longer keep staring the existing
> tests without vomitting.

I think my similar gastric reaction is what caused me to stop looking at
it long ago. But it may also have been the test brian mentioned that
explicitly checks that this case works (and for which he had to set SHA1
prereq).

> I am reasonably happy with the "let's keep the vanilla untouched one
> in .git/config-initial, refrain from using [core] and other sections
> that MUST be in the initial configuration for testing, and use a
> wrapper that reads expected addition to the initial one from the
> standard input for validation" approach I came up with, but I am not
> happy with the name 'compare_expect'; 'validate_config_result' might
> be a better name.

IMHO this is worse than just using "config --file" in most of the tests.
It's more steps to remember to deal with. And most tests do not care at
all what the source file is. There are a few that check the order of
lookup with respect to system and user files, but they could probably be
run what non-destructive changes.

That said, most of the effort is in the tedium of switching each
individual test. I am happy for whoever volunteers to do that work to
have the final say in the approach.

-Peff

Re: [PATCH 1/7] t1300: test "set all" mode with value_regex

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> I am reasonably happy with the "let's keep the vanilla untouched one
>> in .git/config-initial, refrain from using [core] and other sections
>> that MUST be in the initial configuration for testing, and use a
>> wrapper that reads expected addition to the initial one from the
>> standard input for validation" approach I came up with, but I am not
>> happy with the name 'compare_expect'; 'validate_config_result' might
>> be a better name.
>
> IMHO this is worse than just using "config --file" in most of the tests.
> It's more steps to remember to deal with. And most tests do not care at
> all what the source file is.

"Most tests do not care" only indicates the lack of test coverage.

Knowing the implementation, it probably is OK to assume that things
would work fine as long as "--file <file>" works correctly, though
;-) 

Not having to keep the minimum parts of the real configuration file,
and being able to use a throw-away file for each test, certainly
makes things cleaner.

> That said, most of the effort is in the tedium of switching each
> individual test. I am happy for whoever volunteers to do that work to
> have the final say in the approach.

Yup.

[PATCH 2/7] t1300: add test for --replace-all with value_regex

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The --replace-all option was added in 4ddba79d (git-config-set: add more
options) but was not tested along with the 'value_regex' parameter.
Since we will be updating this option to optionally treat 'value_regex'
as a fixed string, let's add a test here that documents the current
behavior.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index ef56b08070..74e0f84c0a 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1949,4 +1949,20 @@ test_expect_success 'set all config with value_regex' '
 	test_cmp expect .git/config
 '
 
+test_expect_success '--replace-all and value_regex' '
+	q_to_tab >.git/config <<-\EOF &&
+	[abc]
+	Qkey = one
+	Qkey = two
+	Qkey = three
+	EOF
+	q_to_tab >expect <<-\EOF &&
+	[abc]
+	Qkey = four
+	Qkey = three
+	EOF
+	git config --replace-all abc.key four "o+" &&
+	test_cmp expect .git/config
+'
+
 test_done
-- 
gitgitgadget

[PATCH 3/7] config: convert multi_replace to flags

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

We will extend the flexibility of the config API. Before doing so, let's
take an existing 'int multi_replace' parameter and replace it with a new
'int flags' parameter that can take multiple options as a bit field.

Update all callers that specified multi_replace to now specify the
CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
documentation of git_config_set_multivar_in_file() including a clear
labeling of its arguments. Other config API methods in config.h do not
require a change to their prototypes, but they have an equivalent update
to their implementations.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/branch.c |  4 ++--
 builtin/config.c |  6 ++++--
 builtin/remote.c |  8 +++++---
 config.c         | 24 ++++++++++++------------
 config.h         | 17 +++++++++++++----
 5 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..5ce3844d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.merge", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		if (filter.kind != FILTER_REFS_BRANCHES)
diff --git a/builtin/config.c b/builtin/config.c
index 5e39f61885..e7c7f3d455 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -823,7 +823,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 1);
+							      argv[0], value, argv[2],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -859,7 +860,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], NULL, argv[1], 1);
+							      argv[0], NULL, argv[1],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/builtin/remote.c b/builtin/remote.c
index c8240e9fcd..29b1652975 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -708,7 +708,7 @@ static int mv(int argc, const char **argv)
 
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
-	git_config_set_multivar(buf.buf, NULL, NULL, 1);
+	git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
 	for (i = 0; i < oldremote->fetch.raw_nr; i++) {
 		char *ptr;
@@ -1485,7 +1485,8 @@ static int update(int argc, const char **argv)
 
 static int remove_all_fetch_refspecs(const char *key)
 {
-	return git_config_set_multivar_gently(key, NULL, NULL, 1);
+	return git_config_set_multivar_gently(key, NULL, NULL,
+					      CONFIG_FLAGS_MULTI_REPLACE);
 }
 
 static void add_branches(struct remote *remote, const char **branches,
@@ -1674,7 +1675,8 @@ static int set_url(int argc, const char **argv)
 	if (!delete_mode)
 		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
 	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+		git_config_set_multivar(name_buf.buf, NULL, oldurl,
+					CONFIG_FLAGS_MULTI_REPLACE);
 out:
 	strbuf_release(&name_buf);
 	return 0;
diff --git a/config.c b/config.c
index 2b79fe76ad..4841c68a91 100644
--- a/config.c
+++ b/config.c
@@ -2716,9 +2716,9 @@ void git_config_set(const char *key, const char *value)
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
  * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
  *     (only add a new one)
- * if multi_replace==0, nothing, or only one matching key/value is replaced,
- *     else all matching key/values (regardless how many) are removed,
- *     before the new pair is written.
+ * if (flags & CONFIG_FLAGS_MULTI_REPLACE) == 0, at most one matching
+ *     key/value is replaced, else all matching key/values (regardless
+ *     how many) are removed, before the new pair is written.
  *
  * Returns 0 on success.
  *
@@ -2739,7 +2739,7 @@ void git_config_set(const char *key, const char *value)
 int git_config_set_multivar_in_file_gently(const char *config_filename,
 					   const char *key, const char *value,
 					   const char *value_regex,
-					   int multi_replace)
+					   int flags)
 {
 	int fd = -1, in_fd = -1;
 	int ret;
@@ -2756,7 +2756,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	if (ret)
 		goto out_free;
 
-	store.multi_replace = multi_replace;
+	store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0;
 
 	if (!config_filename)
 		config_filename = filename_buf = git_pathdup("config");
@@ -2845,7 +2845,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 		/* if nothing to unset, or too many matches, error out */
 		if ((store.seen_nr == 0 && value == NULL) ||
-		    (store.seen_nr > 1 && multi_replace == 0)) {
+		    (store.seen_nr > 1 && !store.multi_replace)) {
 			ret = CONFIG_NOTHING_SET;
 			goto out_free;
 		}
@@ -2984,10 +2984,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key, const char *value,
-				     const char *value_regex, int multi_replace)
+				     const char *value_regex, int flags)
 {
 	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
-						    value_regex, multi_replace))
+						    value_regex, flags))
 		return;
 	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
@@ -2996,17 +2996,17 @@ void git_config_set_multivar_in_file(const char *config_filename,
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
-				   const char *value_regex, int multi_replace)
+				   const char *value_regex, int flags)
 {
 	return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex,
-						      multi_replace);
+						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
-			     const char *value_regex, int multi_replace)
+			     const char *value_regex, int flags)
 {
 	git_config_set_multivar_in_file(NULL, key, value, value_regex,
-					multi_replace);
+					flags);
 }
 
 static int section_name_match (const char *buf, const char *name)
diff --git a/config.h b/config.h
index 060874488f..266eb22e46 100644
--- a/config.h
+++ b/config.h
@@ -70,6 +70,13 @@ enum config_event_t {
 	CONFIG_EVENT_ERROR
 };
 
+/*
+ * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
+ * are removed before a new pair is written. If the flag is not present,
+ * then set operations replace only the first match.
+ */
+#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
+
 /*
  * The parser event function (if not NULL) is called with the event type and
  * the begin/end offsets of the parsed elements.
@@ -276,13 +283,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
  * - the value regex, as a string. It will disregard key/value pairs where value
  *   does not match.
  *
- * - a multi_replace value, as an int. If value is equal to zero, nothing or only
- *   one matching key/value is replaced, else all matching key/values (regardless
- *   how many) are removed, before the new pair is written.
+ * - a flags value with bits corresponding to the CONFIG_FLAG_* macros.
  *
  * It returns 0 on success.
  */
-void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+void git_config_set_multivar_in_file(const char *config_filename,
+				     const char *key,
+				     const char *value,
+				     const char *value_regex,
+				     int flags);
 
 /**
  * rename or remove sections in the config file
-- 
gitgitgadget

Re: [PATCH 3/7] config: convert multi_replace to flags

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> We will extend the flexibility of the config API. Before doing so, let's
> take an existing 'int multi_replace' parameter and replace it with a new
> 'int flags' parameter that can take multiple options as a bit field.

Nice.  I was afraid we may have to _add_ a new parameter, but here,
we can turn an existing bool into a flags word, which is great.  But
please use "unsigned" for such flags, just out of inertia/convention.

> @@ -70,6 +70,13 @@ enum config_event_t {
>  	CONFIG_EVENT_ERROR
>  };
>  
> +/*
> + * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
> + * are removed before a new pair is written. If the flag is not present,
> + * then set operations replace only the first match.
> + */
> +#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
> +

The description is clear, but how far is this comment from the
actual parameter definition where "flag" is used?  What I am trying
to get at is if it is clear enough to the readers, when we say "when
... is specified", where they can specify it.  If it is not clear,
perhaps we want to start the above comment like so:

	/*
	 * These are bits in the flags word to be given to functions
	 * like git_config_set_multivar_in_file(), etc.
	 *
	 * When CONFIG_FLAGS_MULTI_REPLACE is specified, ...


> @@ -276,13 +283,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
>   * - the value regex, as a string. It will disregard key/value pairs where value
>   *   does not match.
>   *
> - * - a multi_replace value, as an int. If value is equal to zero, nothing or only
> - *   one matching key/value is replaced, else all matching key/values (regardless
> - *   how many) are removed, before the new pair is written.
> + * - a flags value with bits corresponding to the CONFIG_FLAG_* macros.

This side is good---it tells readers what the bits in the flags word
are called (i.e. they can look for CONFIG_FLAGS_ in the file).

>   *
>   * It returns 0 on success.
>   */
> -void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
> +void git_config_set_multivar_in_file(const char *config_filename,
> +				     const char *key,
> +				     const char *value,
> +				     const char *value_regex,
> +				     int flags);
>  
>  /**
>   * rename or remove sections in the config file

[PATCH 4/7] config: add --literal-value option, un-implemented

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git config' builtin takes a 'value_regex' parameter for several
actions. This can cause confusion when expecting exact value matches
instead of regex matches, especially when the input string contains glob
characters. While callers can escape the patterns themselves, it would
be more friendly to allow an argument to disable the pattern matching in
favor of an exact string match.

Add a new '--literal-value' option that does not currently change the
behavior. The implementation will follow for each appropriate action.
For now, check and test that --literal-value will abort the command when
included with an incompatible action.

The name '--literal-value' was chosen over something simpler like
'--literal' because some commands allow regular expressions on the
key in addition to the value.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-config.txt | 20 +++++++++++++-------
 builtin/config.c             | 17 +++++++++++++++++
 t/t1300-config.sh            | 13 +++++++++++++
 3 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7573160f21..e244dd73f0 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,15 +9,15 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] [--literal-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [--type=<type>] --add name value
-'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [--literal-value] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] [--name-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] --unset name [value_regex]
-'git config' [<file-option>] --unset-all name [value_regex]
+'git config' [<file-option>] [--literal-value] --unset name [value_regex]
+'git config' [<file-option>] [--literal-value] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
@@ -165,6 +165,12 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
+--literal-value::
+	When used with the `value_regex` argument, treat `value_regex` as
+	an exact string instead of a regular expression. This will restrict
+	the name/value pairs that are matched to only those where the value
+	is exactly equal to the `value_regex`.
+
 --type <type>::
   'git config' will ensure that any input or output is valid under the given
   type constraint(s), and will canonicalize outgoing values in `<type>`'s
diff --git a/builtin/config.c b/builtin/config.c
index e7c7f3d455..ad6c695737 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
 static int show_scope;
+static int literal;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -52,6 +53,16 @@ static int show_scope;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+#define ACTION_LITERAL_ALLOWED (\
+	ACTION_GET |\
+	ACTION_GET_ALL |\
+	ACTION_GET_REGEXP |\
+	ACTION_REPLACE_ALL |\
+	ACTION_UNSET |\
+	ACTION_UNSET_ALL |\
+	ACTION_SET_ALL\
+)
+
 /*
  * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
  * one line of output and which should therefore be paged.
@@ -141,6 +152,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
+	OPT_BOOL(0, "literal-value", &literal, N_("use literal equality when matching values")),
 	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
@@ -745,6 +757,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
+		error(_("--literal only applies with 'value_regex'"));
+		usage_builtin_config();
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 74e0f84c0a..73f5ca4361 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1965,4 +1965,17 @@ test_expect_success '--replace-all and value_regex' '
 	test_cmp expect .git/config
 '
 
+test_expect_success 'refuse --literal-value for incompatible actions' '
+	git config dev.null bogus &&
+	test_must_fail git config --literal-value --add dev.null bogus &&
+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --literal-value --rename-section dev null &&
+	test_must_fail git config --literal-value --remove-section dev &&
+	test_must_fail git config --literal-value --list &&
+	test_must_fail git config --literal-value --get-color dev.null &&
+	test_must_fail git config --literal-value --get-colorbool dev.null &&
+	test_must_fail git config --literal-value --edit
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 4/7] config: add --literal-value option, un-implemented

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +--literal-value::
> +	When used with the `value_regex` argument, treat `value_regex` as
> +	an exact string instead of a regular expression. This will restrict
> +	the name/value pairs that are matched to only those where the value
> +	is exactly equal to the `value_regex`.

Makes sense.

>  --type <type>::
>    'git config' will ensure that any input or output is valid under the given
>    type constraint(s), and will canonicalize outgoing values in `<type>`'s
> diff --git a/builtin/config.c b/builtin/config.c
> index e7c7f3d455..ad6c695737 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
>  static struct config_options config_options;
>  static int show_origin;
>  static int show_scope;
> +static int literal;
>  
>  #define ACTION_GET (1<<0)
>  #define ACTION_GET_ALL (1<<1)
> @@ -52,6 +53,16 @@ static int show_scope;
>  #define ACTION_GET_COLORBOOL (1<<14)
>  #define ACTION_GET_URLMATCH (1<<15)
>  
> +#define ACTION_LITERAL_ALLOWED (\
> +	ACTION_GET |\
> +	ACTION_GET_ALL |\
> +	ACTION_GET_REGEXP |\
> +	ACTION_REPLACE_ALL |\
> +	ACTION_UNSET |\
> +	ACTION_UNSET_ALL |\
> +	ACTION_SET_ALL\
> +)

I am not sure about this, though.

Even if an action can potentially take value_regex, e.g.

    git config --get name [value_regex]

should we allow litral when value_regex is not given?  IOW

    $ git config --get --literal-value vari.able v2.26.0+next

may make sense, but shouldn't

    $ git config --get --literal-value vari.able

be an error?

To put it differently, I think the macro being defined is not
ACTION_LITERAL_ALLOWED, but ACTION_VALUE_REGEX_ALLOWED. i.e. list of
actions that can potentially take value_regex.  A command line that
gives value_regex to an action that is not listed there is an error.

> +	if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
> +		error(_("--literal only applies with 'value_regex'"));
> +		usage_builtin_config();
> +	}

This check is not incorrect per-se, because an action that never
takes value_regex is by definition incompatible with the literal
value option.  But it does not flag it as an error to ask a
value_regex to be treated as a literal string when there is no
value_regex given.

Having said that, I think it is OK, at least for now, to leave such
an error undetected---it falls into the "if it hurts, don't do it"
category.

Thanks.

>  	if (actions & PAGING_ACTIONS)
>  		setup_auto_pager("config", 1);
>  
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 74e0f84c0a..73f5ca4361 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1965,4 +1965,17 @@ test_expect_success '--replace-all and value_regex' '
>  	test_cmp expect .git/config
>  '
>  
> +test_expect_success 'refuse --literal-value for incompatible actions' '
> +	git config dev.null bogus &&
> +	test_must_fail git config --literal-value --add dev.null bogus &&
> +	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --literal-value --rename-section dev null &&
> +	test_must_fail git config --literal-value --remove-section dev &&
> +	test_must_fail git config --literal-value --list &&
> +	test_must_fail git config --literal-value --get-color dev.null &&
> +	test_must_fail git config --literal-value --get-colorbool dev.null &&
> +	test_must_fail git config --literal-value --edit
> +'
> +
>  test_done

Re: [PATCH 4/7] config: add --literal-value option, un-implemented

[Martin Ågren]

On Thu, 19 Nov 2020 at 16:55, Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> +       OPT_BOOL(0, "literal-value", &literal, N_("use literal equality when matching values")),

> +       if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
> +               error(_("--literal only applies with 'value_regex'"));

s/literal/&-value/

> +               usage_builtin_config();
> +       }


Martin

[PATCH 5/7] config: plumb --literal-value into config API

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The git_config_set_multivar_in_file_gently() and related methods now
take a 'flags' bitfield, so add a new bit representing the
--literal-value option from 'git config'. This alters the purpose of the
value_regex parameter to be an exact string match. This requires some
initialization changes in git_config_set_multivar_in_file_gently() and a
new strcmp() call in the matches() method.

The new CONFIG_FLAGS_LITERAL_VALUE flag is initialized in
builtin/config.c based on the --literal-value option, and that needs to
be updated in several callers.

This patch only affects some of the modes of 'git config', and the rest
will be completed in the next change.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 16 +++++++++++-----
 config.c          |  5 +++++
 config.h          |  7 +++++++
 t/t1300-config.sh | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index ad6c695737..6ab5a0d68f 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -626,6 +626,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
 	char *value;
+	int flags = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -761,6 +762,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		error(_("--literal only applies with 'value_regex'"));
 		usage_builtin_config();
 	}
+	if (literal)
+		flags = CONFIG_FLAGS_LITERAL_VALUE;
 
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
@@ -823,7 +826,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 0);
+							      argv[0], value, argv[2],
+							      flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
@@ -832,7 +836,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value,
-							      CONFIG_REGEX_NONE, 0);
+							      CONFIG_REGEX_NONE,
+							      flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
@@ -841,7 +846,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value, argv[2],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -868,7 +873,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file_gently(given_config_source.file,
-								      argv[0], NULL, argv[1], 0);
+								      argv[0], NULL, argv[1],
+								      flags);
 		else
 			return git_config_set_in_file_gently(given_config_source.file,
 							     argv[0], NULL);
@@ -878,7 +884,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], NULL, argv[1],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/config.c b/config.c
index 4841c68a91..5028c1e736 100644
--- a/config.c
+++ b/config.c
@@ -2402,6 +2402,7 @@ struct config_store_data {
 	size_t baselen;
 	char *key;
 	int do_not_match;
+	const char *literal_value;
 	regex_t *value_regex;
 	int multi_replace;
 	struct {
@@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
+	if (store->literal_value)
+		return !strcmp(store->literal_value, value);
 	if (!store->value_regex)
 		return 1; /* always matches */
 	if (store->value_regex == CONFIG_REGEX_NONE)
@@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			store.value_regex = NULL;
 		else if (value_regex == CONFIG_REGEX_NONE)
 			store.value_regex = CONFIG_REGEX_NONE;
+		else if (flags & CONFIG_FLAGS_LITERAL_VALUE)
+			store.literal_value = value_regex;
 		else {
 			if (value_regex[0] == '!') {
 				store.do_not_match = 1;
diff --git a/config.h b/config.h
index 266eb22e46..ddb81e1e32 100644
--- a/config.h
+++ b/config.h
@@ -77,6 +77,13 @@ enum config_event_t {
  */
 #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
 
+/*
+ * When CONFIG_FLAGS_LITERAL_VALUE is specified, match key/value pairs
+ * by string comparison (not regex match) to the provided value_regex
+ * parameter.
+ */
+#define CONFIG_FLAGS_LITERAL_VALUE (1 << 1)
+
 /*
  * The parser event function (if not NULL) is called with the event type and
  * the begin/end offsets of the parsed elements.
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 73f5ca4361..ed12b4f7a0 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1978,4 +1978,52 @@ test_expect_success 'refuse --literal-value for incompatible actions' '
 	test_must_fail git config --literal-value --edit
 '
 
+test_expect_success '--literal-value uses exact string matching' '
+	GLOB="a+b*c?d[e]f.g" &&
+	q_to_tab >initial <<-EOF &&
+	[literal]
+	Qtest = $GLOB
+	EOF
+
+	cp initial .git/config &&
+	git config literal.test bogus "$GLOB" &&
+	q_to_tab >expect <<-EOF &&
+	[literal]
+	Qtest = $GLOB
+	Qtest = bogus
+	EOF
+	test_cmp expect .git/config &&
+	cp initial .git/config &&
+	git config --literal-value literal.test bogus "$GLOB" &&
+	q_to_tab >expect <<-EOF &&
+	[literal]
+	Qtest = bogus
+	EOF
+	test_cmp expect .git/config &&
+
+	cp initial .git/config &&
+	test_must_fail git config --unset literal.test "$GLOB" &&
+	git config --literal-value --unset literal.test "$GLOB" &&
+	test_must_fail git config literal.test &&
+
+	cp initial .git/config &&
+	test_must_fail git config --unset-all literal.test "$GLOB" &&
+	git config --literal-value --unset-all literal.test "$GLOB" &&
+	test_must_fail git config literal.test &&
+
+	cp initial .git/config &&
+	git config --replace-all literal.test bogus "$GLOB" &&
+	q_to_tab >expect <<-EOF &&
+	[literal]
+	Qtest = $GLOB
+	Qtest = bogus
+	EOF
+	test_cmp expect .git/config &&
+
+	cp initial .git/config &&
+	git config --replace-all literal.test bogus "$GLOB" &&
+	git config --literal-value --replace-all literal.test bogus "$GLOB" &&
+	test_cmp_config bogus literal.test
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 5/7] config: plumb --literal-value into config API

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/config.c b/config.c
> index 4841c68a91..5028c1e736 100644
> --- a/config.c
> +++ b/config.c
> @@ -2402,6 +2402,7 @@ struct config_store_data {
>  	size_t baselen;
>  	char *key;
>  	int do_not_match;
> +	const char *literal_value;
>  	regex_t *value_regex;
>  	int multi_replace;
>  	struct {
> @@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
>  {
>  	if (strcmp(key, store->key))
>  		return 0; /* not ours */
> +	if (store->literal_value)
> +		return !strcmp(store->literal_value, value);
>  	if (!store->value_regex)
>  		return 1; /* always matches */
>  	if (store->value_regex == CONFIG_REGEX_NONE)
> @@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>  			store.value_regex = NULL;
>  		else if (value_regex == CONFIG_REGEX_NONE)
>  			store.value_regex = CONFIG_REGEX_NONE;
> +		else if (flags & CONFIG_FLAGS_LITERAL_VALUE)
> +			store.literal_value = value_regex;
>  		else {
>  			if (value_regex[0] == '!') {
>  				store.do_not_match = 1;

This is nice.  It is a surprisingly small change.

Thanks.

[PATCH 6/7] config: implement --literal-value with --get*

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The config builtin does its own regex matching of values for the --get,
--get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
to the get_value() method so we can initialize the value_regex argument
as a fixed string instead of a regex pattern.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 15 ++++++++++-----
 t/t1300-config.sh | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 6ab5a0d68f..1d5cfab8f1 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -14,6 +14,7 @@ static const char *const builtin_config_usage[] = {
 
 static char *key;
 static regex_t *key_regexp;
+static const char *value_regex;
 static regex_t *regexp;
 static int show_keys;
 static int omit_values;
@@ -298,6 +299,8 @@ 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 (literal && strcmp(value_regex, (value_?value_:"")))
+		return 0;
 	if (regexp != NULL &&
 	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
@@ -308,7 +311,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	return format_config(&values->items[values->nr++], key_, value_);
 }
 
-static int get_value(const char *key_, const char *regex_)
+static int get_value(const char *key_, const char *regex_, int flags)
 {
 	int ret = CONFIG_GENERIC_ERROR;
 	struct strbuf_list values = {NULL};
@@ -345,7 +348,9 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	if (regex_) {
+	if (regex_ && (flags & CONFIG_FLAGS_LITERAL_VALUE))
+		value_regex = regex_;
+	else if (regex_) {
 		if (regex_[0] == '!') {
 			do_not_match = 1;
 			regex_++;
@@ -850,19 +855,19 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_URLMATCH) {
 		check_argc(argc, 2, 2);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index ed12b4f7a0..8531d01c7c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2026,4 +2026,28 @@ test_expect_success '--literal-value uses exact string matching' '
 	test_cmp_config bogus literal.test
 '
 
+test_expect_success '--get and --get-all with --literal-value' '
+	GLOB="a+b*c?d[e]f.g" &&
+	q_to_tab >.git/config <<-EOF &&
+	[literal]
+	Qtest = bogus
+	Qtest = $GLOB
+	EOF
+
+	git config --get literal.test bogus &&
+	test_must_fail git config --get literal.test "$GLOB" &&
+	git config --get --literal-value literal.test "$GLOB" &&
+	test_must_fail git config --get --literal-value literal.test non-existent &&
+
+	git config --get-all literal.test bogus &&
+	test_must_fail git config --get-all literal.test "$GLOB" &&
+	git config --get-all --literal-value literal.test "$GLOB" &&
+	test_must_fail git config --get-all --literal-value literal.test non-existent &&
+
+	git config --get-regexp literal+ bogus &&
+	test_must_fail git config  --get-regexp literal+ "$GLOB" &&
+	git config --get-regexp --literal-value literal+ "$GLOB" &&
+	test_must_fail git config --get-regexp --literal-value literal+ non-existent
+'
+
 test_done
-- 
gitgitgadget

[PATCH 7/7] maintenance: use 'git config --literal-value'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

When a repository's leading directories contain regex glob characters,
the config calls for 'git maintenance register' and 'git maintenance
unregister' are not careful enough. Use the new --literal-value option
to direct the config machinery to use exact string matches. This is a
more robust option than excaping these arguments in a piecemeal fashion.

Reported-by: Emily Shaffer <emilyshaffer@google.com>
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/gc.c           |  5 +++--
 t/t7900-maintenance.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e3098ef6a1..0784bbdc6a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1452,7 +1452,8 @@ static int maintenance_register(void)
 		git_config_set("maintenance.strategy", "incremental");
 
 	config_get.git_cmd = 1;
-	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
+	strvec_pushl(&config_get.args, "config", "--global", "--get",
+		     "--literal-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 			 NULL);
@@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
 
 	config_unset.git_cmd = 1;
 	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "maintenance.repo",
+		     "--literal-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 		     NULL);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 20184e96e1..2ee5512158 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
+test_expect_success 'register and unregister with glob characters' '
+	GLOB="a+b*c" &&
+	git init "$GLOB" &&
+	git -C "$GLOB" maintenance register &&
+	git config --get-all --show-origin maintenance.repo &&
+	git config --get-all --global --literal-value \
+		maintenance.repo "$(pwd)/$GLOB" &&
+	git -C "$GLOB" maintenance unregister &&
+	test_must_fail git config --get-all --global --literal-value \
+		maintenance.repo "$(pwd)/$GLOB"
+'
+
 test_expect_success 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
-- 
gitgitgadget

Re: [PATCH 7/7] maintenance: use 'git config --literal-value'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When a repository's leading directories contain regex glob characters,
> the config calls for 'git maintenance register' and 'git maintenance
> unregister' are not careful enough. Use the new --literal-value option
> to direct the config machinery to use exact string matches. This is a
> more robust option than excaping these arguments in a piecemeal fashion.
>
> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Reported-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/gc.c           |  5 +++--
>  t/t7900-maintenance.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index e3098ef6a1..0784bbdc6a 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1452,7 +1452,8 @@ static int maintenance_register(void)
>  		git_config_set("maintenance.strategy", "incremental");
>  
>  	config_get.git_cmd = 1;
> -	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
> +	strvec_pushl(&config_get.args, "config", "--global", "--get",
> +		     "--literal-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  			 NULL);
> @@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
>  
>  	config_unset.git_cmd = 1;
>  	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "maintenance.repo",
> +		     "--literal-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  		     NULL);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 20184e96e1..2ee5512158 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
>  	test_cmp before actual
>  '
>  
> +test_expect_success 'register and unregister with glob characters' '
> +	GLOB="a+b*c" &&
> +	git init "$GLOB" &&
> +	git -C "$GLOB" maintenance register &&
> +	git config --get-all --show-origin maintenance.repo &&
> +	git config --get-all --global --literal-value \
> +		maintenance.repo "$(pwd)/$GLOB" &&
> +	git -C "$GLOB" maintenance unregister &&
> +	test_must_fail git config --get-all --global --literal-value \
> +		maintenance.repo "$(pwd)/$GLOB"
> +'
> +

Makes sense.  Thanks.

As to the topic-branch organization, my plan is for the final shape
(I haven't really reviewed the early 6 yet so I do not know if they
require rerolling at this point) would be to:

 - create ds/config-literal-value topic on somewhere old enough and
   queue [1/7] thru [6/7] on it.

 - merge the above into ds/maintenance-part-3

 - apply [7/7] on top of ds/maintenance-part-3

Re: [PATCH 0/7] config: add --literal-value option

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:

> As reported [1], 'git maintenance unregister' fails when a repository is
> located in a directory with regex glob characters.

Just as bikeshedding on the name: Did you consider something
thematically similar to the corresponding git-grep option,
i.e. --fixed-string[s]. I see -F is also free in git-config(1).

Re: [PATCH 0/7] config: add --literal-value option

[Derrick Stolee]

On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
> 
>> As reported [1], 'git maintenance unregister' fails when a repository is
>> located in a directory with regex glob characters.
> 
> Just as bikeshedding on the name: Did you consider something
> thematically similar to the corresponding git-grep option,
> i.e. --fixed-string[s]. I see -F is also free in git-config(1).

I definitely wanted to be specific about "value" in the name,
since some options include regexes on the key as well. I'm open
to new ideas, and combining your idea with mine would introduce
"--fixed-value". Thoughts?

Thanks,
-Stolee

Re: [PATCH 0/7] config: add --literal-value option

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>> 
>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>> located in a directory with regex glob characters.
>> 
>> Just as bikeshedding on the name: Did you consider something
>> thematically similar to the corresponding git-grep option,
>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>
> I definitely wanted to be specific about "value" in the name,
> since some options include regexes on the key as well. I'm open
> to new ideas, and combining your idea with mine would introduce
> "--fixed-value". Thoughts?

I very much appreciate "value" is in the name, with the current
semantics that this only controls how the pattern matching is done
on the value side and not on the key side.  When making an obvious
addition of a separate option to control how the pattern matching is
done on keys in the future, we would regret if we called this option
"--fixed-strings" today.  And no, I do not think it is an acceptable
option to introduce "--fixed-strings" that only affects value side
and then later change its behaviour to affect also on the key side.

	Side note.  It _is_ possible to ship such a "--fixed-strings"
	option that does not work on the key side and document it as
	a known bug, later to be fixed.  I am not sure if I like it.

But stepping back a bit, is the extra flexibility that allows us to
control the matching on keys and values separately with such a
scheme really worth the complexity (at the end-user facing interface
level, not the implementation level)?

So an alternative may be to use a single option, whose name would
probably be one of "--(literal|fixed)-(match|strings)", but extend
the implementation in this series to make the single option affect
both the value and key matching the same way.

That would however be more work in the shorter term.  Offhand, I am
not sure if I like it (i.e. spending time and effort that is more
than the absolute minimum necessary to fix a breakage.  And the end
result of doing so is less powerful/flexible, even though it may be
easier to explain to users simply because the feature is less
powerful than it could be).  It would be easier, if I can convince
myself that the extra flexibility is not worth it, to just declare
that simpler is better here, but I am not quite ready to do so yet.

As to "-F", I do not think it is a good idea; in some context "-F"
means work on a <file> given via that option, i.e. "-F <file>".

Thanks.

Re: [PATCH 0/7] config: add --literal-value option

[Derrick Stolee]

On 11/20/2020 1:30 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>>> located in a directory with regex glob characters.
>>>
>>> Just as bikeshedding on the name: Did you consider something
>>> thematically similar to the corresponding git-grep option,
>>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>>
>> I definitely wanted to be specific about "value" in the name,
>> since some options include regexes on the key as well. I'm open
>> to new ideas, and combining your idea with mine would introduce
>> "--fixed-value". Thoughts?
> 
> I very much appreciate "value" is in the name, with the current
> semantics that this only controls how the pattern matching is done
> on the value side and not on the key side.  When making an obvious
> addition of a separate option to control how the pattern matching is
> done on keys in the future, we would regret if we called this option
> "--fixed-strings" today.  And no, I do not think it is an acceptable
> option to introduce "--fixed-strings" that only affects value side
> and then later change its behaviour to affect also on the key side.
> 
> 	Side note.  It _is_ possible to ship such a "--fixed-strings"
> 	option that does not work on the key side and document it as
> 	a known bug, later to be fixed.  I am not sure if I like it.
> 
> But stepping back a bit, is the extra flexibility that allows us to
> control the matching on keys and values separately with such a
> scheme really worth the complexity (at the end-user facing interface
> level, not the implementation level)?
>
> So an alternative may be to use a single option, whose name would
> probably be one of "--(literal|fixed)-(match|strings)", but extend
> the implementation in this series to make the single option affect
> both the value and key matching the same way.
> 
> That would however be more work in the shorter term.  Offhand, I am
> not sure if I like it (i.e. spending time and effort that is more
> than the absolute minimum necessary to fix a breakage.  And the end
> result of doing so is less powerful/flexible, even though it may be
> easier to explain to users simply because the feature is less
> powerful than it could be).  It would be easier, if I can convince
> myself that the extra flexibility is not worth it, to just declare
> that simpler is better here, but I am not quite ready to do so yet.
I had not thought about making this option be related to the key
names at all. In particular, we already have the --get-regexp option:

--get-regexp::
	Like --get-all, but interprets the name as a regular expression and
	writes out the key names.  Regular expression matching is currently
	case-sensitive and done against a canonicalized version of the key
	in which section and variable names are lowercased, but subsection
	names are not.

I suppose that there could be reason to create a similar --unset-regexp
that is the same equivalent of --unset-all. Perhaps the --replace-all
mode could benefit, too? These are also more dangerous to use with regex
matches since they also _change_ the config, not just query it.

Since there is already asymmetry between the key and value (only one
mode, --get-regexp, has a "name_regex"), I would prefer to treat the
value_regex in isolation here.

Thanks,
-Stolee

Re: [PATCH 0/7] config: add --literal-value option

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> Since there is already asymmetry between the key and value (only one
> mode, --get-regexp, has a "name_regex"), I would prefer to treat the
> value_regex in isolation here.

Ah, I forgot about --get-regexp vs --get; so the ship has already
sailed---I agree with you that --(fixed|literal)-value is all we
need.

Thanks.

Re: [PATCH 0/7] config: add --literal-value option

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 20 2020, Derrick Stolee wrote:

> On 11/20/2020 8:19 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Nov 19 2020, Derrick Stolee via GitGitGadget wrote:
>> 
>>> As reported [1], 'git maintenance unregister' fails when a repository is
>>> located in a directory with regex glob characters.
>> 
>> Just as bikeshedding on the name: Did you consider something
>> thematically similar to the corresponding git-grep option,
>> i.e. --fixed-string[s]. I see -F is also free in git-config(1).
>
> I definitely wanted to be specific about "value" in the name,
> since some options include regexes on the key as well. I'm open
> to new ideas, and combining your idea with mine would introduce
> "--fixed-value". Thoughts?

Hi. I see you've already sent a v2 with that. Looks good to me. FWIW I
didn't feel strongly one way or the other about it, either would be a
fine choice by someone who's eyeballing this more than my cursory
glance.

I just wanted to point out the grep option in case you'd missed it,
since we've got a bazillion CLI options all over the place, and
sometimes there's an existing option that's thematically similar to a
proposed new one that's easy to miss. So we can re-use (or partially
re-use) the name, behavior, or not.

[PATCH v2 0/7] config: add --fixed-value option

[Derrick Stolee via GitGitGadget]

As reported [1], 'git maintenance unregister' fails when a repository is
located in a directory with regex glob characters.

[1] 
https://lore.kernel.org/git/2c2db228-069a-947d-8446-89f4d3f6181a@gmail.com/T/#mb96fa4187a0d6aeda097cd95804a8aafc0273022

The discussed solution was to modify 'git config' to specify that the
'value_regex' argument should be treated as an exact string match. This is
the primary change in this series, with an additional patch at the end to
make 'git maintenance [un]register' use this option, when necessary.

Updates in V2
=============

 * Updated the test structure for the new tests in t1300-config.sh
   
   
 * Renamed option from '--literal-value' to '--fixed-value'
   
   
 * Use 'unsigned flags' instead of 'int flags'. This changes more prototypes
   than before.
   
   
 * Move flag definitions closer to method declarations.
   
   
 * Test the uses of --fixed-value to reject when value_regex is missing.
   
   

Thanks, -Stolee

Derrick Stolee (7):
  t1300: test "set all" mode with value_regex
  t1300: add test for --replace-all with value_regex
  config: convert multi_replace to flags
  config: add --fixed-value option, un-implemented
  config: plumb --fixed-value into config API
  config: implement --fixed-value with --get*
  maintenance: use 'git config --fixed-value'

 Documentation/git-config.txt |  20 +++--
 builtin/branch.c             |   4 +-
 builtin/config.c             |  59 +++++++++++---
 builtin/gc.c                 |   5 +-
 builtin/remote.c             |   8 +-
 config.c                     |  29 ++++---
 config.h                     |  36 +++++++--
 t/t1300-config.sh            | 148 +++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh       |  12 +++
 9 files changed, 278 insertions(+), 43 deletions(-)


base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-796%2Fderrickstolee%2Fmaintenance%2Fconfig-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-796/derrickstolee/maintenance/config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/796

Range-diff vs v1:

 1:  2da2131114 ! 1:  ea3099719c t1300: test "set all" mode with value_regex
     @@ Commit message
             'key=value' pair is appended, making this 'key' a multi-valued
             config setting.
      
     -    Add a test that demonstrates these options.
     +    Add a test that demonstrates these options. Break from the existing
     +    pattern in t1300-config.sh to use 'git config --file=<file>' instead of
     +    modifying .git/config directly. Also use 'git config --file=<file>
     +    --list' for config state comparison instead of the config file format.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ t/t1300-config.sh: test_expect_success '--replace-all does not invent newlines'
       '
       
      +test_expect_success 'set all config with value_regex' '
     -+	q_to_tab >initial <<-\EOF &&
     -+	[abc]
     -+	Qkey = one
     -+	EOF
     ++	git config --file=initial abc.key one &&
      +
     -+	cp initial .git/config &&
     -+	git config abc.key two a+ &&
     -+	q_to_tab >expect <<-\EOF &&
     -+	[abc]
     -+	Qkey = one
     -+	Qkey = two
     ++	cp initial config &&
     ++	git config --file=config abc.key two a+ &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-\EOF &&
     ++	abc.key=one
     ++	abc.key=two
      +	EOF
     -+	test_cmp expect .git/config &&
     ++	test_cmp expect actual &&
      +
     -+	test_must_fail git config abc.key three o+ 2>err &&
     ++	test_must_fail git config --file=config abc.key three o+ 2>err &&
      +	test_i18ngrep "has multiple values" err &&
     -+	git config abc.key three a+ &&
     -+	q_to_tab >expect <<-\EOF &&
     -+	[abc]
     -+	Qkey = one
     -+	Qkey = two
     -+	Qkey = three
     ++	git config --file=config abc.key three a+ &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-\EOF &&
     ++	abc.key=one
     ++	abc.key=two
     ++	abc.key=three
      +	EOF
     -+	test_cmp expect .git/config &&
     ++	test_cmp expect actual &&
      +
     -+	cp initial .git/config &&
     -+	git config abc.key three o+ &&
     -+	q_to_tab >expect <<-\EOF &&
     -+	[abc]
     -+	Qkey = three
     ++	cp initial config &&
     ++	git config --file=config abc.key three o+ &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-\EOF &&
     ++	abc.key=three
      +	EOF
     -+	test_cmp expect .git/config
     ++	test_cmp expect actual
      +'
      +
       test_done
 2:  1237289706 ! 2:  829d0ccd8c t1300: add test for --replace-all with value_regex
     @@ Commit message
      
       ## t/t1300-config.sh ##
      @@ t/t1300-config.sh: test_expect_success 'set all config with value_regex' '
     - 	test_cmp expect .git/config
     + 	test_cmp expect actual
       '
       
      +test_expect_success '--replace-all and value_regex' '
     -+	q_to_tab >.git/config <<-\EOF &&
     -+	[abc]
     -+	Qkey = one
     -+	Qkey = two
     -+	Qkey = three
     ++	rm -f config &&
     ++	git config --file=config --add abc.key one &&
     ++	git config --file=config --add abc.key two &&
     ++	git config --file=config --add abc.key three &&
     ++	git config --file=config --replace-all abc.key four "o+" &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-\EOF &&
     ++	abc.key=four
     ++	abc.key=three
      +	EOF
     -+	q_to_tab >expect <<-\EOF &&
     -+	[abc]
     -+	Qkey = four
     -+	Qkey = three
     -+	EOF
     -+	git config --replace-all abc.key four "o+" &&
     -+	test_cmp expect .git/config
     ++	test_cmp expect actual
      +'
      +
       test_done
 3:  c9caed3854 ! 3:  0c152faa00 config: convert multi_replace to flags
     @@ Commit message
      
          We will extend the flexibility of the config API. Before doing so, let's
          take an existing 'int multi_replace' parameter and replace it with a new
     -    'int flags' parameter that can take multiple options as a bit field.
     +    'unsigned flags' parameter that can take multiple options as a bit field.
      
          Update all callers that specified multi_replace to now specify the
          CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
          documentation of git_config_set_multivar_in_file() including a clear
     -    labeling of its arguments. Other config API methods in config.h do not
     -    require a change to their prototypes, but they have an equivalent update
     -    to their implementations.
     +    labeling of its arguments. Other config API methods in config.h require
     +    only a change of the final parameter from 'int' to 'unsigned'.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ config.c: void git_config_set(const char *key, const char *value)
       					   const char *key, const char *value,
       					   const char *value_regex,
      -					   int multi_replace)
     -+					   int flags)
     ++					   unsigned flags)
       {
       	int fd = -1, in_fd = -1;
       	int ret;
     @@ config.c: int git_config_set_multivar_in_file_gently(const char *config_filename
       void git_config_set_multivar_in_file(const char *config_filename,
       				     const char *key, const char *value,
      -				     const char *value_regex, int multi_replace)
     -+				     const char *value_regex, int flags)
     ++				     const char *value_regex, unsigned flags)
       {
       	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
      -						    value_regex, multi_replace))
     @@ config.c: void git_config_set_multivar_in_file(const char *config_filename,
       
       int git_config_set_multivar_gently(const char *key, const char *value,
      -				   const char *value_regex, int multi_replace)
     -+				   const char *value_regex, int flags)
     ++				   const char *value_regex, unsigned flags)
       {
       	return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex,
      -						      multi_replace);
     @@ config.c: void git_config_set_multivar_in_file(const char *config_filename,
       
       void git_config_set_multivar(const char *key, const char *value,
      -			     const char *value_regex, int multi_replace)
     -+			     const char *value_regex, int flags)
     ++			     const char *value_regex, unsigned flags)
       {
       	git_config_set_multivar_in_file(NULL, key, value, value_regex,
      -					multi_replace);
     @@ config.c: void git_config_set_multivar_in_file(const char *config_filename,
       static int section_name_match (const char *buf, const char *name)
      
       ## config.h ##
     -@@ config.h: enum config_event_t {
     - 	CONFIG_EVENT_ERROR
     - };
     +@@ config.h: void git_config_set(const char *, const char *);
       
     + int git_config_parse_key(const char *, char **, size_t *);
     + int git_config_key_is_valid(const char *key);
     +-int git_config_set_multivar_gently(const char *, const char *, const char *, int);
     +-void git_config_set_multivar(const char *, const char *, const char *, int);
     +-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int);
     ++
     ++/*
     ++ * The following macros specify flag bits that alter the behavior
     ++ * of the git_config_set_multivar*() methods.
     ++ */
     ++
      +/*
      + * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
      + * are removed before a new pair is written. If the flag is not present,
     @@ config.h: enum config_event_t {
      + */
      +#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
      +
     - /*
     -  * The parser event function (if not NULL) is called with the event type and
     -  * the begin/end offsets of the parsed elements.
     ++int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
     ++void git_config_set_multivar(const char *, const char *, const char *, unsigned);
     ++int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
     + 
     + /**
     +  * takes four parameters:
      @@ config.h: int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
        * - the value regex, as a string. It will disregard key/value pairs where value
        *   does not match.
     @@ config.h: int git_config_set_multivar_in_file_gently(const char *, const char *,
      +				     const char *key,
      +				     const char *value,
      +				     const char *value_regex,
     -+				     int flags);
     ++				     unsigned flags);
       
       /**
        * rename or remove sections in the config file
 4:  28493184b6 ! 4:  0e6a7371ed config: add --literal-value option, un-implemented
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    config: add --literal-value option, un-implemented
     +    config: add --fixed-value option, un-implemented
      
          The 'git config' builtin takes a 'value_regex' parameter for several
          actions. This can cause confusion when expecting exact value matches
     @@ Commit message
          be more friendly to allow an argument to disable the pattern matching in
          favor of an exact string match.
      
     -    Add a new '--literal-value' option that does not currently change the
     +    Add a new '--fixed-value' option that does not currently change the
          behavior. The implementation will follow for each appropriate action.
     -    For now, check and test that --literal-value will abort the command when
     -    included with an incompatible action.
     +    For now, check and test that --fixed-value will abort the command when
     +    included with an incompatible action or without a 'value_regex'
     +    argument.
      
     -    The name '--literal-value' was chosen over something simpler like
     -    '--literal' because some commands allow regular expressions on the
     +    The name '--fixed-value' was chosen over something simpler like
     +    '--fixed' because some commands allow regular expressions on the
          key in addition to the value.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
     @@ Documentation/git-config.txt: git-config - Get and set repository or global opti
       --------
       [verse]
      -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
     -+'git config' [<file-option>] [--type=<type>] [--literal-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
     ++'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
       'git config' [<file-option>] [--type=<type>] --add name value
      -'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
      -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
      -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
      -'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--literal-value] --replace-all name value [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get name [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get-all name [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] [--name-only] --get-regexp name_regex [value_regex]
     ++'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value_regex]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value_regex]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value_regex]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value_regex]
       'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
      -'git config' [<file-option>] --unset name [value_regex]
      -'git config' [<file-option>] --unset-all name [value_regex]
     -+'git config' [<file-option>] [--literal-value] --unset name [value_regex]
     -+'git config' [<file-option>] [--literal-value] --unset-all name [value_regex]
     ++'git config' [<file-option>] [--fixed-value] --unset name [value_regex]
     ++'git config' [<file-option>] [--fixed-value] --unset-all name [value_regex]
       'git config' [<file-option>] --rename-section old_name new_name
       'git config' [<file-option>] --remove-section name
       'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
     @@ Documentation/git-config.txt: See also <<FILES>>.
       --list::
       	List all variables set in config file, along with their values.
       
     -+--literal-value::
     ++--fixed-value::
      +	When used with the `value_regex` argument, treat `value_regex` as
      +	an exact string instead of a regular expression. This will restrict
      +	the name/value pairs that are matched to only those where the value
     @@ builtin/config.c: static int respect_includes_opt = -1;
       static struct config_options config_options;
       static int show_origin;
       static int show_scope;
     -+static int literal;
     ++static int fixed_value;
       
       #define ACTION_GET (1<<0)
       #define ACTION_GET_ALL (1<<1)
     -@@ builtin/config.c: static int show_scope;
     - #define ACTION_GET_COLORBOOL (1<<14)
     - #define ACTION_GET_URLMATCH (1<<15)
     - 
     -+#define ACTION_LITERAL_ALLOWED (\
     -+	ACTION_GET |\
     -+	ACTION_GET_ALL |\
     -+	ACTION_GET_REGEXP |\
     -+	ACTION_REPLACE_ALL |\
     -+	ACTION_UNSET |\
     -+	ACTION_UNSET_ALL |\
     -+	ACTION_SET_ALL\
     -+)
     -+
     - /*
     -  * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
     -  * one line of output and which should therefore be paged.
      @@ builtin/config.c: static struct option builtin_config_options[] = {
       	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
       	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
       	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
     -+	OPT_BOOL(0, "literal-value", &literal, N_("use literal equality when matching values")),
     ++	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
       	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
       	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
       	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       		usage_builtin_config();
       	}
       
     -+	if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
     -+		error(_("--literal only applies with 'value_regex'"));
     -+		usage_builtin_config();
     ++	if (fixed_value) {
     ++		int allowed_usage = 0;
     ++
     ++		switch (actions) {
     ++		case ACTION_GET:
     ++		case ACTION_GET_ALL:
     ++		case ACTION_GET_REGEXP:
     ++		case ACTION_UNSET:
     ++		case ACTION_UNSET_ALL:
     ++			allowed_usage = argc > 1 && !!argv[1];
     ++			break;
     ++
     ++		case ACTION_SET_ALL:
     ++		case ACTION_REPLACE_ALL:
     ++			allowed_usage = argc > 2 && !!argv[2];
     ++			break;
     ++		}
     ++
     ++		if (!allowed_usage) {
     ++			error(_("--fixed-value only applies with 'value_regex'"));
     ++			usage_builtin_config();
     ++		}
      +	}
      +
       	if (actions & PAGING_ACTIONS)
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
      
       ## t/t1300-config.sh ##
      @@ t/t1300-config.sh: test_expect_success '--replace-all and value_regex' '
     - 	test_cmp expect .git/config
     + 	test_cmp expect actual
       '
       
     -+test_expect_success 'refuse --literal-value for incompatible actions' '
     -+	git config dev.null bogus &&
     -+	test_must_fail git config --literal-value --add dev.null bogus &&
     -+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
     -+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
     -+	test_must_fail git config --literal-value --rename-section dev null &&
     -+	test_must_fail git config --literal-value --remove-section dev &&
     -+	test_must_fail git config --literal-value --list &&
     -+	test_must_fail git config --literal-value --get-color dev.null &&
     -+	test_must_fail git config --literal-value --get-colorbool dev.null &&
     -+	test_must_fail git config --literal-value --edit
     ++test_expect_success 'refuse --fixed-value for incompatible actions' '
     ++	git config --file=config dev.null bogus &&
     ++
     ++	# These modes do not allow --fixed-value at all
     ++	test_must_fail git config --file=config --fixed-value --add dev.null bogus &&
     ++	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
     ++	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
     ++	test_must_fail git config --file=config --fixed-value --rename-section dev null &&
     ++	test_must_fail git config --file=config --fixed-value --remove-section dev &&
     ++	test_must_fail git config --file=config --fixed-value --list &&
     ++	test_must_fail git config --file=config --fixed-value --get-color dev.null &&
     ++	test_must_fail git config --file=config --fixed-value --get-colorbool dev.null &&
     ++
     ++	# These modes complain when --fixed-value has no value_regex
     ++	test_must_fail git config --file=config --fixed-value dev.null bogus &&
     ++	test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus &&
     ++	test_must_fail git config --file=config --fixed-value --get dev.null &&
     ++	test_must_fail git config --file=config --fixed-value --get-all dev.null &&
     ++	test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" &&
     ++	test_must_fail git config --file=config --fixed-value --unset dev.null &&
     ++	test_must_fail git config --file=config --fixed-value --unset-all dev.null
      +'
      +
       test_done
 5:  5881b2d987 ! 5:  39718048cd config: plumb --literal-value into config API
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    config: plumb --literal-value into config API
     +    config: plumb --fixed-value into config API
      
          The git_config_set_multivar_in_file_gently() and related methods now
     -    take a 'flags' bitfield, so add a new bit representing the
     -    --literal-value option from 'git config'. This alters the purpose of the
     -    value_regex parameter to be an exact string match. This requires some
     -    initialization changes in git_config_set_multivar_in_file_gently() and a
     -    new strcmp() call in the matches() method.
     +    take a 'flags' bitfield, so add a new bit representing the --fixed-value
     +    option from 'git config'. This alters the purpose of the value_regex
     +    parameter to be an exact string match. This requires some initialization
     +    changes in git_config_set_multivar_in_file_gently() and a new strcmp()
     +    call in the matches() method.
      
     -    The new CONFIG_FLAGS_LITERAL_VALUE flag is initialized in
     -    builtin/config.c based on the --literal-value option, and that needs to
     -    be updated in several callers.
     +    The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
     +    based on the --fixed-value option, and that needs to be updated in
     +    several callers.
      
          This patch only affects some of the modes of 'git config', and the rest
          will be completed in the next change.
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
       
      @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
     - 		error(_("--literal only applies with 'value_regex'"));
     - 		usage_builtin_config();
     + 			error(_("--fixed-value only applies with 'value_regex'"));
     + 			usage_builtin_config();
     + 		}
     ++
     ++		flags = CONFIG_FLAGS_FIXED_VALUE;
       	}
     -+	if (literal)
     -+		flags = CONFIG_FLAGS_LITERAL_VALUE;
       
       	if (actions & PAGING_ACTIONS)
     - 		setup_auto_pager("config", 1);
      @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
       		value = normalize_value(argv[0], argv[1]);
       		UNLEAK(value);
     @@ config.c: int git_config_set_multivar_in_file_gently(const char *config_filename
       			store.value_regex = NULL;
       		else if (value_regex == CONFIG_REGEX_NONE)
       			store.value_regex = CONFIG_REGEX_NONE;
     -+		else if (flags & CONFIG_FLAGS_LITERAL_VALUE)
     ++		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
      +			store.literal_value = value_regex;
       		else {
       			if (value_regex[0] == '!') {
       				store.do_not_match = 1;
      
       ## config.h ##
     -@@ config.h: enum config_event_t {
     +@@ config.h: int git_config_key_is_valid(const char *key);
        */
       #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
       
      +/*
     -+ * When CONFIG_FLAGS_LITERAL_VALUE is specified, match key/value pairs
     ++ * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
      + * by string comparison (not regex match) to the provided value_regex
      + * parameter.
      + */
     -+#define CONFIG_FLAGS_LITERAL_VALUE (1 << 1)
     ++#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
      +
     - /*
     -  * The parser event function (if not NULL) is called with the event type and
     -  * the begin/end offsets of the parsed elements.
     + int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
     + void git_config_set_multivar(const char *, const char *, const char *, unsigned);
     + int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
      
       ## t/t1300-config.sh ##
     -@@ t/t1300-config.sh: test_expect_success 'refuse --literal-value for incompatible actions' '
     - 	test_must_fail git config --literal-value --edit
     +@@ t/t1300-config.sh: test_expect_success 'refuse --fixed-value for incompatible actions' '
     + 	test_must_fail git config --file=config --fixed-value --unset-all dev.null
       '
       
     -+test_expect_success '--literal-value uses exact string matching' '
     ++test_expect_success '--fixed-value uses exact string matching' '
      +	GLOB="a+b*c?d[e]f.g" &&
     -+	q_to_tab >initial <<-EOF &&
     -+	[literal]
     -+	Qtest = $GLOB
     -+	EOF
     ++	rm -f initial &&
     ++	git config --file=initial fixed.test "$GLOB" &&
      +
     -+	cp initial .git/config &&
     -+	git config literal.test bogus "$GLOB" &&
     -+	q_to_tab >expect <<-EOF &&
     -+	[literal]
     -+	Qtest = $GLOB
     -+	Qtest = bogus
     ++	cp initial config &&
     ++	git config --file=config fixed.test bogus "$GLOB" &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-EOF &&
     ++	fixed.test=$GLOB
     ++	fixed.test=bogus
      +	EOF
     -+	test_cmp expect .git/config &&
     -+	cp initial .git/config &&
     -+	git config --literal-value literal.test bogus "$GLOB" &&
     -+	q_to_tab >expect <<-EOF &&
     -+	[literal]
     -+	Qtest = bogus
     -+	EOF
     -+	test_cmp expect .git/config &&
     ++	test_cmp expect actual &&
     ++
     ++	cp initial config &&
     ++	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
     ++	git config --file=config --list >actual &&
     ++	printf "fixed.test=bogus\n" >expect &&
     ++	test_cmp expect actual &&
      +
     -+	cp initial .git/config &&
     -+	test_must_fail git config --unset literal.test "$GLOB" &&
     -+	git config --literal-value --unset literal.test "$GLOB" &&
     -+	test_must_fail git config literal.test &&
     ++	cp initial config &&
     ++	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
     ++	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config fixed.test &&
      +
     -+	cp initial .git/config &&
     -+	test_must_fail git config --unset-all literal.test "$GLOB" &&
     -+	git config --literal-value --unset-all literal.test "$GLOB" &&
     -+	test_must_fail git config literal.test &&
     ++	cp initial config &&
     ++	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
     ++	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config fixed.test &&
      +
     -+	cp initial .git/config &&
     -+	git config --replace-all literal.test bogus "$GLOB" &&
     -+	q_to_tab >expect <<-EOF &&
     -+	[literal]
     -+	Qtest = $GLOB
     -+	Qtest = bogus
     ++	cp initial config &&
     ++	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-EOF &&
     ++	fixed.test=$GLOB
     ++	fixed.test=bogus
      +	EOF
     -+	test_cmp expect .git/config &&
     ++	test_cmp expect actual &&
      +
     -+	cp initial .git/config &&
     -+	git config --replace-all literal.test bogus "$GLOB" &&
     -+	git config --literal-value --replace-all literal.test bogus "$GLOB" &&
     -+	test_cmp_config bogus literal.test
     ++	cp initial config &&
     ++	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-EOF &&
     ++	fixed.test=$GLOB
     ++	fixed.test=bogus
     ++	EOF
     ++	test_cmp expect actual &&
     ++
     ++	git config --file=config --fixed-value --replace-all fixed.test bogus "$GLOB" &&
     ++	git config --file=config --list >actual &&
     ++	cat >expect <<-EOF &&
     ++	fixed.test=bogus
     ++	fixed.test=bogus
     ++	EOF
     ++	test_cmp expect actual
      +'
      +
       test_done
 6:  7c8e3769c7 ! 6:  8e0111c7b4 config: implement --literal-value with --get*
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    config: implement --literal-value with --get*
     +    config: implement --fixed-value with --get*
      
          The config builtin does its own regex matching of values for the --get,
          --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
     @@ builtin/config.c: static int collect_config(const char *key_, const char *value_
       		return 0;
       	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
       		return 0;
     -+	if (literal && strcmp(value_regex, (value_?value_:"")))
     ++	if (fixed_value && strcmp(value_regex, (value_?value_:"")))
      +		return 0;
       	if (regexp != NULL &&
       	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
     @@ builtin/config.c: static int get_value(const char *key_, const char *regex_)
       	}
       
      -	if (regex_) {
     -+	if (regex_ && (flags & CONFIG_FLAGS_LITERAL_VALUE))
     ++	if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE))
      +		value_regex = regex_;
      +	else if (regex_) {
       		if (regex_[0] == '!') {
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       		check_argc(argc, 2, 2);
      
       ## t/t1300-config.sh ##
     -@@ t/t1300-config.sh: test_expect_success '--literal-value uses exact string matching' '
     - 	test_cmp_config bogus literal.test
     +@@ t/t1300-config.sh: test_expect_success '--fixed-value uses exact string matching' '
     + 	test_cmp expect actual
       '
       
     -+test_expect_success '--get and --get-all with --literal-value' '
     ++test_expect_success '--get and --get-all with --fixed-value' '
      +	GLOB="a+b*c?d[e]f.g" &&
     -+	q_to_tab >.git/config <<-EOF &&
     -+	[literal]
     -+	Qtest = bogus
     -+	Qtest = $GLOB
     -+	EOF
     ++	rm -f config &&
     ++	git config --file=config fixed.test bogus &&
     ++	git config --file=config --add fixed.test "$GLOB" &&
      +
     -+	git config --get literal.test bogus &&
     -+	test_must_fail git config --get literal.test "$GLOB" &&
     -+	git config --get --literal-value literal.test "$GLOB" &&
     -+	test_must_fail git config --get --literal-value literal.test non-existent &&
     ++	git config --file=config --get fixed.test bogus &&
     ++	test_must_fail git config --file=config --get fixed.test "$GLOB" &&
     ++	git config --file=config --get --fixed-value fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
      +
     -+	git config --get-all literal.test bogus &&
     -+	test_must_fail git config --get-all literal.test "$GLOB" &&
     -+	git config --get-all --literal-value literal.test "$GLOB" &&
     -+	test_must_fail git config --get-all --literal-value literal.test non-existent &&
     ++	git config --file=config --get-all fixed.test bogus &&
     ++	test_must_fail git config --file=config --get-all fixed.test "$GLOB" &&
     ++	git config --file=config --get-all --fixed-value fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
      +
     -+	git config --get-regexp literal+ bogus &&
     -+	test_must_fail git config  --get-regexp literal+ "$GLOB" &&
     -+	git config --get-regexp --literal-value literal+ "$GLOB" &&
     -+	test_must_fail git config --get-regexp --literal-value literal+ non-existent
     ++	git config --file=config --get-regexp fixed+ bogus &&
     ++	test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" &&
     ++	git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" &&
     ++	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
      +'
      +
       test_done
 7:  1d1c2fd23e ! 7:  5a3acf8119 maintenance: use 'git config --literal-value'
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    maintenance: use 'git config --literal-value'
     +    maintenance: use 'git config --fixed-value'
      
          When a repository's leading directories contain regex glob characters,
          the config calls for 'git maintenance register' and 'git maintenance
     -    unregister' are not careful enough. Use the new --literal-value option
     +    unregister' are not careful enough. Use the new --fixed-value option
          to direct the config machinery to use exact string matches. This is a
          more robust option than excaping these arguments in a piecemeal fashion.
      
     +    For the test, require that we are not running on Windows since the '+'
     +    character is not allowed on that filesystem.
     +
          Reported-by: Emily Shaffer <emilyshaffer@google.com>
          Reported-by: Jonathan Nieder <jrnieder@gmail.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
     @@ builtin/gc.c: static int maintenance_register(void)
       	config_get.git_cmd = 1;
      -	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
      +	strvec_pushl(&config_get.args, "config", "--global", "--get",
     -+		     "--literal-value", "maintenance.repo",
     ++		     "--fixed-value", "maintenance.repo",
       		     the_repository->worktree ? the_repository->worktree
       					      : the_repository->gitdir,
       			 NULL);
     @@ builtin/gc.c: static int maintenance_unregister(void)
       	config_unset.git_cmd = 1;
       	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
      -		     "maintenance.repo",
     -+		     "--literal-value", "maintenance.repo",
     ++		     "--fixed-value", "maintenance.repo",
       		     the_repository->worktree ? the_repository->worktree
       					      : the_repository->gitdir,
       		     NULL);
     @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '
       	test_cmp before actual
       '
       
     -+test_expect_success 'register and unregister with glob characters' '
     ++test_expect_success !MINGW 'register and unregister with glob characters' '
      +	GLOB="a+b*c" &&
      +	git init "$GLOB" &&
      +	git -C "$GLOB" maintenance register &&
      +	git config --get-all --show-origin maintenance.repo &&
     -+	git config --get-all --global --literal-value \
     ++	git config --get-all --global --fixed-value \
      +		maintenance.repo "$(pwd)/$GLOB" &&
      +	git -C "$GLOB" maintenance unregister &&
     -+	test_must_fail git config --get-all --global --literal-value \
     ++	test_must_fail git config --get-all --global --fixed-value \
      +		maintenance.repo "$(pwd)/$GLOB"
      +'
      +

-- 
gitgitgadget

[PATCH v2 1/7] t1300: test "set all" mode with value_regex

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Without additional modifiers, 'git config' attempts to set a single
value in the .git/config file. When the value_regex parameter is
supplied, this command behaves in a non-trivial manner.

Consider 'git config key value value_regex'. The expected behavior
is as follows:

1. If there are multiple existing values that match 'value_regex',
   then the command fails. Users should use --replace-all instead.

2. If there is one existing value that matches 'value_regex', then
   the new config has one entry where 'key=value'.

3. If there is no existing values match 'value_regex', then the
   'key=value' pair is appended, making this 'key' a multi-valued
   config setting.

Add a test that demonstrates these options. Break from the existing
pattern in t1300-config.sh to use 'git config --file=<file>' instead of
modifying .git/config directly. Also use 'git config --file=<file>
--list' for config state comparison instead of the config file format.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..19836ec83b 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1914,4 +1914,36 @@ test_expect_success '--replace-all does not invent newlines' '
 	test_cmp expect .git/config
 '
 
+test_expect_success 'set all config with value_regex' '
+	git config --file=initial abc.key one &&
+
+	cp initial config &&
+	git config --file=config abc.key two a+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=one
+	abc.key=two
+	EOF
+	test_cmp expect actual &&
+
+	test_must_fail git config --file=config abc.key three o+ 2>err &&
+	test_i18ngrep "has multiple values" err &&
+	git config --file=config abc.key three a+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=one
+	abc.key=two
+	abc.key=three
+	EOF
+	test_cmp expect actual &&
+
+	cp initial config &&
+	git config --file=config abc.key three o+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=three
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 1/7] t1300: test "set all" mode with value_regex

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:01PM +0000, Derrick Stolee via GitGitGadget wrote:
> Add a test that demonstrates these options. Break from the existing
> pattern in t1300-config.sh to use 'git config --file=<file>' instead of
> modifying .git/config directly. Also use 'git config --file=<file>
> --list' for config state comparison instead of the config file format.

I'd normally say "but *why* do you want to do that?" but I think it's
actually pretty clear in the diff based on the naming of the config
files (and clever - I like it). I do think pointing out that you're
breaking from existing style is useful since the existing style isn't
contained in the diff alone.

> +test_expect_success 'set all config with value_regex' '
> +	git config --file=initial abc.key one &&
> +
> +	cp initial config &&
> +	git config --file=config abc.key two a+ &&
Make sure that if the value_regex misses existing keys, it can still add
normally.
> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=one
> +	abc.key=two
> +	EOF
> +	test_cmp expect actual &&
> +
> +	test_must_fail git config --file=config abc.key three o+ 2>err &&
> +	test_i18ngrep "has multiple values" err &&
Try to change the value of the config, but it's a multiconfig already.
Ok.

> +	git config --file=config abc.key three a+ &&
But /a+/ doesn't match any configs that already exist, so this one gets
added fine.

> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=one
> +	abc.key=two
> +	abc.key=three
> +	EOF
> +	test_cmp expect actual &&
> +
> +	cp initial config &&
Get back the config before the last exercise (with just 'one').
> +	git config --file=config abc.key three o+ &&
And if the value_regex matches exactly one config, then replace that
one. I could see this case being a little bit more compelling if the
value_regex was doing some lifting, e.g.

abc.key=nil
abc.key=one

git config abc.key two o+

abc.key=nil
abc.key=two

This criteria was kind of implied in #2 in your commit message - does it
work?

> +	git config --file=config --list >actual &&
> +	cat >expect <<-\EOF &&
> +	abc.key=three
> +	EOF
> +	test_cmp expect actual
> +'

Anyway, it's a pain, but I'd like to see either comments describing
which case each one is, or individual tests.

 - Emily

[PATCH v2 2/7] t1300: add test for --replace-all with value_regex

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The --replace-all option was added in 4ddba79d (git-config-set: add more
options) but was not tested along with the 'value_regex' parameter.
Since we will be updating this option to optionally treat 'value_regex'
as a fixed string, let's add a test here that documents the current
behavior.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 19836ec83b..8783767d4f 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1946,4 +1946,18 @@ test_expect_success 'set all config with value_regex' '
 	test_cmp expect actual
 '
 
+test_expect_success '--replace-all and value_regex' '
+	rm -f config &&
+	git config --file=config --add abc.key one &&
+	git config --file=config --add abc.key two &&
+	git config --file=config --add abc.key three &&
+	git config --file=config --replace-all abc.key four "o+" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=four
+	abc.key=three
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 2/7] t1300: add test for --replace-all with value_regex

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:02PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> 
> The --replace-all option was added in 4ddba79d (git-config-set: add more
> options) but was not tested along with the 'value_regex' parameter.
> Since we will be updating this option to optionally treat 'value_regex'
> as a fixed string, let's add a test here that documents the current
> behavior.

I like the commit message and the diff a lot. Concise and complete,
explains when the problem came from. I don't think it even needs the
rationale about adding more options later - there's missing test
coverage and you're adding it. But it's useful to reviewers and a handy
step on the journey of this topic.

> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

[PATCH v2 3/7] config: convert multi_replace to flags

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

We will extend the flexibility of the config API. Before doing so, let's
take an existing 'int multi_replace' parameter and replace it with a new
'unsigned flags' parameter that can take multiple options as a bit field.

Update all callers that specified multi_replace to now specify the
CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
documentation of git_config_set_multivar_in_file() including a clear
labeling of its arguments. Other config API methods in config.h require
only a change of the final parameter from 'int' to 'unsigned'.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/branch.c |  4 ++--
 builtin/config.c |  6 ++++--
 builtin/remote.c |  8 +++++---
 config.c         | 24 ++++++++++++------------
 config.h         | 29 ++++++++++++++++++++++-------
 5 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..5ce3844d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.merge", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		if (filter.kind != FILTER_REFS_BRANCHES)
diff --git a/builtin/config.c b/builtin/config.c
index 5e39f61885..e7c7f3d455 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -823,7 +823,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 1);
+							      argv[0], value, argv[2],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -859,7 +860,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], NULL, argv[1], 1);
+							      argv[0], NULL, argv[1],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/builtin/remote.c b/builtin/remote.c
index c8240e9fcd..29b1652975 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -708,7 +708,7 @@ static int mv(int argc, const char **argv)
 
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
-	git_config_set_multivar(buf.buf, NULL, NULL, 1);
+	git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
 	for (i = 0; i < oldremote->fetch.raw_nr; i++) {
 		char *ptr;
@@ -1485,7 +1485,8 @@ static int update(int argc, const char **argv)
 
 static int remove_all_fetch_refspecs(const char *key)
 {
-	return git_config_set_multivar_gently(key, NULL, NULL, 1);
+	return git_config_set_multivar_gently(key, NULL, NULL,
+					      CONFIG_FLAGS_MULTI_REPLACE);
 }
 
 static void add_branches(struct remote *remote, const char **branches,
@@ -1674,7 +1675,8 @@ static int set_url(int argc, const char **argv)
 	if (!delete_mode)
 		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
 	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+		git_config_set_multivar(name_buf.buf, NULL, oldurl,
+					CONFIG_FLAGS_MULTI_REPLACE);
 out:
 	strbuf_release(&name_buf);
 	return 0;
diff --git a/config.c b/config.c
index 2b79fe76ad..096f2eae0d 100644
--- a/config.c
+++ b/config.c
@@ -2716,9 +2716,9 @@ void git_config_set(const char *key, const char *value)
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
  * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
  *     (only add a new one)
- * if multi_replace==0, nothing, or only one matching key/value is replaced,
- *     else all matching key/values (regardless how many) are removed,
- *     before the new pair is written.
+ * if (flags & CONFIG_FLAGS_MULTI_REPLACE) == 0, at most one matching
+ *     key/value is replaced, else all matching key/values (regardless
+ *     how many) are removed, before the new pair is written.
  *
  * Returns 0 on success.
  *
@@ -2739,7 +2739,7 @@ void git_config_set(const char *key, const char *value)
 int git_config_set_multivar_in_file_gently(const char *config_filename,
 					   const char *key, const char *value,
 					   const char *value_regex,
-					   int multi_replace)
+					   unsigned flags)
 {
 	int fd = -1, in_fd = -1;
 	int ret;
@@ -2756,7 +2756,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	if (ret)
 		goto out_free;
 
-	store.multi_replace = multi_replace;
+	store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0;
 
 	if (!config_filename)
 		config_filename = filename_buf = git_pathdup("config");
@@ -2845,7 +2845,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 		/* if nothing to unset, or too many matches, error out */
 		if ((store.seen_nr == 0 && value == NULL) ||
-		    (store.seen_nr > 1 && multi_replace == 0)) {
+		    (store.seen_nr > 1 && !store.multi_replace)) {
 			ret = CONFIG_NOTHING_SET;
 			goto out_free;
 		}
@@ -2984,10 +2984,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key, const char *value,
-				     const char *value_regex, int multi_replace)
+				     const char *value_regex, unsigned flags)
 {
 	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
-						    value_regex, multi_replace))
+						    value_regex, flags))
 		return;
 	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
@@ -2996,17 +2996,17 @@ void git_config_set_multivar_in_file(const char *config_filename,
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
-				   const char *value_regex, int multi_replace)
+				   const char *value_regex, unsigned flags)
 {
 	return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex,
-						      multi_replace);
+						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
-			     const char *value_regex, int multi_replace)
+			     const char *value_regex, unsigned flags)
 {
 	git_config_set_multivar_in_file(NULL, key, value, value_regex,
-					multi_replace);
+					flags);
 }
 
 static int section_name_match (const char *buf, const char *name)
diff --git a/config.h b/config.h
index 060874488f..80844604ab 100644
--- a/config.h
+++ b/config.h
@@ -256,9 +256,22 @@ void git_config_set(const char *, const char *);
 
 int git_config_parse_key(const char *, char **, size_t *);
 int git_config_key_is_valid(const char *key);
-int git_config_set_multivar_gently(const char *, const char *, const char *, int);
-void git_config_set_multivar(const char *, const char *, const char *, int);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int);
+
+/*
+ * The following macros specify flag bits that alter the behavior
+ * of the git_config_set_multivar*() methods.
+ */
+
+/*
+ * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
+ * are removed before a new pair is written. If the flag is not present,
+ * then set operations replace only the first match.
+ */
+#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
+
+int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
+void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**
  * takes four parameters:
@@ -276,13 +289,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
  * - the value regex, as a string. It will disregard key/value pairs where value
  *   does not match.
  *
- * - a multi_replace value, as an int. If value is equal to zero, nothing or only
- *   one matching key/value is replaced, else all matching key/values (regardless
- *   how many) are removed, before the new pair is written.
+ * - a flags value with bits corresponding to the CONFIG_FLAG_* macros.
  *
  * It returns 0 on success.
  */
-void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+void git_config_set_multivar_in_file(const char *config_filename,
+				     const char *key,
+				     const char *value,
+				     const char *value_regex,
+				     unsigned flags);
 
 /**
  * rename or remove sections in the config file
-- 
gitgitgadget

Re: [PATCH v2 3/7] config: convert multi_replace to flags

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:03PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> 
> We will extend the flexibility of the config API. Before doing so, let's
> take an existing 'int multi_replace' parameter and replace it with a new
> 'unsigned flags' parameter that can take multiple options as a bit field.
> 
> Update all callers that specified multi_replace to now specify the
> CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
> documentation of git_config_set_multivar_in_file() including a clear
> labeling of its arguments. Other config API methods in config.h require
> only a change of the final parameter from 'int' to 'unsigned'.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> index e82301fb1b..5ce3844d22 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			die(_("Branch '%s' has no upstream information"), branch->name);
>  
>  		strbuf_addf(&buf, "branch.%s.remote", branch->name);
> -		git_config_set_multivar(buf.buf, NULL, NULL, 1);
> +		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
>  		strbuf_reset(&buf);
>  		strbuf_addf(&buf, "branch.%s.merge", branch->name);
> -		git_config_set_multivar(buf.buf, NULL, NULL, 1);
> +		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);

Converting callers. Straightforward.

[snipping more similar work]

> diff --git a/config.c b/config.c
> index 2b79fe76ad..096f2eae0d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2716,9 +2716,9 @@ void git_config_set(const char *key, const char *value)
>   * if value_regex!=NULL, disregard key/value pairs where value does not match.
>   * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
>   *     (only add a new one)
> - * if multi_replace==0, nothing, or only one matching key/value is replaced,
> - *     else all matching key/values (regardless how many) are removed,
> - *     before the new pair is written.
> + * if (flags & CONFIG_FLAGS_MULTI_REPLACE) == 0, at most one matching
> + *     key/value is replaced, else all matching key/values (regardless
> + *     how many) are removed, before the new pair is written.

This documentation to me sounds like the question you asked on-list the
other day: "does replace-all turn many configs into one, or many configs
into many with the same value?" Is it reflected in user-facing
documentation? Looks like no - you might have a good opportunity here to
make that more clear.

>   *
>   * Returns 0 on success.
>   *
> @@ -2739,7 +2739,7 @@ void git_config_set(const char *key, const char *value)
>  int git_config_set_multivar_in_file_gently(const char *config_filename,
>  					   const char *key, const char *value,
>  					   const char *value_regex,
> -					   int multi_replace)
> +					   unsigned flags)

Well, I wanted to complain about using 'unsigned' instead of 'unsigned
int', but 'git grep -P "unsigned(?! int)"' tells me that it's not a
thing anybody else seems to mind. So I'll just grumble in my corner
instead :)

>  {
>  	int fd = -1, in_fd = -1;
>  	int ret;
> @@ -2756,7 +2756,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>  	if (ret)
>  		goto out_free;
>  
> -	store.multi_replace = multi_replace;
> +	store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0;
>  
>  	if (!config_filename)
>  		config_filename = filename_buf = git_pathdup("config");
> @@ -2845,7 +2845,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>  
>  		/* if nothing to unset, or too many matches, error out */
>  		if ((store.seen_nr == 0 && value == NULL) ||
> -		    (store.seen_nr > 1 && multi_replace == 0)) {
> +		    (store.seen_nr > 1 && !store.multi_replace)) {

Huh, I wonder why 'store.multi_replace' wasn't used here before, since
it was bothered to be set at earlier. Ah well.

>  void git_config_set_multivar_in_file(const char *config_filename,
>  				     const char *key, const char *value,
> -				     const char *value_regex, int multi_replace)
> +				     const char *value_regex, unsigned flags)

And some more signature conversions. [snip]

>  /**
>   * takes four parameters:
> @@ -276,13 +289,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
>   * - the value regex, as a string. It will disregard key/value pairs where value
>   *   does not match.
>   *
> - * - a multi_replace value, as an int. If value is equal to zero, nothing or only
> - *   one matching key/value is replaced, else all matching key/values (regardless
> - *   how many) are removed, before the new pair is written.
> + * - a flags value with bits corresponding to the CONFIG_FLAG_* macros.
>   *
>   * It returns 0 on success.
>   */
> -void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
> +void git_config_set_multivar_in_file(const char *config_filename,
> +				     const char *key,
> +				     const char *value,
> +				     const char *value_regex,
> +				     unsigned flags);

Nice opportunity to make the header a little easier to read here.
Thanks.

With just one optional comment,

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

[PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git config' builtin takes a 'value_regex' parameter for several
actions. This can cause confusion when expecting exact value matches
instead of regex matches, especially when the input string contains glob
characters. While callers can escape the patterns themselves, it would
be more friendly to allow an argument to disable the pattern matching in
favor of an exact string match.

Add a new '--fixed-value' option that does not currently change the
behavior. The implementation will follow for each appropriate action.
For now, check and test that --fixed-value will abort the command when
included with an incompatible action or without a 'value_regex'
argument.

The name '--fixed-value' was chosen over something simpler like
'--fixed' because some commands allow regular expressions on the
key in addition to the value.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-config.txt | 20 +++++++++++++-------
 builtin/config.c             | 26 ++++++++++++++++++++++++++
 t/t1300-config.sh            | 23 +++++++++++++++++++++++
 3 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7573160f21..d4bb928ea7 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,15 +9,15 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [--type=<type>] --add name value
-'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] --unset name [value_regex]
-'git config' [<file-option>] --unset-all name [value_regex]
+'git config' [<file-option>] [--fixed-value] --unset name [value_regex]
+'git config' [<file-option>] [--fixed-value] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
@@ -165,6 +165,12 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
+--fixed-value::
+	When used with the `value_regex` argument, treat `value_regex` as
+	an exact string instead of a regular expression. This will restrict
+	the name/value pairs that are matched to only those where the value
+	is exactly equal to the `value_regex`.
+
 --type <type>::
   'git config' will ensure that any input or output is valid under the given
   type constraint(s), and will canonicalize outgoing values in `<type>`'s
diff --git a/builtin/config.c b/builtin/config.c
index e7c7f3d455..bfb55a96df 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
 static int show_scope;
+static int fixed_value;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -141,6 +142,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
+	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
 	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
@@ -745,6 +747,30 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	if (fixed_value) {
+		int allowed_usage = 0;
+
+		switch (actions) {
+		case ACTION_GET:
+		case ACTION_GET_ALL:
+		case ACTION_GET_REGEXP:
+		case ACTION_UNSET:
+		case ACTION_UNSET_ALL:
+			allowed_usage = argc > 1 && !!argv[1];
+			break;
+
+		case ACTION_SET_ALL:
+		case ACTION_REPLACE_ALL:
+			allowed_usage = argc > 2 && !!argv[2];
+			break;
+		}
+
+		if (!allowed_usage) {
+			error(_("--fixed-value only applies with 'value_regex'"));
+			usage_builtin_config();
+		}
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 8783767d4f..6dc8117241 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1960,4 +1960,27 @@ test_expect_success '--replace-all and value_regex' '
 	test_cmp expect actual
 '
 
+test_expect_success 'refuse --fixed-value for incompatible actions' '
+	git config --file=config dev.null bogus &&
+
+	# These modes do not allow --fixed-value at all
+	test_must_fail git config --file=config --fixed-value --add dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --rename-section dev null &&
+	test_must_fail git config --file=config --fixed-value --remove-section dev &&
+	test_must_fail git config --file=config --fixed-value --list &&
+	test_must_fail git config --file=config --fixed-value --get-color dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-colorbool dev.null &&
+
+	# These modes complain when --fixed-value has no value_regex
+	test_must_fail git config --file=config --fixed-value dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-all dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" &&
+	test_must_fail git config --file=config --fixed-value --unset dev.null &&
+	test_must_fail git config --file=config --fixed-value --unset-all dev.null
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	if (fixed_value) {
> +		int allowed_usage = 0;
> +
> +		switch (actions) {
> +		case ACTION_GET:
> +		case ACTION_GET_ALL:
> +		case ACTION_GET_REGEXP:
> +		case ACTION_UNSET:
> +		case ACTION_UNSET_ALL:
> +			allowed_usage = argc > 1 && !!argv[1];
> +			break;
> +
> +		case ACTION_SET_ALL:
> +		case ACTION_REPLACE_ALL:
> +			allowed_usage = argc > 2 && !!argv[2];
> +			break;

Implicitly all other ops do not want to work with the --fixed-value
option, which makes sense.

> +		}
> +
> +		if (!allowed_usage) {
> +			error(_("--fixed-value only applies with 'value_regex'"));
> +			usage_builtin_config();
> +		}
> +	}

I was afraid that abandoning the bitmask based approach of the
previous round may bloat the code but seeing the result here makes
me quite happy ;-)  It is quite clear what is going on.

Nice.

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:04PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> 
> The 'git config' builtin takes a 'value_regex' parameter for several
> actions. This can cause confusion when expecting exact value matches
> instead of regex matches, especially when the input string contains glob
> characters. While callers can escape the patterns themselves, it would
> be more friendly to allow an argument to disable the pattern matching in
> favor of an exact string match.
> 
> Add a new '--fixed-value' option that does not currently change the
> behavior. The implementation will follow for each appropriate action.
> For now, check and test that --fixed-value will abort the command when
> included with an incompatible action or without a 'value_regex'
> argument.
> 
> The name '--fixed-value' was chosen over something simpler like
> '--fixed' because some commands allow regular expressions on the
> key in addition to the value.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-config.txt | 20 +++++++++++++-------
>  builtin/config.c             | 26 ++++++++++++++++++++++++++
>  t/t1300-config.sh            | 23 +++++++++++++++++++++++
>  3 files changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 7573160f21..d4bb928ea7 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
>  static struct config_options config_options;
>  static int show_origin;
>  static int show_scope;
> +static int fixed_value;
>  
>  #define ACTION_GET (1<<0)
>  #define ACTION_GET_ALL (1<<1)
> @@ -141,6 +142,7 @@ static struct option builtin_config_options[] = {
>  	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
>  	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
>  	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
> +	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
I'm not sure how to feel about this phrasing. I wonder if it would be
clearer to say something like 'treat 'value_regex' as a literal string
instead'? Hmmm.

> +	if (fixed_value) {
> +		int allowed_usage = 0;
> +
> +		switch (actions) {
> +		case ACTION_GET:
> +		case ACTION_GET_ALL:
> +		case ACTION_GET_REGEXP:
> +		case ACTION_UNSET:
> +		case ACTION_UNSET_ALL:
> +			allowed_usage = argc > 1 && !!argv[1];
> +			break;
> +
> +		case ACTION_SET_ALL:
> +		case ACTION_REPLACE_ALL:
> +			allowed_usage = argc > 2 && !!argv[2];
> +			break;
> +		}
> +
> +		if (!allowed_usage) {
> +			error(_("--fixed-value only applies with 'value_regex'"));
> +			usage_builtin_config();
> +		}
> +	}
> +

To me this really needs a comment. I think you are checking whether the
value_regex is actually present, and the position of that regex changes
depending on the rest of the arg list... What about something like:

  /* If set, ensure 'value_regex' was provided also */
  if (fixed_value) {
    ...
    /* 'git config --unset-all <key> <value_regex>' */
    case ACTION_UNSET_ALL:
    ...
    /* 'git config --replace-all <key> <new value> <value_regex>' */
    case ACTION_REPLACE_ALL:
    ...

> +test_expect_success 'refuse --fixed-value for incompatible actions' '
> +	git config --file=config dev.null bogus &&
> +
> +	# These modes do not allow --fixed-value at all
> +	test_must_fail git config --file=config --fixed-value --add dev.null bogus &&
> +	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --file=config --fixed-value --rename-section dev null &&
> +	test_must_fail git config --file=config --fixed-value --remove-section dev &&
> +	test_must_fail git config --file=config --fixed-value --list &&
> +	test_must_fail git config --file=config --fixed-value --get-color dev.null &&
> +	test_must_fail git config --file=config --fixed-value --get-colorbool dev.null &&
> +
> +	# These modes complain when --fixed-value has no value_regex
> +	test_must_fail git config --file=config --fixed-value dev.null bogus &&
> +	test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus &&
> +	test_must_fail git config --file=config --fixed-value --get dev.null &&
> +	test_must_fail git config --file=config --fixed-value --get-all dev.null &&
> +	test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" &&
> +	test_must_fail git config --file=config --fixed-value --unset dev.null &&
> +	test_must_fail git config --file=config --fixed-value --unset-all dev.null
> +'
> +

I'd expect to see a positive test as well, which you could later evolve
into a test to ensure behavior.

 - Emily

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

>>  	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
>> +	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
> I'm not sure how to feel about this phrasing. I wonder if it would be
> clearer to say something like 'treat 'value_regex' as a literal string
> instead'? Hmmm.

Update the document and help text with s/value_regex/value_pattern/
and say "use value_pattern as a fixed string, not an extended regexp",
perhaps?

>   /* If set, ensure 'value_regex' was provided also */
>   if (fixed_value) {
>     ...
>     /* 'git config --unset-all <key> <value_regex>' */
>     case ACTION_UNSET_ALL:

Nice.

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Derrick Stolee]

On 11/23/2020 5:41 PM, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
>>>  	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
>>> +	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
>> I'm not sure how to feel about this phrasing. I wonder if it would be
>> clearer to say something like 'treat 'value_regex' as a literal string
>> instead'? Hmmm.
> 
> Update the document and help text with s/value_regex/value_pattern/
> and say "use value_pattern as a fixed string, not an extended regexp",
> perhaps?

If I go about changing all documentation and error messages to say
"value_pattern" instead of "value_regex", should I also update the uses
in the *.po translation files? Or, should I leave them unmodified to
trigger manual intervention by the translators?

Thanks,
-Stolee

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Derrick Stolee]

On 11/25/2020 9:08 AM, Derrick Stolee wrote:
> On 11/23/2020 5:41 PM, Junio C Hamano wrote:
>> Emily Shaffer <emilyshaffer@google.com> writes:
>>
>>>>  	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
>>>> +	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
>>> I'm not sure how to feel about this phrasing. I wonder if it would be
>>> clearer to say something like 'treat 'value_regex' as a literal string
>>> instead'? Hmmm.
>>
>> Update the document and help text with s/value_regex/value_pattern/
>> and say "use value_pattern as a fixed string, not an extended regexp",
>> perhaps?
> 
> If I go about changing all documentation and error messages to say
> "value_pattern" instead of "value_regex", should I also update the uses
> in the *.po translation files? Or, should I leave them unmodified to
> trigger manual intervention by the translators?

I have discovered that changing these causes conflicts with automated
line number updates. I'll leave these unchanged with a note for
translators.

Thanks,
-Stolee

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Eric Sunshine]

On Wed, Nov 25, 2020 at 9:09 AM Derrick Stolee <stolee@gmail.com> wrote:
> On 11/23/2020 5:41 PM, Junio C Hamano wrote:
> > Update the document and help text with s/value_regex/value_pattern/
> > and say "use value_pattern as a fixed string, not an extended regexp",
> > perhaps?
>
> If I go about changing all documentation and error messages to say
> "value_pattern" instead of "value_regex", should I also update the uses
> in the *.po translation files? Or, should I leave them unmodified to
> trigger manual intervention by the translators?

A minor request: If you are going to put in the work to make that
substitution, perhaps change it to "value-pattern" rather than
"value_pattern" since a hyphen is more common in documentation for
this sort of thing than underscore.

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Nov 25, 2020 at 9:09 AM Derrick Stolee <stolee@gmail.com> wrote:
>> On 11/23/2020 5:41 PM, Junio C Hamano wrote:
>> > Update the document and help text with s/value_regex/value_pattern/
>> > and say "use value_pattern as a fixed string, not an extended regexp",
>> > perhaps?
>>
>> If I go about changing all documentation and error messages to say
>> "value_pattern" instead of "value_regex", should I also update the uses
>> in the *.po translation files? Or, should I leave them unmodified to
>> trigger manual intervention by the translators?
>
> A minor request: If you are going to put in the work to make that
> substitution, perhaps change it to "value-pattern" rather than
> "value_pattern" since a hyphen is more common in documentation for
> this sort of thing than underscore.

Yup, I misspoke.  s/value_regex/value-regex/ unless it is a variable
name where '-' cannot appear.

Re: [PATCH v2 4/7] config: add --fixed-value option, un-implemented

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 11/23/2020 5:41 PM, Junio C Hamano wrote:
>> Emily Shaffer <emilyshaffer@google.com> writes:
>> 
>>>>  	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
>>>> +	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
>>> I'm not sure how to feel about this phrasing. I wonder if it would be
>>> clearer to say something like 'treat 'value_regex' as a literal string
>>> instead'? Hmmm.
>> 
>> Update the document and help text with s/value_regex/value_pattern/
>> and say "use value_pattern as a fixed string, not an extended regexp",
>> perhaps?
>
> If I go about changing all documentation and error messages to say
> "value_pattern" instead of "value_regex", should I also update the uses
> in the *.po translation files? Or, should I leave them unmodified to
> trigger manual intervention by the translators?

If you do, you do not have to worry.  The i18n/l10n coordinator will
update the po/git.pot file when we near the code freeze using an
automated tool that extracts strings from the sources, and the
po/xx.po files for languages are updated from the updated po/git.pot
mechanically with another tool, reusing unmodified entries, adding
new ones, and marking near-hit ones to help avoid unnecessary work
by translators.

I earlier thought that the "when matching values" phrase you chose
in this round, without such a clean-up, would be OK in the context
of this topic (which is depended on by a bugfix topic), but after
seeing how we need to clarify the way '!' negation prefix works in
the documentation that has deeply ingrained assumption that value is
matched using ERE, it may be necessary to bite the bullet and do the
regex->pattern now.

Thanks.

[PATCH v2 5/7] config: plumb --fixed-value into config API

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The git_config_set_multivar_in_file_gently() and related methods now
take a 'flags' bitfield, so add a new bit representing the --fixed-value
option from 'git config'. This alters the purpose of the value_regex
parameter to be an exact string match. This requires some initialization
changes in git_config_set_multivar_in_file_gently() and a new strcmp()
call in the matches() method.

The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
based on the --fixed-value option, and that needs to be updated in
several callers.

This patch only affects some of the modes of 'git config', and the rest
will be completed in the next change.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 16 ++++++++-----
 config.c          |  5 +++++
 config.h          |  7 ++++++
 t/t1300-config.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index bfb55a96df..3e49e04411 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -616,6 +616,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
 	char *value;
+	int flags = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -769,6 +770,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			error(_("--fixed-value only applies with 'value_regex'"));
 			usage_builtin_config();
 		}
+
+		flags = CONFIG_FLAGS_FIXED_VALUE;
 	}
 
 	if (actions & PAGING_ACTIONS)
@@ -832,7 +835,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 0);
+							      argv[0], value, argv[2],
+							      flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
@@ -841,7 +845,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value,
-							      CONFIG_REGEX_NONE, 0);
+							      CONFIG_REGEX_NONE,
+							      flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
@@ -850,7 +855,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value, argv[2],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -877,7 +882,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file_gently(given_config_source.file,
-								      argv[0], NULL, argv[1], 0);
+								      argv[0], NULL, argv[1],
+								      flags);
 		else
 			return git_config_set_in_file_gently(given_config_source.file,
 							     argv[0], NULL);
@@ -887,7 +893,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], NULL, argv[1],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/config.c b/config.c
index 096f2eae0d..64bab2b08e 100644
--- a/config.c
+++ b/config.c
@@ -2402,6 +2402,7 @@ struct config_store_data {
 	size_t baselen;
 	char *key;
 	int do_not_match;
+	const char *literal_value;
 	regex_t *value_regex;
 	int multi_replace;
 	struct {
@@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
+	if (store->literal_value)
+		return !strcmp(store->literal_value, value);
 	if (!store->value_regex)
 		return 1; /* always matches */
 	if (store->value_regex == CONFIG_REGEX_NONE)
@@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			store.value_regex = NULL;
 		else if (value_regex == CONFIG_REGEX_NONE)
 			store.value_regex = CONFIG_REGEX_NONE;
+		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
+			store.literal_value = value_regex;
 		else {
 			if (value_regex[0] == '!') {
 				store.do_not_match = 1;
diff --git a/config.h b/config.h
index 80844604ab..977e690be8 100644
--- a/config.h
+++ b/config.h
@@ -269,6 +269,13 @@ int git_config_key_is_valid(const char *key);
  */
 #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
 
+/*
+ * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
+ * by string comparison (not regex match) to the provided value_regex
+ * parameter.
+ */
+#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
+
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 6dc8117241..30e80ae9cb 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1983,4 +1983,61 @@ test_expect_success 'refuse --fixed-value for incompatible actions' '
 	test_must_fail git config --file=config --fixed-value --unset-all dev.null
 '
 
+test_expect_success '--fixed-value uses exact string matching' '
+	GLOB="a+b*c?d[e]f.g" &&
+	rm -f initial &&
+	git config --file=initial fixed.test "$GLOB" &&
+
+	cp initial config &&
+	git config --file=config fixed.test bogus "$GLOB" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=$GLOB
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	cp initial config &&
+	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
+	git config --file=config --list >actual &&
+	printf "fixed.test=bogus\n" >expect &&
+	test_cmp expect actual &&
+
+	cp initial config &&
+	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
+	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
+	test_must_fail git config --file=config fixed.test &&
+
+	cp initial config &&
+	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
+	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
+	test_must_fail git config --file=config fixed.test &&
+
+	cp initial config &&
+	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=$GLOB
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	cp initial config &&
+	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=$GLOB
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	git config --file=config --fixed-value --replace-all fixed.test bogus "$GLOB" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=bogus
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> 
> The git_config_set_multivar_in_file_gently() and related methods now
> take a 'flags' bitfield, so add a new bit representing the --fixed-value
> option from 'git config'. This alters the purpose of the value_regex
> parameter to be an exact string match. This requires some initialization
> changes in git_config_set_multivar_in_file_gently() and a new strcmp()
> call in the matches() method.
> 
> The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
> based on the --fixed-value option, and that needs to be updated in
> several callers.
> 
> This patch only affects some of the modes of 'git config', and the rest
> will be completed in the next change.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/config.c  | 16 ++++++++-----
>  config.c          |  5 +++++
>  config.h          |  7 ++++++
>  t/t1300-config.sh | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index bfb55a96df..3e49e04411 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -616,6 +616,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit = !startup_info->have_repository;
>  	char *value;
> +	int flags = 0;
>  
>  	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
>  
> @@ -769,6 +770,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  			error(_("--fixed-value only applies with 'value_regex'"));
>  			usage_builtin_config();
>  		}
> +
> +		flags = CONFIG_FLAGS_FIXED_VALUE;

I wonder whether using |= here will save someone from a headache later,
when they want to add another flag value or move the
CONFIG_FLAGS_MULTI_REPLACE calculation out of the tail calls below.

[snip]
>  	}
>  	else if (actions == ACTION_REPLACE_ALL) {
>  		check_write();
> @@ -850,7 +855,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		UNLEAK(value);
>  		return git_config_set_multivar_in_file_gently(given_config_source.file,
>  							      argv[0], value, argv[2],
> -							      CONFIG_FLAGS_MULTI_REPLACE);
> +							      flags | CONFIG_FLAGS_MULTI_REPLACE);
[snip]
> @@ -887,7 +893,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		check_argc(argc, 1, 2);
>  		return git_config_set_multivar_in_file_gently(given_config_source.file,
>  							      argv[0], NULL, argv[1],
> -							      CONFIG_FLAGS_MULTI_REPLACE);
> +							      flags | CONFIG_FLAGS_MULTI_REPLACE);

> --- a/config.c
> +++ b/config.c
> @@ -2402,6 +2402,7 @@ struct config_store_data {
>  	size_t baselen;
>  	char *key;
>  	int do_not_match;
> +	const char *literal_value;
>  	regex_t *value_regex;
>  	int multi_replace;
>  	struct {
> @@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
>  {
>  	if (strcmp(key, store->key))
>  		return 0; /* not ours */
> +	if (store->literal_value)
> +		return !strcmp(store->literal_value, value);

Nice. Short-circuiting the entire regex process if necessary.

> @@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>  			store.value_regex = NULL;
>  		else if (value_regex == CONFIG_REGEX_NONE)
>  			store.value_regex = CONFIG_REGEX_NONE;
> +		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
> +			store.literal_value = value_regex;

Ah, so we use .literal_value instead of pulling the string from
value_regex because value_regex undergoes some special parsing and is
packed into a regex_t instead.

>  		else {
>  			if (value_regex[0] == '!') {
>  				store.do_not_match = 1;
> diff --git a/config.h b/config.h
> index 80844604ab..977e690be8 100644
> --- a/config.h
> +++ b/config.h
> @@ -269,6 +269,13 @@ int git_config_key_is_valid(const char *key);
>   */
>  #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
>  
> +/*
> + * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
> + * by string comparison (not regex match) to the provided value_regex
In contrast to my comment elsewhere, I think here it makes sense to talk
about strcmp like you did. But notice that here in the dev-facing
documentation you said "not regex" - so I think that you thought it was
a useful distinction, but forgot to say so to the user.
> + * parameter.
> + */
> +#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
> +

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 6dc8117241..30e80ae9cb 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> +test_expect_success '--fixed-value uses exact string matching' '
> +	GLOB="a+b*c?d[e]f.g" &&
> +	rm -f initial &&
This tells me that when you used 'initial' earlier you probably should
have done something like 'test_when_finished rm initial' in this test
and your earlier ones. Whoops.

> +	git config --file=initial fixed.test "$GLOB" &&
> +
> +	cp initial config &&
> +	git config --file=config fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&
> +
> +	cp initial config &&
> +	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	printf "fixed.test=bogus\n" >expect &&
It is jarring to me to see a printf here when everywhere else we use
heredocs. 'git grep' tells me it's not unheard of, but it looks like
those are cases where the whole file doesn't use heredocs.

> +	test_cmp expect actual &&
> +
> +	cp initial config &&
> +	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
> +	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config fixed.test &&
Is this one supposed to verify that there is a 'fixed.test' value
already in 'config'? I'd prefer to see that explicitly checked with 'git
config --get' rather than watching for a symptom, that is, fail to set.
This comment applies to the next case too.

> +
> +	cp initial config &&
> +	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
> +	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config fixed.test &&
> +
> +	cp initial config &&
> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&
Hm, isn't this the same functionality as the tests you added at the
beginning of this series? I guess you are setting up for the last case
with --replace-all...
> +
> +	cp initial config &&
> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=$GLOB
> +	fixed.test=bogus
> +	EOF
> +	test_cmp expect actual &&

Is this one identical to the previous one? I think it is, but if it
isn't and I can't tell, all the more reason that each case here should
either be labeled with a comment or separated into its own test. (Bonus
- you could extend the individual tests from patch 1 to make sure they
work correctly with --fixed-value too ;) )

> +
> +	git config --file=config --fixed-value --replace-all fixed.test bogus "$GLOB" &&
> +	git config --file=config --list >actual &&
> +	cat >expect <<-EOF &&
> +	fixed.test=bogus
> +	fixed.test=bogus

Hum, it does what it says on the box, but is this a valid config, I
wonder?

  $ git config --file=test foo.test aaa
  $ git config --file=test --add foo.test bbb
  $ git config --list --file=test
  foo.test=aaa
  foo.test=bbb
  $ git config --file=test --replace-all foo.test bbb a+
  $ git config --list --file=test
  foo.test=bbb
  foo.test=bbb

So I guess it's fine :)

 - Emily

Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

[Eric Sunshine]

On Mon, Nov 23, 2020 at 5:22 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
> > +     printf "fixed.test=bogus\n" >expect &&
> It is jarring to me to see a printf here when everywhere else we use
> heredocs. 'git grep' tells me it's not unheard of, but it looks like
> those are cases where the whole file doesn't use heredocs.

For creating a one-line file, the verbosity of a here-doc may be
overkill. At any rate, `echo` would be more idiomatic than `printf`:

    echo "fixed.test=bogus" >expect &&

(Though, this alone is likely not worth a re-roll.)

Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

[Derrick Stolee]

On 11/23/2020 5:21 PM, Emily Shaffer wrote:
> On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +
>> +		flags = CONFIG_FLAGS_FIXED_VALUE;
> 
> I wonder whether using |= here will save someone from a headache later,
> when they want to add another flag value or move the
> CONFIG_FLAGS_MULTI_REPLACE calculation out of the tail calls below.

Good catch.

>> @@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
>>  			store.value_regex = NULL;
>>  		else if (value_regex == CONFIG_REGEX_NONE)
>>  			store.value_regex = CONFIG_REGEX_NONE;
>> +		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
>> +			store.literal_value = value_regex;
> 
> Ah, so we use .literal_value instead of pulling the string from
> value_regex because value_regex undergoes some special parsing and is
> packed into a regex_t instead.

Reminds me to call this 'fixed_value'.

>> +test_expect_success '--fixed-value uses exact string matching' '
>> +	GLOB="a+b*c?d[e]f.g" &&
>> +	rm -f initial &&
> This tells me that when you used 'initial' earlier you probably should
> have done something like 'test_when_finished rm initial' in this test
> and your earlier ones. Whoops.

Good idea. Will do.

>> +	cp initial config &&
>> +	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	printf "fixed.test=bogus\n" >expect &&
> It is jarring to me to see a printf here when everywhere else we use
> heredocs. 'git grep' tells me it's not unheard of, but it looks like
> those are cases where the whole file doesn't use heredocs.

I can use a heredoc just to be consistent.

(To also respond to Eric's message, I tend to use printf instead
of echo because echo starts a process while printf does not.)

>> +	test_cmp expect actual &&
>> +
>> +	cp initial config &&
>> +	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
>> +	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
>> +	test_must_fail git config --file=config fixed.test &&
> Is this one supposed to verify that there is a 'fixed.test' value
> already in 'config'? I'd prefer to see that explicitly checked with 'git
> config --get' rather than watching for a symptom, that is, fail to set.
> This comment applies to the next case too.

If no value is provided, then 'git config <name>' _is_ a query. It's not a
failed set. 'git config <name> ""' would be the way to try and set the value
to an empty string.

>> +
>> +	cp initial config &&
>> +	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
>> +	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
>> +	test_must_fail git config --file=config fixed.test &&
>> +
>> +	cp initial config &&
>> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	cat >expect <<-EOF &&
>> +	fixed.test=$GLOB
>> +	fixed.test=bogus
>> +	EOF
>> +	test_cmp expect actual &&
> Hm, isn't this the same functionality as the tests you added at the
> beginning of this series? I guess you are setting up for the last case
> with --replace-all...

This is specifically demonstrating the difference that --fixed-value
provides. The tests here show "the match doesn't work by default, but
then works with --fixed-value". I'll make this clearer in the commit
message.

>> +
>> +	cp initial config &&
>> +	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
>> +	git config --file=config --list >actual &&
>> +	cat >expect <<-EOF &&
>> +	fixed.test=$GLOB
>> +	fixed.test=bogus
>> +	EOF
>> +	test_cmp expect actual &&
> 
> Is this one identical to the previous one? I think it is, but if it
> isn't and I can't tell, all the more reason that each case here should
> either be labeled with a comment or separated into its own test. (Bonus
> - you could extend the individual tests from patch 1 to make sure they
> work correctly with --fixed-value too ;) )

Yes, accidentally over-zealous copy/paste.

I'm less in favor of splitting the tests, since they rely on a shared
initial config. Any failure in one part of this test is likely to also
fail the rest of the commands, so grouping them by this toggle makes
sense to me.

Thanks,
-Stolee

Re: [PATCH v2 5/7] config: plumb --fixed-value into config API

[Eric Sunshine]

On Wed, Nov 25, 2020 at 10:49 AM Derrick Stolee <stolee@gmail.com> wrote:
> On 11/23/2020 5:21 PM, Emily Shaffer wrote:
> > On Mon, Nov 23, 2020 at 04:05:05PM +0000, Derrick Stolee via GitGitGadget wrote:
> >> +    printf "fixed.test=bogus\n" >expect &&
> > It is jarring to me to see a printf here when everywhere else we use
> > heredocs. 'git grep' tells me it's not unheard of, but it looks like
> > those are cases where the whole file doesn't use heredocs.
>
> I can use a heredoc just to be consistent.
>
> (To also respond to Eric's message, I tend to use printf instead
> of echo because echo starts a process while printf does not.)

`echo` is a builtin[1] in many or most shells (just as `printf` also
often is), thus is unlikely to start a process.

[1] Aside: You can use `type <cmd>` or `command -V <cmd>` to see if
<cmd> is a builtin in your shell.

[PATCH v2 6/7] config: implement --fixed-value with --get*

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The config builtin does its own regex matching of values for the --get,
--get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
to the get_value() method so we can initialize the value_regex argument
as a fixed string instead of a regex pattern.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 15 ++++++++++-----
 t/t1300-config.sh | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 3e49e04411..d3772b5efe 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -14,6 +14,7 @@ static const char *const builtin_config_usage[] = {
 
 static char *key;
 static regex_t *key_regexp;
+static const char *value_regex;
 static regex_t *regexp;
 static int show_keys;
 static int omit_values;
@@ -288,6 +289,8 @@ 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 (fixed_value && strcmp(value_regex, (value_?value_:"")))
+		return 0;
 	if (regexp != NULL &&
 	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
@@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	return format_config(&values->items[values->nr++], key_, value_);
 }
 
-static int get_value(const char *key_, const char *regex_)
+static int get_value(const char *key_, const char *regex_, int flags)
 {
 	int ret = CONFIG_GENERIC_ERROR;
 	struct strbuf_list values = {NULL};
@@ -335,7 +338,9 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	if (regex_) {
+	if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE))
+		value_regex = regex_;
+	else if (regex_) {
 		if (regex_[0] == '!') {
 			do_not_match = 1;
 			regex_++;
@@ -859,19 +864,19 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_URLMATCH) {
 		check_argc(argc, 2, 2);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 30e80ae9cb..13211b6bf4 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2040,4 +2040,26 @@ test_expect_success '--fixed-value uses exact string matching' '
 	test_cmp expect actual
 '
 
+test_expect_success '--get and --get-all with --fixed-value' '
+	GLOB="a+b*c?d[e]f.g" &&
+	rm -f config &&
+	git config --file=config fixed.test bogus &&
+	git config --file=config --add fixed.test "$GLOB" &&
+
+	git config --file=config --get fixed.test bogus &&
+	test_must_fail git config --file=config --get fixed.test "$GLOB" &&
+	git config --file=config --get --fixed-value fixed.test "$GLOB" &&
+	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
+
+	git config --file=config --get-all fixed.test bogus &&
+	test_must_fail git config --file=config --get-all fixed.test "$GLOB" &&
+	git config --file=config --get-all --fixed-value fixed.test "$GLOB" &&
+	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
+
+	git config --file=config --get-regexp fixed+ bogus &&
+	test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" &&
+	git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" &&
+	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 6/7] config: implement --fixed-value with --get*

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -	if (regex_) {
> +	if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE))
> +		value_regex = regex_;
> +	else if (regex_) {
>  		if (regex_[0] == '!') {
>  			do_not_match = 1;
>  			regex_++;

When the --fixed-value option is in effect, there is no way to say
"replace values that do not match this string", because unlike the
regex case, we do not special case '!' at the beginning.

I think it is a good design decision.  The special case has an
escape hatch to start your value_regex with "[!]" when ERE is in
use, but there is no such escape hatch possible with --fixed-value.

Depending on how this '!' negation is documented to the end-users
for existing value_regex that is ERE, the documentation for the new
option may want to talk about the lack of negation explicitly.

	... goes and looks ...

It is worse than I thought.  Here is what "git config" description
has (and some of the badness is not the fault of this series):

    Multiple lines can be added to an option by using the `--add` option.
    If you want to update or unset an option which can occur on multiple
    lines, a POSIX regexp `value_regex` needs to be given.  Only the
    existing values that match the regexp are updated or unset.  If
    you want to handle the lines that do *not* match the regex, just
    prepend a single exclamation mark in front (see also <<EXAMPLES>>).

The end-users these days no longer see "lines" because they do not
hand edit .git/config, so we may need a replacement phrase to refer
to a single "vari.able=value" setting, but we should leave it out of
the scope of this series to clean that up---"git config --help" is
full of references to "line".

It was not "a POSIX regexp" (which does not say if it is BRE or
ERE), and it no longer is a regexp with the new option.

    ... which can occur on multiple lines, a pattern `value_regex`
    needs to be given.  Only the existing values that match the
    pattern are updated or unset.  The pattern is by default matched
    as an extended regular expression, but with the `--fixed-value`
    option, taken as a literal string and values must be identical
    to it to be considered a match.  You can prepend an exclamation
    point '!' to affect lines that do *not* match the pattern, but
    this is applicable only when not using the `--fixed-value`
    option.

Ideally we probably should do s/value_regex/value_pattern/ in the
documentation and error messages eventually, but I do not know if
it is warranted in the scope of this series.

Re: [PATCH v2 6/7] config: implement --fixed-value with --get*

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:06PM +0000, Derrick Stolee via GitGitGadget wrote:
> 

Allowing myself a sensible chuckle at the commit subject using glob
syntax on a series about regex matching. ;)

> 
> The config builtin does its own regex matching of values for the --get,
> --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
> to the get_value() method so we can initialize the value_regex argument
> as a fixed string instead of a regex pattern.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/config.c  | 15 ++++++++++-----
>  t/t1300-config.sh | 22 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 3e49e04411..d3772b5efe 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
>  	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
>  		return 0;
> +	if (fixed_value && strcmp(value_regex, (value_?value_:"")))
> +		return 0;
Ooh, I can see you're matching style, but the combination of the
spaceless ternary and the trailing underscore is making me so itchy ;)

>  	if (regexp != NULL &&
>  	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
>  		return 0;
> @@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
>  	return format_config(&values->items[values->nr++], key_, value_);
>  }
>  
> -static int get_value(const char *key_, const char *regex_)
> +static int get_value(const char *key_, const char *regex_, int flags)
Very reasonable, and I appreciate passing 'flags' instead of passing
'fixed' here. Room for growth :)

[snip] a bunch of straightforward plumbing-through.

> +test_expect_success '--get and --get-all with --fixed-value' '
> +	GLOB="a+b*c?d[e]f.g" &&
> +	rm -f config &&
Again this tells me that your other tests want 'test_when_finished'
instead.

> +	git config --file=config fixed.test bogus &&
> +	git config --file=config --add fixed.test "$GLOB" &&
> +
> +	git config --file=config --get fixed.test bogus &&
> +	test_must_fail git config --file=config --get fixed.test "$GLOB" &&
> +	git config --file=config --get --fixed-value fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
> +
> +	git config --file=config --get-all fixed.test bogus &&
> +	test_must_fail git config --file=config --get-all fixed.test "$GLOB" &&
> +	git config --file=config --get-all --fixed-value fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
> +
> +	git config --file=config --get-regexp fixed+ bogus &&
> +	test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" &&
> +	git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" &&
> +	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
> +'

Otherwise the test seems fine to me, although I wouldn't yell if it grew
some comments :)

With the exception of the 'test_when_finished', which might not even
match style (I didn't look):

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

[PATCH v2 7/7] maintenance: use 'git config --fixed-value'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

When a repository's leading directories contain regex glob characters,
the config calls for 'git maintenance register' and 'git maintenance
unregister' are not careful enough. Use the new --fixed-value option
to direct the config machinery to use exact string matches. This is a
more robust option than excaping these arguments in a piecemeal fashion.

For the test, require that we are not running on Windows since the '+'
character is not allowed on that filesystem.

Reported-by: Emily Shaffer <emilyshaffer@google.com>
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/gc.c           |  5 +++--
 t/t7900-maintenance.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e3098ef6a1..6dde3ce1bb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1452,7 +1452,8 @@ static int maintenance_register(void)
 		git_config_set("maintenance.strategy", "incremental");
 
 	config_get.git_cmd = 1;
-	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
+	strvec_pushl(&config_get.args, "config", "--global", "--get",
+		     "--fixed-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 			 NULL);
@@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
 
 	config_unset.git_cmd = 1;
 	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "maintenance.repo",
+		     "--fixed-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 		     NULL);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 20184e96e1..c4e5564c31 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
+test_expect_success !MINGW 'register and unregister with glob characters' '
+	GLOB="a+b*c" &&
+	git init "$GLOB" &&
+	git -C "$GLOB" maintenance register &&
+	git config --get-all --show-origin maintenance.repo &&
+	git config --get-all --global --fixed-value \
+		maintenance.repo "$(pwd)/$GLOB" &&
+	git -C "$GLOB" maintenance unregister &&
+	test_must_fail git config --get-all --global --fixed-value \
+		maintenance.repo "$(pwd)/$GLOB"
+'
+
 test_expect_success 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
-- 
gitgitgadget

Re: [PATCH v2 7/7] maintenance: use 'git config --fixed-value'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> For the test, require that we are not running on Windows since the '+'
> character is not allowed on that filesystem.

Not a huge deal, but we do not have to rely on '+', though.  Using
"a*b*c" instead of "a+b*c" would perfectly be fine.  It would only
match "a*b*c" literally and not as ERE.


> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Reported-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/gc.c           |  5 +++--
>  t/t7900-maintenance.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index e3098ef6a1..6dde3ce1bb 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1452,7 +1452,8 @@ static int maintenance_register(void)
>  		git_config_set("maintenance.strategy", "incremental");
>  
>  	config_get.git_cmd = 1;
> -	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
> +	strvec_pushl(&config_get.args, "config", "--global", "--get",
> +		     "--fixed-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  			 NULL);
> @@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
>  
>  	config_unset.git_cmd = 1;
>  	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "maintenance.repo",
> +		     "--fixed-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  		     NULL);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 20184e96e1..c4e5564c31 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
>  	test_cmp before actual
>  '
>  
> +test_expect_success !MINGW 'register and unregister with glob characters' '
> +	GLOB="a+b*c" &&
> +	git init "$GLOB" &&
> +	git -C "$GLOB" maintenance register &&
> +	git config --get-all --show-origin maintenance.repo &&
> +	git config --get-all --global --fixed-value \
> +		maintenance.repo "$(pwd)/$GLOB" &&
> +	git -C "$GLOB" maintenance unregister &&
> +	test_must_fail git config --get-all --global --fixed-value \
> +		maintenance.repo "$(pwd)/$GLOB"
> +'
> +
>  test_expect_success 'start from empty cron table' '
>  	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&

Re: [PATCH v2 7/7] maintenance: use 'git config --fixed-value'

[Emily Shaffer]

On Mon, Nov 23, 2020 at 04:05:07PM +0000, Derrick Stolee via GitGitGadget wrote:
> 
> 
> When a repository's leading directories contain regex glob characters,
Minor "well, actually" - I'm not sure 'glob' is the right word to use
here.

> the config calls for 'git maintenance register' and 'git maintenance
> unregister' are not careful enough. Use the new --fixed-value option
> to direct the config machinery to use exact string matches. This is a
> more robust option than excaping these arguments in a piecemeal fashion.
Typo on 'escaping'.

> 
> For the test, require that we are not running on Windows since the '+'
> character is not allowed on that filesystem.
> 
> Reported-by: Emily Shaffer <emilyshaffer@google.com>
> Reported-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/gc.c           |  5 +++--
>  t/t7900-maintenance.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index e3098ef6a1..6dde3ce1bb 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1452,7 +1452,8 @@ static int maintenance_register(void)
>  		git_config_set("maintenance.strategy", "incremental");
>  
>  	config_get.git_cmd = 1;
> -	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
> +	strvec_pushl(&config_get.args, "config", "--global", "--get",
> +		     "--fixed-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  			 NULL);
> @@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
>  
>  	config_unset.git_cmd = 1;
>  	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
> -		     "maintenance.repo",
> +		     "--fixed-value", "maintenance.repo",
>  		     the_repository->worktree ? the_repository->worktree
>  					      : the_repository->gitdir,
>  		     NULL);
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 20184e96e1..c4e5564c31 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
>  	test_cmp before actual
>  '
>  
> +test_expect_success !MINGW 'register and unregister with glob characters' '
> +	GLOB="a+b*c" &&
> +	git init "$GLOB" &&
> +	git -C "$GLOB" maintenance register &&
> +	git config --get-all --show-origin maintenance.repo &&
Hm, what's the reason for --show-origin when the output isn't captured
or checked? Is this leftover?

> +	git config --get-all --global --fixed-value \
> +		maintenance.repo "$(pwd)/$GLOB" &&
> +	git -C "$GLOB" maintenance unregister &&
> +	test_must_fail git config --get-all --global --fixed-value \
> +		maintenance.repo "$(pwd)/$GLOB"
> +'
> +
>  test_expect_success 'start from empty cron table' '
>  	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
>  

And after all that buildup, this patch is a very straightforward test
fix: explain the problem, add the arg in question, add a regression
test. Thanks.

Other than the one extraneous(?) line,
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

Re: [PATCH v2 7/7] maintenance: use 'git config --fixed-value'

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Mon, Nov 23, 2020 at 04:05:07PM +0000, Derrick Stolee via GitGitGadget wrote:
>> 
>> 
>> When a repository's leading directories contain regex glob characters,
> Minor "well, actually" - I'm not sure 'glob' is the right word to use
> here.

A careful reader ;-)  "metacharacters" is probably what we want.

Re: [PATCH v2 0/7] config: add --fixed-value option

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  * Updated the test structure for the new tests in t1300-config.sh

Patches [1/7] and [2/7] got much less scary with this change.

Nice.

Thanks, will replace (but with other topics depend on this series,
it may take a bit more time than usual).

[PATCH v3 0/8] config: add --fixed-value option

[Derrick Stolee via GitGitGadget]

As reported [1], 'git maintenance unregister' fails when a repository is
located in a directory with regex glob characters.

[1] 
https://lore.kernel.org/git/2c2db228-069a-947d-8446-89f4d3f6181a@gmail.com/T/#mb96fa4187a0d6aeda097cd95804a8aafc0273022

The discussed solution was to modify 'git config' to specify that the
'value_regex' argument should be treated as an exact string match. This is
the primary change in this series, with an additional patch at the end to
make 'git maintenance [un]register' use this option, when necessary.

While we're here, let's rename 'value_regex' to 'value_pattern' to make
things a bit clearer.

Updates in V3
=============

 * Renamed 'value_regex' to 'value_pattern' in code and 'value-pattern' in
   docs (except po/)
   
   
 * Reordered commits slightly to help with that rename.
   
   
 * Updated tests to use 'test_when_finished rm -f ...'
   
   
 * Changed all references to "glob" characters to "meta" characters.
   
   
 * Several other test modifications. Thanks, Emily, for the review!
   
   

Thanks, -Stolee

P.S. Happy Thanksgiving to those celebrating!

Derrick Stolee (8):
  config: convert multi_replace to flags
  config: replace 'value_regex' with 'value_pattern'
  t1300: test "set all" mode with value-pattern
  t1300: add test for --replace-all with value-pattern
  config: add --fixed-value option, un-implemented
  config: plumb --fixed-value into config API
  config: implement --fixed-value with --get*
  maintenance: use 'git config --fixed-value'

 Documentation/git-config.txt |  26 +++---
 builtin/branch.c             |   4 +-
 builtin/config.c             |  81 +++++++++++++++----
 builtin/gc.c                 |   5 +-
 builtin/remote.c             |   8 +-
 config.c                     |  75 ++++++++++--------
 config.h                     |  36 +++++++--
 t/t1300-config.sh            | 149 +++++++++++++++++++++++++++++++++++
 t/t7900-maintenance.sh       |  12 +++
 9 files changed, 321 insertions(+), 75 deletions(-)


base-commit: 0016b618182f642771dc589cf0090289f9fe1b4f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-796%2Fderrickstolee%2Fmaintenance%2Fconfig-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-796/derrickstolee/maintenance/config-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/796

Range-diff vs v2:

 3:  0c152faa00 ! 1:  f0ed492096 config: convert multi_replace to flags
     @@ config.c: void git_config_set(const char *key, const char *value)
      - * if multi_replace==0, nothing, or only one matching key/value is replaced,
      - *     else all matching key/values (regardless how many) are removed,
      - *     before the new pair is written.
     -+ * if (flags & CONFIG_FLAGS_MULTI_REPLACE) == 0, at most one matching
     -+ *     key/value is replaced, else all matching key/values (regardless
     -+ *     how many) are removed, before the new pair is written.
     ++ * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching
     ++ *     key/values are removed before a single new pair is written. If the
     ++ *     flag is not present, then replace only the first match.
        *
        * Returns 0 on success.
        *
     @@ config.h: void git_config_set(const char *, const char *);
      +
      +/*
      + * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
     -+ * are removed before a new pair is written. If the flag is not present,
     -+ * then set operations replace only the first match.
     ++ * are removed before a single new pair is written. If the flag is not
     ++ * present, then set operations replace only the first match.
      + */
      +#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
      +
 -:  ---------- > 2:  f135b001ad config: replace 'value_regex' with 'value_pattern'
 1:  ea3099719c ! 3:  3b72082326 t1300: test "set all" mode with value_regex
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    t1300: test "set all" mode with value_regex
     +    t1300: test "set all" mode with value-pattern
      
     -    Without additional modifiers, 'git config' attempts to set a single
     -    value in the .git/config file. When the value_regex parameter is
     -    supplied, this command behaves in a non-trivial manner.
     +    Without additional modifiers, 'git config <key> <value>' attempts
     +    to set a single value in the .git/config file. When the
     +    value-pattern parameter is supplied, this command behaves in a
     +    non-trivial manner.
      
     -    Consider 'git config key value value_regex'. The expected behavior
     -    is as follows:
     +    Consider 'git config <key> <value> <value-pattern>'. The expected
     +    behavior is as follows:
      
     -    1. If there are multiple existing values that match 'value_regex',
     +    1. If there are multiple existing values that match 'value-pattern',
             then the command fails. Users should use --replace-all instead.
      
     -    2. If there is one existing value that matches 'value_regex', then
     -       the new config has one entry where 'key=value'.
     -
     -    3. If there is no existing values match 'value_regex', then the
     +    2. If there is no existing values match 'value-pattern', then the
             'key=value' pair is appended, making this 'key' a multi-valued
             config setting.
      
     +    3. If there is one existing value that matches 'value-pattern', then
     +       the new config has one entry where 'key=value'.
     +
          Add a test that demonstrates these options. Break from the existing
          pattern in t1300-config.sh to use 'git config --file=<file>' instead of
     -    modifying .git/config directly. Also use 'git config --file=<file>
     -    --list' for config state comparison instead of the config file format.
     +    modifying .git/config directly to prevent possibly incompatible repo
     +    states. Also use 'git config --file=<file> --list' for config state
     +    comparison instead of the config file format. This makes the tests
     +    more readable.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     @@ t/t1300-config.sh: test_expect_success '--replace-all does not invent newlines'
       	test_cmp expect .git/config
       '
       
     -+test_expect_success 'set all config with value_regex' '
     ++test_expect_success 'set all config with value-pattern' '
     ++	test_when_finished rm -f config initial &&
      +	git config --file=initial abc.key one &&
      +
     ++	# no match => add new entry
      +	cp initial config &&
      +	git config --file=config abc.key two a+ &&
      +	git config --file=config --list >actual &&
     @@ t/t1300-config.sh: test_expect_success '--replace-all does not invent newlines'
      +	EOF
      +	test_cmp expect actual &&
      +
     ++	# multiple matches => failure
      +	test_must_fail git config --file=config abc.key three o+ 2>err &&
      +	test_i18ngrep "has multiple values" err &&
     ++
     ++	# multiple values, no match => add
      +	git config --file=config abc.key three a+ &&
      +	git config --file=config --list >actual &&
      +	cat >expect <<-\EOF &&
     @@ t/t1300-config.sh: test_expect_success '--replace-all does not invent newlines'
      +	EOF
      +	test_cmp expect actual &&
      +
     -+	cp initial config &&
     -+	git config --file=config abc.key three o+ &&
     ++	# single match => replace
     ++	git config --file=config abc.key four h+ &&
      +	git config --file=config --list >actual &&
      +	cat >expect <<-\EOF &&
     -+	abc.key=three
     ++	abc.key=one
     ++	abc.key=two
     ++	abc.key=four
      +	EOF
      +	test_cmp expect actual
      +'
 2:  829d0ccd8c ! 4:  75fb74da83 t1300: add test for --replace-all with value_regex
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    t1300: add test for --replace-all with value_regex
     +    t1300: add test for --replace-all with value-pattern
      
          The --replace-all option was added in 4ddba79d (git-config-set: add more
     -    options) but was not tested along with the 'value_regex' parameter.
     -    Since we will be updating this option to optionally treat 'value_regex'
     +    options) but was not tested along with the 'value-pattern' parameter.
     +    Since we will be updating this option to optionally treat 'value-pattern'
          as a fixed string, let's add a test here that documents the current
          behavior.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
       ## t/t1300-config.sh ##
     -@@ t/t1300-config.sh: test_expect_success 'set all config with value_regex' '
     +@@ t/t1300-config.sh: test_expect_success 'set all config with value-pattern' '
       	test_cmp expect actual
       '
       
     -+test_expect_success '--replace-all and value_regex' '
     -+	rm -f config &&
     ++test_expect_success '--replace-all and value-pattern' '
     ++	test_when_finished rm -f config &&
      +	git config --file=config --add abc.key one &&
      +	git config --file=config --add abc.key two &&
      +	git config --file=config --add abc.key three &&
 4:  0e6a7371ed ! 5:  0c276ffcee config: add --fixed-value option, un-implemented
     @@ Metadata
       ## Commit message ##
          config: add --fixed-value option, un-implemented
      
     -    The 'git config' builtin takes a 'value_regex' parameter for several
     +    The 'git config' builtin takes a 'value-pattern' parameter for several
          actions. This can cause confusion when expecting exact value matches
     -    instead of regex matches, especially when the input string contains glob
     -    characters. While callers can escape the patterns themselves, it would
     -    be more friendly to allow an argument to disable the pattern matching in
     -    favor of an exact string match.
     +    instead of regex matches, especially when the input string contains
     +    metacharacters. While callers can escape the patterns themselves, it
     +    would be more friendly to allow an argument to disable the pattern
     +    matching in favor of an exact string match.
      
          Add a new '--fixed-value' option that does not currently change the
     -    behavior. The implementation will follow for each appropriate action.
     -    For now, check and test that --fixed-value will abort the command when
     -    included with an incompatible action or without a 'value_regex'
     -    argument.
     +    behavior. The implementation will be filled in by later changes for
     +    each appropriate action. For now, check and test that --fixed-value
     +    will abort the command when included with an incompatible action or
     +    without a 'value-pattern' argument.
      
          The name '--fixed-value' was chosen over something simpler like
          '--fixed' because some commands allow regular expressions on the
     @@ Documentation/git-config.txt: git-config - Get and set repository or global opti
       SYNOPSIS
       --------
       [verse]
     --'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
     -+'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
     +-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]]
     ++'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]]
       'git config' [<file-option>] [--type=<type>] --add name value
     --'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
     --'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
     --'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
     --'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value_regex]
     -+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value_regex]
     +-'git config' [<file-option>] [--type=<type>] --replace-all name value [value-pattern]
     +-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value-pattern]
     +-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value-pattern]
     +-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern]
     ++'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value-pattern]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value-pattern]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value-pattern]
     ++'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value-pattern]
       'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
     --'git config' [<file-option>] --unset name [value_regex]
     --'git config' [<file-option>] --unset-all name [value_regex]
     -+'git config' [<file-option>] [--fixed-value] --unset name [value_regex]
     -+'git config' [<file-option>] [--fixed-value] --unset-all name [value_regex]
     +-'git config' [<file-option>] --unset name [value-pattern]
     +-'git config' [<file-option>] --unset-all name [value-pattern]
     ++'git config' [<file-option>] [--fixed-value] --unset name [value-pattern]
     ++'git config' [<file-option>] [--fixed-value] --unset-all name [value-pattern]
       'git config' [<file-option>] --rename-section old_name new_name
       'git config' [<file-option>] --remove-section name
       'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
     @@ Documentation/git-config.txt: See also <<FILES>>.
       	List all variables set in config file, along with their values.
       
      +--fixed-value::
     -+	When used with the `value_regex` argument, treat `value_regex` as
     ++	When used with the `value-pattern` argument, treat `value-pattern` as
      +	an exact string instead of a regular expression. This will restrict
      +	the name/value pairs that are matched to only those where the value
     -+	is exactly equal to the `value_regex`.
     ++	is exactly equal to the `value-pattern`.
      +
       --type <type>::
         'git config' will ensure that any input or output is valid under the given
     @@ builtin/config.c: static struct option builtin_config_options[] = {
       	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
       	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
       	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
     -+	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when matching values")),
     ++	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")),
       	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
       	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
       	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       		usage_builtin_config();
       	}
       
     ++	/* check usage of --fixed-value */
      +	if (fixed_value) {
      +		int allowed_usage = 0;
      +
      +		switch (actions) {
     ++		/* git config --get <name> <value-pattern> */
      +		case ACTION_GET:
     ++		/* git config --get-all <name> <value-pattern> */
      +		case ACTION_GET_ALL:
     ++		/* git config --get-regexp <name-pattern> <value-pattern> */
      +		case ACTION_GET_REGEXP:
     ++		/* git config --unset <name> <value-pattern> */
      +		case ACTION_UNSET:
     ++		/* git config --unset-all <name> <value-pattern> */
      +		case ACTION_UNSET_ALL:
      +			allowed_usage = argc > 1 && !!argv[1];
      +			break;
      +
     ++		/* git config <name> <value> <value-pattern> */
      +		case ACTION_SET_ALL:
     ++		/* git config --replace-all <name> <value> <value-pattern> */
      +		case ACTION_REPLACE_ALL:
      +			allowed_usage = argc > 2 && !!argv[2];
      +			break;
     ++
     ++		/* other options don't allow --fixed-value */
      +		}
      +
      +		if (!allowed_usage) {
     -+			error(_("--fixed-value only applies with 'value_regex'"));
     ++			error(_("--fixed-value only applies with 'value-pattern'"));
      +			usage_builtin_config();
      +		}
      +	}
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       
      
       ## t/t1300-config.sh ##
     -@@ t/t1300-config.sh: test_expect_success '--replace-all and value_regex' '
     +@@ t/t1300-config.sh: test_expect_success '--replace-all and value-pattern' '
       	test_cmp expect actual
       '
       
      +test_expect_success 'refuse --fixed-value for incompatible actions' '
     ++	test_when_finished rm -f config &&
      +	git config --file=config dev.null bogus &&
      +
      +	# These modes do not allow --fixed-value at all
     @@ t/t1300-config.sh: test_expect_success '--replace-all and value_regex' '
      +	test_must_fail git config --file=config --fixed-value --get-color dev.null &&
      +	test_must_fail git config --file=config --fixed-value --get-colorbool dev.null &&
      +
     -+	# These modes complain when --fixed-value has no value_regex
     ++	# These modes complain when --fixed-value has no value-pattern
      +	test_must_fail git config --file=config --fixed-value dev.null bogus &&
      +	test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus &&
      +	test_must_fail git config --file=config --fixed-value --get dev.null &&
 5:  39718048cd ! 6:  763401da97 config: plumb --fixed-value into config API
     @@ Commit message
      
          The git_config_set_multivar_in_file_gently() and related methods now
          take a 'flags' bitfield, so add a new bit representing the --fixed-value
     -    option from 'git config'. This alters the purpose of the value_regex
     +    option from 'git config'. This alters the purpose of the value_pattern
          parameter to be an exact string match. This requires some initialization
          changes in git_config_set_multivar_in_file_gently() and a new strcmp()
          call in the matches() method.
     @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix
       	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
       
      @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix)
     - 			error(_("--fixed-value only applies with 'value_regex'"));
     + 			error(_("--fixed-value only applies with 'value-pattern'"));
       			usage_builtin_config();
       		}
      +
     -+		flags = CONFIG_FLAGS_FIXED_VALUE;
     ++		flags |= CONFIG_FLAGS_FIXED_VALUE;
       	}
       
       	if (actions & PAGING_ACTIONS)
     @@ config.c: struct config_store_data {
       	size_t baselen;
       	char *key;
       	int do_not_match;
     -+	const char *literal_value;
     - 	regex_t *value_regex;
     ++	const char *fixed_value;
     + 	regex_t *value_pattern;
       	int multi_replace;
       	struct {
      @@ config.c: static int matches(const char *key, const char *value,
       {
       	if (strcmp(key, store->key))
       		return 0; /* not ours */
     -+	if (store->literal_value)
     -+		return !strcmp(store->literal_value, value);
     - 	if (!store->value_regex)
     ++	if (store->fixed_value)
     ++		return !strcmp(store->fixed_value, value);
     + 	if (!store->value_pattern)
       		return 1; /* always matches */
     - 	if (store->value_regex == CONFIG_REGEX_NONE)
     + 	if (store->value_pattern == CONFIG_REGEX_NONE)
      @@ config.c: int git_config_set_multivar_in_file_gently(const char *config_filename,
     - 			store.value_regex = NULL;
     - 		else if (value_regex == CONFIG_REGEX_NONE)
     - 			store.value_regex = CONFIG_REGEX_NONE;
     + 			store.value_pattern = NULL;
     + 		else if (value_pattern == CONFIG_REGEX_NONE)
     + 			store.value_pattern = CONFIG_REGEX_NONE;
      +		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
     -+			store.literal_value = value_regex;
     ++			store.fixed_value = value_pattern;
       		else {
     - 			if (value_regex[0] == '!') {
     + 			if (value_pattern[0] == '!') {
       				store.do_not_match = 1;
      
       ## config.h ##
     @@ config.h: int git_config_key_is_valid(const char *key);
       
      +/*
      + * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
     -+ * by string comparison (not regex match) to the provided value_regex
     ++ * by string comparison (not regex match) to the provided value_pattern
      + * parameter.
      + */
      +#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
     @@ t/t1300-config.sh: test_expect_success 'refuse --fixed-value for incompatible ac
       '
       
      +test_expect_success '--fixed-value uses exact string matching' '
     -+	GLOB="a+b*c?d[e]f.g" &&
     -+	rm -f initial &&
     -+	git config --file=initial fixed.test "$GLOB" &&
     ++	test_when_finished rm -f config initial &&
     ++	META="a+b*c?d[e]f.g" &&
     ++	git config --file=initial fixed.test "$META" &&
      +
      +	cp initial config &&
     -+	git config --file=config fixed.test bogus "$GLOB" &&
     ++	git config --file=config fixed.test bogus "$META" &&
      +	git config --file=config --list >actual &&
      +	cat >expect <<-EOF &&
     -+	fixed.test=$GLOB
     ++	fixed.test=$META
      +	fixed.test=bogus
      +	EOF
      +	test_cmp expect actual &&
      +
      +	cp initial config &&
     -+	git config --file=config --fixed-value fixed.test bogus "$GLOB" &&
     ++	git config --file=config --fixed-value fixed.test bogus "$META" &&
      +	git config --file=config --list >actual &&
     -+	printf "fixed.test=bogus\n" >expect &&
     ++	cat >expect <<-\EOF &&
     ++	fixed.test=bogus
     ++	EOF
      +	test_cmp expect actual &&
      +
      +	cp initial config &&
     -+	test_must_fail git config --file=config --unset fixed.test "$GLOB" &&
     -+	git config --file=config --fixed-value --unset fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --unset fixed.test "$META" &&
     ++	git config --file=config --fixed-value --unset fixed.test "$META" &&
      +	test_must_fail git config --file=config fixed.test &&
      +
      +	cp initial config &&
     -+	test_must_fail git config --file=config --unset-all fixed.test "$GLOB" &&
     -+	git config --file=config --fixed-value --unset-all fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --unset-all fixed.test "$META" &&
     ++	git config --file=config --fixed-value --unset-all fixed.test "$META" &&
      +	test_must_fail git config --file=config fixed.test &&
      +
      +	cp initial config &&
     -+	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
     -+	git config --file=config --list >actual &&
     -+	cat >expect <<-EOF &&
     -+	fixed.test=$GLOB
     -+	fixed.test=bogus
     -+	EOF
     -+	test_cmp expect actual &&
     -+
     -+	cp initial config &&
     -+	git config --file=config --replace-all fixed.test bogus "$GLOB" &&
     ++	git config --file=config --replace-all fixed.test bogus "$META" &&
      +	git config --file=config --list >actual &&
      +	cat >expect <<-EOF &&
     -+	fixed.test=$GLOB
     ++	fixed.test=$META
      +	fixed.test=bogus
      +	EOF
      +	test_cmp expect actual &&
      +
     -+	git config --file=config --fixed-value --replace-all fixed.test bogus "$GLOB" &&
     ++	git config --file=config --fixed-value --replace-all fixed.test bogus "$META" &&
      +	git config --file=config --list >actual &&
      +	cat >expect <<-EOF &&
      +	fixed.test=bogus
 6:  8e0111c7b4 ! 7:  d813c84275 config: implement --fixed-value with --get*
     @@ Commit message
      
          The config builtin does its own regex matching of values for the --get,
          --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
     -    to the get_value() method so we can initialize the value_regex argument
     +    to the get_value() method so we can initialize the value-pattern argument
          as a fixed string instead of a regex pattern.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
     @@ builtin/config.c: static const char *const builtin_config_usage[] = {
       
       static char *key;
       static regex_t *key_regexp;
     -+static const char *value_regex;
     ++static const char *value_pattern;
       static regex_t *regexp;
       static int show_keys;
       static int omit_values;
     @@ builtin/config.c: static int collect_config(const char *key_, const char *value_
       		return 0;
       	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
       		return 0;
     -+	if (fixed_value && strcmp(value_regex, (value_?value_:"")))
     ++	if (fixed_value && strcmp(value_pattern, (value_?value_:"")))
      +		return 0;
       	if (regexp != NULL &&
       	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
     @@ builtin/config.c: static int collect_config(const char *key_, const char *value_
       }
       
      -static int get_value(const char *key_, const char *regex_)
     -+static int get_value(const char *key_, const char *regex_, int flags)
     ++static int get_value(const char *key_, const char *regex_, unsigned flags)
       {
       	int ret = CONFIG_GENERIC_ERROR;
       	struct strbuf_list values = {NULL};
     @@ builtin/config.c: static int get_value(const char *key_, const char *regex_)
       
      -	if (regex_) {
      +	if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE))
     -+		value_regex = regex_;
     ++		value_pattern = regex_;
      +	else if (regex_) {
       		if (regex_[0] == '!') {
       			do_not_match = 1;
     @@ t/t1300-config.sh: test_expect_success '--fixed-value uses exact string matching
       '
       
      +test_expect_success '--get and --get-all with --fixed-value' '
     -+	GLOB="a+b*c?d[e]f.g" &&
     -+	rm -f config &&
     ++	test_when_finished rm -f config &&
     ++	META="a+b*c?d[e]f.g" &&
      +	git config --file=config fixed.test bogus &&
     -+	git config --file=config --add fixed.test "$GLOB" &&
     ++	git config --file=config --add fixed.test "$META" &&
      +
      +	git config --file=config --get fixed.test bogus &&
     -+	test_must_fail git config --file=config --get fixed.test "$GLOB" &&
     -+	git config --file=config --get --fixed-value fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --get fixed.test "$META" &&
     ++	git config --file=config --get --fixed-value fixed.test "$META" &&
      +	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
      +
      +	git config --file=config --get-all fixed.test bogus &&
     -+	test_must_fail git config --file=config --get-all fixed.test "$GLOB" &&
     -+	git config --file=config --get-all --fixed-value fixed.test "$GLOB" &&
     ++	test_must_fail git config --file=config --get-all fixed.test "$META" &&
     ++	git config --file=config --get-all --fixed-value fixed.test "$META" &&
      +	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
      +
      +	git config --file=config --get-regexp fixed+ bogus &&
     -+	test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" &&
     -+	git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" &&
     ++	test_must_fail git config --file=config --get-regexp fixed+ "$META" &&
     ++	git config --file=config --get-regexp --fixed-value fixed+ "$META" &&
      +	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
      +'
      +
 7:  5a3acf8119 ! 8:  558775f83d maintenance: use 'git config --fixed-value'
     @@ Metadata
       ## Commit message ##
          maintenance: use 'git config --fixed-value'
      
     -    When a repository's leading directories contain regex glob characters,
     +    When a repository's leading directories contain regex metacharacters,
          the config calls for 'git maintenance register' and 'git maintenance
          unregister' are not careful enough. Use the new --fixed-value option
          to direct the config machinery to use exact string matches. This is a
     -    more robust option than excaping these arguments in a piecemeal fashion.
     +    more robust option than escaping these arguments in a piecemeal fashion.
      
          For the test, require that we are not running on Windows since the '+'
     -    character is not allowed on that filesystem.
     +    and '*' characters are not allowed on that filesystem.
      
          Reported-by: Emily Shaffer <emilyshaffer@google.com>
          Reported-by: Jonathan Nieder <jrnieder@gmail.com>
     @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '
       	test_cmp before actual
       '
       
     -+test_expect_success !MINGW 'register and unregister with glob characters' '
     -+	GLOB="a+b*c" &&
     -+	git init "$GLOB" &&
     -+	git -C "$GLOB" maintenance register &&
     ++test_expect_success !MINGW 'register and unregister with regex metacharacters' '
     ++	META="a+b*c" &&
     ++	git init "$META" &&
     ++	git -C "$META" maintenance register &&
      +	git config --get-all --show-origin maintenance.repo &&
      +	git config --get-all --global --fixed-value \
     -+		maintenance.repo "$(pwd)/$GLOB" &&
     -+	git -C "$GLOB" maintenance unregister &&
     ++		maintenance.repo "$(pwd)/$META" &&
     ++	git -C "$META" maintenance unregister &&
      +	test_must_fail git config --get-all --global --fixed-value \
     -+		maintenance.repo "$(pwd)/$GLOB"
     ++		maintenance.repo "$(pwd)/$META"
      +'
      +
       test_expect_success 'start from empty cron table' '

-- 
gitgitgadget

[PATCH v3 1/8] config: convert multi_replace to flags

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

We will extend the flexibility of the config API. Before doing so, let's
take an existing 'int multi_replace' parameter and replace it with a new
'unsigned flags' parameter that can take multiple options as a bit field.

Update all callers that specified multi_replace to now specify the
CONFIG_FLAGS_MULTI_REPLACE flag. To add more clarity, extend the
documentation of git_config_set_multivar_in_file() including a clear
labeling of its arguments. Other config API methods in config.h require
only a change of the final parameter from 'int' to 'unsigned'.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/branch.c |  4 ++--
 builtin/config.c |  6 ++++--
 builtin/remote.c |  8 +++++---
 config.c         | 24 ++++++++++++------------
 config.h         | 29 ++++++++++++++++++++++-------
 5 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e82301fb1b..5ce3844d22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -829,10 +829,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("Branch '%s' has no upstream information"), branch->name);
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "branch.%s.merge", branch->name);
-		git_config_set_multivar(buf.buf, NULL, NULL, 1);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 		strbuf_release(&buf);
 	} else if (argc > 0 && argc <= 2) {
 		if (filter.kind != FILTER_REFS_BRANCHES)
diff --git a/builtin/config.c b/builtin/config.c
index 5e39f61885..e7c7f3d455 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -823,7 +823,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 1);
+							      argv[0], value, argv[2],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -859,7 +860,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], NULL, argv[1], 1);
+							      argv[0], NULL, argv[1],
+							      CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/builtin/remote.c b/builtin/remote.c
index c8240e9fcd..29b1652975 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -708,7 +708,7 @@ static int mv(int argc, const char **argv)
 
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
-	git_config_set_multivar(buf.buf, NULL, NULL, 1);
+	git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
 	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
 	for (i = 0; i < oldremote->fetch.raw_nr; i++) {
 		char *ptr;
@@ -1485,7 +1485,8 @@ static int update(int argc, const char **argv)
 
 static int remove_all_fetch_refspecs(const char *key)
 {
-	return git_config_set_multivar_gently(key, NULL, NULL, 1);
+	return git_config_set_multivar_gently(key, NULL, NULL,
+					      CONFIG_FLAGS_MULTI_REPLACE);
 }
 
 static void add_branches(struct remote *remote, const char **branches,
@@ -1674,7 +1675,8 @@ static int set_url(int argc, const char **argv)
 	if (!delete_mode)
 		git_config_set_multivar(name_buf.buf, newurl, oldurl, 0);
 	else
-		git_config_set_multivar(name_buf.buf, NULL, oldurl, 1);
+		git_config_set_multivar(name_buf.buf, NULL, oldurl,
+					CONFIG_FLAGS_MULTI_REPLACE);
 out:
 	strbuf_release(&name_buf);
 	return 0;
diff --git a/config.c b/config.c
index 2b79fe76ad..f34a11d94b 100644
--- a/config.c
+++ b/config.c
@@ -2716,9 +2716,9 @@ void git_config_set(const char *key, const char *value)
  * if value_regex!=NULL, disregard key/value pairs where value does not match.
  * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
  *     (only add a new one)
- * if multi_replace==0, nothing, or only one matching key/value is replaced,
- *     else all matching key/values (regardless how many) are removed,
- *     before the new pair is written.
+ * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching
+ *     key/values are removed before a single new pair is written. If the
+ *     flag is not present, then replace only the first match.
  *
  * Returns 0 on success.
  *
@@ -2739,7 +2739,7 @@ void git_config_set(const char *key, const char *value)
 int git_config_set_multivar_in_file_gently(const char *config_filename,
 					   const char *key, const char *value,
 					   const char *value_regex,
-					   int multi_replace)
+					   unsigned flags)
 {
 	int fd = -1, in_fd = -1;
 	int ret;
@@ -2756,7 +2756,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	if (ret)
 		goto out_free;
 
-	store.multi_replace = multi_replace;
+	store.multi_replace = (flags & CONFIG_FLAGS_MULTI_REPLACE) != 0;
 
 	if (!config_filename)
 		config_filename = filename_buf = git_pathdup("config");
@@ -2845,7 +2845,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 		/* if nothing to unset, or too many matches, error out */
 		if ((store.seen_nr == 0 && value == NULL) ||
-		    (store.seen_nr > 1 && multi_replace == 0)) {
+		    (store.seen_nr > 1 && !store.multi_replace)) {
 			ret = CONFIG_NOTHING_SET;
 			goto out_free;
 		}
@@ -2984,10 +2984,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key, const char *value,
-				     const char *value_regex, int multi_replace)
+				     const char *value_regex, unsigned flags)
 {
 	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
-						    value_regex, multi_replace))
+						    value_regex, flags))
 		return;
 	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
@@ -2996,17 +2996,17 @@ void git_config_set_multivar_in_file(const char *config_filename,
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
-				   const char *value_regex, int multi_replace)
+				   const char *value_regex, unsigned flags)
 {
 	return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex,
-						      multi_replace);
+						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
-			     const char *value_regex, int multi_replace)
+			     const char *value_regex, unsigned flags)
 {
 	git_config_set_multivar_in_file(NULL, key, value, value_regex,
-					multi_replace);
+					flags);
 }
 
 static int section_name_match (const char *buf, const char *name)
diff --git a/config.h b/config.h
index 060874488f..58726c34d6 100644
--- a/config.h
+++ b/config.h
@@ -256,9 +256,22 @@ void git_config_set(const char *, const char *);
 
 int git_config_parse_key(const char *, char **, size_t *);
 int git_config_key_is_valid(const char *key);
-int git_config_set_multivar_gently(const char *, const char *, const char *, int);
-void git_config_set_multivar(const char *, const char *, const char *, int);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, int);
+
+/*
+ * The following macros specify flag bits that alter the behavior
+ * of the git_config_set_multivar*() methods.
+ */
+
+/*
+ * When CONFIG_FLAGS_MULTI_REPLACE is specified, all matching key/values
+ * are removed before a single new pair is written. If the flag is not
+ * present, then set operations replace only the first match.
+ */
+#define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
+
+int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
+void git_config_set_multivar(const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**
  * takes four parameters:
@@ -276,13 +289,15 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
  * - the value regex, as a string. It will disregard key/value pairs where value
  *   does not match.
  *
- * - a multi_replace value, as an int. If value is equal to zero, nothing or only
- *   one matching key/value is replaced, else all matching key/values (regardless
- *   how many) are removed, before the new pair is written.
+ * - a flags value with bits corresponding to the CONFIG_FLAG_* macros.
  *
  * It returns 0 on success.
  */
-void git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
+void git_config_set_multivar_in_file(const char *config_filename,
+				     const char *key,
+				     const char *value,
+				     const char *value_regex,
+				     unsigned flags);
 
 /**
  * rename or remove sections in the config file
-- 
gitgitgadget

[PATCH v3 2/8] config: replace 'value_regex' with 'value_pattern'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'value_regex' argument in the 'git config' builtin is poorly named,
especially related to an upcoming change that allows exact string
matches instead of ERE pattern matches.

Perform a mostly mechanical change of every instance of 'value_regex' to
'value_pattern' in the codebase. This is only critical for documentation
and error messages, but it is best to be consistent inside the codebase,
too.

For documentation, use 'value-pattern' which is better punctuation. This
affects Documentation/git-config.txt and the usage in builtin/config.c,
which was already mixed between 'value_regex' and 'value-regex'.

I gave some thought to leaving the value_regex variables inside config.c
that are regex_t pointers. However, it is probably best to keep the name
consistent with the rest of the variables.

This does not update the translations inside the po/ directory, as that
creates conflicts with ongoing work. The input strings should
automatically update through automation, and a few of the output strings
currently use "[value_regex]" directly.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-config.txt | 20 ++++++-------
 builtin/config.c             | 12 ++++----
 config.c                     | 54 ++++++++++++++++++------------------
 config.h                     |  2 +-
 4 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7573160f21..0be5499952 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,15 +9,15 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]]
 'git config' [<file-option>] [--type=<type>] --add name value
-'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] --replace-all name value [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern]
 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] --unset name [value_regex]
-'git config' [<file-option>] --unset-all name [value_regex]
+'git config' [<file-option>] --unset name [value-pattern]
+'git config' [<file-option>] --unset-all name [value-pattern]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
@@ -33,7 +33,7 @@ escaped.
 
 Multiple lines can be added to an option by using the `--add` option.
 If you want to update or unset an option which can occur on multiple
-lines, a POSIX regexp `value_regex` needs to be given.  Only the
+lines, a POSIX regexp `value-pattern` needs to be given.  Only the
 existing values that match the regexp are updated or unset.  If
 you want to handle the lines that do *not* match the regex, just
 prepend a single exclamation mark in front (see also <<EXAMPLES>>).
@@ -73,11 +73,11 @@ OPTIONS
 
 --replace-all::
 	Default behavior is to replace at most one line. This replaces
-	all lines matching the key (and optionally the value_regex).
+	all lines matching the key (and optionally the `value-pattern`).
 
 --add::
 	Adds a new line to the option without altering any existing
-	values.  This is the same as providing '^$' as the value_regex
+	values.  This is the same as providing '^$' as the `value-pattern`
 	in `--replace-all`.
 
 --get::
diff --git a/builtin/config.c b/builtin/config.c
index e7c7f3d455..9d97091561 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -130,14 +130,14 @@ static struct option builtin_config_options[] = {
 	OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),
 	OPT_STRING(0, "blob", &given_config_source.blob, N_("blob-id"), N_("read config from given blob object")),
 	OPT_GROUP(N_("Action")),
-	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
-	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
-	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
+	OPT_BIT(0, "get", &actions, N_("get value: name [value-pattern]"), ACTION_GET),
+	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-pattern]"), ACTION_GET_ALL),
+	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-pattern]"), ACTION_GET_REGEXP),
 	OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
-	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
+	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value-pattern]"), ACTION_REPLACE_ALL),
 	OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
-	OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-regex]"), ACTION_UNSET),
-	OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-regex]"), ACTION_UNSET_ALL),
+	OPT_BIT(0, "unset", &actions, N_("remove a variable: name [value-pattern]"), ACTION_UNSET),
+	OPT_BIT(0, "unset-all", &actions, N_("remove all matches: name [value-pattern]"), ACTION_UNSET_ALL),
 	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
diff --git a/config.c b/config.c
index f34a11d94b..ac6aace166 100644
--- a/config.c
+++ b/config.c
@@ -2402,7 +2402,7 @@ struct config_store_data {
 	size_t baselen;
 	char *key;
 	int do_not_match;
-	regex_t *value_regex;
+	regex_t *value_pattern;
 	int multi_replace;
 	struct {
 		size_t begin, end;
@@ -2416,10 +2416,10 @@ struct config_store_data {
 static void config_store_data_clear(struct config_store_data *store)
 {
 	free(store->key);
-	if (store->value_regex != NULL &&
-	    store->value_regex != CONFIG_REGEX_NONE) {
-		regfree(store->value_regex);
-		free(store->value_regex);
+	if (store->value_pattern != NULL &&
+	    store->value_pattern != CONFIG_REGEX_NONE) {
+		regfree(store->value_pattern);
+		free(store->value_pattern);
 	}
 	free(store->parsed);
 	free(store->seen);
@@ -2431,13 +2431,13 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
-	if (!store->value_regex)
+	if (!store->value_pattern)
 		return 1; /* always matches */
-	if (store->value_regex == CONFIG_REGEX_NONE)
+	if (store->value_pattern == CONFIG_REGEX_NONE)
 		return 0; /* never matches */
 
 	return store->do_not_match ^
-		(value && !regexec(store->value_regex, value, 0, NULL, 0));
+		(value && !regexec(store->value_pattern, value, 0, NULL, 0));
 }
 
 static int store_aux_event(enum config_event_t type,
@@ -2713,8 +2713,8 @@ void git_config_set(const char *key, const char *value)
 
 /*
  * If value==NULL, unset in (remove from) config,
- * if value_regex!=NULL, disregard key/value pairs where value does not match.
- * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
+ * if value_pattern!=NULL, disregard key/value pairs where value does not match.
+ * if value_pattern==CONFIG_REGEX_NONE, do not match any existing values
  *     (only add a new one)
  * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching
  *     key/values are removed before a single new pair is written. If the
@@ -2738,7 +2738,7 @@ void git_config_set(const char *key, const char *value)
  */
 int git_config_set_multivar_in_file_gently(const char *config_filename,
 					   const char *key, const char *value,
-					   const char *value_regex,
+					   const char *value_pattern,
 					   unsigned flags)
 {
 	int fd = -1, in_fd = -1;
@@ -2799,22 +2799,22 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		int i, new_line = 0;
 		struct config_options opts;
 
-		if (value_regex == NULL)
-			store.value_regex = NULL;
-		else if (value_regex == CONFIG_REGEX_NONE)
-			store.value_regex = CONFIG_REGEX_NONE;
+		if (value_pattern == NULL)
+			store.value_pattern = NULL;
+		else if (value_pattern == CONFIG_REGEX_NONE)
+			store.value_pattern = CONFIG_REGEX_NONE;
 		else {
-			if (value_regex[0] == '!') {
+			if (value_pattern[0] == '!') {
 				store.do_not_match = 1;
-				value_regex++;
+				value_pattern++;
 			} else
 				store.do_not_match = 0;
 
-			store.value_regex = (regex_t*)xmalloc(sizeof(regex_t));
-			if (regcomp(store.value_regex, value_regex,
+			store.value_pattern = (regex_t*)xmalloc(sizeof(regex_t));
+			if (regcomp(store.value_pattern, value_pattern,
 					REG_EXTENDED)) {
-				error(_("invalid pattern: %s"), value_regex);
-				FREE_AND_NULL(store.value_regex);
+				error(_("invalid pattern: %s"), value_pattern);
+				FREE_AND_NULL(store.value_pattern);
 				ret = CONFIG_INVALID_PATTERN;
 				goto out_free;
 			}
@@ -2984,10 +2984,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 
 void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key, const char *value,
-				     const char *value_regex, unsigned flags)
+				     const char *value_pattern, unsigned flags)
 {
 	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
-						    value_regex, flags))
+						    value_pattern, flags))
 		return;
 	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
@@ -2996,16 +2996,16 @@ void git_config_set_multivar_in_file(const char *config_filename,
 }
 
 int git_config_set_multivar_gently(const char *key, const char *value,
-				   const char *value_regex, unsigned flags)
+				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_regex,
+	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
 						      flags);
 }
 
 void git_config_set_multivar(const char *key, const char *value,
-			     const char *value_regex, unsigned flags)
+			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_regex,
+	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
 					flags);
 }
 
diff --git a/config.h b/config.h
index 58726c34d6..8ad1085b74 100644
--- a/config.h
+++ b/config.h
@@ -296,7 +296,7 @@ int git_config_set_multivar_in_file_gently(const char *, const char *, const cha
 void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *key,
 				     const char *value,
-				     const char *value_regex,
+				     const char *value_pattern,
 				     unsigned flags);
 
 /**
-- 
gitgitgadget

Re: [PATCH v3 2/8] config: replace 'value_regex' with 'value_pattern'

[Eric Sunshine]

On Wed, Nov 25, 2020 at 5:13 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> For documentation, use 'value-pattern' which is better punctuation. This
> affects Documentation/git-config.txt and the usage in builtin/config.c,
> which was already mixed between 'value_regex' and 'value-regex'.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern]
>  'git config' [<file-option>] --rename-section old_name new_name

A mini-project for someone to tackle some day would be to replace the
remaining underscores with hyphens in the documentation, such as the
above "name_regex", "old_name", "new_name". That's outside the scope
of this series, though, and can easily be done at any time.

[PATCH v3 3/8] t1300: test "set all" mode with value-pattern

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Without additional modifiers, 'git config <key> <value>' attempts
to set a single value in the .git/config file. When the
value-pattern parameter is supplied, this command behaves in a
non-trivial manner.

Consider 'git config <key> <value> <value-pattern>'. The expected
behavior is as follows:

1. If there are multiple existing values that match 'value-pattern',
   then the command fails. Users should use --replace-all instead.

2. If there is no existing values match 'value-pattern', then the
   'key=value' pair is appended, making this 'key' a multi-valued
   config setting.

3. If there is one existing value that matches 'value-pattern', then
   the new config has one entry where 'key=value'.

Add a test that demonstrates these options. Break from the existing
pattern in t1300-config.sh to use 'git config --file=<file>' instead of
modifying .git/config directly to prevent possibly incompatible repo
states. Also use 'git config --file=<file> --list' for config state
comparison instead of the config file format. This makes the tests
more readable.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..72f930ada0 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1914,4 +1914,43 @@ test_expect_success '--replace-all does not invent newlines' '
 	test_cmp expect .git/config
 '
 
+test_expect_success 'set all config with value-pattern' '
+	test_when_finished rm -f config initial &&
+	git config --file=initial abc.key one &&
+
+	# no match => add new entry
+	cp initial config &&
+	git config --file=config abc.key two a+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=one
+	abc.key=two
+	EOF
+	test_cmp expect actual &&
+
+	# multiple matches => failure
+	test_must_fail git config --file=config abc.key three o+ 2>err &&
+	test_i18ngrep "has multiple values" err &&
+
+	# multiple values, no match => add
+	git config --file=config abc.key three a+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=one
+	abc.key=two
+	abc.key=three
+	EOF
+	test_cmp expect actual &&
+
+	# single match => replace
+	git config --file=config abc.key four h+ &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=one
+	abc.key=two
+	abc.key=four
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 4/8] t1300: add test for --replace-all with value-pattern

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The --replace-all option was added in 4ddba79d (git-config-set: add more
options) but was not tested along with the 'value-pattern' parameter.
Since we will be updating this option to optionally treat 'value-pattern'
as a fixed string, let's add a test here that documents the current
behavior.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1300-config.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 72f930ada0..040b9f7506 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1953,4 +1953,18 @@ test_expect_success 'set all config with value-pattern' '
 	test_cmp expect actual
 '
 
+test_expect_success '--replace-all and value-pattern' '
+	test_when_finished rm -f config &&
+	git config --file=config --add abc.key one &&
+	git config --file=config --add abc.key two &&
+	git config --file=config --add abc.key three &&
+	git config --file=config --replace-all abc.key four "o+" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	abc.key=four
+	abc.key=three
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 5/8] config: add --fixed-value option, un-implemented

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The 'git config' builtin takes a 'value-pattern' parameter for several
actions. This can cause confusion when expecting exact value matches
instead of regex matches, especially when the input string contains
metacharacters. While callers can escape the patterns themselves, it
would be more friendly to allow an argument to disable the pattern
matching in favor of an exact string match.

Add a new '--fixed-value' option that does not currently change the
behavior. The implementation will be filled in by later changes for
each appropriate action. For now, check and test that --fixed-value
will abort the command when included with an incompatible action or
without a 'value-pattern' argument.

The name '--fixed-value' was chosen over something simpler like
'--fixed' because some commands allow regular expressions on the
key in addition to the value.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/git-config.txt | 20 +++++++++++++-------
 builtin/config.c             | 36 ++++++++++++++++++++++++++++++++++++
 t/t1300-config.sh            | 24 ++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 0be5499952..09a1d273a9 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,15 +9,15 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]]
+'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] name [value [value-pattern]]
 'git config' [<file-option>] [--type=<type>] --add name value
-'git config' [<file-option>] [--type=<type>] --replace-all name value [value-pattern]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value-pattern]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value-pattern]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all name value [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value-pattern]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value-pattern]
 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] --unset name [value-pattern]
-'git config' [<file-option>] --unset-all name [value-pattern]
+'git config' [<file-option>] [--fixed-value] --unset name [value-pattern]
+'git config' [<file-option>] [--fixed-value] --unset-all name [value-pattern]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
@@ -165,6 +165,12 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
+--fixed-value::
+	When used with the `value-pattern` argument, treat `value-pattern` as
+	an exact string instead of a regular expression. This will restrict
+	the name/value pairs that are matched to only those where the value
+	is exactly equal to the `value-pattern`.
+
 --type <type>::
   'git config' will ensure that any input or output is valid under the given
   type constraint(s), and will canonicalize outgoing values in `<type>`'s
diff --git a/builtin/config.c b/builtin/config.c
index 9d97091561..d0adbed872 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
 static int show_scope;
+static int fixed_value;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -141,6 +142,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
+	OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")),
 	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
@@ -745,6 +747,40 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	/* check usage of --fixed-value */
+	if (fixed_value) {
+		int allowed_usage = 0;
+
+		switch (actions) {
+		/* git config --get <name> <value-pattern> */
+		case ACTION_GET:
+		/* git config --get-all <name> <value-pattern> */
+		case ACTION_GET_ALL:
+		/* git config --get-regexp <name-pattern> <value-pattern> */
+		case ACTION_GET_REGEXP:
+		/* git config --unset <name> <value-pattern> */
+		case ACTION_UNSET:
+		/* git config --unset-all <name> <value-pattern> */
+		case ACTION_UNSET_ALL:
+			allowed_usage = argc > 1 && !!argv[1];
+			break;
+
+		/* git config <name> <value> <value-pattern> */
+		case ACTION_SET_ALL:
+		/* git config --replace-all <name> <value> <value-pattern> */
+		case ACTION_REPLACE_ALL:
+			allowed_usage = argc > 2 && !!argv[2];
+			break;
+
+		/* other options don't allow --fixed-value */
+		}
+
+		if (!allowed_usage) {
+			error(_("--fixed-value only applies with 'value-pattern'"));
+			usage_builtin_config();
+		}
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 040b9f7506..f6f071006d 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1967,4 +1967,28 @@ test_expect_success '--replace-all and value-pattern' '
 	test_cmp expect actual
 '
 
+test_expect_success 'refuse --fixed-value for incompatible actions' '
+	test_when_finished rm -f config &&
+	git config --file=config dev.null bogus &&
+
+	# These modes do not allow --fixed-value at all
+	test_must_fail git config --file=config --fixed-value --add dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --rename-section dev null &&
+	test_must_fail git config --file=config --fixed-value --remove-section dev &&
+	test_must_fail git config --file=config --fixed-value --list &&
+	test_must_fail git config --file=config --fixed-value --get-color dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-colorbool dev.null &&
+
+	# These modes complain when --fixed-value has no value-pattern
+	test_must_fail git config --file=config --fixed-value dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --replace-all dev.null bogus &&
+	test_must_fail git config --file=config --fixed-value --get dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-all dev.null &&
+	test_must_fail git config --file=config --fixed-value --get-regexp "dev.*" &&
+	test_must_fail git config --file=config --fixed-value --unset dev.null &&
+	test_must_fail git config --file=config --fixed-value --unset-all dev.null
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v3 5/8] config: add --fixed-value option, un-implemented

[Eric Sunshine]

On Wed, Nov 25, 2020 at 5:13 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [...]
> Add a new '--fixed-value' option that does not currently change the
> behavior. The implementation will be filled in by later changes for
> each appropriate action. For now, check and test that --fixed-value
> will abort the command when included with an incompatible action or
> without a 'value-pattern' argument.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -9,15 +9,15 @@ git-config - Get and set repository or global options
> +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get name [value-pattern]
> +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all name [value-pattern]
> +'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp name_regex [value-pattern]

Didn't notice this before (since I wasn't paying close attention), but
portions of the command which are to be filled-in by the user are
normally surrounded by angle brackets. Without the angle brackets, the
interpretation is that the argument is to be typed literally as shown.
So, the above should really be <value-pattern> and <name_regex> (or
even better <name-regex>), just as other fill-in arguments
<file-option> and <type> are formatted already.

I'm not suggesting that this series should tackle such a change since
that would be straying quite far from the original goal (which, as I
recall, was fixing a misbehavior of git-maintenance), but just
pointing out a possible mini-project for someone to tackle at some
point.

> +--fixed-value::
> +       When used with the `value-pattern` argument, treat `value-pattern` as
> +       an exact string instead of a regular expression. This will restrict
> +       the name/value pairs that are matched to only those where the value
> +       is exactly equal to the `value-pattern`.

Likewise, this would also use the angle brackets as `<value-pattern>`
(or '<value-pattern>' with single quotes rather than backquotes, for
which, I believe, Junio recently stated his preference)...

>  --type <type>::
>    'git config' will ensure that any input or output is valid under the given
>    type constraint(s), and will canonicalize outgoing values in `<type>`'s

... just like <type> is shown in angle brackets.

> +       OPT_BOOL(0, "fixed-value", &fixed_value, N_("use string equality when comparing values to 'value-pattern'")),

Here, I think omitting angle brackets is common, so this is probably okay.

[PATCH v3 6/8] config: plumb --fixed-value into config API

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The git_config_set_multivar_in_file_gently() and related methods now
take a 'flags' bitfield, so add a new bit representing the --fixed-value
option from 'git config'. This alters the purpose of the value_pattern
parameter to be an exact string match. This requires some initialization
changes in git_config_set_multivar_in_file_gently() and a new strcmp()
call in the matches() method.

The new CONFIG_FLAGS_FIXED_VALUE flag is initialized in builtin/config.c
based on the --fixed-value option, and that needs to be updated in
several callers.

This patch only affects some of the modes of 'git config', and the rest
will be completed in the next change.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 16 ++++++++++-----
 config.c          |  5 +++++
 config.h          |  7 +++++++
 t/t1300-config.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index d0adbed872..8049d8f5fc 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -616,6 +616,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 {
 	int nongit = !startup_info->have_repository;
 	char *value;
+	int flags = 0;
 
 	given_config_source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
 
@@ -779,6 +780,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			error(_("--fixed-value only applies with 'value-pattern'"));
 			usage_builtin_config();
 		}
+
+		flags |= CONFIG_FLAGS_FIXED_VALUE;
 	}
 
 	if (actions & PAGING_ACTIONS)
@@ -842,7 +845,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1]);
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
-							      argv[0], value, argv[2], 0);
+							      argv[0], value, argv[2],
+							      flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
@@ -851,7 +855,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value,
-							      CONFIG_REGEX_NONE, 0);
+							      CONFIG_REGEX_NONE,
+							      flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
@@ -860,7 +865,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value, argv[2],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -887,7 +892,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		if (argc == 2)
 			return git_config_set_multivar_in_file_gently(given_config_source.file,
-								      argv[0], NULL, argv[1], 0);
+								      argv[0], NULL, argv[1],
+								      flags);
 		else
 			return git_config_set_in_file_gently(given_config_source.file,
 							     argv[0], NULL);
@@ -897,7 +903,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], NULL, argv[1],
-							      CONFIG_FLAGS_MULTI_REPLACE);
+							      flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		int ret;
diff --git a/config.c b/config.c
index ac6aace166..d827167d83 100644
--- a/config.c
+++ b/config.c
@@ -2402,6 +2402,7 @@ struct config_store_data {
 	size_t baselen;
 	char *key;
 	int do_not_match;
+	const char *fixed_value;
 	regex_t *value_pattern;
 	int multi_replace;
 	struct {
@@ -2431,6 +2432,8 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
+	if (store->fixed_value)
+		return !strcmp(store->fixed_value, value);
 	if (!store->value_pattern)
 		return 1; /* always matches */
 	if (store->value_pattern == CONFIG_REGEX_NONE)
@@ -2803,6 +2806,8 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 			store.value_pattern = NULL;
 		else if (value_pattern == CONFIG_REGEX_NONE)
 			store.value_pattern = CONFIG_REGEX_NONE;
+		else if (flags & CONFIG_FLAGS_FIXED_VALUE)
+			store.fixed_value = value_pattern;
 		else {
 			if (value_pattern[0] == '!') {
 				store.do_not_match = 1;
diff --git a/config.h b/config.h
index 8ad1085b74..34901e2b64 100644
--- a/config.h
+++ b/config.h
@@ -269,6 +269,13 @@ int git_config_key_is_valid(const char *key);
  */
 #define CONFIG_FLAGS_MULTI_REPLACE (1 << 0)
 
+/*
+ * When CONFIG_FLAGS_FIXED_VALUE is specified, match key/value pairs
+ * by string comparison (not regex match) to the provided value_pattern
+ * parameter.
+ */
+#define CONFIG_FLAGS_FIXED_VALUE (1 << 1)
+
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f6f071006d..413fcbba03 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1991,4 +1991,54 @@ test_expect_success 'refuse --fixed-value for incompatible actions' '
 	test_must_fail git config --file=config --fixed-value --unset-all dev.null
 '
 
+test_expect_success '--fixed-value uses exact string matching' '
+	test_when_finished rm -f config initial &&
+	META="a+b*c?d[e]f.g" &&
+	git config --file=initial fixed.test "$META" &&
+
+	cp initial config &&
+	git config --file=config fixed.test bogus "$META" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=$META
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	cp initial config &&
+	git config --file=config --fixed-value fixed.test bogus "$META" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-\EOF &&
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	cp initial config &&
+	test_must_fail git config --file=config --unset fixed.test "$META" &&
+	git config --file=config --fixed-value --unset fixed.test "$META" &&
+	test_must_fail git config --file=config fixed.test &&
+
+	cp initial config &&
+	test_must_fail git config --file=config --unset-all fixed.test "$META" &&
+	git config --file=config --fixed-value --unset-all fixed.test "$META" &&
+	test_must_fail git config --file=config fixed.test &&
+
+	cp initial config &&
+	git config --file=config --replace-all fixed.test bogus "$META" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=$META
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual &&
+
+	git config --file=config --fixed-value --replace-all fixed.test bogus "$META" &&
+	git config --file=config --list >actual &&
+	cat >expect <<-EOF &&
+	fixed.test=bogus
+	fixed.test=bogus
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 7/8] config: implement --fixed-value with --get*

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

The config builtin does its own regex matching of values for the --get,
--get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
to the get_value() method so we can initialize the value-pattern argument
as a fixed string instead of a regex pattern.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/config.c  | 15 ++++++++++-----
 t/t1300-config.sh | 22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 8049d8f5fc..82e36c595e 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -14,6 +14,7 @@ static const char *const builtin_config_usage[] = {
 
 static char *key;
 static regex_t *key_regexp;
+static const char *value_pattern;
 static regex_t *regexp;
 static int show_keys;
 static int omit_values;
@@ -288,6 +289,8 @@ 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 (fixed_value && strcmp(value_pattern, (value_?value_:"")))
+		return 0;
 	if (regexp != NULL &&
 	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
@@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 	return format_config(&values->items[values->nr++], key_, value_);
 }
 
-static int get_value(const char *key_, const char *regex_)
+static int get_value(const char *key_, const char *regex_, unsigned flags)
 {
 	int ret = CONFIG_GENERIC_ERROR;
 	struct strbuf_list values = {NULL};
@@ -335,7 +338,9 @@ static int get_value(const char *key_, const char *regex_)
 		}
 	}
 
-	if (regex_) {
+	if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE))
+		value_pattern = regex_;
+	else if (regex_) {
 		if (regex_[0] == '!') {
 			do_not_match = 1;
 			regex_++;
@@ -869,19 +874,19 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_ALL) {
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_REGEXP) {
 		show_keys = 1;
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
-		return get_value(argv[0], argv[1]);
+		return get_value(argv[0], argv[1], flags);
 	}
 	else if (actions == ACTION_GET_URLMATCH) {
 		check_argc(argc, 2, 2);
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 413fcbba03..ea38ba9ab1 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2041,4 +2041,26 @@ test_expect_success '--fixed-value uses exact string matching' '
 	test_cmp expect actual
 '
 
+test_expect_success '--get and --get-all with --fixed-value' '
+	test_when_finished rm -f config &&
+	META="a+b*c?d[e]f.g" &&
+	git config --file=config fixed.test bogus &&
+	git config --file=config --add fixed.test "$META" &&
+
+	git config --file=config --get fixed.test bogus &&
+	test_must_fail git config --file=config --get fixed.test "$META" &&
+	git config --file=config --get --fixed-value fixed.test "$META" &&
+	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
+
+	git config --file=config --get-all fixed.test bogus &&
+	test_must_fail git config --file=config --get-all fixed.test "$META" &&
+	git config --file=config --get-all --fixed-value fixed.test "$META" &&
+	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
+
+	git config --file=config --get-regexp fixed+ bogus &&
+	test_must_fail git config --file=config --get-regexp fixed+ "$META" &&
+	git config --file=config --get-regexp --fixed-value fixed+ "$META" &&
+	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 8/8] maintenance: use 'git config --fixed-value'

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

When a repository's leading directories contain regex metacharacters,
the config calls for 'git maintenance register' and 'git maintenance
unregister' are not careful enough. Use the new --fixed-value option
to direct the config machinery to use exact string matches. This is a
more robust option than escaping these arguments in a piecemeal fashion.

For the test, require that we are not running on Windows since the '+'
and '*' characters are not allowed on that filesystem.

Reported-by: Emily Shaffer <emilyshaffer@google.com>
Reported-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 builtin/gc.c           |  5 +++--
 t/t7900-maintenance.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index e3098ef6a1..6dde3ce1bb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1452,7 +1452,8 @@ static int maintenance_register(void)
 		git_config_set("maintenance.strategy", "incremental");
 
 	config_get.git_cmd = 1;
-	strvec_pushl(&config_get.args, "config", "--global", "--get", "maintenance.repo",
+	strvec_pushl(&config_get.args, "config", "--global", "--get",
+		     "--fixed-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 			 NULL);
@@ -1483,7 +1484,7 @@ static int maintenance_unregister(void)
 
 	config_unset.git_cmd = 1;
 	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
-		     "maintenance.repo",
+		     "--fixed-value", "maintenance.repo",
 		     the_repository->worktree ? the_repository->worktree
 					      : the_repository->gitdir,
 		     NULL);
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 20184e96e1..bc2207edd6 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -367,6 +367,18 @@ test_expect_success 'register and unregister' '
 	test_cmp before actual
 '
 
+test_expect_success !MINGW 'register and unregister with regex metacharacters' '
+	META="a+b*c" &&
+	git init "$META" &&
+	git -C "$META" maintenance register &&
+	git config --get-all --show-origin maintenance.repo &&
+	git config --get-all --global --fixed-value \
+		maintenance.repo "$(pwd)/$META" &&
+	git -C "$META" maintenance unregister &&
+	test_must_fail git config --get-all --global --fixed-value \
+		maintenance.repo "$(pwd)/$META"
+'
+
 test_expect_success 'start from empty cron table' '
 	GIT_TEST_CRONTAB="test-tool crontab cron.txt" git maintenance start &&
 
-- 
gitgitgadget

Re: [PATCH v3 8/8] maintenance: use 'git config --fixed-value'

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> When a repository's leading directories contain regex metacharacters,
> the config calls for 'git maintenance register' and 'git maintenance
> unregister' are not careful enough. Use the new --fixed-value option
> to direct the config machinery to use exact string matches. This is a
> more robust option than escaping these arguments in a piecemeal fashion.
>
> For the test, require that we are not running on Windows since the '+'
> and '*' characters are not allowed on that filesystem.

Thanks.  

I was hoping there would be some byte we could use, but it seems
"a[]b" would not work if we go back beyond VFAT or NTFS.  !MINGW
it is, then.

Re: [PATCH v3 0/8] config: add --fixed-value option

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> As reported [1], 'git maintenance unregister' fails when a repository is
> located in a directory with regex glob characters.
>
> [1] 
> https://lore.kernel.org/git/2c2db228-069a-947d-8446-89f4d3f6181a@gmail.com/T/#mb96fa4187a0d6aeda097cd95804a8aafc0273022
>
> The discussed solution was to modify 'git config' to specify that the
> 'value_regex' argument should be treated as an exact string match. This is
> the primary change in this series, with an additional patch at the end to
> make 'git maintenance [un]register' use this option, when necessary.
>
> While we're here, let's rename 'value_regex' to 'value_pattern' to make
> things a bit clearer.
>
> Updates in V3
> =============
>
>  * Renamed 'value_regex' to 'value_pattern' in code and 'value-pattern' in
>    docs (except po/)
>    
>    
>  * Reordered commits slightly to help with that rename.
>    
>    
>  * Updated tests to use 'test_when_finished rm -f ...'
>    
>    
>  * Changed all references to "glob" characters to "meta" characters.
>    
>    
>  * Several other test modifications. Thanks, Emily, for the review!
>    
>    
>
> Thanks, -Stolee
>
> P.S. Happy Thanksgiving to those celebrating!

Thanks.  The only thing that still stand out, after the first 7
patches (queued as its own "add --fixed-value option to config"
topic) are queued, is that the early part of the documentation,
namely, a paragraph where it mentions value-pattern, still lives
in the world where it can only be a regexp.

Perhaps insert the following between step 7 and 8?

--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
Subject: config doc: value-pattern is not necessarily a regexp

The introductory part of the "git config --help" mentions the
optional value-pattern argument, but give no hint that it can be
something other than a regular expression (worse, it just says
"POSIX regexp", which usually means BRE but the regexp the command
takes is ERE).  Also, it needs to be documented that the '!' prefix
to negate the match, which is only mentioned in this part of the
document, works only with regexp and not with the --fixed-value.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-config.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git c/Documentation/git-config.txt w/Documentation/git-config.txt
index 09a1d273a9..0e9351d3cb 100644
--- c/Documentation/git-config.txt
+++ w/Documentation/git-config.txt
@@ -33,10 +33,13 @@ escaped.
 
 Multiple lines can be added to an option by using the `--add` option.
 If you want to update or unset an option which can occur on multiple
-lines, a POSIX regexp `value-pattern` needs to be given.  Only the
-existing values that match the regexp are updated or unset.  If
-you want to handle the lines that do *not* match the regex, just
-prepend a single exclamation mark in front (see also <<EXAMPLES>>).
+lines, a `value-pattern` (which is an extended regular expression,
+unless the `--fixed-value` option is given) needs to be given.  Only the
+existing values that match the pattern are updated or unset.  If
+you want to handle the lines that do *not* match the pattern, just
+prepend a single exclamation mark in front (see also <<EXAMPLES>>),
+but note that this only works when the `--fixed-value` option is not
+in use.
 
 The `--type=<type>` option instructs 'git config' to ensure that incoming and
 outgoing values are canonicalize-able under the given <type>.  If no

Re: [PATCH v3 0/8] config: add --fixed-value option

[Derrick Stolee]

On 11/25/2020 6:00 PM, Junio C Hamano wrote:

> Thanks.  The only thing that still stand out, after the first 7
> patches (queued as its own "add --fixed-value option to config"
> topic) are queued, is that the early part of the documentation,
> namely, a paragraph where it mentions value-pattern, still lives
> in the world where it can only be a regexp.
> 
> Perhaps insert the following between step 7 and 8?

This patch LGTM. Thanks!

-Stolee
 
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
> Subject: config doc: value-pattern is not necessarily a regexp
> 
> The introductory part of the "git config --help" mentions the
> optional value-pattern argument, but give no hint that it can be
> something other than a regular expression (worse, it just says
> "POSIX regexp", which usually means BRE but the regexp the command
> takes is ERE).  Also, it needs to be documented that the '!' prefix
> to negate the match, which is only mentioned in this part of the
> document, works only with regexp and not with the --fixed-value.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-config.txt | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git c/Documentation/git-config.txt w/Documentation/git-config.txt
> index 09a1d273a9..0e9351d3cb 100644
> --- c/Documentation/git-config.txt
> +++ w/Documentation/git-config.txt
> @@ -33,10 +33,13 @@ escaped.
>  
>  Multiple lines can be added to an option by using the `--add` option.
>  If you want to update or unset an option which can occur on multiple
> -lines, a POSIX regexp `value-pattern` needs to be given.  Only the
> -existing values that match the regexp are updated or unset.  If
> -you want to handle the lines that do *not* match the regex, just
> -prepend a single exclamation mark in front (see also <<EXAMPLES>>).
> +lines, a `value-pattern` (which is an extended regular expression,
> +unless the `--fixed-value` option is given) needs to be given.  Only the
> +existing values that match the pattern are updated or unset.  If
> +you want to handle the lines that do *not* match the pattern, just
> +prepend a single exclamation mark in front (see also <<EXAMPLES>>),
> +but note that this only works when the `--fixed-value` option is not
> +in use.
>  
>  The `--type=<type>` option instructs 'git config' to ensure that incoming and
>  outgoing values are canonicalize-able under the given <type>.  If no
> 

Re: [PATCH v3 0/8] config: add --fixed-value option

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

> On 11/25/2020 6:00 PM, Junio C Hamano wrote:
>
>> Thanks.  The only thing that still stand out, after the first 7
>> patches (queued as its own "add --fixed-value option to config"
>> topic) are queued, is that the early part of the documentation,
>> namely, a paragraph where it mentions value-pattern, still lives
>> in the world where it can only be a regexp.
>> 
>> Perhaps insert the following between step 7 and 8?
>
> This patch LGTM. Thanks!

Thanks.