From: Wincent Colaiuta <win@wincent.com>
Date: Sat, 13 Oct 2007 17:14:19 GMT
Subject: Re: [PATCH] Color support added to git-add--interactive.
Message-ID: <6E1CBEF5-C2EA-447A-9FED-1423A17C2D19@wincent.com>
In-Reply-To: <4710F47D.2070306@gmx.ch>

El 13/10/2007, a las 18:38, Jean-Luc Herren escribió:

> 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.)

Yes, I saw that the other functions didn't use prototypes and I agree  
that consistency would be a good thing. I liked the idea of it in  
this case because it makes explicit the fact that the function takes  
two params plus a third, optional one. So definitely not necessary,  
but a preference of mine. In any case, a change to all the other  
functions is a question for a separate patch.

> 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) = @_;
> [...]

Yes, I actually did write it that way first but then my doubts about  
Perl made me write it the longer way; but if they are equivalent then  
I prefer the shorter way.

>> +    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.

True. I had may brain in the C-world.

>> +    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;
> 	}

Again, I actually wrote it that way the first time, and then changed  
it, this time because I thought they were the same. Like I said, not  
a perl hacker.

> 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.

You're probably right, although it is also duplicating the work  
that's already done elsewhere. In general I favor making the simplest  
change that would work, and tweaking a few of the regexes did look  
simpler than re-implementing the colorization logic.

But the approach you suggest might be more robust, perhaps, seeing as  
there's not much to the diff output. As far as I can tell there are  
really only five or six different things to look for, and they'd be  
fairly easy to catch:

- lines beginning with "@@ " (hunk headers)

- lines beginning with "+" (insertions)

- lines beginning with "-" (deletions)

- lines beginning with " " (context lines, no color)

- lines beginning with "\" (things like "\ No newline at end of  
file", again, no color)

- everything else; ie. the diff header stuff (eg "diff --git a/foo b/ 
foo")

The only special cases seem to be the "+++" and "---" lines in the  
header, which look like insertions and deletions when they're not.

Trickier would be the highlighting of dubious whitespace, and that's  
when it starts to sound like re-inventing the wheel and duplicating  
the logic for the detection that's defined elsewhere (possibly in  
diff-lib.c? haven't found the exact spot yet).

> Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space)

Yes, that was one of the things I didn't like about the sloppy  
regexes. I couldn't really make them any stricter though because I  
wasn't confident about the range of possible characters that might be  
included in the escape sequences.

> 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.

Yeah, well I didn't choose the colours and I didn't really want to  
get into it. Before being considered for inclusion a patch like this  
would need to tap in to the existing config settings for color.diff  
and color.diff.<slot> anyway...

Wincent