threads / patch / 59838

patchadd: check color.ui for interactive add

Subject: [PATCH] add: check color.ui for interactive add

## tl;dr

12 messages between Jun 5, 2023 and Jun 12, 2023. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Derrick Stolee via GitGitGadget· Jun 5, 2023, 19:42 UTC · lore
From: Derrick Stolee <derrickstolee@github.com>

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/pull.1539.git.1685716420.gitgitgadget@gmail.com/

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.

Reported-by: Greg Alexander <gitgreg@galexander.org>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    add: check color.ui for interactive add
    
    This was reported by Greg Alexander gitgreg@galexander.org 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/pull.1530.git.1683745654800.gitgitgadget@gmail.com/
    
    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(-)
Show changes to 2 files +34 −1

builtin/add.c, t/t3701-add-interactive.sh

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 &&
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

-- 
gitgitgadget
Junio C Hamano· Jun 6, 2023, 01:01 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH] add: check color.ui for interactive add

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> +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 ...

Show 5 quoted lines
> +	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· Jun 6, 2023, 01:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] add: check color.ui for interactive add

Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
>> +	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?

Jeff King· Jun 6, 2023, 02:13 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH] add: check color.ui for interactive add

On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 5 quoted lines
> 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
Derrick Stolee· Jun 6, 2023, 13:32 UTC · re: Jeff King · lore

Re: [PATCH] add: check color.ui for interactive add

On 6/5/2023 10:13 PM, Jeff King wrote:
Show 17 quoted lines
> 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

Derrick Stolee via GitGitGadget· Jun 6, 2023, 14:20 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2] add: check color.ui for interactive add

From: Derrick Stolee <derrickstolee@github.com>

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/pull.1539.git.1685716420.gitgitgadget@gmail.com/

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.

Reported-by: Greg Alexander <gitgreg@galexander.org>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    add: check color.ui for interactive add
    
    This was reported by Greg Alexander gitgreg@galexander.org 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/pull.1530.git.1683745654800.gitgitgadget@gmail.com/
    
    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(-)
Show changes to 2 files +16 −1

builtin/add.c, t/t3701-add-interactive.sh

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 &&
 

base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09

-- 
gitgitgadget
Phillip Wood· Jun 7, 2023, 11:09 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v2] add: check color.ui for interactive add

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>
 >
Show 17 quoted lines
>       -+	*** 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
Derrick Stolee· Jun 7, 2023, 13:33 UTC · re: Phillip Wood · lore

Re: [PATCH v2] add: check color.ui for interactive add

On 6/7/2023 7:09 AM, Phillip Wood wrote:
Show 28 quoted lines
> 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

Junio C Hamano· Jun 12, 2023, 17:09 UTC · re: Phillip Wood · lore

Re: [PATCH v2] add: check color.ui for interactive add

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 10 quoted lines
>>       -+	*** 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).

Derrick Stolee· Jun 7, 2023, 13:44 UTC · re: Derrick Stolee via GitGitGadget · lore

[Patch v2 2/2] add: test use of brackets when color is disabled

From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <derrickstolee@github.com>
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'.

Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
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(+)
Show changes to t/t3701-add-interactive.sh +23 −0
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index a93fe54e2ad..df3e85fc8d6 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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
Phillip Wood· Jun 7, 2023, 15:31 UTC · re: Derrick Stolee · lore

Re: [Patch v2 2/2] add: test use of brackets when color is disabled

Hi Stolee
On 07/06/2023 14:44, Derrick Stolee wrote:
Show 22 quoted lines
>  From 02156b81bbb2cafb19d702c55d45714fcf224048 Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <derrickstolee@github.com>
> 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'.
> 
> Reported-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---

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
Show 46 quoted lines
> 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(+)
> 
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index a93fe54e2ad..df3e85fc8d6 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -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" &&
Junio C Hamano· Jun 12, 2023, 17:13 UTC · re: Phillip Wood · lore

Re: [Patch v2 2/2] add: test use of brackets when color is disabled

Phillip Wood <phillip.wood123@gmail.com> writes:
> 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.

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.

← back to recent threads