[PATCH v2 0/5] builtin/config: bug fixes for "get" subcommand with "--type=color"
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 15, 2025, 12:52 UTC
- Message-ID
- <20250915-pks-config-color-v2-0-e4290bd8d13c@pks.im>
- In-Reply-To
- <20250911-pks-config-color-v1-0-3a7c79df65b1@pks.im>
Hi,
this small patch series contains two bug fixes for `git config get --type=color`:
- We restore the behaviour where we can now parse colors without a
config key. - We stop spawning the pager when the user requests to print ANSI
color escape sequences.Furthermore, the patch series does some lighter refactorings of t1300. That test file still has its fair share of issues, but at least it looks a bit less dirty now.
Changes in v2: - Improve commit messages. - Use "\EOF" and "-EOF" in more cases. - Move a style fixup from the first commit into the second commit. - Link to v1: https://lore.kernel.org/r/20250911-pks-config-color-v1-0-3a7c79df65b1@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (5):
t1300: write test expectations in the test's body
t1300: small style fixups
builtin/config: do not die in `get_color()`
builtin/config: special-case retrieving colors without a key
builtin/config: do not spawn pager when printing color codesbuiltin/config.c | 20 +++- t/t1300-config.sh | 349 +++++++++++++++++++++++++++--------------------------- 2 files changed, 187 insertions(+), 182 deletions(-)
Range-diff versus v1:
1: 2920521b26 ! 1: e58033502e t1300: write test expectations in the test's body
@@ Commit message
match our modern test style, and there isn't really a reason why this
would need to happen outside of the test bodies.
- Convert those to instead do so as part of the test itself.
+ Convert those to instead do so as part of the test itself. While at it,
+ normalize these tests to use `<<\EOF` for those that don't use variable
+ expansion and `<<-EOF` for those that aren't sensitive to indentation.
- Note that there are two exceptions that we don't convert. In both of
- these cases the expectation is reused across multiple tests, and thus a
- conversion where we'd move that into the first test that uses the
- expectation would be invalid. Those are simply left as-is for now.
+ Note that there are two exceptions that we leave as-is for now since
+ they are reused across tests.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ t/t1300-config.sh: test_expect_success 'clear default config' '
-cat > expect << EOF
+test_expect_success 'initial' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[section]
penguin = little blue
EOF
@@ t/t1300-config.sh: test_expect_success 'clear default config' '
-cat > expect << EOF
+test_expect_success 'mixed case' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[section]
penguin = little blue
Movie = BadPhysics
@@ t/t1300-config.sh: test_expect_success 'clear default config' '
-cat > expect << EOF
+test_expect_success 'similar section' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[section]
penguin = little blue
Movie = BadPhysics
@@ t/t1300-config.sh: test_expect_success 'clear default config' '
-cat > expect << EOF
+test_expect_success 'uppercase section' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[section]
penguin = little blue
Movie = BadPhysics
@@ t/t1300-config.sh: test_expect_success 'replace with non-match (actually matchin
-cat > expect << EOF
+test_expect_success 'append comments' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[section]
Movie = BadPhysics
UPPERCASE = true
@@ t/t1300-config.sh: cat > expect << EOF
git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
git config ${mode_set} --comment="find fish" section.disposition peckish &&
git config ${mode_set} --comment="#abc" section.foo bar &&
-@@ t/t1300-config.sh: test_expect_success 'Prohibited LF in comment' '
- test_must_fail git config ${mode_set} --comment="a${LF}b" section.k v
- '
-
--test_expect_success 'non-match result' 'test_cmp expect .git/config'
-+test_expect_success 'non-match result' '
-+ test_cmp expect .git/config
-+'
-
- test_expect_success 'find mixed-case key by canonical name' '
- test_cmp_config Second sections.whatever
@@ t/t1300-config.sh: test_expect_success 'unset with cont. lines' '
git config ${mode_unset} beta.baz
'
@@ t/t1300-config.sh: test_expect_success '--replace-all' '
[nextSection] noNewline = ouch
EOF
-test_expect_success 'really mean test' '
-+
git config ${mode_set} beta.haha alpha &&
test_cmp expect .git/config
'
@@ t/t1300-config.sh: test_expect_success '--list without repo produces empty outpu
-EOF
-
test_expect_success '--name-only --list' '
-+ cat >expect <<-EOF &&
++ cat >expect <<-\EOF &&
+ beta.noindent
+ nextsection.nonewline
+ 123456.a123
@@ t/t1300-config.sh: test_expect_success '--list without repo produces empty outpu
-EOF
-
test_expect_success '--get-regexp' '
-+ cat >expect <<-EOF &&
++ cat >expect <<-\EOF &&
+ beta.noindent sillyValue
+ nextsection.nonewline wow2 for me
+ EOF
@@ t/t1300-config.sh: test_expect_success '--list without repo produces empty outpu
-EOF
-
test_expect_success '--name-only --get-regexp' '
-+ cat >expect <<-EOF &&
++ cat >expect <<-\EOF &&
+ beta.noindent
+ nextsection.nonewline
+ EOF
@@ t/t1300-config.sh: test_expect_success '--list without repo produces empty outpu
-EOF
-
test_expect_success '--add' '
-+ cat >expect <<-EOF &&
++ cat >expect <<-\EOF &&
+ wow2 for me
+ wow4 for you
+ EOF
@@ t/t1300-config.sh: test_expect_success 'get variable with empty value' '
-echo false > expect
-
test_expect_success 'get bool variable with empty value' '
-+ echo false > expect &&
++ echo false >expect &&
git config --bool emptyvalue.variable > output &&
test_cmp expect output
'
@@ t/t1300-config.sh: cat > .git/config << EOF
-cat > expect << EOF
+test_expect_success 'new section is partial match of another' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[a.b]
c = d
[a]
@@ t/t1300-config.sh: cat > .git/config << EOF
-cat > expect << EOF
+test_expect_success 'new variable inserts into proper section' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[a.b]
c = d
[a]
@@ t/t1300-config.sh: cat > other-config << EOF
-EOF
-
test_expect_success 'alternative GIT_CONFIG' '
-+ cat >expect <<-EOF &&
++ cat >expect <<-\EOF &&
+ ein.bahn=strasse
+ EOF
GIT_CONFIG=other-config git config ${mode_prefix}list >output &&
@@ t/t1300-config.sh: test_expect_success 'invalid bool (set)' '
-cat > expect <<\EOF
+test_expect_success 'set --bool' '
-+ cat >expect<<\EOF &&
++ cat >expect <<\EOF &&
[bool]
true1 = true
true2 = true
@@ t/t1300-config.sh: test_expect_success 'get --type=color' '
-cat >expect << EOF
+test_expect_success 'set --type=color' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[foo]
color = red
EOF
@@ t/t1300-config.sh: test_expect_success 'set --type=color barfs on non-color' '
-cat > expect << EOF
+test_expect_success 'quoting' '
-+ cat >expect <<EOF &&
++ cat >expect <<\EOF &&
[quote]
leading = " test"
ending = "test "
@@ t/t1300-config.sh: inued
EOF
-cat > expect <<\EOF
-+test_expect_success 'value continued on next line' '
-+ cat >expect <<\EOF &&
- section.continued=continued
- section.noncont=not continued
- section.quotecont=cont;inued
- EOF
+-section.continued=continued
+-section.noncont=not continued
+-section.quotecont=cont;inued
+-EOF
-
--test_expect_success 'value continued on next line' '
+ test_expect_success 'value continued on next line' '
++ cat >expect <<-\EOF &&
++ section.continued=continued
++ section.noncont=not continued
++ section.quotecont=cont;inued
++ EOF
git config ${mode_prefix}list > result &&
test_cmp expect result
'
2: a6a38b98c2 ! 2: c37605959e t1300: small style fixups
@@ Commit message
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## t/t1300-config.sh ##
+@@ t/t1300-config.sh: test_expect_success 'Prohibited LF in comment' '
+ test_must_fail git config ${mode_set} --comment="a${LF}b" section.k v
+ '
+
+-test_expect_success 'non-match result' 'test_cmp expect .git/config'
++test_expect_success 'non-match result' '
++ test_cmp expect .git/config
++'
+
+ test_expect_success 'find mixed-case key by canonical name' '
+ test_cmp_config Second sections.whatever
@@ t/t1300-config.sh: EOF
test_cmp expect .git/config
'
@@ t/t1300-config.sh: test_expect_success bool '
+'
test_expect_success 'set --bool' '
- cat >expect<<\EOF &&
+ cat >expect <<\EOF &&
@@ t/t1300-config.sh: EOF
git config --path path.home "~/" &&
git config --path path.normal "/dev/null" &&
3: b9fe7190a1 = 3: d88e317762 builtin/config: do not die in `get_color()`
4: 1cd6acd0e9 ! 4: 6dfb71e7ce builtin/config: special-case retrieving colors without a key
@@ Commit message
$ git config get --type=color --default="reset" ""
- What this command is supposed to do is to not parse any configuration
- key at all. Instead, it is expected to parse the "reset" default value
- and turn it into a proper ANSI color escape sequence.
+ This command is not supposed to parse any configuration keys. Instead,
+ it is expected to parse the "reset" default value and turn it into a
+ proper ANSI color escape sequence.
It was reported though [1] that this command doesn't work:
$ git config get --type=color --default="reset" ""
error: key does not contain a section:
- This error was introduced with 4e51389000 (builtin/config: introduce
- "get" subcommand, 2024-05-06), where we introduced the new "get"
- subcommand to retrieve configuration values. The preimage of that commit
- used `git config --get-color "" "reset"` instead, which still works
- nowadays.
+ This error was introduced in 4e51389000 (builtin/config: introduce "get"
+ subcommand, 2024-05-06), where we introduced the "get" subcommand to
+ retrieve configuration values. The preimage of that commit used `git
+ config --get-color "" "reset"` instead, which still works.
This use case is really quite specific to parsing colors, as it wouldn't
make sense to give git-config(1) a default value and an empty config key
only to return that default value unmodified. But with `--type=color` we
- don't return the value directly, but we instead parse the value into an
- ANSI escape sequence.
+ don't return the value directly; we instead parse the value into an ANSI
+ escape sequence.
- As such, we can easily special-case this one use case: if the provided
- config key is empty, the user is asking for a color code and the user
- has provided a value, then we call `get_color()` directly. Do so to
- make the documented command work as expected.
+ As such, we can easily special-case this one use case:
+
+ - If the provided config key is empty;
+
+ - the user is asking for a color code and the user; and
+
+ - the user has provided a default value,
+
+ then we call `get_color()` directly. Do so to make the documented
+ command work as expected.
[1]: <aI+oQvQgnNtC6DVw@szeder.dev>
@@ t/t1300-config.sh: test_expect_success 'get --type=color' '
+'
+
test_expect_success 'set --type=color' '
- cat >expect <<EOF &&
+ cat >expect <<\EOF &&
[foo]
5: 46fc98e1ec ! 5: ad443d744f builtin/config: do not spawn pager when printing color codes
@@ Commit message
The printed string can then for example be used as part of shell scripts
to reuse the same colors as Git.
- Right now though we set up the auto-pager though, which means that the
- string may instead be written to the pager command. This is of course
- quite nonsensical: there shouldn't be any use case where the color code
- should end up in the pager instead of in the TTY.
+ Right now though we set up the auto-pager, which means that the string
+ may instead be written to the pager command. This is of course quite
+ nonsensical; there shouldn't be any use case where the color code should
+ end up in the pager instead of in the TTY.
Fix this by disabling the pager in case the user is asking us to print
color sequences.
@@ t/t1300-config.sh: test_expect_success 'get --type=color with default value only
+'
+
test_expect_success 'set --type=color' '
- cat >expect <<EOF &&
+ cat >expect <<\EOF &&
[foo]--- base-commit: ab427cd991100e94792fce124b0934135abdea4b change-id: 20250911-pks-config-color-e5b8a213e895