["BUG"] builtin add-interactive does not honor 'color.diff. frag'

["BUG"] builtin add-interactive does not honor 'color.diff. frag'

[Philippe Blain]

Hi Johannes,

I tried the builtin add-interactive for the first time today, 
and noticed a very minor difference with the Perl version.
The title says it all.

Repro:

# Change the default color
$ git config color.diff.frag "magenta bold"
# Compare
$ git add -p
# and
$ git -c add.interactive.useBuiltin=true add -p 
# I'm on latest master
$ git --version
git version 2.29.2.154.g7f7ebe054a

Cheers,
Philippe.

Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'

[Jeff King]

On Fri, Nov 06, 2020 at 11:31:32AM -0500, Philippe Blain wrote:

> I tried the builtin add-interactive for the first time today, 
> and noticed a very minor difference with the Perl version.
> The title says it all.
> 
> Repro:
> 
> # Change the default color
> $ git config color.diff.frag "magenta bold"
> # Compare
> $ git add -p
> # and
> $ git -c add.interactive.useBuiltin=true add -p 
> # I'm on latest master
> $ git --version
> git version 2.29.2.154.g7f7ebe054a

It looks like the code in add-interactive.c uses "fraginfo" instead of
"frag". But there's something much more puzzling, which is that it
checks color.interactive.frag. AFAIK that has never been a valid config
option (there are some color.interactive.* values, but they are all for
interactive-specific things).

So something like this seems to go in the right direction (and makes
your example work):

diff --git a/add-interactive.c b/add-interactive.c
index a14c0feaa2..1eb69c5160 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -12,10 +12,11 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
+		       const char *section,
 		       const char *slot_name, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s.%s", section, slot_name);
 	const char *value;
 
 	if (!s->use_color)
