Re: [PATCH] Color support added to git-add--interactive.

From: Jean-Luc Herren <jlh@gmx.ch>
Date: Oct 13, 2007, 16:38 UTC
Message-ID: <4710F47D.2070306@gmx.ch>
In-Reply-To: <19271E58-5C4F-41AF-8F9D-F114F36A34AC@wincent.com>

Hi!

I really like the idea of colorizing git add -i, especially the prompt. Here are my two cents.

Wincent Colaiuta wrote:

> +sub print_ansi_color($$;$) {
> +    my $color = shift;
> +    my $string = shift;
> +    my $trailer = shift;

None of the other subs in this file have a prototype, so for consistency I'd suggest to not add it on this function either. However maybe a patch that adds it to all subs would be welcome. (I wouldn't see the necessity though.)

And the common way of getting the arguments is reading @_ (see all other subs in the file). So maybe instead write:

[...]
sub print_ansi_color {
	my ($color, $string, $trailer) = @_;
[...]

> +    if ($use_color) {
> +        printf '%s%s%s', Term::ANSIColor::color($color), $string,
> +            Term::ANSIColor::color('clear');
> +    } else {

Why use printf when you could directly use print here? It's only used for concatenating.

> +    if ($trailer) {
> +        print $trailer;
> +    }

This will fail to print $trailer when $trailer happens to be a string that evaluates to false in bool context, like '0'. Write this as:

	if (defined $trailer) {
	    print $trailer;
	}

IMHO, parsing the output of 'git diff-files --color' is a very bad idea and it makes all regexes uglier and more difficult to read. You're much better off recolorizing it yourself, which makes it a more localized change. Especially, I don't think that you have any guarantee that escape sequences won't ever contain the characters '+', '-' or ' ' (space), which would break your code on lines like this:

> +        if ($line =~ /^[^-+ ]*\+/) {

Finally -- and this might be just my eyes -- blue is a very nice color, but it looks a bit too dark on black background. Maybe choose a default color that looks reasonable on black *and* white background.

Cheers, jlh