Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano <gitster@pobox.com>]

Dan Zwell <dzwell@zwell.net> writes:

> @@ -8,12 +9,18 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
>  	eval { require Term::ANSIColor; };
>  	if (!$@) {
>  		$use_color = 1;
> +		# Set interactive colors:
> +
> +		# Grab the 3 main colors in git color string format, with sane
> +		# (visible) defaults:
> +		my $repo = Git->repository();
> +		$prompt_color = Git::color_to_ansi_code(
> +			Git::config($repo, "color.interactive.prompt") || "bold blue");
> +		$header_color = Git::color_to_ansi_code(
> +			Git::config($repo, "color.interactive.header") || "bold");
> +		$help_color = Git::color_to_ansi_code(
> +			Git::config($repo, "color.interactive.help") || "red bold");
> +		$normal_color = Git::color_to_ansi_code("normal");

Makes me wonder if you are better off with two new helper
functions defined in Git.pm, as in:

	$prompt_color = $repo->config_color("interactive.prompt") || "bold blue")
	$normal_color = Git::color_to_ansi_code("normal");

> +sub color_to_ansi_code {
> +	my ($git_string) = @_;
> +	my @ansi_words;
> +	my %attrib_mappings = (
> +		"bold"    => "bold",
> +		"ul"      => "underline",
> +		"blink"   => "blink",
> +		# not supported:
> +		#"dim"     => "",
> +		"reverse" => "reverse"
> +	);

I do not like a hash variable name that says it is a "mapping".
It being a hash is enough indication that it is a mapping.

You are better off naming such a mapping as if it is a function
that takes one parameter (in this case, git name) and returns a
single value (perl name).  So I'd probably say:

	my %perl_attrib = ( ... );

(or "git_attrib_to_perl").  A use site of such a hash would look
like this:

	push @perl_words, $perl_attrib{'ul'};
        push @perl_names, $git_attrib_to_perl{'blink'};

Maybe it is just me, but aren't these easier to read?

> +	my ($fg_done, $word);
> +
> +	foreach $word (split /\s+/, $git_string) {
> +		if ($word =~ /normal/) {

	$word eq 'normal' ?

> +			$fg_done = "true";
> +		}
> +		elsif ($word =~ /black|red|green|yellow/ ||
> +			   $word =~ /blue|magenta|cyan|white/) {

	exists $color_name{$word}

with

	my %color_name = map { $_ => 1 } qw(black red ... white);

at the beginning?

> +			# is a color.
> +			if ($fg_done) {
> +				# this is the background
> +				push @ansi_words, "on_" . $word;
> +			}
> +			else {
> +				# this is foreground
> +				$fg_done = "true";
> +				push @ansi_words, $word;
> +			}
> +		}
> +		else {
> +			# this is an attribute, not a color.
> +			if ($attrib_mappings{$word}) {

	exists $git_attrib_to_perl{$word}

?