Re: [PATCH 0/3] Adding colors to git-add--interactive

[Jeff King <peff@peff.net>]

On Sat, Nov 10, 2007 at 06:01:09PM -0600, Dan Zwell wrote:

> A bit of a recap--this feature was requested by a user a few weeks

Thanks for the recap; there have been enough iterations of this series
that at least I forgot what was going on. The patches look reasonable,
but I have a few comments (hopefully you have enough "umph" for one more
iteration). I'll just inline them here.

[patch 1/3]:
> +my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
> +my $color_config = qx(git config --get color.interactive);

Why call git config here manually, but Git::config later (I think the
answer is "because we don't call Git::config until a later patch", but it
is probably best to remain consistent).

> +sub colored {
> +	my $color = shift;
> +	my $string = join("", @_);
> +
> +	if ($use_color) {
> +		# Put a color code at the beginning of each line, a reset at the end
> +		# color after newlines that are not at the end of the string
> +		$string =~ s/(\n+)(.)/$1$color$2/g;
> +		# reset before newlines
> +		$string =~ s/(\n+)/$normal_color$1/g;
> +		# codes at beginning and end (if necessary):
> +		$string =~ s/^/$color/;
> +		$string =~ s/$/$normal_color/ unless $string =~ /\n$/;
> +	}
> +	return $string;
> +}

This also seems like a candidate for lib-ification in Git.pm, alongside
color_to_ansi_code.

> -			print "$opts->{HEADER}\n";
> +			print colored $header_color, "$opts->{HEADER}\n";

I don't know if we have a style policy on calling

  user_defined_function $foo, $bar;

rather than

  user_defined_function($foo, $bar);

In fact, I don't know that we have much perl style policy at all. But I
tend to shy away from the former because then the syntax requires that
"colored" is always defined before the calling spot.

[patch 2/3]:
> +		# 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");

It is much more common (and proper OO, in the face of inheritance) to
use

   $repo->config("color.interactive.prompt")

> +=item color_to_ansi_code ( COLOR )
> +
> +Converts a git-style color string, like "underline blue white" to
> +an ANSI color code. The code is generated by Term::ANSIColor,
> +after the string is parsed into the format that is accepted by
> +that module. Used as follows:
> +
> +	print color_to_ansi_code("underline blue white");
> +	print "some text";
> +	print color_to_ansi_code("normal");

Yay, documentation!  It would also be nice to have a test script that
runs this through a few more complex git color specs.

> +	my %attrib_mappings = (
> +		"bold"    => "bold",
> +		"ul"      => "underline",
> +		"blink"   => "blink",
> +		# not supported:
> +		#"dim"     => "",

Why not? I don't especially care about "dim" support, but if there is a
good reason, then you should note it.

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

Why a regex instead of 'eq'? Also, should this be case insensitive?

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

It looks like you are doing two regexes here just to meet whitespace
guidelines. Look into the '/x' modifier to make your regex prettier (but
again, consider 'eq').

> +	return Term::ANSIColor::color(join(" ", @ansi_words)||"reset");

Style: whitespace around ||

[patch 3/3]:
> +			# Not implemented:
> +			#$whitespace_color = Git::color_to_ansi_code(
> +				#Git::config($repo, "color.diff.whitespace") || "normal red");

Personally I would have just excluded the parsing, since it isn't
implemented, but I don't think it matters.

> +sub colored_diff_hunk {

Perhaps this should also go in Git.pm? Though right now I don't know
which other perl scripts would actually want to colorize a diff, so I
don't think it matters.

> -	system(qw(git diff-index -p --cached HEAD --),
> -	       map { $_->{VALUE} } @them);
> +	system(qw(git diff -p --cached HEAD --), map { $_->{VALUE} } @them);

Now this was a surprise after reading the commit message.

-Peff