| Message ID | pull.1541.v2.git.1686061219078.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7cf3b49f47f02ed1cab5b1cd03a5e27acaa13c99 |
| Headers | show |
| Series | [v2] add: check color.ui for interactive add | expand |
Hi Stolee Hi Stolee Thanks for fixing this On 06/06/2023 15:20, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > -+ *** 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 > +' 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
On 6/7/2023 7:09 AM, Phillip Wood wrote: > On 06/06/2023 15:20, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <derrickstolee@github.com> >> >> -+ *** 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 >> +' > > 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. 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
Phillip Wood <phillip.wood123@gmail.com> writes: >> -+ *** 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 >>... > ... 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. 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).
diff --git a/builtin/add.c b/builtin/add.c index 76cc026a68a..6137e7b4ad7 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -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_( diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 3982b6b49dc..a93fe54e2ad 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -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 &&