add: check color.ui for interactive add

From: Derrick Stolee <[email protected]>

When 'git add -i' and 'git add -p' were converted to a builtin, they introduced a color bug: the 'color.ui' config setting is ignored.

The included test demonstrates an example that is similar to the previous test, which focuses on customizing colors. Here, we are demonstrating that colors are not being used at all.

The fix is simple, to use git_color_default_config() as the fallback for git_add_config(). A more robust change would instead encapsulate the git_use_color_default global in methods that would check the config setting if it has not been initialized yet. Some ideas are being discussed on this front [1], but nothing has been finalized.

[1] https://lore.kernel.org/git/[email protected]/

This test case naturally bisects down to 0527ccb1b55 (add -i: default to the built-in implementation, 2021-11-30), but the fix makes it clear that this would be broken even if we added the config to use the builtin earlier than this.

---
add: check color.ui for interactive add

This was reported by Greg Alexander [email protected] during Git IRC Standup [2].

[2]
https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05

This is also a reoccurrence of the "config not loaded" bug from [3].

[3]
https://lore.kernel.org/git/[email protected]/

I linked above to my RFC on lazy-loading global Git config, and these are the same "root cause" (not loading something early enough in the process) and my RFC proposes to fix this by changing our access patterns. By encapsulating these globals, we can make sure they are initialized from config before they are accessed.

But that's a discussion for another thread. For now, fix the bug and we'll worry about the "better" (and bigger) thing to do another time.

Thanks, -Stolee

P.S. This fails the whitespace check due to the necessary left-padding spaces in the expected output in the test file.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1541

 builtin/add.c              |  2 +-
 t/t3701-add-interactive.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

index 76cc026a68a..6137e7b4ad7 100644
@@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_color_default_config(var, value, cb);
 }
 
 static const char embedded_advice[] = N_(

index 3982b6b49dc..00081418ea2 100755
@@ -734,6 +734,39 @@ test_expect_success 'colors can be overridden' '
 	test_cmp expect actual
 '
 
+test_expect_success 'colors can be skipped with color.ui=false' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.ui=false \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	cat >expect <<-\EOF &&
+	           staged     unstaged path
+	  1:        +3/-0        +2/-1 color-test
+
+	*** Commands ***
+	  1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
+	  5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
+	What now> status        - show paths with changes
+	update        - add working tree state to the staged set of changes
+	revert        - revert staged set of changes back to the HEAD version
+	patch         - pick hunks and update selectively
+	diff          - view diff between HEAD and index
+	add untracked - add contents of untracked files to the staged set of changes
+	*** Commands ***
+	  1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
+	  5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
+	What now> Bye.
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

gitgitgadget

Another thing. We are interested in the configuration to disable color, so instead of spelling out exact output we expect, which is brittle when we anticipate that the contents used for the test may change, wouldn't it work better to ensure that actual.raw and actual are identical?

I think it should be OK to load the color config here, but note...

...that this case is a little different than the core.replaceRefs one. One of the reasons we don't just load all config by default is that it was an intentional scheme that not all programs would respect all config, and color in particular was one of the things that wasn't meant to be supported everywhere.

In other words, the idea was that porcelain like "git diff" would use git_diff_ui_config(), which would load all the bells and whistles (like color). But plumbing like git-diff-tree uses git_diff_basic_config(), which skips those. And that way we can freely introduce new config options without worrying that they will unexpectedly change the behavior of plumbing commands (because each command has to manually opt into the new config).

Now I won't claim that this approach hasn't created all sorts of headaches over the years, and we might not be better off with something more explicit (e.g., we parse all the config, but plumbing sets a flag to say "I am plumbing; do not respect color.ui"). But it is roughly the approach we've taken, so I'm mostly warning you that there may be dragons here. :)

I say "roughly" because I actually think the rules have been bent a bit over the years. In particular, I think that git_use_color_default is initialized to COLOR_AUTO, so something like:

git diff-tree -p HEAD

ends up colorizing, even though it's plumbing. Which is maybe not so bad, but it's doubly silly that:

git -c color.diff=false diff-tree -p HEAD

still colorizes, even though "git diff" in an equivalent situation would not! That seems like a bug, but it's one that I suspect has been there since we flipped color on by default many years ago, and nobody has really complained.

