* ["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 related [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 related [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 public 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).