Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive
- From
Jeff King <peff@peff.net>
- Date
- Nov 22, 2007, 12:25 UTC
- Message-ID
- <20071122122540.GH12913@sigill.intra.peff.net>
- In-Reply-To
- <20071122045624.405e2b2b@paradox.zwell.net>
On Thu, Nov 22, 2007 at 04:56:24AM -0600, Dan Zwell wrote:
Show 6 quoted lines
> - else { # set up colors
> - # Grab the 3 main colors in git color string format, with sane
> - # (visible) defaults:
> - $prompt_color = Git::color_to_ansi_code(
> - scalar $repo->config_default('color.interactive.prompt',
> - 'bold blue'));These were just added in the last patch. I know sometimes it is worth showing the progression of work as the patches go, but in this case, I think it is simpler for the reviewers if the first patch which adds a chunk of code does it in the final way (even if you need to just say "I did it this way because there will be reasons later on.").
Show 5 quoted lines
> + sub get_color {
> + my ($key, $default) = @_;
> + return Git::color_to_ansi_code(
> + scalar $repo->config_default($key, $default));
> + }Ah, so you agree that this is a good route. I think this should probably be Git::config_color.
There is also a subtle issue, which is that it pulls the "$repo" variable from the outer lexical scope (as Git::config_color, it would take it as the first parameter).
> + $prompt_color = get_color('color.interactive.prompt', 'bold blue');
> + $header_color = get_color('color.interactive.header', 'bold');
> + $help_color = get_color('color.interactive.help', 'red bold');
> + $normal_color = Git::color_to_ansi_code('normal');Yeah, much nicer to read.
Show 9 quoted lines
> + if ($diff_use_color) {
> + $new_color = get_color('color.diff.new', 'green');
> + $old_color = get_color('color.diff.old', 'red');
> + $fraginfo_color = get_color('color.diff.frag', 'cyan');
> + $metainfo_color = get_color('color.diff.meta', 'bold');
> + $normal_color = Git::color_to_ansi_code('normal');
> + # Not implemented:
> + #$whitespace_color = get_color('color.diff.whitespace',
> + #'normal red');Unfortunately, there is a historical wart that probably still needs supporting, which is that the original names were diff.color.*. Or have we officially removed support for that yet?
-Peff