{"thread":{"id":"59838","subject":"[PATCH] add: check color.ui for interactive add","startedAt":"2023-06-05T19:42:51Z","lastAt":"2023-06-12T17:13:14Z","messageCount":12,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Jeff King","Derrick Stolee","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478052","messageId":"pull.1541.git.1685994164018.gitgitgadget@gmail.com","threadId":"59838","inReplyTo":null,"subject":"[PATCH] add: check color.ui for interactive add","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-05T19:42:43Z","receivedAt":"2023-06-05T19:42:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen 'git add -i' and 'git add -p' were converted to a builtin, they\nintroduced a color bug: the 'color.ui' config setting is ignored.\n\nThe included test demonstrates an example that is similar to the\nprevious test, which focuses on customizing colors. Here, we are\ndemonstrating that colors are not being used at all.\n\nThe fix is simple, to use git_color_default_config() as the fallback for\ngit_add_config(). A more robust change would instead encapsulate the\ngit_use_color_default global in methods that would check the config\nsetting if it has not been initialized yet. Some ideas are being\ndiscussed on this front [1], but nothing has been finalized.\n\n[1] https://lore.kernel.org/git/pull.1539.git.1685716420.gitgitgadget@gmail.com/\n\nThis test case naturally bisects down to 0527ccb1b55 (add -i: default to\nthe built-in implementation, 2021-11-30), but the fix makes it clear\nthat this would be broken even if we added the config to use the builtin\nearlier than this.\n\nReported-by: Greg Alexander \nSigned-off-by: Derrick Stolee \n---\n add: check color.ui for interactive add\n \n This was reported by Greg Alexander gitgreg@galexander.org during Git\n IRC Standup [2].\n \n [2]\n https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05\n \n This is also a reoccurrence of the \"config not loaded\" bug from [3].\n \n [3]\n https://lore.kernel.org/git/pull.1530.git.1683745654800.gitgitgadget@gmail.com/\n \n I linked above to my RFC on lazy-loading global Git config, and these\n are the same \"root cause\" (not loading something early enough in the\n process) and my RFC proposes to fix this by changing our access\n patterns. By encapsulating these globals, we can make sure they are\n initialized from config before they are accessed.\n \n But that's a discussion for another thread. For now, fix the bug and\n we'll worry about the \"better\" (and bigger) thing to do another time.\n \n Thanks, -Stolee\n \n P.S. This fails the whitespace check due to the necessary left-padding\n spaces in the expected output in the test file.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1541\n\n builtin/add.c | 2 +-\n t/t3701-add-interactive.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 76cc026a68a..6137e7b4ad7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, cb);\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static const char embedded_advice[] = N_(\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3982b6b49dc..00081418ea2 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -734,6 +734,39 @@ test_expect_success 'colors can be overridden' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'colors can be skipped with color.ui=false' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f color-test\" &&\n+\ttest_write_lines context old more-context >color-test &&\n+\tgit add color-test &&\n+\ttest_write_lines context new more-context another-one >color-test &&\n+\n+\ttest_write_lines help quit >input &&\n+\tforce_color git \\\n+\t\t-c color.ui=false \\\n+\t\tadd -i >actual.raw actual &&\n+\tcat >expect <<-\\EOF &&\n+\t staged unstaged path\n+\t 1: +3/-0 +2/-1 color-test\n+\n+\t*** Commands ***\n+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n+\tWhat now> status - show paths with changes\n+\tupdate - add working tree state to the staged set of changes\n+\trevert - revert staged set of changes back to the HEAD version\n+\tpatch - pick hunks and update selectively\n+\tdiff - view diff between HEAD and index\n+\tadd untracked - add contents of untracked files to the staged set of changes\n+\t*** Commands ***\n+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n+\tWhat now> Bye.\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \tgit reset --hard &&\n \n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478056","messageId":"xmqqsfb54d4u.fsf@gitster.g","threadId":"59838","inReplyTo":"pull.1541.git.1685994164018.gitgitgadget@gmail.com","subject":"Re: [PATCH] add: check color.ui for interactive add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-06T01:01:53Z","receivedAt":"2023-06-06T01:01:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +test_expect_success 'colors can be skipped with color.ui=false' '\n> +\tgit reset --hard &&\n> +\ttest_when_finished \"git rm -f color-test\" &&\n> +\ttest_write_lines context old more-context >color-test &&\n> +\tgit add color-test &&\n> +\ttest_write_lines context new more-context another-one >color-test &&\n> +\n> +\ttest_write_lines help quit >input &&\n> +\tforce_color git \\\n> +\t\t-c color.ui=false \\\n> +\t\tadd -i >actual.raw +\ttest_decode_color actual &&\n\nIt is a minor thing, but for expected output that _wants_ to be\nindented in a non-standard way, it would be prudent to protect the\nlines from \"git apply --whitespace=fix\" and in-transit corruption,\nperhaps doing something like ...\n\n> +\tcat >expect <<-\\EOF &&\n> +\t staged unstaged path\n> +\t 1: +3/-0 +2/-1 color-test\n> +\n> +\t*** Commands ***\n\n... this.\n\n\tsed -e \"s/^|//\" >expect <<-\\EOF &&\n\t| staged unstaged path\n\t| 1: +3/-0 +2/-1 color-test\n\t|\n\t|*** Commands ***\n\nAlthough this patch does not add such lines, the same principle\napplies to expected output lines that _wants_ to have trailing\nwhitespaces.\n\nThanks.\n"},{"id":"478057","messageId":"xmqqo7lt4cyy.fsf@gitster.g","threadId":"59838","inReplyTo":"xmqqsfb54d4u.fsf@gitster.g","subject":"Re: [PATCH] add: check color.ui for interactive add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-06T01:05:25Z","receivedAt":"2023-06-06T01:05:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\ttest_decode_color actual &&\n>\n> It is a minor thing, but for expected output that _wants_ to be\n> indented in a non-standard way, it would be prudent to protect the\n> lines from \"git apply --whitespace=fix\" and in-transit corruption,\n> perhaps doing something like ...\n>\n>> +\tcat >expect <<-\\EOF &&\n>> +\t staged unstaged path\n>> +\t 1: +3/-0 +2/-1 color-test\n>> +\n>> +\t*** Commands ***\n>\n> ... this.\n>\n> \tsed -e \"s/^|//\" >expect <<-\\EOF &&\n> \t| staged unstaged path\n> \t| 1: +3/-0 +2/-1 color-test\n> \t|\n> \t|*** Commands ***\n>\n> Although this patch does not add such lines, the same principle\n> applies to expected output lines that _wants_ to have trailing\n> whitespaces.\n\nAnother thing. We are interested in the configuration to disable\ncolor, so instead of spelling out exact output we expect, which is\nbrittle when we anticipate that the contents used for the test may\nchange, wouldn't it work better to ensure that actual.raw and actual\nare identical?\n"},{"id":"478072","messageId":"20230606021349.GC89456@coredump.intra.peff.net","threadId":"59838","inReplyTo":"pull.1541.git.1685994164018.gitgitgadget@gmail.com","subject":"Re: [PATCH] add: check color.ui for interactive add","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-06-06T02:13:49Z","receivedAt":"2023-06-06T02:14:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> The fix is simple, to use git_color_default_config() as the fallback for\n> git_add_config(). A more robust change would instead encapsulate the\n> git_use_color_default global in methods that would check the config\n> setting if it has not been initialized yet. Some ideas are being\n> discussed on this front [1], but nothing has been finalized.\n\nI think it should be OK to load the color config here, but note...\n\n> This is also a reoccurrence of the \"config not loaded\" bug from [3].\n\n...that this case is a little different than the core.replaceRefs one.\nOne of the reasons we don't just load all config by default is that it\nwas an intentional scheme that not all programs would respect all\nconfig, and color in particular was one of the things that wasn't meant\nto be supported everywhere.\n\nIn other words, the idea was that porcelain like \"git diff\" would use\ngit_diff_ui_config(), which would load all the bells and whistles (like\ncolor). But plumbing like git-diff-tree uses git_diff_basic_config(),\nwhich skips those. And that way we can freely introduce new config\noptions without worrying that they will unexpectedly change the behavior\nof plumbing commands (because each command has to manually opt into the\nnew config).\n\nNow I won't claim that this approach hasn't created all sorts of\nheadaches over the years, and we might not be better off with something\nmore explicit (e.g., we parse all the config, but plumbing sets a flag\nto say \"I am plumbing; do not respect color.ui\"). But it is roughly the\napproach we've taken, so I'm mostly warning you that there may be\ndragons here. :)\n\nI say \"roughly\" because I actually think the rules have been bent a bit\nover the years. In particular, I think that git_use_color_default is\ninitialized to COLOR_AUTO, so something like:\n\n git diff-tree -p HEAD\n\nends up colorizing, even though it's plumbing. Which is maybe not so\nbad, but it's doubly silly that:\n\n git -c color.diff=false diff-tree -p HEAD\n\nstill colorizes, even though \"git diff\" in an equivalent situation would\nnot! That seems like a bug, but it's one that I suspect has been there\nsince we flipped color on by default many years ago, and nobody has\nreally complained.\n\nSo all of this is a big digression from your patch. I think for \"git\nadd\" it is probably OK to enable color by default. But I mostly want to\npoint out that trying to roll this into a more elaborate fix may run\nafoul of all kinds of rabbit holes.\n\n-Peff\n"},{"id":"478093","messageId":"e6c9b1a4-4700-1205-9606-86f611a7f9c2@github.com","threadId":"59838","inReplyTo":"20230606021349.GC89456@coredump.intra.peff.net","subject":"Re: [PATCH] add: check color.ui for interactive add","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-06-06T13:32:13Z","receivedAt":"2023-06-06T13:32:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/5/2023 10:13 PM, Jeff King wrote:\n> On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> The fix is simple, to use git_color_default_config() as the fallback for\n>> git_add_config(). A more robust change would instead encapsulate the\n>> git_use_color_default global in methods that would check the config\n>> setting if it has not been initialized yet. Some ideas are being\n>> discussed on this front [1], but nothing has been finalized.\n> \n> I think it should be OK to load the color config here, but note...\n> \n>> This is also a reoccurrence of the \"config not loaded\" bug from [3].\n> \n> ...that this case is a little different than the core.replaceRefs one.\n> One of the reasons we don't just load all config by default is that it\n> was an intentional scheme that not all programs would respect all\n> config, and color in particular was one of the things that wasn't meant\n> to be supported everywhere.\n\n...snipping valuable context...\n\n> So all of this is a big digression from your patch. I think for \"git\n> add\" it is probably OK to enable color by default. But I mostly want to\n> point out that trying to roll this into a more elaborate fix may run\n> afoul of all kinds of rabbit holes.\n\nThank you for this context, which I will keep in mind as an important\nfeature in this space. The default config doesn't have this property,\nso I'll remain focused on those values in the \"lazy load\" work.\n\nJust riffing: it's likely that we could load the color config in git.c\nbased on the \"porcelain\" vs \"plumbing\" category of a builtin. We have\nthat specification in command-list.txt, but _not_ in 'struct cmd_struct'\nso it would take some work to introduce that concept.\n\nThanks,\n-Stolee\n"},{"id":"478095","messageId":"pull.1541.v2.git.1686061219078.gitgitgadget@gmail.com","threadId":"59838","inReplyTo":"pull.1541.git.1685994164018.gitgitgadget@gmail.com","subject":"[PATCH v2] add: check color.ui for interactive add","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-06T14:20:18Z","receivedAt":"2023-06-06T14:20:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhen 'git add -i' and 'git add -p' were converted to a builtin, they\nintroduced a color bug: the 'color.ui' config setting is ignored.\n\nThe included test demonstrates an example that is similar to the\nprevious test, which focuses on customizing colors. Here, we are\ndemonstrating that colors are not being used at all by comparing the raw\noutput and the color-decoded version of that output.\n\nThe fix is simple, to use git_color_default_config() as the fallback for\ngit_add_config(). A more robust change would instead encapsulate the\ngit_use_color_default global in methods that would check the config\nsetting if it has not been initialized yet. Some ideas are being\ndiscussed on this front [1], but nothing has been finalized.\n\n[1] https://lore.kernel.org/git/pull.1539.git.1685716420.gitgitgadget@gmail.com/\n\nThis test case naturally bisects down to 0527ccb1b55 (add -i: default to\nthe built-in implementation, 2021-11-30), but the fix makes it clear\nthat this would be broken even if we added the config to use the builtin\nearlier than this.\n\nReported-by: Greg Alexander \nSigned-off-by: Derrick Stolee \n---\n add: check color.ui for interactive add\n \n This was reported by Greg Alexander gitgreg@galexander.org during Git\n IRC Standup [2].\n \n [2]\n https://colabti.org/irclogger/irclogger_log/git-devel?date=2023-06-05\n \n This is also a reoccurrence of the \"config not loaded\" bug from [3].\n \n [3]\n https://lore.kernel.org/git/pull.1530.git.1683745654800.gitgitgadget@gmail.com/\n \n I linked above to my RFC on lazy-loading global Git config, and these\n are the same \"root cause\" (not loading something early enough in the\n process) and my RFC proposes to fix this by changing our access\n patterns. By encapsulating these globals, we can make sure they are\n initialized from config before they are accessed.\n \n But that's a discussion for another thread. For now, fix the bug and\n we'll worry about the \"better\" (and bigger) thing to do another time.\n \n \n Update in v2\n ============\n \n * The test is simplified to compare the raw output to the color-decoded\n output. This makes the test more robust to possible future changes to\n interactive add.\n \n Thanks, -Stolee\n \n P.S. This fails the whitespace check due to the necessary left-padding\n spaces in the expected output in the test file.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1541%2Fderrickstolee%2Fadd-interactive-color-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1541/derrickstolee/add-interactive-color-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1541\n\nRange-diff vs v1:\n\n 1: a76893de8a7 ! 1: 6d3e34d51c5 add: check color.ui for interactive add\n @@ Commit message\n \n The included test demonstrates an example that is similar to the\n previous test, which focuses on customizing colors. Here, we are\n - demonstrating that colors are not being used at all.\n + demonstrating that colors are not being used at all by comparing the raw\n + output and the color-decoded version of that output.\n \n The fix is simple, to use git_color_default_config() as the fallback for\n git_add_config(). A more robust change would instead encapsulate the\n @@ t/t3701-add-interactive.sh: test_expect_success 'colors can be overridden' '\n +\t\t-c color.ui=false \\\n +\t\tadd -i >actual.raw actual &&\n -+\tcat >expect <<-\\EOF &&\n -+\t staged unstaged path\n -+\t 1: +3/-0 +2/-1 color-test\n -+\n -+\t*** Commands ***\n -+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n -+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n -+\tWhat now> status - show paths with changes\n -+\tupdate - add working tree state to the staged set of changes\n -+\trevert - revert staged set of changes back to the HEAD version\n -+\tpatch - pick hunks and update selectively\n -+\tdiff - view diff between HEAD and index\n -+\tadd untracked - add contents of untracked files to the staged set of changes\n -+\t*** Commands ***\n -+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n -+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n -+\tWhat now> Bye.\n -+\tEOF\n -+\ttest_cmp expect actual\n ++\ttest_cmp actual.raw actual\n +'\n +\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n\n\n builtin/add.c | 2 +-\n t/t3701-add-interactive.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 76cc026a68a..6137e7b4ad7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -365,7 +365,7 @@ static int add_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\treturn git_default_config(var, value, cb);\n+\treturn git_color_default_config(var, value, cb);\n }\n \n static const char embedded_advice[] = N_(\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 3982b6b49dc..a93fe54e2ad 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -734,6 +734,21 @@ test_expect_success 'colors can be overridden' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'colors can be skipped with color.ui=false' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f color-test\" &&\n+\ttest_write_lines context old more-context >color-test &&\n+\tgit add color-test &&\n+\ttest_write_lines context new more-context another-one >color-test &&\n+\n+\ttest_write_lines help quit >input &&\n+\tforce_color git \\\n+\t\t-c color.ui=false \\\n+\t\tadd -i >actual.raw actual &&\n+\ttest_cmp actual.raw actual\n+'\n+\n test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '\n \tgit reset --hard &&\n \n\nbase-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09\n-- \ngitgitgadget\n"},{"id":"478141","messageId":"a76a91a0-cefe-807f-f1f6-4a277d724630@gmail.com","threadId":"59838","inReplyTo":"pull.1541.v2.git.1686061219078.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] add: check color.ui for interactive add","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-07T11:09:45Z","receivedAt":"2023-06-07T11:10:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nHi Stolee\n\nThanks for fixing this\n\nOn 06/06/2023 15:20, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n >\n> -+\t*** Commands ***\n> -+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n> -+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n> -+\tWhat now> status - show paths with changes\n> -+\tupdate - add working tree state to the staged set of changes\n> -+\trevert - revert staged set of changes back to the HEAD version\n> -+\tpatch - pick hunks and update selectively\n> -+\tdiff - view diff between HEAD and index\n> -+\tadd untracked - add contents of untracked files to the staged set of changes\n> -+\t*** Commands ***\n> -+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n> -+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n> -+\tWhat now> Bye.\n> -+\tEOF\n> -+\ttest_cmp expect actual\n> ++\ttest_cmp actual.raw actual\n> +'\n\nI know Junio suggested this change, but I'm in two minds as to whether \nit is a good idea. The reason is that we're no-longer testing that we \nadd \"[]\" around the text that would have been highlighted if color was \nenabled. That is with --color we print \"1: status\" with the \"s\" \nhighlighted rather than \"1: [s]tatus\". So while the revised patch tests \nthere is no color in the output, it does not test that we print the \noutput correctly in that case.\n\nBest Wishes\n\nPhillip\n"},{"id":"478142","messageId":"4b0e8026-4372-67ea-356c-b8073a6da20e@github.com","threadId":"59838","inReplyTo":"a76a91a0-cefe-807f-f1f6-4a277d724630@gmail.com","subject":"Re: [PATCH v2] add: check color.ui for interactive add","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-06-07T13:33:58Z","receivedAt":"2023-06-07T13:34:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/7/2023 7:09 AM, Phillip Wood wrote:\n> On 06/06/2023 15:20, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>>       -+    *** Commands ***\n>>       -+      1: [s]tatus      2: [u]pdate      3: [r]evert      4: [a]dd untracked\n>>       -+      5: [p]atch      6: [d]iff      7: [q]uit      8: [h]elp\n>>       -+    What now> status        - show paths with changes\n>>       -+    update        - add working tree state to the staged set of changes\n>>       -+    revert        - revert staged set of changes back to the HEAD version\n>>       -+    patch         - pick hunks and update selectively\n>>       -+    diff          - view diff between HEAD and index\n>>       -+    add untracked - add contents of untracked files to the staged set of changes\n>>       -+    *** Commands ***\n>>       -+      1: [s]tatus      2: [u]pdate      3: [r]evert      4: [a]dd untracked\n>>       -+      5: [p]atch      6: [d]iff      7: [q]uit      8: [h]elp\n>>       -+    What now> Bye.\n>>       -+    EOF\n>>       -+    test_cmp expect actual\n>>       ++    test_cmp actual.raw actual\n>>        +'\n> \n> I know Junio suggested this change, but I'm in two minds as to whether\n> it is a good idea. The reason is that we're no-longer testing that we\n> add \"[]\" around the text that would have been highlighted if color was\n> enabled. That is with --color we print \"1: status\" with the \"s\" highlighted\n> rather than \"1: [s]tatus\". So while the revised patch tests there is no\n> color in the output, it does not test that we print the output correctly\n> in that case.\n\nThis is a good point, and something that I somewhat expected to find as\nan example check in the rest of the test script. I think the color would\nbe disabled if we didn't do force_color, even before this fix.\n\nHowever, I'll double-check that by adding a separate test for just that\nbit of the UI, keeping this color.ui=false test focused on the color side\nof things.\n\nThanks,\n-Stolee\n"},{"id":"478143","messageId":"281431b8-af40-9de9-f4b4-c596c5dbb3af@github.com","threadId":"59838","inReplyTo":"pull.1541.v2.git.1686061219078.gitgitgadget@gmail.com","subject":"[Patch v2 2/2] add: test use of brackets when color is disabled","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-06-07T13:44:48Z","receivedAt":"2023-06-07T13:45:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001\nFrom: Derrick Stolee \nDate: Wed, 7 Jun 2023 09:39:01 -0400\nSubject: [PATCH v2 2/2] add: test use of brackets when color is disabled\n\nThe interactive add command, 'git add -i', displays a menu of options\nusing full words. When color is enabled, the first letter of each word\nis changed to a highlight color to signal that the first letter could be\nused as a command. Without color, brackets (\"[]\") are used around these\nfirst letters.\n\nThis behavior was not previously tested directly in t3701, so add a test\nfor it now. Since we use 'git add -i >actual \nSigned-off-by: Derrick Stolee \n---\n\nHere is the patch to add this test on top of the previous change.\n\nI've only validated this on my local computer, not through the\nGitGitGadget PR. If needed, I could send a v3 via GitGitGadget,\nbut thought this would be a simple-enough addition here.\n\nThanks,\n-Stolee\n\n t/t3701-add-interactive.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex a93fe54e2ad..df3e85fc8d6 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -734,6 +734,29 @@ test_expect_success 'colors can be overridden' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'brackets appear without color' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git rm -f bracket-test\" &&\n+\ttest_write_lines context old more-context >bracket-test &&\n+\tgit add bracket-test &&\n+\ttest_write_lines context new more-context another-one >bracket-test &&\n+\n+\ttest_write_lines quit >input &&\n+\tgit add -i >actual expect <<-\\EOF &&\n+\t| staged unstaged path\n+\t| 1: +3/-0 +2/-1 bracket-test\n+\t|\n+\t|*** Commands ***\n+\t| 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n+\t| 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n+\t|What now> Bye.\n+\tEOF\n+\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'colors can be skipped with color.ui=false' '\n \tgit reset --hard &&\n \ttest_when_finished \"git rm -f color-test\" &&\n-- \n2.40.1.vfs.0.0\n\n"},{"id":"478145","messageId":"2143c98e-238b-8edc-a16c-be448a8b56e8@gmail.com","threadId":"59838","inReplyTo":"281431b8-af40-9de9-f4b4-c596c5dbb3af@github.com","subject":"Re: [Patch v2 2/2] add: test use of brackets when color is disabled","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-07T15:31:12Z","receivedAt":"2023-06-07T15:31:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stolee\n\nOn 07/06/2023 14:44, Derrick Stolee wrote:\n> From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001\n> From: Derrick Stolee \n> Date: Wed, 7 Jun 2023 09:39:01 -0400\n> Subject: [PATCH v2 2/2] add: test use of brackets when color is disabled\n> \n> The interactive add command, 'git add -i', displays a menu of options\n> using full words. When color is enabled, the first letter of each word\n> is changed to a highlight color to signal that the first letter could be\n> used as a command. Without color, brackets (\"[]\") are used around these\n> first letters.\n> \n> This behavior was not previously tested directly in t3701, so add a test\n> for it now. Since we use 'git add -i >actual 'force_color', the color system recognizes that colors are not available\n> on stdout and will be disabled by default.\n> \n> This test would reproduce correctly with or without the fix in the\n> previous commit to make sure that color.ui is respected in 'git add'.\n> \n> Reported-by: Phillip Wood \n> Signed-off-by: Derrick Stolee \n> ---\n\nThanks for adding this, the patch looks good. Strictly speaking you \ndon't need the \"g\" at the end of the sed expression as it only ever \nmatches a single instance within each line but that's not worth worrying \nabout.\n\nBest Wishes\n\nPhillip\n\n> Here is the patch to add this test on top of the previous change.\n> \n> I've only validated this on my local computer, not through the\n> GitGitGadget PR. If needed, I could send a v3 via GitGitGadget,\n> but thought this would be a simple-enough addition here.\n> \n> Thanks,\n> -Stolee\n> \n> t/t3701-add-interactive.sh | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n> \n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index a93fe54e2ad..df3e85fc8d6 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -734,6 +734,29 @@ test_expect_success 'colors can be overridden' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'brackets appear without color' '\n> +\tgit reset --hard &&\n> +\ttest_when_finished \"git rm -f bracket-test\" &&\n> +\ttest_write_lines context old more-context >bracket-test &&\n> +\tgit add bracket-test &&\n> +\ttest_write_lines context new more-context another-one >bracket-test &&\n> +\n> +\ttest_write_lines quit >input &&\n> +\tgit add -i >actual +\n> +\tsed \"s/^|//g\" >expect <<-\\EOF &&\n> +\t| staged unstaged path\n> +\t| 1: +3/-0 +2/-1 bracket-test\n> +\t|\n> +\t|*** Commands ***\n> +\t| 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n> +\t| 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n> +\t|What now> Bye.\n> +\tEOF\n> +\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'colors can be skipped with color.ui=false' '\n> \tgit reset --hard &&\n> \ttest_when_finished \"git rm -f color-test\" &&\n"},{"id":"478284","messageId":"xmqqlegoboa3.fsf@gitster.g","threadId":"59838","inReplyTo":"a76a91a0-cefe-807f-f1f6-4a277d724630@gmail.com","subject":"Re: [PATCH v2] add: check color.ui for interactive add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T17:09:40Z","receivedAt":"2023-06-12T17:10:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> -+\t*** Commands ***\n>> -+\t 1: [s]tatus\t 2: [u]pdate\t 3: [r]evert\t 4: [a]dd untracked\n>> -+\t 5: [p]atch\t 6: [d]iff\t 7: [q]uit\t 8: [h]elp\n>>...\n> ... The reason is that we're no-longer testing that we\n> add \"[]\" around the text that would have been highlighted if color was\n> enabled. That is with --color we print \"1: status\" with the \"s\"\n> highlighted rather than \"1: [s]tatus\". So while the revised patch\n> tests there is no color in the output, it does not test that we print\n> the output correctly in that case.\n\nInteresting.\n\nMy understanding was that the new test is about when coloring gets\ntriggered (namely, does the configuration variable set to false\ndisable the coloring?), and we had test coverage about how coloring\naffects the output elsewhere (hence this one does not have to test\nthe same thing).\n\n\n"},{"id":"478285","messageId":"xmqqh6rcbo4a.fsf@gitster.g","threadId":"59838","inReplyTo":"2143c98e-238b-8edc-a16c-be448a8b56e8@gmail.com","subject":"Re: [Patch v2 2/2] add: test use of brackets when color is disabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T17:13:09Z","receivedAt":"2023-06-12T17:13:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Thanks for adding this, the patch looks good. Strictly speaking you\n> don't need the \"g\" at the end of the sed expression as it only ever\n> matches a single instance within each line but that's not worth\n> worrying about.\n\nYeah, as the pattern is anchored at the left edge, \"g\" would cause\nno harm in this case, but the intention is \"we have one extra bar at\nthe left end to protect runs of spaces, and we want to strip that\nsingle bar\" and it makes it clear not to write \"g\" there for that\nreason.\n\nGood eyes. Thanks.\n"}]}