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