Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Jeff King <peff@peff.net>]

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