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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
