On Sat, Nov 10, 2007 at 06:01:09PM -0600, Dan Zwell wrote:
> A bit of a recap--this feature was requested by a user a few weeks
Thanks for the recap; there have been enough iterations of this series
that at least I forgot what was going on. The patches look reasonable,
but I have a few comments (hopefully you have enough "umph" for one more
iteration). I'll just inline them here.
[patch 1/3]:
> +my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
> +my $color_config = qx(git config --get color.interactive);
Why call git config here manually, but Git::config later (I think the
answer is "because we don't call Git::config until a later patch", but it
is probably best to remain consistent).
> +sub colored {
> + my $color = shift;
> + my $string = join("", @_);
> +
> + if ($use_color) {
> + # Put a color code at the beginning of each line, a reset at the end
> + # color after newlines that are not at the end of the string
> + $string =~ s/(\n+)(.)/$1$color$2/g;
> + # reset before newlines
> + $string =~ s/(\n+)/$normal_color$1/g;
> + # codes at beginning and end (if necessary):
> + $string =~ s/^/$color/;
> + $string =~ s/$/$normal_color/ unless $string =~ /\n$/;
> + }
> + return $string;
> +}
This also seems like a candidate for lib-ification in Git.pm, alongside
color_to_ansi_code.
> - print "$opts->{HEADER}\n";
> + print colored $header_color, "$opts->{HEADER}\n";
I don't know if we have a style policy on calling
user_defined_function $foo, $bar;
rather than
user_defined_function($foo, $bar);
In fact, I don't know that we have much perl style policy at all. But I
tend to shy away from the former because then the syntax requires that
"colored" is always defined before the calling spot.
[patch 2/3]:
> + # Grab the 3 main colors in git color string format, with sane
> + # (visible) defaults:
> + my $repo = Git->repository();
> + $prompt_color = Git::color_to_ansi_code(
> + Git::config($repo, "color.interactive.prompt") || "bold blue");
> + $header_color = Git::color_to_ansi_code(
> + Git::config($repo, "color.interactive.header") || "bold");
> + $help_color = Git::color_to_ansi_code(
> + Git::config($repo, "color.interactive.help") || "red bold");
> + $normal_color = Git::color_to_ansi_code("normal");
It is much more common (and proper OO, in the face of inheritance) to
use
$repo->config("color.interactive.prompt")
> +=item color_to_ansi_code ( COLOR )
> +
> +Converts a git-style color string, like "underline blue white" to
> +an ANSI color code. The code is generated by Term::ANSIColor,
> +after the string is parsed into the format that is accepted by
> +that module. Used as follows:
> +
> + print color_to_ansi_code("underline blue white");
> + print "some text";
> + print color_to_ansi_code("normal");
Yay, documentation! It would also be nice to have a test script that
runs this through a few more complex git color specs.
> + my %attrib_mappings = (
> + "bold" => "bold",
> + "ul" => "underline",
> + "blink" => "blink",
> + # not supported:
> + #"dim" => "",
Why not? I don't especially care about "dim" support, but if there is a
good reason, then you should note it.
> + foreach $word (split /\s+/, $git_string) {
> + if ($word =~ /normal/) {
> + $fg_done = "true";
> + }
Why a regex instead of 'eq'? Also, should this be case insensitive?
> + elsif ($word =~ /black|red|green|yellow/ ||
> + $word =~ /blue|magenta|cyan|white/) {
It looks like you are doing two regexes here just to meet whitespace
guidelines. Look into the '/x' modifier to make your regex prettier (but
again, consider 'eq').
> + return Term::ANSIColor::color(join(" ", @ansi_words)||"reset");
Style: whitespace around ||
[patch 3/3]:
> + # Not implemented:
> + #$whitespace_color = Git::color_to_ansi_code(
> + #Git::config($repo, "color.diff.whitespace") || "normal red");
Personally I would have just excluded the parsing, since it isn't
implemented, but I don't think it matters.
> +sub colored_diff_hunk {
Perhaps this should also go in Git.pm? Though right now I don't know
which other perl scripts would actually want to colorize a diff, so I
don't think it matters.
> - system(qw(git diff-index -p --cached HEAD --),
> - map { $_->{VALUE} } @them);
> + system(qw(git diff -p --cached HEAD --), map { $_->{VALUE} } @them);
Now this was a surprise after reading the commit message.
-Peff
-
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