So all of this is a big digression from your patch. I think for "git add" it is probably OK to enable color by default. But I mostly want to point out that trying to roll this into a more elaborate fix may run afoul of all kinds of rabbit holes.

-Peff

...snipping valuable context...

Thank you for this context, which I will keep in mind as an important feature in this space. The default config doesn't have this property, so I'll remain focused on those values in the "lazy load" work.

Just riffing: it's likely that we could load the color config in git.c based on the "porcelain" vs "plumbing" category of a builtin. We have that specification in command-list.txt, but _not_ in 'struct cmd_struct' so it would take some work to introduce that concept.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

When 'git add -i' and 'git add -p' were converted to a builtin, they introduced a color bug: the 'color.ui' config setting is ignored.

The included test demonstrates an example that is similar to the previous test, which focuses on customizing colors. Here, we are demonstrating that colors are not being used at all by comparing the raw output and the color-decoded version of that output.

The fix is simple, to use git_color_default_config() as the fallback for git_add_config(). A more robust change would instead encapsulate the git_use_color_default global in methods that would check the config setting if it has not been initialized yet. Some ideas are being discussed on this front [1], but nothing has been finalized.

[1] https://lore.kernel.org/git/[email protected]/

This test case naturally bisects down to 0527ccb1b55 (add -i: default to the built-in implementation, 2021-11-30), but the fix makes it clear that this would be broken even if we added the config to use the builtin earlier than this.

---
add: check color.ui for interactive add

This was reported by Greg Alexander [email protected] during Git IRC Standup [2].

[2]
https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05

This is also a reoccurrence of the "config not loaded" bug from [3].

[3]
https://lore.kernel.org/git/[email protected]/

I linked above to my RFC on lazy-loading global Git config, and these are the same "root cause" (not loading something early enough in the process) and my RFC proposes to fix this by changing our access patterns. By encapsulating these globals, we can make sure they are initialized from config before they are accessed.

But that's a discussion for another thread. For now, fix the bug and we'll worry about the "better" (and bigger) thing to do another time.

Update in v2
============

* The test is simplified to compare the raw output to the color-decoded output. This makes the test more robust to possible future changes to interactive add.

Thanks, -Stolee

P.S. This fails the whitespace check due to the necessary left-padding spaces in the expected output in the test file.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1541

Range-diff vs v1:

1: a76893de8a7 ! 1: 6d3e34d51c5 add: check color.ui for interactive add
@@ Commit message

The included test demonstrates an example that is similar to the previous test, which focuses on customizing colors. Here, we are
- demonstrating that colors are not being used at all.
+ demonstrating that colors are not being used at all by comparing the raw
+ output and the color-decoded version of that output.

The fix is simple, to use git_color_default_config() as the fallback for git_add_config(). A more robust change would instead encapsulate the
@@ t/t3701-add-interactive.sh: test_expect_success 'colors can be overridden' '
+ -c color.ui=false \
+ add -i >actual.raw <input &&
+ test_decode_color <actual.raw >actual &&
-+ cat >expect <<-\EOF &&
-+ staged unstaged path
-+ 1: +3/-0 +2/-1 color-test
-+
-+ *** Commands ***
-+ 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked
-+ 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp
-+ What now> status - show paths with changes
-+ update - add working tree state to the staged set of changes
-+ revert - revert staged set of changes back to the HEAD version
-+ patch - pick hunks and update selectively
-+ diff - view diff between HEAD and index
-+ add untracked - add contents of untracked files to the staged set of changes
-+ *** Commands ***
-+ 1: [s]tatus 2: [u]pdate 3: [r]evert 4: [a]dd untracked
-+ 5: [p]atch 6: [d]iff 7: [q]uit 8: [h]elp
-+ What now> Bye.
-+ EOF
-+ test_cmp expect actual
++ test_cmp actual.raw actual
+'
+
test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '

 builtin/add.c              |  2 +-
 t/t3701-add-interactive.sh | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index 76cc026a68a..6137e7b4ad7 100644
