On Thu, Nov 22, 2007 at 04:56:24AM -0600, Dan Zwell wrote:
> - 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.").
> + 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.
> + 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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
