git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* ["BUG"] builtin add-interactive does not honor 'color.diff. frag'
@ 2020-11-06 16:31 Philippe Blain
  2020-11-06 17:03 ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Philippe Blain @ 2020-11-06 16:31 UTC (permalink / raw)
  To: Git mailing list; +Cc: Johannes Schindelin

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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'
  2020-11-06 16:31 ["BUG"] builtin add-interactive does not honor 'color.diff. frag' Philippe Blain
@ 2020-11-06 17:03 ` Jeff King
  2020-11-10 16:06   ` Johannes Schindelin
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2020-11-06 17:03 UTC (permalink / raw)
  To: Philippe Blain; +Cc: Git mailing list, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'
  2020-11-06 17:03 ` Jeff King
@ 2020-11-10 16:06   ` Johannes Schindelin
  2020-11-10 18:01     ` Jeff King
  2020-11-10 18:28     ` [PATCH] add-interactive.c: use correct names to load color.diff.* config Jeff King
  0 siblings, 2 replies; 8+ messages in thread
From: Johannes Schindelin @ 2020-11-10 16:06 UTC (permalink / raw)
  To: Jeff King; +Cc: Philippe Blain, Git mailing list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: ["BUG"] builtin add-interactive does not honor 'color.diff. frag'
  2020-11-10 16:06   ` Johannes Schindelin
@ 2020-11-10 18:01     ` Jeff King
  2020-11-10 18:28     ` [PATCH] add-interactive.c: use correct names to load color.diff.* config Jeff King
  1 sibling, 0 replies; 8+ messages in thread
From: Jeff King @ 2020-11-10 18:01 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Philippe Blain, Git mailing list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] add-interactive.c: use correct names to load color.diff.* config
  2020-11-10 16:06   ` Johannes Schindelin
  2020-11-10 18:01     ` Jeff King
@ 2020-11-10 18:28     ` Jeff King
  2020-11-10 19:17       ` Johannes Schindelin
  1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2020-11-10 18:28 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Philippe Blain, Git mailing list

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config
  2020-11-10 18:28     ` [PATCH] add-interactive.c: use correct names to load color.diff.* config Jeff King
@ 2020-11-10 19:17       ` Johannes Schindelin
  2020-11-10 19:48         ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2020-11-10 19:17 UTC (permalink / raw)
  To: Jeff King; +Cc: Philippe Blain, Git mailing list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config
  2020-11-10 19:17       ` Johannes Schindelin
@ 2020-11-10 19:48         ` Jeff King
  2020-11-10 22:30           ` Johannes Schindelin
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2020-11-10 19:48 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Philippe Blain, Git mailing list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] add-interactive.c: use correct names to load color.diff.* config
  2020-11-10 19:48         ` Jeff King
@ 2020-11-10 22:30           ` Johannes Schindelin
  0 siblings, 0 replies; 8+ messages in thread
From: Johannes Schindelin @ 2020-11-10 22:30 UTC (permalink / raw)
  To: Jeff King; +Cc: Philippe Blain, Git mailing list

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-11-10 22:31 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-06 16:31 ["BUG"] builtin add-interactive does not honor 'color.diff. frag' Philippe Blain
2020-11-06 17:03 ` Jeff King
2020-11-10 16:06   ` Johannes Schindelin
2020-11-10 18:01     ` Jeff King
2020-11-10 18:28     ` [PATCH] add-interactive.c: use correct names to load color.diff.* config Jeff King
2020-11-10 19:17       ` Johannes Schindelin
2020-11-10 19:48         ` Jeff King
2020-11-10 22:30           ` Johannes Schindelin

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).