From: Andreas Ericsson <ae@op5.se>
Date: Sat, 13 Oct 2007 18:31:18 GMT
Subject: Re: [PATCH] Color support added to git-add--interactive.
Message-ID: <47110EF6.7090108@op5.se>
In-Reply-To: <4710F47D.2070306@gmx.ch>

Jean-Luc Herren wrote:
> 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.
> 

Red for removed and green for added seems to be the standard, although
I know it makes life terribly difficult for red-green colorblind people,
who usually prefer yellow/lightblue for black bg, or beige/blue for
white background.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231