@@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_color_default_config(var, value, cb);
 }
 
 static const char embedded_advice[] = N_(

index 3982b6b49dc..a93fe54e2ad 100755
@@ -734,6 +734,21 @@ test_expect_success 'colors can be overridden' '
 	test_cmp expect actual
 '
 
+test_expect_success 'colors can be skipped with color.ui=false' '
+	git reset --hard &&
+	test_when_finished "git rm -f color-test" &&
+	test_write_lines context old more-context >color-test &&
+	git add color-test &&
+	test_write_lines context new more-context another-one >color-test &&
+
+	test_write_lines help quit >input &&
+	force_color git \
+		-c color.ui=false \
+		add -i >actual.raw <input &&
+	test_decode_color <actual.raw >actual &&
+	test_cmp actual.raw actual
+'
+
 test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	git reset --hard &&
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

gitgitgadget

Hi Stolee

Hi Stolee

Thanks for fixing this

I know Junio suggested this change, but I'm in two minds as to whether it is a good idea. The reason is that we're no-longer testing that we add "[]" around the text that would have been highlighted if color was enabled. That is with --color we print "1: status" with the "s" highlighted rather than "1: [s]tatus". So while the revised patch tests there is no color in the output, it does not test that we print the output correctly in that case.

Best Wishes

Phillip

This is a good point, and something that I somewhat expected to find as an example check in the rest of the test script. I think the color would be disabled if we didn't do force_color, even before this fix.

However, I'll double-check that by adding a separate test for just that bit of the UI, keeping this color.ui=false test focused on the color side of things.

Thanks,
-Stolee

From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <[email protected]>
Date: Wed, 7 Jun 2023 09:39:01 -0400
Subject: [PATCH v2 2/2] add: test use of brackets when color is disabled

The interactive add command, 'git add -i', displays a menu of options using full words. When color is enabled, the first letter of each word is changed to a highlight color to signal that the first letter could be used as a command. Without color, brackets ("[]") are used around these first letters.

This behavior was not previously tested directly in t3701, so add a test for it now. Since we use 'git add -i >actual <input' without 'force_color', the color system recognizes that colors are not available on stdout and will be disabled by default.

This test would reproduce correctly with or without the fix in the previous commit to make sure that color.ui is respected in 'git add'.

---

Here is the patch to add this test on top of the previous change.

I've only validated this on my local computer, not through the GitGitGadget PR. If needed, I could send a v3 via GitGitGadget, but thought this would be a simple-enough addition here.

Thanks,
-Stolee

 t/t3701-add-interactive.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

index a93fe54e2ad..df3e85fc8d6 100755
@@ -734,6 +734,29 @@ test_expect_success 'colors can be overridden' '
 	test_cmp expect actual
 '
 
+test_expect_success 'brackets appear without color' '
+	git reset --hard &&
+	test_when_finished "git rm -f bracket-test" &&
+	test_write_lines context old more-context >bracket-test &&
+	git add bracket-test &&
+	test_write_lines context new more-context another-one >bracket-test &&
+
+	test_write_lines quit >input &&
+	git add -i >actual <input &&
+
+	sed "s/^|//g" >expect <<-\EOF &&
+	|           staged     unstaged path
+	|  1:        +3/-0        +2/-1 bracket-test
+	|
+	|*** Commands ***
+	|  1: [s]tatus	  2: [u]pdate	  3: [r]evert	  4: [a]dd untracked
+	|  5: [p]atch	  6: [d]iff	  7: [q]uit	  8: [h]elp
+	|What now> Bye.
+	EOF
+
+	test_cmp expect actual
+'
+
 test_expect_success 'colors can be skipped with color.ui=false' '
 	git reset --hard &&
 	test_when_finished "git rm -f color-test" &&

2.40.1.vfs.0.0

Hi Stolee

Thanks for adding this, the patch looks good. Strictly speaking you don't need the "g" at the end of the sed expression as it only ever matches a single instance within each line but that's not worth worrying about.

Best Wishes

Phillip

Interesting.

My understanding was that the new test is about when coloring gets triggered (namely, does the configuration variable set to false disable the coloring?), and we had test coverage about how coloring affects the output elsewhere (hence this one does not have to test the same thing).

Yeah, as the pattern is anchored at the left edge, "g" would cause no harm in this case, but the intention is "we have one extra bar at the left end to protect runs of spaces, and we want to strip that single bar" and it makes it clear not to write "g" there for that reason.

Good eyes. Thanks.