@@ -40,18 +41,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive", "header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive", "help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive", "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive", "error", s->error_color, GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff", "reset", s->reset_color, GIT_COLOR_RESET);
+	init_color(r, s, "diff", "frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff", "context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff", "old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff", "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	FREE_AND_NULL(s->interactive_diff_filter);

but that is missing another thing: for historical reasons we allow both
color.diff.frag and diff.color.frag. TBH, I wouldn't be too sad to see
us drop support for the historical versions in this setting. But it
makes me wonder if we could be reusing the parsing from
git_diff_basic_config(), which handles both cases.

I'm not entirely clear on how add-interactive.c invokes the diff, and
whether it would be unhappy to pick up diff config. But it seems like
this:

diff --git a/builtin/add.c b/builtin/add.c
index a825887c50..47bb6ea10b 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -362,7 +362,7 @@ static int add_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	return git_diff_basic_config(var, value, cb);
 }
 
 static const char embedded_advice[] = N_(

might be the simplest thing (and drop the extra diff-config parsing from
add-interactive.c that I was touching above). It does need to be done in
each program that calls into the add-interactive code though (so
checkout, etc). Obviously it would be easy for init_add_i_state() to
make a similar call, though that may open us up to loading the diff
config twice. I suspect that would probably be OK, but it could lead to
weirdness if there are any multi-valued config options.

-Peff

Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'

[Johannes Schindelin]

Hi Peff & Philippe,

On Fri, 6 Nov 2020, Jeff King wrote:

> On Fri, Nov 06, 2020 at 11:31:32AM -0500, Philippe Blain wrote:
>
> > I tried the builtin add-interactive for the first time today,
> > and noticed a very minor difference with the Perl version.
> > The title says it all.
> >
> > Repro:
> >
> > # Change the default color
> > $ git config color.diff.frag "magenta bold"
> > # Compare
> > $ git add -p
> > # and
> > $ git -c add.interactive.useBuiltin=true add -p
> > # I'm on latest master
> > $ git --version
> > git version 2.29.2.154.g7f7ebe054a
>
> It looks like the code in add-interactive.c uses "fraginfo" instead of
> "frag".


Aaargh, yes, I missed that.

> But there's something much more puzzling, which is that it checks
> color.interactive.frag. AFAIK that has never been a valid config option
> (there are some color.interactive.* values, but they are all for
> interactive-specific things).

And that. This is the code in `git-add--interactive.perl`:

-- snip --
my $menu_use_color = $repo->get_colorbool('color.interactive');
my ($prompt_color, $header_color, $help_color) =
        $menu_use_color ? (
                $repo->get_color('color.interactive.prompt', 'bold blue'),
                $repo->get_color('color.interactive.header', 'bold'),
                $repo->get_color('color.interactive.help', 'red bold'),
        ) : ();
my $error_color = ();
if ($menu_use_color) {
        my $help_color_spec = ($repo->config('color.interactive.help') or
                                'red bold');
        $error_color = $repo->get_color('color.interactive.error',
                                        $help_color_spec);
}

my $diff_use_color = $repo->get_colorbool('color.diff');
my ($fraginfo_color) =
        $diff_use_color ? (
                $repo->get_color('color.diff.frag', 'cyan'),
        ) : ();
my ($diff_plain_color) =
        $diff_use_color ? (
                $repo->get_color('color.diff.plain', ''),
        ) : ();
my ($diff_old_color) =
        $diff_use_color ? (
                $repo->get_color('color.diff.old', 'red'),
        ) : ();
my ($diff_new_color) =
        $diff_use_color ? (
                $repo->get_color('color.diff.new', 'green'),
        ) : ();

my $normal_color = $repo->get_color("", "reset");
-- snap --

> So something like this seems to go in the right direction (and makes
> your example work):
>
> diff --git a/add-interactive.c b/add-interactive.c
> index a14c0feaa2..1eb69c5160 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -12,10 +12,11 @@
>  #include "prompt.h"
>
>  static void init_color(struct repository *r, struct add_i_state *s,
> +		       const char *section,
>  		       const char *slot_name, char *dst,
>  		       const char *default_color)
>  {
> -	char *key = xstrfmt("color.interactive.%s", slot_name);
> +	char *key = xstrfmt("color.%s.%s", section, slot_name);
>  	const char *value;
>
>  	if (!s->use_color)
> @@ -40,18 +41,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>  			git_config_colorbool("color.interactive", value);
>  	s->use_color = want_color(s->use_color);
>
> -	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
> -	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
> -	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
> -	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
> -	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
> -	init_color(r, s, "fraginfo", s->fraginfo_color,
> +	init_color(r, s, "interactive", "header", s->header_color, GIT_COLOR_BOLD);
> +	init_color(r, s, "interactive", "help", s->help_color, GIT_COLOR_BOLD_RED);
> +	init_color(r, s, "interactive", "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
> +	init_color(r, s, "interactive", "error", s->error_color, GIT_COLOR_BOLD_RED);
> +
> +	init_color(r, s, "diff", "reset", s->reset_color, GIT_COLOR_RESET);
> +	init_color(r, s, "diff", "frag", s->fraginfo_color,
>  		   diff_get_color(s->use_color, DIFF_FRAGINFO));
> -	init_color(r, s, "context", s->context_color,
> +	init_color(r, s, "diff", "context", s->context_color,
>  		diff_get_color(s->use_color, DIFF_CONTEXT));
> -	init_color(r, s, "old", s->file_old_color,
> +	init_color(r, s, "diff", "old", s->file_old_color,
>  		diff_get_color(s->use_color, DIFF_FILE_OLD));
> -	init_color(r, s, "new", s->file_new_color,
> +	init_color(r, s, "diff", "new", s->file_new_color,
>  		diff_get_color(s->use_color, DIFF_FILE_NEW));
>
>  	FREE_AND_NULL(s->interactive_diff_filter);
>
> but that is missing another thing: for historical reasons we allow both
> color.diff.frag and diff.color.frag. TBH, I wouldn't be too sad to see
> us drop support for the historical versions in this setting. But it
> makes me wonder if we could be reusing the parsing from
> git_diff_basic_config(), which handles both cases.

Hmm. The Perl version uses the Git.pm's `get_color()` function, which in
turn invokes `git config --get-color` with the exact keys I pasted above.

All along the way to `builtin/config.c`'s `git_get_color_config()`, I fail
to see any `color.diff`/`diff.color` munging. So I believe that `git add
--interactive` _never_ looked at the `diff.color` section to begin with.

> I'm not entirely clear on how add-interactive.c invokes the diff, and
> whether it would be unhappy to pick up diff config. But it seems like
> this:
>
> diff --git a/builtin/add.c b/builtin/add.c
> index a825887c50..47bb6ea10b 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -362,7 +362,7 @@ static int add_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>
> -	return git_default_config(var, value, cb);
> +	return git_diff_basic_config(var, value, cb);
>  }
>
>  static const char embedded_advice[] = N_(
>
> might be the simplest thing (and drop the extra diff-config parsing from
> add-interactive.c that I was touching above). It does need to be done in
> each program that calls into the add-interactive code though (so
> checkout, etc). Obviously it would be easy for init_add_i_state() to
> make a similar call, though that may open us up to loading the diff
> config twice. I suspect that would probably be OK, but it could lead to
> weirdness if there are any multi-valued config options.

Hmm. I don't like it. So far, we were very careful to keep respecting
`struct repository *r` in the built-in interactive add command. This would
go the exact opposite way.

Combined with the fact that we never supported `diff.color.*` in the
interactive add command anyway, I would rather continue with the first
variant you presented.

A bigger puzzle for me was: why did we not catch that earlier? I vaguely
remember that we introduced _specifically_ code to test coloring, and to
make those test work on Windows (rather than skipping them all).

*clicketyclick* ah, we only tests the menu of `git add -i`, and we do not
even override the colors...

Will try to set aside some time to work on fixing this,
Dscho

Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'

[Jeff King]

On Tue, Nov 10, 2020 at 05:06:59PM +0100, Johannes Schindelin wrote:

> > but that is missing another thing: for historical reasons we allow both
> > color.diff.frag and diff.color.frag. TBH, I wouldn't be too sad to see
> > us drop support for the historical versions in this setting. But it
> > makes me wonder if we could be reusing the parsing from
> > git_diff_basic_config(), which handles both cases.
> 
> Hmm. The Perl version uses the Git.pm's `get_color()` function, which in
> turn invokes `git config --get-color` with the exact keys I pasted above.
> 
> All along the way to `builtin/config.c`'s `git_get_color_config()`, I fail
> to see any `color.diff`/`diff.color` munging. So I believe that `git add
> --interactive` _never_ looked at the `diff.color` section to begin with.

Yeah, I think you're right. And I'm not too surprised. The same "I
wouldn't be too sad" probably applied back then, too.

Yep: https://lore.kernel.org/git/20071122122540.GH12913@sigill.intra.peff.net/

> > I'm not entirely clear on how add-interactive.c invokes the diff, and
> > whether it would be unhappy to pick up diff config. But it seems like
> > this:
> [...]
> 
> Hmm. I don't like it. So far, we were very careful to keep respecting
> `struct repository *r` in the built-in interactive add command. This would
> go the exact opposite way.
> 
> Combined with the fact that we never supported `diff.color.*` in the
> interactive add command anyway, I would rather continue with the first
> variant you presented.

Yeah, that seems quite sensible to me.

-Peff

[PATCH] add-interactive.c: use correct names to load color.diff.* config

[Jeff King]

On Tue, Nov 10, 2020 at 05:06:59PM +0100, Johannes Schindelin wrote:

> A bigger puzzle for me was: why did we not catch that earlier? I vaguely
> remember that we introduced _specifically_ code to test coloring, and to
> make those test work on Windows (rather than skipping them all).
> 
> *clicketyclick* ah, we only tests the menu of `git add -i`, and we do not
> even override the colors...

Yeah, the test coverage could definitely be improved.

> Will try to set aside some time to work on fixing this,

How about this?

-- >8 --
Subject: add-interactive.c: use correct names to load color.diff.* config

The builtin version of add-interactive mistakenly loads diff colors from
color.interactive.* instead of color.diff.*. It also accidentally spells
"frag" as "fraginfo".

Let's fix that, and add some test coverage:

  - check that color.diff.* is respected (this passes with the perl
    version, but without this patch fails if GIT_TEST_ADD_I_USE_BUILTIN
    is set)

  - check that color.interactive.* is respected; this passes already
    with both versions, but confirms we didn't break anything

Note that neither test is exhaustive over the set of color config, but
this is enough to sanity check the system (and we do check frag
explicitly because of its typo).

Note also that we don't respect the historical "diff.color.*". The perl
version never did, and those have been deprecated since 2007.

Signed-off-by: Jeff King <peff@peff.net>
---
I don't love that we have to repeat the set of color config here (and I
guess maybe somebody would complain that we don't respect all of the
weird "moved" variants?). But I agree that it's hard to re-use the
existing diff code without stomping all over global variables. So I'd
consider this the minimal fix bringing us in line with the perl version,
but we may want to later revisit the diff color-config code as a whole.

 add-interactive.c          | 22 ++++++++++++----------
 t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 555c4abf32..208a058a68 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -12,10 +12,11 @@
 #include "prompt.h"
 
 static void init_color(struct repository *r, struct add_i_state *s,
+		       const char *section,
 		       const char *slot_name, char *dst,
 		       const char *default_color)
 {
-	char *key = xstrfmt("color.interactive.%s", slot_name);
+	char *key = xstrfmt("color.%s.%s", section, slot_name);
 	const char *value;
 
 	if (!s->use_color)
@@ -40,18 +41,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
 			git_config_colorbool("color.interactive", value);
 	s->use_color = want_color(s->use_color);
 
-	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
-	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
-	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
-	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
-	init_color(r, s, "fraginfo", s->fraginfo_color,
+	init_color(r, s, "interactive", "header", s->header_color, GIT_COLOR_BOLD);
+	init_color(r, s, "interactive", "help", s->help_color, GIT_COLOR_BOLD_RED);
+	init_color(r, s, "interactive", "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
+	init_color(r, s, "interactive", "error", s->error_color, GIT_COLOR_BOLD_RED);
+
+	init_color(r, s, "diff", "reset", s->reset_color, GIT_COLOR_RESET);
+	init_color(r, s, "diff", "frag", s->fraginfo_color,
 		   diff_get_color(s->use_color, DIFF_FRAGINFO));
-	init_color(r, s, "context", s->context_color,
+	init_color(r, s, "diff", "context", s->context_color,
 		diff_get_color(s->use_color, DIFF_CONTEXT));
-	init_color(r, s, "old", s->file_old_color,
+	init_color(r, s, "diff", "old", s->file_old_color,
 		diff_get_color(s->use_color, DIFF_FILE_OLD));
-	init_color(r, s, "new", s->file_new_color,
+	init_color(r, s, "diff", "new", s->file_new_color,
 		diff_get_color(s->use_color, DIFF_FILE_NEW));
 
 	FREE_AND_NULL(s->interactive_diff_filter);
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index ca04fac417..7c3107a04a 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -602,6 +602,27 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
 	grep "old<" output
 '
 
+test_expect_success 'colorized diffs respect color config' '
+	git reset --hard &&
+
+	echo content >test &&
+	printf y >y &&
+	force_color git \
+		-c color.diff.meta="bold red" \
+		-c color.diff.frag="bold blue" \
+		-c color.diff.old="green" \
+		-c color.diff.new="red" \
+		add -p >output.raw 2>&1 <y &&
+	test_decode_color <output.raw >output &&
+
+	# do not check the full output, which would make the test brittle;
+	# just make sure the items we configured were colored correctly
+	grep "^<BOLD;RED>diff" output &&
+	grep "^<BOLD;BLUE>@@" output &&
+	grep "^<GREEN>-" output &&
+	grep "^<RED>\+" output
+'
+
 test_expect_success 'diffFilter filters diff' '
 	git reset --hard &&
 
@@ -884,4 +905,15 @@ test_expect_success 'show help from add--helper' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'interactive colors can be configured' '
+	git reset --hard &&
+
+	test_write_lines h |
+	force_color git \
+		-c color.interactive.help="bold yellow" \
+		add -i >actual.colored &&
+	test_decode_color <actual.colored >actual &&
+	test_i18ngrep "^<BOLD;YELLOW>update" actual
+'
+
 test_done
-- 
2.29.2.640.g9e24689a4c

Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config

[Johannes Schindelin]

Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

> On Tue, Nov 10, 2020 at 05:06:59PM +0100, Johannes Schindelin wrote:
>
> > A bigger puzzle for me was: why did we not catch that earlier? I vaguely
> > remember that we introduced _specifically_ code to test coloring, and to
> > make those test work on Windows (rather than skipping them all).
> >
> > *clicketyclick* ah, we only tests the menu of `git add -i`, and we do not
> > even override the colors...
>
> Yeah, the test coverage could definitely be improved.

I was actually already working on a comprehensive test case.

> > Will try to set aside some time to work on fixing this,
>
> How about this?
>
> -- >8 --
> Subject: add-interactive.c: use correct names to load color.diff.* config
>
> The builtin version of add-interactive mistakenly loads diff colors from
> color.interactive.* instead of color.diff.*. It also accidentally spells
> "frag" as "fraginfo".
>
> Let's fix that, and add some test coverage:
>
>   - check that color.diff.* is respected (this passes with the perl
>     version, but without this patch fails if GIT_TEST_ADD_I_USE_BUILTIN
>     is set)
>
>   - check that color.interactive.* is respected; this passes already
>     with both versions, but confirms we didn't break anything
>
> Note that neither test is exhaustive over the set of color config, but
> this is enough to sanity check the system (and we do check frag
> explicitly because of its typo).
>
> Note also that we don't respect the historical "diff.color.*". The perl
> version never did, and those have been deprecated since 2007.
>
> Signed-off-by: Jeff King <peff@peff.net>

If you don't mind, I'd like to integrate your work into mine and make you
a co-author. Objections?

Ciao,
Dscho

> ---
> I don't love that we have to repeat the set of color config here (and I
> guess maybe somebody would complain that we don't respect all of the
> weird "moved" variants?). But I agree that it's hard to re-use the
> existing diff code without stomping all over global variables. So I'd
> consider this the minimal fix bringing us in line with the perl version,
> but we may want to later revisit the diff color-config code as a whole.
>
>  add-interactive.c          | 22 ++++++++++++----------
>  t/t3701-add-interactive.sh | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 10 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 555c4abf32..208a058a68 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -12,10 +12,11 @@
>  #include "prompt.h"
>
>  static void init_color(struct repository *r, struct add_i_state *s,
> +		       const char *section,
>  		       const char *slot_name, char *dst,
>  		       const char *default_color)
>  {
> -	char *key = xstrfmt("color.interactive.%s", slot_name);
> +	char *key = xstrfmt("color.%s.%s", section, slot_name);
>  	const char *value;
>
>  	if (!s->use_color)
> @@ -40,18 +41,19 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
>  			git_config_colorbool("color.interactive", value);
>  	s->use_color = want_color(s->use_color);
>
> -	init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD);
> -	init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED);
> -	init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
> -	init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED);
> -	init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET);
> -	init_color(r, s, "fraginfo", s->fraginfo_color,
> +	init_color(r, s, "interactive", "header", s->header_color, GIT_COLOR_BOLD);
> +	init_color(r, s, "interactive", "help", s->help_color, GIT_COLOR_BOLD_RED);
> +	init_color(r, s, "interactive", "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE);
> +	init_color(r, s, "interactive", "error", s->error_color, GIT_COLOR_BOLD_RED);
> +
> +	init_color(r, s, "diff", "reset", s->reset_color, GIT_COLOR_RESET);
> +	init_color(r, s, "diff", "frag", s->fraginfo_color,
>  		   diff_get_color(s->use_color, DIFF_FRAGINFO));
> -	init_color(r, s, "context", s->context_color,
> +	init_color(r, s, "diff", "context", s->context_color,
>  		diff_get_color(s->use_color, DIFF_CONTEXT));
> -	init_color(r, s, "old", s->file_old_color,
> +	init_color(r, s, "diff", "old", s->file_old_color,
>  		diff_get_color(s->use_color, DIFF_FILE_OLD));
> -	init_color(r, s, "new", s->file_new_color,
> +	init_color(r, s, "diff", "new", s->file_new_color,
>  		diff_get_color(s->use_color, DIFF_FILE_NEW));
>
>  	FREE_AND_NULL(s->interactive_diff_filter);
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index ca04fac417..7c3107a04a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -602,6 +602,27 @@ test_expect_success 'colorized diffs respect diff.wsErrorHighlight' '
>  	grep "old<" output
>  '
>
> +test_expect_success 'colorized diffs respect color config' '
> +	git reset --hard &&
> +
> +	echo content >test &&
> +	printf y >y &&
> +	force_color git \
> +		-c color.diff.meta="bold red" \
> +		-c color.diff.frag="bold blue" \
> +		-c color.diff.old="green" \
> +		-c color.diff.new="red" \
> +		add -p >output.raw 2>&1 <y &&
> +	test_decode_color <output.raw >output &&
> +
> +	# do not check the full output, which would make the test brittle;
> +	# just make sure the items we configured were colored correctly
> +	grep "^<BOLD;RED>diff" output &&
> +	grep "^<BOLD;BLUE>@@" output &&
> +	grep "^<GREEN>-" output &&
> +	grep "^<RED>\+" output
> +'
> +
>  test_expect_success 'diffFilter filters diff' '
>  	git reset --hard &&
>
> @@ -884,4 +905,15 @@ test_expect_success 'show help from add--helper' '
>  	test_i18ncmp expect actual
>  '
>
> +test_expect_success 'interactive colors can be configured' '
> +	git reset --hard &&
> +
> +	test_write_lines h |
> +	force_color git \
> +		-c color.interactive.help="bold yellow" \
> +		add -i >actual.colored &&
> +	test_decode_color <actual.colored >actual &&
> +	test_i18ngrep "^<BOLD;YELLOW>update" actual
> +'
> +
>  test_done
> --
> 2.29.2.640.g9e24689a4c
>
>

Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config

[Jeff King]

On Tue, Nov 10, 2020 at 08:17:04PM +0100, Johannes Schindelin wrote:

> If you don't mind, I'd like to integrate your work into mine and make you
> a co-author. Objections?

Nope, no problem at all (I was worried we might be duplicating, but I
thought maybe you'd have quit for the day already. Silly timezones).

-Peff

Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config

[Johannes Schindelin]

Hi Peff,

On Tue, 10 Nov 2020, Jeff King wrote:

> On Tue, Nov 10, 2020 at 08:17:04PM +0100, Johannes Schindelin wrote:
>
> > If you don't mind, I'd like to integrate your work into mine and make you
> > a co-author. Objections?
>
> Nope, no problem at all (I was worried we might be duplicating, but I
> thought maybe you'd have quit for the day already. Silly timezones).

Right. Normally, you would be right, I would have called it quits for the
day. For this bug, I kind of set aside my movie time today to fix the bug.

As soon as https://github.com/gitgitgadget/git/pull/785 passes the PR
build, I will submit it.

Ciao,
Dscho