| Message ID | pull.1541.git.1685994164018.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add: check color.ui for interactive add | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > +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 && It is a minor thing, but for expected output that _wants_ to be indented in a non-standard way, it would be prudent to protect the lines from "git apply --whitespace=fix" and in-transit corruption, perhaps doing something like ... > + cat >expect <<-\EOF && > + staged unstaged path > + 1: +3/-0 +2/-1 color-test > + > + *** Commands *** ... this. sed -e "s/^|//" >expect <<-\EOF && | staged unstaged path | 1: +3/-0 +2/-1 color-test | |*** Commands *** Although this patch does not add such lines, the same principle applies to expected output lines that _wants_ to have trailing whitespaces. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> + test_decode_color <actual.raw >actual && > > It is a minor thing, but for expected output that _wants_ to be > indented in a non-standard way, it would be prudent to protect the > lines from "git apply --whitespace=fix" and in-transit corruption, > perhaps doing something like ... > >> + cat >expect <<-\EOF && >> + staged unstaged path >> + 1: +3/-0 +2/-1 color-test >> + >> + *** Commands *** > > ... this. > > sed -e "s/^|//" >expect <<-\EOF && > | staged unstaged path > | 1: +3/-0 +2/-1 color-test > | > |*** Commands *** > > Although this patch does not add such lines, the same principle > applies to expected output lines that _wants_ to have trailing > whitespaces. 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?
On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote: > 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. I think it should be OK to load the color config here, but note... > This is also a reoccurrence of the "config not loaded" bug from [3]. ...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
On 6/5/2023 10:13 PM, Jeff King wrote: > On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote: > >> 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. > > I think it should be OK to load the color config here, but note... > >> This is also a reoccurrence of the "config not loaded" bug from [3]. > > ...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. ...snipping valuable context... > 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. 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
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..00081418ea2 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -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 &&