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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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