Color support added to git-add--interactive.

Recently there was some talk of color for git-add--interactive, but the person who said he already had a patch didn't produce it.

-Reads configuration from git-config (using a new key,
color.add-interactive), respects "auto" if called from a script
-Uses the library Term::ANSIColor, which is included with modern
versions of perl.

There is one problem--a block is commented out, because adding the "--color" option to git-diff-files somehow breaks git-add--interactive, and I would love some help from someone who knows a little more about the rest of the script than I do. Also, this is the first perl I have written, and criticism is welcome. A gzipped patch is attached, in case thunderbird mangles the tabs. Feel free to replace the colors that I chose with something that better conforms to the "git style", if there is such a thing.

index be68814..f55d787 100755
@@ -2,6 +2,18 @@
 
 use strict;
 
+my $use_color;
+my $color_config = qx(git config --get color.add-interactive);
+if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {
+	$use_color = "true";
+	require Term::ANSIColor;
+}
+sub print_ansi_color {
+	if ($use_color) {
+		print Term::ANSIColor::color($_[0]);
+	}
+}
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;

@@ -175,7 +187,9 @@ sub list_and_choose {
 			if (!$opts->{LIST_FLAT}) {
 				print "     ";
 			}
+			print_ansi_color "bold";
 			print "$opts->{HEADER}\n";
+			print_ansi_color "clear";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';

@@ -205,7 +219,9 @@ sub list_and_choose {
 
 		return if ($opts->{LIST_ONLY});
 
+		print_ansi_color "bold blue";
 		print $opts->{PROMPT};
+		print_ansi_color "reset";
 		if ($opts->{SINGLETON}) {
 			print "> ";
 		}

@@ -338,6 +354,16 @@ sub add_untracked_cmd {
 
 sub parse_diff {
 	my ($path) = @_;
+	# FIXME: the following breaks git, and I'm not sure why. When
+	# the following is uncommented, git no longer asks whether we
+	# want to add given hunks.
+	#my @diff;
+	#if ($use_color) {
+	#    #@diff = run_cmd_pipe(qw(git diff-files --color -p --), $path);
+	#}
+	#else {
+	#    #@diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	#}
 	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
 	my (@hunk) = { TEXT => [] };
 

@@ -544,6 +570,7 @@ sub coalesce_overlapping_hunks {
 }
 
 sub help_patch_cmd {
+	print_ansi_color "blue";
 	print <<\EOF ;
 y - stage this hunk
 n - do not stage this hunk

@@ -555,6 +582,7 @@ k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
 s - split the current hunk into smaller hunks
 EOF
+	print_ansi_color "clear";
 }
 
 sub patch_update_cmd {

@@ -619,7 +647,9 @@ sub patch_update_cmd {
 		for (@{$hunk[$ix]{TEXT}}) {
 			print;
 		}
+		print_ansi_color "bold";
 		print "Stage this hunk [y/n/a/d$other/?]? ";
+		print_ansi_color "reset";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {

1.5.3.4.207.gc0ee

Dan Zwell

Meh, I really need to start posting the stuff I've hacked into git. First the git-stash changes, now this. Sigh. ^_^

I have a variant of git-add--interactive that properly adds coloration to diffs, taking the config file values already set for the color.diff key and colorizing the unadorned diffs internally (rather than expecting the output of git-diff/git-diff-files to be colorized).

Give me a couple of hours (still setting up my Macbook after repaving it and installing Ubuntu) and I'll post what I've got for others to tear apart and point out where I screwed up. ;)

... and now Evolution is screwing up my From: address (it should be "[email protected]"; probably since I'm routing everything through Google Apps). Ah well, one more thing to fix first....

Based on a couple of the suggestions you've received I made a couple of changes to your patch and given it a quick try-out. I'm no perl hacker so there may be better ways.

- as per Jeff's suggestion, changed your print_ansi_color method, modelling it after the print_color_ln and color_vprintf functions defined in color.c; accepts a color, a string, and an optional trailer (where if there is a newline you pass it as the trailer)

- as Johannes pointed out, "clear" and "reset" are not used consistently even though the Term::ANSIColor documentation says that they're the same, so settled on "clear"; although in any case, the changes to the print_ansi_color function mean that it is now the only site where clearing takes place

- changed the regex as suggested by Johannes, and a couple of others that are used when splitting hunks

- used more explicit notation for regex as proposed by Frank

Took it for a basic spin here and seems to work. Didn't even think about implementing user-settable colors.

Cheers,
Wincent

index be68814..ae3d11e 100755
@@ -2,6 +2,28 @@
 
  use strict;
 
+my $use_color;
+my $color_config = qx(git config --get color.add-interactive);
+if ($color_config =~ /true/ || -t STDOUT && $color_config =~ /auto/) {
+	$use_color = "true";
+	require Term::ANSIColor;
+}
+
+sub print_ansi_color($$;$) {
+	my $color = shift;
+	my $string = shift;
+	my $trailer = shift;
+	if ($use_color) {
+		printf '%s%s%s', Term::ANSIColor::color($color), $string,
+		    Term::ANSIColor::color('clear');
+	} else {
+		print $string;
+	}
+	if ($trailer) {
+		print $trailer;
+	}
+}
+
  sub run_cmd_pipe {
  	if ($^O eq 'MSWin32') {
  		my @invalid = grep {m/[":*]/} @_;

@@ -175,7 +197,7 @@ sub list_and_choose {
  			if (!$opts->{LIST_FLAT}) {
  				print "     ";
  			}
-			print "$opts->{HEADER}\n";
+			print_ansi_color "bold", "$opts->{HEADER}", "\n";
  		}
  		for ($i = 0; $i < @stuff; $i++) {
  			my $chosen = $chosen[$i] ? '*' : ' ';

@@ -205,7 +227,7 @@ sub list_and_choose {
 
  		return if ($opts->{LIST_ONLY});
 
-		print $opts->{PROMPT};
+		print_ansi_color "bold blue", $opts->{PROMPT};
  		if ($opts->{SINGLETON}) {
  			print "> ";
  		}

@@ -338,11 +360,17 @@ sub add_untracked_cmd {
 
  sub parse_diff {
  	my ($path) = @_;
-	my @diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	my @diff;
+	if ($use_color) {
+		@diff = run_cmd_pipe(qw(git diff-files --color -p --), $path);
+	}
+	else {
+		@diff = run_cmd_pipe(qw(git diff-files -p --), $path);
+	}
  	my (@hunk) = { TEXT => [] };
 
  	for (@diff) {
-		if (/^@@ /) {
+		if (/^[^-+ ]*@@ /) {
  			push @hunk, { TEXT => [] };
  		}
  		push @{$hunk[-1]{TEXT}}, $_;

@@ -360,7 +388,7 @@ sub hunk_splittable {
  sub parse_hunk_header {
  	my ($line) = @_;
  	my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
-	    $line =~ /^@@ -(\d+)(?:,(\d+)) \+(\d+)(?:,(\d+)) @@/;
+	    $line =~ /^[^-+ ]*@@ -(\d+)(?:,(\d+)) \+(\d+)(?:,(\d+)) @@/;
  	return ($o_ofs, $o_cnt, $n_ofs, $n_cnt);
  }
 

@@ -426,7 +454,7 @@ sub split_hunk {
  			}
  			push @{$this->{TEXT}}, $line;
  			$this->{ADDDEL}++;
-			if ($line =~ /^-/) {
+			if ($line =~ /^[^-+ ]*-/) {
  				$this->{OCNT}++;
  			}
  			else {

@@ -483,7 +511,7 @@ sub merge_hunk {
  	$o_cnt = $n_cnt = 0;
  	for ($i = 1; $i < @{$prev->{TEXT}}; $i++) {
  		my $line = $prev->{TEXT}[$i];
-		if ($line =~ /^\+/) {
+		if ($line =~ /^[^-+ ]*\+/) {
  			$n_cnt++;
  			push @line, $line;
  			next;

@@ -501,7 +529,7 @@ sub merge_hunk {
 
  	for ($i = 1; $i < @{$this->{TEXT}}; $i++) {
  		my $line = $this->{TEXT}[$i];
-		if ($line =~ /^\+/) {
+		if ($line =~ /^[^-+ ]*\+/) {
  			$n_cnt++;
  			push @line, $line;
  			next;

@@ -544,7 +572,7 @@ sub coalesce_overlapping_hunks {
  }
 
  sub help_patch_cmd {
-	print <<\EOF ;
+	my $help = <<\EOF ;
  y - stage this hunk
  n - do not stage this hunk
  a - stage this and all the remaining hunks

@@ -555,6 +583,7 @@ k - leave this hunk undecided, see previous  

undecided hunk
K - leave this hunk undecided, see previous hunk s - split the current hunk into smaller hunks EOF
+ print_ansi_color "blue", $_, "\n" foreach (split /[\r\n]/, $help);
}

sub patch_update_cmd {
@@ -619,7 +648,7 @@ sub patch_update_cmd {
for (@{$hunk[$ix]{TEXT}}) {
print;
}
- print "Stage this hunk [y/n/a/d$other/?]? ";
+ print_ansi_color "bold", "Stage this hunk [y/n/a/d$other/?]? ";
my $line = <STDIN>;
if ($line) {
if ($line =~ /^y/i) {

Please use "reset", as that is the term used by the C color code.

I believe there are other places where the diff output is parsed, and the colors will mess that up, too (e.g., split_hunk). All of those regexes need to be changed, too. I am a bit concerned that we are putting intimate knowledge of the colorization scheme here. As much as it pains me to have two diff colorizers, I wonder if that would be a better solution than having a diff colorizer, and a colorized diff parser.

-Peff

Oops, I see you actually dealt with those already (I just responded to your cover letter first). Though I am still concerned about the robustness of the re-parsing scheme.

-Peff

The importance of the diff coloring pales in comparison to the prompt coloring. Diff coloring is useful, but prompt coloring is a basic usability concern (if people can't easily tell where a hunk begins, the tool becomes annoying). Perhaps we could split this into two patches, merging the first after a few small changes can be taken care of, while the second may need more discussion and testing. The coloring of the prompts is relatively low risk. It just needs to be modified to take color settings from .git/config. I was thinking that this might be the example that I would take settings from:

[color]
add-interactive = auto
[color "add-interactive"]
prompt = bold blue
header = bold
help = blue

For the sake of a unified interface, the "Stage this hunk?" prompt should be colored the same as the other prompts. I will give a bit of thought to the default colors, though it's important to avoid red and green (as those will look like diff output when the second patch is applied).

Also needed is some command line parsing so that "--color" can be specified on the command line (very small change), and all of this should be added to the documentation.

Obviously, the suggestions/fixes from other parts of this thread must be taken into account, as well. I can probably do all this tomorrow (and send low-risk/high-risk patches), unless someone takes it before me.

Dan

Yes, I think it is worth splitting into two patches, here. There seems to be orthogonal discussion on (colorizing and configuration of prompts versus how to handle colorized diffs).

-Peff

Adds color to the prompts and output of git-add--interactive.

-Reads config color.interactive, respects "auto", "true", "always", and anything else.
-Uses the library Term::ANSIColor, which is included with modern
versions of perl. This is optional, and should not need to be present if color.interactive is not on.
-Reads color.interactive.<slot>, where slot is "header", "prompt",
or "help", colorizing output accordingly.

Documentation/config.txt is updated to reflect the new keys. I cannot test this or see how it looks in manpages, however, as I cannot install the documentation build tools.

Unfortunately, I think the default colors are ugly, but all colors that are readable on both black and white backgrounds are probably ugly.

This patch does not colorize the diffs, because that is a larger job, and very distinct from this (simple) task.

Dan

gzipped patch is also attached, just in case.

index 971fd9f..17e29e4 100644
@@ -381,6 +381,27 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in add--interactive.
+	When false (or `never`), never.  When set to `auto`, use
+	colors only when the output is to the terminal. Defaults to
+        false.
+
+color.interactive.<slot>::
+        Use customized color for add--interactive output. `<slot>`
+        may be `prompt`, `header`, or `help`, for three distinct types
+        of common output from interactive programs. The values may be a
+        space-delimited combination of up to three of the following:
++
+(optional attribute, optional foreground color, and optional

background) ++
+dark, bold, underline, underscore, blink, reverse, concealed, +black, red, green, yellow, blue, magenta, cyan, white, on_black, +on_red, on_green, on_yellow, on_blue, on_magenta, on_cyan, on_white ++
+Note: these are not the same colors/attributes that the +rest of git supports, but are specific to git-add--interactive. +
color.pager::
A boolean to enable/disable colored output when the pager is in use (default is true).

index be68814..125655b 100755
@@ -2,6 +2,33 @@
 
 use strict;
 
+my ($use_color, $prompt_color, $header_color, $help_color);
+my $color_config = qx(git config --get color.interactive);
+if ($color_config=~/true|always/ || -t STDOUT &&

$color_config=~/auto/) {
+ $use_color = "true";
+ chomp( $prompt_color = qx(git config --get
color.interactive.prompt) );
+ chomp( $header_color = qx(git config --get
color.interactive.header) );
+ chomp( $help_color = qx(git config --get
color.interactive.help) );

+	$prompt_color ||= "red bold";
+	$header_color ||= "bold";

+ $help_color ||= "blue bold";
+
+ require Term::ANSIColor;
+}
+
+sub print_colored {
+ my $color = shift;
+ my @strings = @_;
+
+ if ($use_color) {
+ print Term::ANSIColor::color($color);
+ print(@strings);
+ print Term::ANSIColor::color("reset");
+ } else {
+ print @strings;
+ }
+}
+
sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
my @invalid = grep {m/[":*]/} @_;
@@ -175,7 +202,7 @@ sub list_and_choose {
if (!$opts->{LIST_FLAT}) {
print " ";
}
- print "$opts->{HEADER}\n";
+ print_colored $header_color,
"$opts->{HEADER}\n"; }
for ($i = 0; $i < @stuff; $i++) {
my $chosen = $chosen[$i] ? '*' : ' ';
@@ -205,7 +232,7 @@ sub list_and_choose {

return if ($opts->{LIST_ONLY});

- print $opts->{PROMPT};
+ print_colored $prompt_color, $opts->{PROMPT};
if ($opts->{SINGLETON}) {
print "> ";
}
@@ -544,7 +571,7 @@ sub coalesce_overlapping_hunks {
}

sub help_patch_cmd {
- print <<\EOF ;
+ print_colored $help_color, <<\EOF ;
y - stage this hunk
n - do not stage this hunk
a - stage this and all the remaining hunks
@@ -619,7 +646,7 @@ sub patch_update_cmd {
for (@{$hunk[$ix]{TEXT}}) {
print;
}
- print "Stage this hunk [y/n/a/d$other/?]? ";
+ print_colored $prompt_color, "Stage this hunk
[y/n/a/d$other/?]? "; my $line = <STDIN>;
if ($line) {
if ($line =~ /^y/i) {
@@ -673,7 +700,7 @@ sub patch_update_cmd {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split =
split_hunk($hunk[$ix]{TEXT}); if (1 < @split) {
- print "Split into ",
+ print_colored "$header_color",
"Split into ", scalar(@split), " hunks.\n";
}
splice(@hunk, $ix, 1,
@@ -769,7 +796,7 @@ sub quit_cmd {
}

sub help_cmd {
- print <<\EOF ;
+ print_colored $help_color, <<\EOF ;

 status        - show paths with changes
 update        - add working tree state to the staged set of changes
 revert        - revert staged set of changes back to the HEAD version

--
1.5.3.4.207.gc0ee

I'm probbaly going to publish this in pu tonight but I have some comments that I think need to be addressed before this graduates any further.

First off, no Signed-off-by? This is big enough that I refuse to put it in the main tree without one. Second it would really have helped if the email was formatted with git format-patch. Copying the message headers and body over for the commit message was less than fun. I have better things to do with my time.

Shawn.

Sorry, that I didn't read the document on submitting patches before this. I will make the other changes you mention and re-send this in the proper formatting.

Makes sense. I am adding a bit of code to parse git color strings into perl color strings (so the user can use the same color names as with the rest of git). I know this is a small change, but I'm learning perl as I go, and I have exams this week, so it will take at least a day or two. I will fix the color issue, and send a properly formatted and signed-off patch. (Yes, I do agree to the Developer's Certificate of Origin wrt to this patch.)

Thanks for your patience,
Dan

I really should have pointed you to Documentation/SubmittingPatches when I responded to your email in the first place. Sorry I didn't do that. Looks like you already found it though so good.

Thanks for working on this. I played around with your patch tonight and although I use git-gui more often than git add -i for hunk manipulation I really preferred your colorized version of git-add -i over the non-colorized one. So I'm looking forward to seeing the final result of this and getting it into the main tree.

Of course there is also no rush to getting your change in. We don't have any sort of release deadlines. So don't stress out about it too much. :)

Shawn.

Added function "print_colored" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print_colored".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   37 +++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

index 971fd9f..c795a35 100644
@@ -381,6 +381,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index be68814..c66ed4d 100755
@@ -2,6 +2,31 @@
 
 use strict;
 
+my ($use_color, $prompt_color, $header_color, $help_color);
+my $color_config = qx(git config --get color.interactive);
+if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
+	$use_color = "true";
+        # Sane (visible) defaults:
+        $prompt_color = "blue bold";
+        $header_color = "bold";
+        $help_color = "red bold";
+
+	require Term::ANSIColor;
+}
+
+sub print_colored {
+	my $color = shift;
+	my @strings = @_;
+
+	if ($use_color) {
+		print Term::ANSIColor::color($color);
+		print(@strings);
+		print Term::ANSIColor::color("reset");
+	} else {
+		print @strings;
+	}
+}
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;

@@ -175,7 +200,7 @@ sub list_and_choose {
 			if (!$opts->{LIST_FLAT}) {
 				print "     ";
 			}
-			print "$opts->{HEADER}\n";
+			print_colored $header_color, "$opts->{HEADER}\n";
 		}
 		for ($i = 0; $i < @stuff; $i++) {
 			my $chosen = $chosen[$i] ? '*' : ' ';

@@ -205,7 +230,7 @@ sub list_and_choose {
 
 		return if ($opts->{LIST_ONLY});
 
-		print $opts->{PROMPT};
+		print_colored $prompt_color, $opts->{PROMPT};
 		if ($opts->{SINGLETON}) {
 			print "> ";
 		}

@@ -544,7 +569,7 @@ sub coalesce_overlapping_hunks {
 }
 
 sub help_patch_cmd {
-	print <<\EOF ;
+	print_colored $help_color, <<\EOF ;
 y - stage this hunk
 n - do not stage this hunk
 a - stage this and all the remaining hunks

@@ -619,7 +644,7 @@ sub patch_update_cmd {
 		for (@{$hunk[$ix]{TEXT}}) {
 			print;
 		}
-		print "Stage this hunk [y/n/a/d$other/?]? ";
+		print_colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
 		my $line = <STDIN>;
 		if ($line) {
 			if ($line =~ /^y/i) {

@@ -673,7 +698,7 @@ sub patch_update_cmd {
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT});
 				if (1 < @split) {
-					print "Split into ",
+					print_colored "$header_color", "Split into ",
 					scalar(@split), " hunks.\n";
 				}
 				splice(@hunk, $ix, 1,

@@ -769,7 +794,7 @@ sub quit_cmd {
 }
 
 sub help_cmd {
-	print <<\EOF ;
+	print_colored $help_color, <<\EOF ;
 status        - show paths with changes
 update        - add working tree state to the staged set of changes
 revert        - revert staged set of changes back to the HEAD version

1.5.3.4.207.gc0ee

I apparently missed the e-mail where Shawn Pearce explained where his repository was. The following patch is my recent change(s), rebased against that.

Dan

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation, then parsed into perl color strings (slightly different). Ugly but visible defaults are still used.

---
This patch is againts Shawn Pearce's "pu" branch.

 Documentation/config.txt  |   17 ++-------
 git-add--interactive.perl |   78 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 76 insertions(+), 19 deletions(-)

index 99b3817..d06f55f 100644
@@ -390,19 +390,10 @@ color.interactive::
 
 color.interactive.<slot>::
 	Use customized color for `git add --interactive`
-	output. `<slot>` may be `prompt`, `header`, or `help`,
-	for three distinct types of common output from interactive
-	programs. The values may be a space-delimited combination
-	of up to three of the following:
-+
-(optional attribute, optional foreground color, and optional background)
-+
-dark, bold, underline, underscore, blink, reverse, concealed,
-black, red, green, yellow, blue, magenta, cyan, white, on_black,
-on_red, on_green, on_yellow, on_blue, on_magenta, on_cyan, on_white
-+
-Note these are not the same colors/attributes that the rest of
-git supports, but are specific to `git-add --interactive`.
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
 
 color.pager::
 	A boolean to enable/disable colored output when the pager is in

1.5.3.4.207.gc0ee

Don't do that. The code in 'pu' is a mess of half-working features. If your patch is accepted, then it has to be picked apart from those half-working features that aren't being accepted (which hopefully isn't hard if nobody has been working in the same area, but can be quite ugly). Base your work on 'master' if possible, or 'next' if it relies on features only in next. If it relies on some topic branch that is _only_ in pu, then mention explicitly which topic.

-Peff

And even when you base work on things in next, don't base it on the tip of next. Base it on a specific topic that is merged into next. Next is also a mess of features, but they are more likely to be in a working state than the features in pu.

Topics in next will merge to master at different times. If your changes depend on more than one topic that may make it more difficult for the maintainer to merge your topic to master.

Fortunately In Dan's case the only topic in pu that impacted git-add--interactive was his dz/color-addi topic, so this probably applies to the tip of it just as well as it does to the tip of pu.

Shawn.

I did too, where you add a third, optional "trailer" parameter to the function where you pass the newline if there is one (following the style of the functions in color.c). Pasting it below.

Having said that, I think this kind of function belongs in Git.pm, and the dependency on Term::ANSIColor should be replaced with dependency-free code that generates the colors itself; this should be easy because the number of possible colors is small, Git thus far only uses a subset of the possible ANSI colors, and the C code for doing it is already there in color.c and just needs to be translated into Perl.

sub print_ansi_color {
	my $color = shift;
	my $string = shift;
	my $trailer = shift;
	if ($use_color) {
		print Term::ANSIColor::color($color), $string,
		    Term::ANSIColor::color('clear');
	} else {
		print $string;
	}
	if ($trailer) {
		print $trailer;
	}
}

Cheers,
Wincent

The problem with that approach is that you can only send in a single line at a time (with the newline detached!), so it makes life harder for the caller. E.g., there is at least one spot that uses a here-doc with many lines; splitting that into a bunch of print_ansi_color calls would be unnecessarily ugly.

Yes! Most of this is obviously library-ish code, and should go into the library.

Out of curiosity, are people really running perl < 5.6? Term::ANSIColor has been in the base distribution for 7 years now.

-Peff

Yes, I agree that it complicates things for the caller. I was just copying the model found in color.c; but seeing as this is Perl splitting into lines inside the printing function would be straightforward.

Cheers,
Wincent

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation, then parsed into perl color strings (slightly different). Ugly but visible defaults are still used.

---
Note: the code to parse git-style color strings to perl-style color strings should eventually be added to Git.pm so that other (perl) parts of git can be configured to read colors from .gitconfig in a nicer way. A git-style string is "ul red black", while perl likes strings like "underline red on_black".

 Documentation/config.txt  |    7 ++++
 git-add--interactive.perl |   76

++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 4 deletions(-)

index c795a35..75a976a 100644
@@ -387,6 +387,13 @@ color.interactive::
 	`auto`, use colors only when the output is to the
 	terminal. Defaults to false.
 
+color.interactive.<slot>::
+	Use customized color for `git add --interactive`
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

1.5.3.4.207.gc0ee

Why not do it as part of this patch, then?

    qx(git config --get color.interactive.prompt) || 'blue bold';

  my $prompt_color = git_color_to_ansicolor(
    qx(git config --get color.interactive.prompt) || 'blue bold');

Will do. I didn't include it in the patch because I need to learn more about perl before I can make this change, though I can probably just find enough examples in the other scripts that use Git.pm.

I agree, now that you mention it. Eventually the string must be split (parsing it left to right by word makes more sense than trying to mutate it with regular expressions, if only because it's a lot harder to make mistakes), but there's no reason not to split the string inside the loop, where it would look nicer/more contained. I will make this change.

Dan

From 29b34bb32846921c9432bf1b74c93d06a0667a44 Mon Sep 17 00:00:00 2001
From: Dan Zwell <[email protected]>
Date: Mon, 22 Oct 2007 15:55:20 -0500
Subject: [PATCH] Added basic color support to git add --interactive

Added function "print_colored" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print_colored".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
I believe this version takes care of the complaints people had with this patch. I hope this is helpful.

 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 6 deletions(-)

index edf50cd..2fd783f 100644
@@ -382,6 +382,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

1.5.3.5.474.g3e4bb

From: Dan Zwell <[email protected]>
Date: Mon, 22 Oct 2007 15:55:20 -0500
Subject: [PATCH] Added basic color support to git add --interactive

Added function "print_colored" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print_colored".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 6 deletions(-)

index edf50cd..2fd783f 100644
@@ -382,6 +382,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

1.5.3.5.474.g3e4bb

I think you would end up spitting out:

COLOR something RESET LF COLOR RESET LF

instead of:

COLOR something RESET LF LF

when you get "something\n\n" if you did that. Not a big deal, though, as at this point we would be human I/O bound.

Making it take a FH would be useful. With that, my proof-of-concept patch to add print_diff_hunk would become:

	sub print_diff_hunk {
        	my ($text) = @_;
		my $pager;

                if ($use_pager) {
	                open($pager, "| less");
		} else {
                	$pager = \*STDOUT;
		}
                for (@$text) {
			print_colored $pager $color ...
		}
                close($pager);
	}

Yes, though I wonder if the former is "more correct" in the sense that we don't know what the attributes are doing, and maybe it matters for them to apply to each line, whether it has text or not.

But I don't think it's possible for a blank line to actually do anything with the attributes we're currently allowing, and we don't have any plans to allow arbitrary terminal control codes in the color specs, so it probably doesn't matter.

-Peff

A bit of a recap--this feature was requested by a user a few weeks ago, and I wrote a short patch that implemented partial coloring. The main criticisms of this patch were:
- The name of the configuration key to turn on interactive coloring was not well chosen.
- The color attribute synonyms "clear" and "reset" were used interchangeably, though "reset" is what the rest of git uses.
- The colors were not user-settable.

When the above were fixed, the new patch garnered the following criticism:
- The color names accepted in .gitconfig were perl color names (to be fed to Term::ANSIColor), and this was not consistent with the rest of git. For example, "red blue ul" would have to be written, "red on_blue underline".

Fixing this (the colors could not be converted by a regex, but had to be parsed) was very libraryish, and also a little confusing. I was given a suggestion or two about how to make it more readable. This parsing also needed to be added to Git.pm instead of git-add--interactive.perl.

In the next iteration, all of the above were fixed, but as Jeff King pointed out, I was printing $color before the beginning of a string, and printing $clear at the end, which allowed ${COLOR}text\n${RESET}, which looks bad on some terminals.

Last iteration, it was pointed out that print_colored must take a file handle to pave the way for pager support, and Junio Hamano and Jeff King both gave suggestions as to how, and Junio sent me few changes that allow printing colored diffs in addition to prompts. I ended up making changes so that the two color functions can be used with perl's print():
print colored($color, "some text")
print color_diff_hunk($hunk)

This way, file handles can be added directly to the print calls, when support for $PAGER is added.

Let me know if other things need correction.

Dan

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

  user_defined_function($foo, $bar);

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.

This hunk makes the "show diff" subcommand honor user's external diff viewer if specified, which is a good change. But it does not belong to the "colored add -i" series.

I mildly suspect that this change might have been my fault, but I think it should be treated in an independent patch anyway.

Yep, that change was part of Junio's hunk coloring patch that he sent in reply to my last set of patches. When I revise this, I'll drop that change.

Dan

There are several changes since the last iteration of the "colors for git-add--interactive" patch set:

- Added parentheses around arguments to all user-defined functions.
- Reading the configuration is done in a more organized, robust, and cleaner way.
- Fixed bug where color keys could inappropriately match ("colored" should not match "red", for example).
- Users can now set a particular color key to the empty string, and this is equivalent to setting it to "normal". (This is the behavior exhibited by git-diff.)
- Color configuration information is case insensitive.
- Added warnings when color configuration keys contain invalid tokens.
- Refactored a few variables for stylistic reasons.

Issues:

- Does not always properly color the output of git-diff --cc, because the diff-coloring regular expressions do not match every diff line. I'm not sure that git-add--interactive normally gets used in the same
situations as git-diff --cc. They don't seem to work well, together, from the little that I tested (without the color patches applied). There are a few solutions, but I haven't thought of one that's both reliable and clean. My impression is that diff --cc is called any time that HEAD has two parents. Is this correct?

Dan

I think the only time that git-add--interactive is likely to see a combined diff is when you have unmerged entries in the index. Something like:

  $ mkdir foo && cd foo && git init
  $ touch file && git add file && git commit -m added
  $ echo master >file && git commit -a -m master
  $ git checkout -b other HEAD^
  $ echo other >file && git commit -a -m other
  $ git merge master
  $ git diff

-Peff

You get combined output when your index is unmerged.

Showing combined output to the user to examine may make sense, but I think you would want to have the user pick from diff between stage#2 and the work tree for an unmerged entry, if you allow to pick hunks during a conflicted merge.

Added function "colored()" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print colored()".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
 Documentation/config.txt  |    6 ++++
 git-add--interactive.perl |   66 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 65 insertions(+), 7 deletions(-)

index 8d5d200..3712d6a 100644
@@ -382,6 +382,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

1.5.3.5.565.gf0b83-dirty

Previously, the Git->repository()->config('non-existent.key') evaluated to as true in a vector context. Call 'return' with no argument, instead.

---
This isn't color related, but the next change I make to Git.pm depends on this.

 perl/Git.pm |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index dca92c8..6603762 100644
@@ -508,7 +508,7 @@ sub config {
 		my $E = shift;
 		if ($E->value() == 1) {
 			# Key not found.
-			return undef;
+			return;
 		} else {
 			throw $E;
 		}

1.5.3.5.565.gf0b83-dirty

I think the reason this works is a subtle issue (well, I had to look it up, anyway): return without an argument automatically checks the calling context and returns the empty list in a list context. So we are returning an empty list now, instead of a list containing a single undef. I think it might be useful to explain this a bit better in the commit message.

-Peff

I didn't realize it at the time, but yes, config_bool needs this (though the only time config_bool is evaluated in a list context should be when it is evaluated as an argument to another function). I'll make the change.

Dan

Method returns a configuration value if defined, or the default value that was passed in, otherwise.

The main purpose of this method is to allow the empty string to be a valid configuration option, and to replace the following construct:

$val = $repo->config('my.key') || $default_val

---
 perl/Git.pm |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

index 6603762..7327300 100644
@@ -549,6 +549,34 @@ sub config_bool {
 	};
 }
 
+=item config_default ( VARIABLE, DEFAULT )
+
+Fetches a configuration option C<VARIABLE>, returning its
+value if it is defined (it is valid to return the empty string,
+if that is its value). Otherwise, C<DEFAULT>, a default
+value, is returned. This method may be a replacement for
+
+	my $value = $repo->config('my.key') || 'default val';
+
+in situations where the empty string is an acceptable return value.
+This method may also be called in a vector context, when expecting
+multivars.
+
+	my @value = $repo->config_default('my.multivar', \@default_vals);
+
+=cut
+
+sub config_default {
+	my ($self, $var, $default) = @_;
+	if (wantarray) {
+		my @value = $self->config($var);
+		return @value ? @value : @$default;
+	}
+	else {
+		my $value = $self->config($var);
+		return (defined $value) ? $value : $default;
+	}
+}
 
 =item ident ( TYPE | IDENTSTR )
 

1.5.3.5.565.gf0b83-dirty

The config subroutine does not currently take a third argument. Is there a particular reason not to make $repo->config('my.key', $default_val) the equivalent of config_default?

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation. The method color_to_ansi_code() in Git.pm parses these strings and returns ANSI color codes (using Term::ANSIColor).

---
 Documentation/config.txt  |    7 ++++
 git-add--interactive.perl |   20 ++++++++---
 perl/Git.pm               |   80 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 100 insertions(+), 7 deletions(-)

index 3712d6a..47c1ab2 100644
@@ -388,6 +388,13 @@ color.interactive::
 	`auto`, use colors only when the output is to the
 	terminal. Defaults to false.
 
+color.interactive.<slot>::
+	Use customized color for `git add --interactive`
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index 2b5559f..f76f008 100755
@@ -6,8 +6,8 @@ use Git;
 my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
 
 {
-	# set color options:
 	my $repo = Git->repository();
+	# set interactive color options:
 	my $color_config = $repo->config('color.interactive');
 	$use_color = 0;
 	if (!defined $color_config) {

@@ -28,11 +28,19 @@ my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
 			$use_color = 0;
 		}
 		else { # set up colors
-			# Sane (visible) defaults:
-			$prompt_color = Term::ANSIColor::color('blue bold');
-			$header_color = Term::ANSIColor::color('bold');
-			$help_color   = Term::ANSIColor::color('red bold');
-			$normal_color = Term::ANSIColor::color('reset');
+			# Grab the 3 main colors in git color string format, with sane
+			# (visible) defaults:
+			$prompt_color = Git::color_to_ansi_code(
+				scalar $repo->config_default('color.interactive.prompt',
+					'bold blue'));
+			$header_color = Git::color_to_ansi_code(
+				scalar $repo->config_default('color.interactive.header',
+					'bold'));
+			$help_color = Git::color_to_ansi_code(
+				scalar $repo->config_default('color.interactive.help',
+					'red bold'));
+
+			$normal_color = Git::color_to_ansi_code('normal');
 		}
 	}
 }

1.5.3.5.565.gf0b83-dirty

I like the config_color() method.

I think the "config_bool with default" also makes sense but it needs to be coded a bit carefully. Issues to consider:

(1) Non default form "$r->config_bool('key')" should keep the original semantics; missing key in the configuration is the same as false (i.e. "undef" in scalar, () in list context).

(2) What should be the second parameter in the form to default to true? '1'? 'true'? Any kind of "true" value in Perl should be accepted?

(3) Same question as (2) but for defaulting to false. Any kind

     of "false"?

Yes. It is not strictly necessary for this patch series, but I think it is nice to stake out a claim on the third argument of config_* functions for consistency sake. But perhaps in the name of avoiding regression, it should come later, when somebody actually wants to use it.

Yes, this is obviously the most important thing.

Hmm. I am tempted to say "yes, any true or any false value" in that the point of config_* is to convert git config values to native perl representations. OTOH, the moral equivalent of

  config_color('my.key', 'bold red');

is probably more appropriately

  config_bool('my.key', 'true');

so I am fine doing it that way, as well (though I think it makes us duplicate the "translate these strings into bools" code into perl).

-Peff

As you said, config_* converts git values to perl values. However, that conversion needs only be done for strings in .gitconfig. Is there any reason why the caller of the function would need to pass a string "false"? I just don't see the need for conversion of any kind.

Further, I think that we could return the default variable directly, without parsing it at all. It would be much simpler, and there would need to be no special cases for dealing with undef or 'false'. It's a perl function, being called with perl arguments, so a user should not be that surprised when 'false' does what perl says it should do.

Dan

I think that is more elegant for config_bool, but it means that config_bool and config_color have slightly different behaviors (the difference being that it is easy to feed a native perl value as the default to config_bool, but to get the same behavior for config_color, you would call Git::color_to_ansi_code manually, which is a pain).

In this instance, I am inclined to sacrifice consistency for convenience and make it:

   my $bool = config_bool('my.key', 0);
   my $color = config_color('my.key', 'bold red');

Note that there is one tricky part of config_bool, which is what config_bool('my.key', undef) should do (is it "default false" or "no default"?).

-Peff

I am glad somebody finally got to the trick question I posed earlier ;-)

But config_bool('key') and config_bool('key', undef) would both return undef to say "The value is false" when key does not exist, so it was not much of a trick. It does not make a difference if the undef came because the default parameter was undef, or because there was no default parameter given and the built-in behaviour of config_bool() was to return undef for a missing key.

Added and integrated method "color_diff_hunk", which colors lines, and returns them in an array. Coloring bad whitespace is not yet supported.

---
 git-add--interactive.perl |   93 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 72 insertions(+), 21 deletions(-)

index f76f008..ba9430c 100755
@@ -4,9 +4,12 @@ use strict;
 use Git;
 
 my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
+my ($diff_use_color, $new_color, $old_color, $fraginfo_color,
+	$metainfo_color, $whitespace_color);
 
 {
 	my $repo = Git->repository();
+
 	# set interactive color options:
 	my $color_config = $repo->config('color.interactive');
 	$use_color = 0;

@@ -21,27 +24,55 @@ my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
 		$use_color = 1;
 	}
 
-	if ($use_color) {
+	# set diff color options
+	my $diff_color_config = $repo->config('color.diff');
+	if (!defined $diff_color_config) {
+		$diff_use_color = 0;
+	}
+	elsif ($diff_color_config =~ /true|always/) {
+		$diff_use_color = 1;
+	}
+	elsif ($diff_color_config eq 'auto' && -t STDOUT &&
+		   $ENV{'TERM'} ne 'dumb') {
+		$diff_use_color = 1;
+	}
+
+	# load color library if needed
+	if ($use_color || $diff_use_color) {
 		eval { require Term::ANSIColor; };
 		if ($@) {
 			# library did not load.
 			$use_color = 0;
+			$diff_use_color = 0;
 		}
-		else { # set up colors
-			# Grab the 3 main colors in git color string format, with sane
-			# (visible) defaults:
-			$prompt_color = Git::color_to_ansi_code(
-				scalar $repo->config_default('color.interactive.prompt',
-					'bold blue'));
-			$header_color = Git::color_to_ansi_code(
-				scalar $repo->config_default('color.interactive.header',
-					'bold'));
-			$help_color = Git::color_to_ansi_code(
-				scalar $repo->config_default('color.interactive.help',
-					'red bold'));
+	}
 
-			$normal_color = Git::color_to_ansi_code('normal');
-		}
+	# convenience function:
+	sub get_color {
+		my ($key, $default) = @_;
+		return Git::color_to_ansi_code(
+			scalar $repo->config_default($key, $default));
+	}
+	# set interactive colors
+	if ($use_color) {
+		# Grab the 3 main colors in git color string format, with sane
+		# (visible) defaults:
+		$prompt_color = get_color('color.interactive.prompt', 'bold blue');
+		$header_color = get_color('color.interactive.header', 'bold');
+		$help_color = get_color('color.interactive.help', 'red bold');
+		$normal_color = Git::color_to_ansi_code('normal');
+	}
+
+	# set diff colors
+	if ($diff_use_color) {
+		$new_color = get_color('color.diff.new', 'green');
+		$old_color = get_color('color.diff.old', 'red');
+		$fraginfo_color = get_color('color.diff.frag', 'cyan');
+		$metainfo_color = get_color('color.diff.meta', 'bold');
+		$normal_color = Git::color_to_ansi_code('normal');
+		# Not implemented:
+		#$whitespace_color = get_color('color.diff.whitespace',
+			#'normal red');
 	}
 }
 

@@ -410,6 +441,30 @@ sub parse_diff {
 	return @hunk;
 }
 
+sub color_diff_hunk {
+	# return the colored text, so that it can be passed to print()
+	my ($text) = @_;
+	if (!$diff_use_color) {
+		return @$text;
+	}
+
+	my @ret;
+	for (@$text) {
+		if (/^\+/) {
+			push @ret, colored($new_color, $_);
+		} elsif (/^\-/) {
+			push @ret, colored($old_color, $_);
+		} elsif (/^\@/) {
+			push @ret, colored($fraginfo_color, $_);
+		} elsif (/^ /) {
+			push @ret, colored($normal_color, $_);
+		} else {
+			push @ret, colored($metainfo_color, $_);
+		}
+	}
+	return @ret;
+}
+
 sub hunk_splittable {
 	my ($text) = @_;
 

@@ -632,9 +687,7 @@ sub patch_update_cmd {
 	my ($ix, $num);
 	my $path = $it->{VALUE};
 	my ($head, @hunk) = parse_diff($path);
-	for (@{$head->{TEXT}}) {
-		print;
-	}
+	print color_diff_hunk($head->{TEXT});
 	$num = scalar @hunk;
 	$ix = 0;
 

@@ -676,9 +729,7 @@ sub patch_update_cmd {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
-		for (@{$hunk[$ix]{TEXT}}) {
-			print;
-		}
+		print color_diff_hunk($hunk[$ix]{TEXT});
 		print colored($prompt_color, "Stage this hunk [y/n/a/d$other/?]? ");
 		my $line = <STDIN>;
 		if ($line) {

1.5.3.5.565.gf0b83-dirty

If you are suggesting to reorganize the series like this:

1/5 Fix to Git.pm for list context;

2/5 Enhance Git.pm to allow config() methods to take default values;

3/5 Enhance Git.pm with get_color() method;

4/5 Teach git-add--interactive to read color settings from the

     config;

5/5 Paint output from git-add--interactive in colors, including
prompt, help and diff hunks.

I think that makes a very good sense. The earlier part of the series would be independent from colorization of "add -i" and can go in before everything else to allow other potential users, e.g. "git remote --color" ;-). I do not see a strong reason to have the separate "Basic color support with hardcoded color" at the beginning, either.

I think it is a matter of taste to either:

(1) Squash 4 and 5 in the above list into one; or

(2) Split 5 into separate commits to color different parts.

Perhaps the former would be simpler and more appropriate for this series.

Neither officially or unofficially yet, but we can start the process of making it official with an early announcement. I do not think we would hurt people as long as a long enough advance notice is given.

I however am wondering if we need to have so many "enable color support" switches. color.status, color.diff, and now yet another color.interactive? Who sets color.status and/or color.interactive to auto without setting color.diff to auto as well?

It may be good that they _can_ be individually controlled, but I strongly suspect that most people would just want to set a single variable color.ui to "auto", and have it give the default value for all the color.$cmd configuration variable that are not explicitly defined.

Andy Parkins added color.* (and removed documentation for *.color) almost a year ago (in a159ca0c). But I don't think there has been an official deprecation notice.

Yes, I have often thought this, as well, and a "color.all" would probably be convenient.

-Peff

Added function "colored()" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print colored".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   44

++++++++++++++++++++++++++++++++++++++------ 2 files changed, 44 insertions(+), 6 deletions(-)

index 8d5d200..3712d6a 100644
@@ -382,6 +382,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index ac598f8..f2b0e56 100755
@@ -2,6 +2,38 @@
 
 use strict;
 
+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/) {
+ eval { require Term::ANSIColor; };
+ if (!$@) {
+ $use_color = 1;
+
+ # Sane (visible) defaults:
+ $prompt_color = Term::ANSIColor::color("blue bold");
+ $header_color = Term::ANSIColor::color("bold");
+ $help_color = Term::ANSIColor::color("red bold");
+ $normal_color = Term::ANSIColor::color("reset");
+ }
+}
+
+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;
+}
+
sub run_cmd_pipe {
if ($^O eq 'MSWin32') {
my @invalid = grep {m/[":*]/} @_;
@@ -175,7 +207,7 @@ sub list_and_choose {
if (!$opts->{LIST_FLAT}) {
print " ";
}
- print "$opts->{HEADER}\n";
+ print colored $header_color,
"$opts->{HEADER}\n"; }
for ($i = 0; $i < @stuff; $i++) {
my $chosen = $chosen[$i] ? '*' : ' ';
@@ -205,7 +237,7 @@ sub list_and_choose {

return if ($opts->{LIST_ONLY});

- print $opts->{PROMPT};
+ print colored $prompt_color, $opts->{PROMPT};
if ($opts->{SINGLETON}) {
print "> ";
}
@@ -544,7 +576,7 @@ sub coalesce_overlapping_hunks {
}

sub help_patch_cmd {
- print <<\EOF ;
+ print colored $help_color, <<\EOF ;
y - stage this hunk
n - do not stage this hunk
a - stage this and all the remaining hunks
@@ -619,7 +651,7 @@ sub patch_update_cmd {
for (@{$hunk[$ix]{TEXT}}) {
print;
}
- print "Stage this hunk [y/n/a/d$other/?]? ";
+ print colored $prompt_color, "Stage this hunk
[y/n/a/d$other/?]? "; my $line = <STDIN>;
if ($line) {
if ($line =~ /^y/i) {
@@ -673,7 +705,7 @@ sub patch_update_cmd {
elsif ($other =~ /s/ && $line =~ /^s/) {
my @split =
split_hunk($hunk[$ix]{TEXT}); if (1 < @split) {
- print "Split into ",
+ print colored $header_color,
"Split into ", scalar(@split), " hunks.\n";
}
splice(@hunk, $ix, 1,
@@ -766,7 +798,7 @@ sub quit_cmd {
}

sub help_cmd {
- print <<\EOF ;
+ print colored $help_color, <<\EOF ;

 status        - show paths with changes
 update        - add working tree state to the staged set of changes
 revert        - revert staged set of changes back to the HEAD version

1.5.3.5.565.gf0b83-dirty

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation. The method color_to_ansi_string() in Git.pm parses these strings and returns ANSI color codes (using Term::ANSIColor).

---
 Documentation/config.txt  |    7 +++++
 git-add--interactive.perl |   19 ++++++++++-----
 perl/Git.pm               |   55

++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 7 deletions(-)

index 3712d6a..47c1ab2 100644
@@ -388,6 +388,13 @@ color.interactive::
 	`auto`, use colors only when the output is to the
 	terminal. Defaults to false.
 
+color.interactive.<slot>::
+	Use customized color for `git add --interactive`
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index f2b0e56..508531f 100755
@@ -1,6 +1,7 @@
 #!/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); @@ -8,12 +9,18 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) { eval { require Term::ANSIColor; }; if (!$@) {
$use_color = 1;
-
- # Sane (visible) defaults:
- $prompt_color = Term::ANSIColor::color("blue bold");
- $header_color = Term::ANSIColor::color("bold");
- $help_color = Term::ANSIColor::color("red bold");
- $normal_color = Term::ANSIColor::color("reset");
+ # Set interactive colors:
+
+ # 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");
}
}

1.5.3.5.565.gf0b83-dirty

Added and integrated method "color_diff_hunk", which colors lines, and returns them in an array. Coloring bad whitespace is not yet supported.

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

index 508531f..d92e8ed 100755
@@ -3,7 +3,11 @@
 use strict;
 use Git;
 
+# Prompt colors:
 my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
+# Diff colors:
+my ($diff_use_color, $new_color, $old_color, $fraginfo_color,
+    $metainfo_color, $whitespace_color);
 my $color_config = qx(git config --get color.interactive);
 if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
 	eval { require Term::ANSIColor; };

@@ -21,6 +25,24 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
 		$help_color = Git::color_to_ansi_code(
 			Git::config($repo, "color.interactive.help") || "red bold");
 		$normal_color = Git::color_to_ansi_code("normal");
+
+		# Do we also set diff colors?
+		my $diff_colors = Git::config($repo, "color.diff");
+		if ($diff_colors=~/true/ ||
+			-t STDOUT && $diff_colors=~/auto/) {
+			$diff_use_color = 1;
+			$new_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.new") || "green");
+			$old_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.old") || "red");
+			$fraginfo_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.frag") || "cyan");
+			$metainfo_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.meta") || "bold");
+			# Not implemented:
+			#$whitespace_color = Git::color_to_ansi_code(
+				#Git::config($repo, "color.diff.whitespace") || "normal red");
+		}
 	}
 }
 

@@ -389,6 +411,31 @@ sub parse_diff {
 	return @hunk;
 }
 
+sub colored_diff_hunk {
+	my ($text) = @_;
+	# return the text, so that it can be passed to print()
+	my @ret;
+	for (@$text) {
+		if (!$diff_use_color) {
+			push @ret, $_;
+			next;
+		}
+
+		if (/^\+/) {
+			push @ret, colored($new_color, $_);
+		} elsif (/^\-/) {
+			push @ret, colored($old_color, $_);
+		} elsif (/^\@/) {
+			push @ret, colored($fraginfo_color, $_);
+		} elsif (/^ /) {
+			push @ret, colored($normal_color, $_);
+		} else {
+			push @ret, colored($metainfo_color, $_);
+		}
+	}
+	return @ret;
+}
+
 sub hunk_splittable {
 	my ($text) = @_;
 

@@ -611,9 +658,7 @@ sub patch_update_cmd {
 	my ($ix, $num);
 	my $path = $it->{VALUE};
 	my ($head, @hunk) = parse_diff($path);
-	for (@{$head->{TEXT}}) {
-		print;
-	}
+	print colored_diff_hunk($head->{TEXT});
 	$num = scalar @hunk;
 	$ix = 0;
 

@@ -655,9 +700,7 @@ sub patch_update_cmd {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
-		for (@{$hunk[$ix]{TEXT}}) {
-			print;
-		}
+		print colored_diff_hunk($hunk[$ix]{TEXT});
 		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
 		my $line = <STDIN>;
 		if ($line) {

@@ -795,8 +838,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 {

1.5.3.5.565.gf0b83-dirty

[Note: I'm resending this because it looks like this e-mail didn't go out properly. Sorry for duplicates.]

A bit of a recap--this feature was
requested by a user a few weeks ago, and I wrote a short patch that implemented partial coloring. The main criticisms of this patch were:
- The name of the configuration key to turn on interactive coloring was not well chosen.
- The color attribute synonyms "clear" and "reset" were used interchangeably, though "reset" is what the rest of git uses.
- The colors were not user-settable.

When the above were fixed, the new patch garnered the following criticism:
- The color names accepted in .gitconfig were perl color names (to be fed to Term::ANSIColor), and this was not consistent with the rest of git. For example, "red blue ul" would have to be written, "red on_blue underline".

Fixing this (the colors could not be converted by a regex, but had to be parsed) was very libraryish, and also a little confusing. I was given a suggestion or two about how to make it more readable. This parsing also needed to be added to Git.pm instead of git-add--interactive.perl.

In the next iteration, all of the above were fixed, but as Jeff King pointed out, I was printing $color before the beginning of a string, and printing $clear at the end, which allowed ${COLOR}text\n${RESET}, which looks bad on some terminals.

Last iteration, it was pointed out that print_colored must take a file handle to pave the way for pager support, and Junio Hamano and Jeff King both gave suggestions as to how, and Junio sent me few changes that allow printing colored diffs in addition to prompts. I ended up making changes so that the two color functions can be used with perl's print():
print colored($color, "some text")
print color_diff_hunk($hunk)

This way, file handles can be added directly to the print calls, when support for $PAGER is added.

Let me know if other things need correction.

Dan

Added function "colored()" that prints text with a color that is passed in. Converted many calls to "print" to being calls to "print colored".

The prompt, the header, and the help output are the 3 types of colorized output, and each has its own color.

Colorization is done through Term::ANSIColor, which is included with modern versions of perl. This is optional, and should not need to be present if color.interactive is not turned on.

---
The last version of this e-mail was line wrapped, if it was successfully sent at all...

 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   44 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 44 insertions(+), 6 deletions(-)

index 8d5d200..3712d6a 100644
@@ -382,6 +382,12 @@ color.diff.<slot>::
 	whitespace).  The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.interactive::
+	When true (or `always`), always use colors in `git add
+	--interactive`.  When false (or `never`), never.  When set to
+	`auto`, use colors only when the output is to the
+	terminal. Defaults to false.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

1.5.3.5.565.gf0b83-dirty

You said "Let me know if other things need correction." I think most of the comments you recieved were about improvements not correction, and I think I am going to say in this message will also mostly fall into that category.

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation. The method color_to_ansi_string() in Git.pm parses these strings and returns ANSI color codes (using Term::ANSIColor).

---
The last version of this e-mail was line wrapped, if it was successfully sent at all.

 Documentation/config.txt  |    7 +++++
 git-add--interactive.perl |   19 ++++++++++-----
 perl/Git.pm               |   55 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 7 deletions(-)

index 3712d6a..47c1ab2 100644
@@ -388,6 +388,13 @@ color.interactive::
 	`auto`, use colors only when the output is to the
 	terminal. Defaults to false.
 
+color.interactive.<slot>::
+	Use customized color for `git add --interactive`
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index f2b0e56..508531f 100755
@@ -1,6 +1,7 @@
 #!/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);

@@ -8,12 +9,18 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
 	eval { require Term::ANSIColor; };
 	if (!$@) {
 		$use_color = 1;
-
-		# Sane (visible) defaults:
-		$prompt_color = Term::ANSIColor::color("blue bold");
-		$header_color = Term::ANSIColor::color("bold");
-		$help_color   = Term::ANSIColor::color("red bold");
-		$normal_color = Term::ANSIColor::color("reset");
+		# Set interactive colors:
+
+		# 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");
 	}
 }
 

1.5.3.5.565.gf0b83-dirty

Sorry, but please disregard. "bold blue" part was also parameter to the string-to-ansi-color-escape function, so the above does not make much sense.

I don't see the advantage of doing it that way. After all, we're pattern matching. Does using a hash, an array, and a call to map() gain us something? I think a regular expression is clearer. Of course, as Jeff pointed out, I should have used a whitespace-agnostic regular expression.
+ elsif ($word =~ /black|red|green|yellow|
+ blue|magenta|cyan|white/x ) {

I agreed with the rest of your suggestions, and will implement them in the next round of changes, later this week.

Dan

I suggested the hash approach only because (1) it is easier to read than two regexp matches that are split only to keep the line less than 80-chars long, and (2) a misconfiguration like "color.foo = fred" can be caught more easily.

I do not quite understand the "after all, we're pattern matching" part, though. Are you talking about "split(/\s+/, $str)" your for-loop iterates over?

I think we're talking about the same thing. I was referring to the split regular expression, and the question is, "for the current element of split(/\s+/, $str), does it match a color?"

Anyway, I preferred the regex version for readability, though I should have used the /x modifier--it would still take two lines, but it would not need to attempt two matches. As for misconfigured color configurations, should we catch that? I wrote this with the intent that it should ignore invalid color names, but it would probably be more useful to print a warning.

Dan

Your regex is wrong, because it doesn't anchor at the beginning and end of the string (so /red/ will match "supercaliredfragilistic", which is probably not what you want). So you probably want /^red$/, which is equivalent to using 'eq' with 'red'. Or, as Junio noted, you are overall trying to say "is element $word in this list"; the canonical perl way of doing that is to make the list a hash for quick lookup.

Your patch doesn't just ignore; sometimes it accidentally matches invalid input (the example above is obviously silly, but consider what accidentally omitting the space in "blinkblack" would do).

-Peff

But the point is, that you are not ignoring invalid color names but instead giving back a random match aren't you?

No, if there's no match, the token is ignored. False matches are possible in some cases (the bogus config option "colored" would match "red", for example), so I will follow your suggestion with the hash, after all. I'll send out the next revised patches in a day or two--I've made most of the changes you and Jeff suggested, but I need to double check.

Dan

Match /\b(?:red|...)\b/ then

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

From: Dan Zwell <[email protected]>
Date: Mon, 22 Oct 2007 16:08:01 -0500
Subject: [PATCH] Let git-add--interactive read colors from .gitconfig

Colors are specified in color.interactive.{prompt,header,help}. They are specified as git color strings as described in the documentation. The method color_to_ansi_string() in Git.pm parses these strings and returns ANSI color codes (using Term::ANSIColor).

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

 Documentation/config.txt  |    7 +++++
 git-add--interactive.perl |   22 ++++++++++++-----
 perl/Git.pm               |   56 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 77 insertions(+), 8 deletions(-)

index 2fd783f..ade3399 100644
@@ -388,6 +388,13 @@ color.interactive::
 	`auto`, use colors only when the output is to the
 	terminal. Defaults to false.
 
+color.interactive.<slot>::
+	Use customized color for `git add --interactive`
+	output. `<slot>` may be `prompt`, `header`, or `help`, for
+	three distinct types of normal output from interactive
+	programs.  The values of these variables may be specified as
+	in color.branch.<slot>.
+
 color.pager::
 	A boolean to enable/disable colored output when the pager is in
 	use (default is true).

index 16dc7b0..2bce5a1 100755
@@ -1,18 +1,26 @@
 #!/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/) {
-	require Term::ANSIColor;
-
 	$use_color = "true";
-	# Sane (visible) defaults:
-	$prompt_color = Term::ANSIColor::color("blue bold");
-	$header_color = Term::ANSIColor::color("bold");
-	$help_color   = Term::ANSIColor::color("red bold");
-	$normal_color = Term::ANSIColor::color("reset");
+	# 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");
 }
 
 sub print_colored {

1.5.3.5.474.g3e4bb

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.

index ac598f8..2bce5a1 100755
@@ -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(-)

index 2bce5a1..1063a34 100755
@@ -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 {

20K on my machine, and part of the core library since Perl 5.6.0. This was released in 2000. With your addition (the eval check to make sure the module is loaded), nobody should be harmed if they don't have a modern perl, either. My vote would be to reimplement the coloring if we actually notice these problems.

<snip>

You mean in general, so that users can view a hunk in the PAGER, then be prompted for what to do with it? (Because it doesn't solve the color problems, because calling "diff --color" here creates other problems.)

Yes. I see it a much bigger problem that we let a long hunk scroll off the top of the screen then ask the user what to do with it, than any coloring of diff.

No, but you can make it so if you want.

Personally, I think it is not so useful for "add -i" to do so, as it is too late in the workflow, unless you add it ways to let you edit and fix the whitespace errors.

Or could you just piggy-back on the settings for color.diff.<slot>?

And if a separate group for git-add is necessary, perhaps "add" would be enough, rather than "add-interactive".

Wincent

I think color.add is better, because git-add--interactive goes beyond coloring diffs. When this is complete, it should probably use color.diff.<slot> for the actual diff output, and color.add.<slot> for colored prompts/commands.

Dan

Or maybe rather color.interactive.<slot>, where <slot> could be 'prompt', 'header', etc. It's better to give it a name that describes what it is for, instead of one that describes which tool uses it. This way it could possibly be used for other potential interactive tools in the future.

jlh

BTW, this approach is totally bogus. The hunks that we store end up getting fed back to git-apply when we stage them (which doesn't understand the color codes).

Just try using your patch to actually stage a hunk; nothing happens (and the error is almost impossible to see, since we show the bogus diff on stderr).

So now I am doubly convinced that colorizing the diffs in add--interactive is the right thing (and it looks like Tom Tobin has already done a fair bit of the work).

-Peff

Hi!

I really like the idea of colorizing git add -i, especially the prompt. Here are my two cents.

sub print_ansi_color {
	my ($color, $string, $trailer) = @_;

	if (defined $trailer) {
	    print $trailer;
	}

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

Red for removed and green for added seems to be the standard, although I know it makes life terribly difficult for red-green colorblind people, who usually prefer yellow/lightblue for black bg, or beige/blue for white background.

Andreas Ericsson                   [email protected]
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

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.

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.

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

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.

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

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.

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

Hi,

FWIW, this is what Documentation/config.txt has to say about it:

color.diff.<slot>::
Use customized color for diff colorization. <slot> specifies which part of the patch to use the specified color, and is one of plain (context text), meta (metainformation), frag (hunk header), old (removed lines), new (added lines), commit (commit headers), or whitespace (highlighting dubious whitespace). The values of these variables may be specified as in color.branch.<slot>.

Hth,
Dscho

Neat, thanks for working on this.

I believe it's because add--interactive parses the output of git-diff-files, so it expects unadorned diffs. I think you may be stuck re-coloring the diffs yourself, which is a little ugly.

Two suggestions:
- every color should be configurable (e.g., see diff color options)
- where possible, use existing color config (e.g., for diffs)

You will never come up with a color scheme that satisfies everyone (e.g., white text on black background versus black text on white background), so configurability is a good idea (not to mention that nobody will ever agree on what looks "good").

Shouldn't these just be 'eq' instead of a regex?

# the beginning of each line, but then turn them off before each
# newline. This should give the effect of covering the whole string
# with the attribute, but not have attributes cross newline boundaries.
sub color_print {
  my $attr = shift;
  local $_ = shift;
  if ($use_color) {
    s/^/Term::ANSIColor::color($attr)/mge;
    s/\n/Term::ANSIColor::color('reset') . $&/ge;
  }
  print $_;
}

What would mean you have to chomp it first. But it should at least be written as =~ /.../ to make it clear that using a regex was a concious decision here and not an accident.

Gruesse,

Frank Lichtenheld <[email protected]>
www: http://www.djpig.de/

Hi,

[Cc'ed Wincent correctly]

        for (@diff) {
                if (/^@@ /) {