Dan Zwell <dzwell@zwell.net> writes:

quoted text
> One thought is that is seems a bit sloppy to call "require Term::ANSIColor"
> within color_to_ansi_code(), but I can't really see a better way. After all,
> that is where the methods from that library are really needed. And I don't
> know why Git.pm should need to know whether color will end up being used.

How big is Term::ANSIColor, and how universally available is it?
Implementing the ANSI "ESC [ %d m" arithmetic color.c in Perl
ourselves does not feel too much effort, compared to the
potential hassle of dealing with extra dependencies and
potential drift between scripts and C implementation.

We may later want to update the C side to take colors from
terminfo, but that is a separate topic ;-)

Since your 2/2 updates on your 1/2, the diff is difficult to
comment on, so I'll comment on the combined effects.

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..2bce5a1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,6 +1,44 @@
 #!/usr/bin/perl -w
 
 use strict;
+use Git;
+
+my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
+my $color_config = qx(git config --get color.interactive);
+if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
+	$use_color = "true";
+	# Grab the 3 main colors in git color string format, with sane
+	# (visible) defaults:
+	my $repo = Git->repository();
+	my $git_prompt_color =
+		Git::config($repo, "color.interactive.prompt")||"bold blue";
+	my $git_header_color =
+		Git::config($repo, "color.interactive.header")||"bold";
+	my $git_help_color =
+		Git::config($repo, "color.interactive.help")||"red bold";
+
+	$prompt_color = Git::color_to_ansi_code($git_prompt_color);
+	$header_color = Git::color_to_ansi_code($git_header_color);
+	$help_color   = Git::color_to_ansi_code($git_help_color);
+	$normal_color = Git::color_to_ansi_code("normal");
+}

If we are to still use Term::ANSIColor, then we might want to
protect ourselves from a broken installation:

        if ($color_config =~ /true|always/ ||
            -t STDOUT && $color_config =~ /auto/) {
                eval { require Term::ANSIColor; };
                if (!$@) {
                        $use_color = 1;
                        ... set up the colors ...
                }
                else {
                        $use_color = 0;
                }
        }

Then you can remove the require from Git::color_to_ansi_code().
Your current calling convention is to require the calling site
to be sure the module is availble; the suggested change merely
makes it responsible to also make sure the module is loaded.

Hmm?

By the way, coloring the diff text itself may be just the matter
of doing something like this (except that you now need to snarf
OLD, NEW, METAINFO and FRAGINFO colors for diff configuration as
well.

In addition to a small matter of testing, a more practical issue
would be to add PAGER support there, I think.

---

 git-add--interactive.perl |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 2bce5a1..1063a34 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -388,6 +388,27 @@ sub parse_diff {
 	return @hunk;
 }
 
+sub print_diff_hunk {
+	my ($text) = @_;
+	for (@$text) {
+		if (!$use_color) {
+			print;
+			next;
+		}
+		if (/^\+/) {
+			print_colored $new_color, $_;
+		} elsif (/^\-/) {
+			print_colored $old_color, $_;
+		} elsif (/^\@/) {
+			print_colored $fraginfo_color, $_;
+		} elsif (/^ /) {
+			print_colored $normal_color, $_;
+		} else {
+			print_colored $metainfo_color, $_;
+		}
+	}
+}
+
 sub hunk_splittable {
 	my ($text) = @_;
 
@@ -610,9 +631,7 @@ sub patch_update_cmd {
 	my ($ix, $num);
 	my $path = $it->{VALUE};
 	my ($head, @hunk) = parse_diff($path);
-	for (@{$head->{TEXT}}) {
-		print;
-	}
+	print_diff_hunk($head->{TEXT});
 	$num = scalar @hunk;
 	$ix = 0;
 
@@ -654,9 +673,7 @@ sub patch_update_cmd {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
-		for (@{$hunk[$ix]{TEXT}}) {
-			print;
-		}
+		print_diff_hunk($hunk[$ix]{TEXT});
 		print_colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
 		my $line = <STDIN>;
 		if ($line) {
@@ -794,8 +811,7 @@ sub diff_cmd {
 				     HEADER => $status_head, },
 				   @mods);
 	return if (!@them);
-	system(qw(git diff-index -p --cached HEAD --),
-	       map { $_->{VALUE} } @them);
+	system(qw(git diff -p --cached HEAD --), map { $_->{VALUE} } @them);
 }
 
 sub quit_cmd {

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html