| Message ID | 20201110182833.GB1362803@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add-interactive.c: use correct names to load color.diff.* config | expand |
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 > >
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
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
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