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