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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"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.

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

[Junio C Hamano]

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?

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

[Jeff King]

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

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

[Derrick Stolee]

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Phillip Wood]

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

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

[Derrick Stolee]

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

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

[Derrick Stolee]

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(+)

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

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

[Phillip Wood]

Hi Stolee

On 07/06/2023 14:44, Derrick Stolee wrote:
>  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

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

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

[Junio C Hamano]

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).

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

[Junio C Hamano]

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.