[PATCH] Color support added to git-add--interactive.

[PATCH] Color support added to git-add--interactive.

[Dan Zwell]

[-- Attachment #1: Type: text/plain, Size: 3175 bytes --]

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.

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..f55d787 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

[-- Attachment #2: Color-add-interactive.patch.gz --]
[-- Type: application/gzip, Size: 1354 bytes --]

Re: [PATCH] Color support added to git-add--interactive.

[Jeff King]

On Fri, Oct 12, 2007 at 11:13:14PM -0500, Dan Zwell wrote:

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

Neat, thanks for working on this.

> 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 

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.

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

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

> +if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {

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

> +			print_ansi_color "bold";
>  			print "$opts->{HEADER}\n";
> +			print_ansi_color "clear";

ISTR some terminals had issues with leaving ANSI attributes set across a
newline. That was the reason for the color_fprintf_ln business in
color.[ch]. You might replace this with something like:

  print_color_ln 'bold', $opts->{HEADER};

where "print_color_ln" turns off the attribute before the newline.

> +	# 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);

See how we are pulling the diff into lines? Look a few lines below and
you will see that we start parsing without regard to the color.

Unfortunately, that parsed form ends up being output to the user, so we
will have to do colorization at that point (fortunately, diff
colorization with regexes isn't _that_ hard).

> +	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";

Hrm, splitting this with print_color_ln as I mentioned above would be a
little painful. Maybe something like this (totally untested):

# Turn on ansi attributes at the beginning of the string and at
# 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 $_;
}

-Peff

Re: [PATCH] Color support added to git-add--interactive.

[Johannes Schindelin]

Hi,

[Cc'ed Wincent correctly]

On Fri, 12 Oct 2007, Dan Zwell wrote:

> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index be68814..f55d787 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -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;
> +}

Good.  If you do not have color enabled, it does not require that package 
that is only default with modern Perl.

> @@ -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";

Here you say "clear", and ...

>  		}
>  		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";

here you say "reset".  Is it because of the added colour?

> @@ -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 => [] };

This fails because of the next two lines:

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

Replace the if with "if (/^[^-+ ]*@@ /)", or something even stricter.  
--color adds magic sequences to make color.

I cannot comment on the Perl style ;-)

Ciao,
Dscho

Re: [PATCH] Color support added to git-add--interactive.

[Frank Lichtenheld]

On Sat, Oct 13, 2007 at 04:12:06AM -0400, Jeff King wrote:
> > +if ($color_config=~"true" || -t STDOUT && $color_config=~"auto") {
> 
> Shouldn't these just be 'eq' instead of a regex?

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 <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] Color support added to git-add--interactive.

[Wincent Colaiuta]

El 13/10/2007, a las 6:13, Dan Zwell escribió:

> Dan Zwell<Color-add-interactive.patch.gz>

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

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..ae3d11e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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) {

Re: [PATCH] Color support added to git-add--interactive.

[Jean-Luc Herren]

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

Re: [PATCH] Color support added to git-add--interactive.

[Johannes Schindelin]

Hi,

On Sat, 13 Oct 2007, Wincent Colaiuta wrote:

> Didn't even think about implementing user-settable colors.

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

Re: [PATCH] Color support added to git-add--interactive.

[Wincent Colaiuta]

El 13/10/2007, a las 18:38, Jean-Luc Herren escribió:

> Here are my two cents.
>
> Wincent Colaiuta wrote:
>> +sub print_ansi_color($$;$) {
>> +    my $color = shift;
>> +    my $string = shift;
>> +    my $trailer = shift;
>
> None of the other subs in this file have a prototype, so for
> consistency I'd suggest to not add it on this function either.
> However maybe a patch that adds it to all subs would be welcome.
> (I wouldn't see the necessity though.)

Yes, I saw that the other functions didn't use prototypes and I agree  
that consistency would be a good thing. I liked the idea of it in  
this case because it makes explicit the fact that the function takes  
two params plus a third, optional one. So definitely not necessary,  
but a preference of mine. In any case, a change to all the other  
functions is a question for a separate patch.

> And the common way of getting the arguments is reading @_ (see all
> other subs in the file).  So maybe instead write:
>
> [...]
> sub print_ansi_color {
> 	my ($color, $string, $trailer) = @_;
> [...]

Yes, I actually did write it that way first but then my doubts about  
Perl made me write it the longer way; but if they are equivalent then  
I prefer the shorter way.

>> +    if ($use_color) {
>> +        printf '%s%s%s', Term::ANSIColor::color($color), $string,
>> +            Term::ANSIColor::color('clear');
>> +    } else {
>
> Why use printf when you could directly use print here?  It's only
> used for concatenating.

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

>> +    if ($trailer) {
>> +        print $trailer;
>> +    }
>
> This will fail to print $trailer when $trailer happens to be a
> string that evaluates to false in bool context, like '0'.  Write
> this as:
>
> 	if (defined $trailer) {
> 	    print $trailer;
> 	}

Again, I actually wrote it that way the first time, and then changed  
it, this time because I thought they were the same. Like I said, not  
a perl hacker.

> IMHO, parsing the output of 'git diff-files --color' is a very bad
> idea and it makes all regexes uglier and more difficult to read.
> You're much better off recolorizing it yourself, which makes it a
> more localized change.

You're probably right, although it is also duplicating the work  
that's already done elsewhere. In general I favor making the simplest  
change that would work, and tweaking a few of the regexes did look  
simpler than re-implementing the colorization logic.

But the approach you suggest might be more robust, perhaps, seeing as  
there's not much to the diff output. As far as I can tell there are  
really only five or six different things to look for, and they'd be  
fairly easy to catch:

- lines beginning with "@@ " (hunk headers)

- lines beginning with "+" (insertions)

- lines beginning with "-" (deletions)

- lines beginning with " " (context lines, no color)

- lines beginning with "\" (things like "\ No newline at end of  
file", again, no color)

- everything else; ie. the diff header stuff (eg "diff --git a/foo b/ 
foo")

The only special cases seem to be the "+++" and "---" lines in the  
header, which look like insertions and deletions when they're not.

Trickier would be the highlighting of dubious whitespace, and that's  
when it starts to sound like re-inventing the wheel and duplicating  
the logic for the detection that's defined elsewhere (possibly in  
diff-lib.c? haven't found the exact spot yet).

> Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space)

Yes, that was one of the things I didn't like about the sloppy  
regexes. I couldn't really make them any stricter though because I  
wasn't confident about the range of possible characters that might be  
included in the escape sequences.

> Finally -- and this might be just my eyes -- blue is a very nice
> color, but it looks a bit too dark on black background.  Maybe
> choose a default color that looks reasonable on black *and* white
> background.

Yeah, well I didn't choose the colours and I didn't really want to  
get into it. Before being considered for inclusion a patch like this  
would need to tap in to the existing config settings for color.diff  
and color.diff.<slot> anyway...

Wincent

Re: [PATCH] Color support added to git-add--interactive.

[Jeff King]

On Sat, Oct 13, 2007 at 04:45:41PM +0200, Wincent Colaiuta wrote:

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

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

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

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

Re: [PATCH] Color support added to git-add--interactive.

[Jeff King]

On Sat, Oct 13, 2007 at 01:27:45PM -0400, Jeff King wrote:

> 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

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

Re: [PATCH] Color support added to git-add--interactive.

[Andreas Ericsson]

Jean-Luc Herren wrote:
> 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.
> 

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                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Color support added to git-add--interactive.

[Dan Zwell]

Jeff King wrote:
> On Sat, Oct 13, 2007 at 01:27:45PM -0400, Jeff King wrote:
> 
> <snip> 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

Re: [PATCH] Color support added to git-add--interactive.

[Tom Tobin]

On Fri, 2007-10-12 at 23:13 -0500, Dan Zwell wrote:
> 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.

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

Re: [PATCH] Color support added to git-add--interactive.

[Tom Tobin]

On Sat, 2007-10-13 at 15:21 -0500, Tom Tobin wrote:
> 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
"korpios@korpios.com"; probably since I'm routing everything through
Google Apps).  Ah well, one more thing to fix first....

Re: [PATCH] Color support added to git-add--interactive.

[Wincent Colaiuta]

El 13/10/2007, a las 22:03, Dan Zwell escribió:

> 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

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

Re: [PATCH] Color support added to git-add--interactive.

[Dan Z]

On 10/13/07, Wincent Colaiuta <win@wincent.com> wrote:
> 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

Re: [PATCH] Color support added to git-add--interactive.

[Jean-Luc Herren]

Dan Z wrote:
> 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.

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

Re: [PATCH] Color support added to git-add--interactive.

[Jeff King]

On Sat, Oct 13, 2007 at 03:03:29PM -0500, Dan Zwell wrote:

> 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 

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

Re: [PATCH] Color support added to git-add--interactive.

[Jeff King]

On Sat, Oct 13, 2007 at 04:45:41PM +0200, Wincent Colaiuta wrote:

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

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

Re: revised: [PATCH] Color support added to git-add--interactive.

[Dan Zwell]

[-- Attachment #1: Type: text/plain, Size: 4889 bytes --]

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.

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 971fd9f..17e29e4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..125655b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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


[-- Attachment #2: color-add--interactive.patch.gz --]
[-- Type: application/x-gzip, Size: 1749 bytes --]

Re: revised: [PATCH] Color support added to git-add--interactive.

[Shawn O. Pearce]

Dan Zwell <dzwell@gmail.com> wrote:
> Adds color to the prompts and output of git-add--interactive.

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.
 
> +color.interactive.<slot>::
> +        Use customized color for add--interactive output. `<slot>`

You probably should talk about `git add --interactive` as that
is what the git-add documentation calls it.  Many end-users don't
even know that `git add -i` is exec()'ing into another program to
accomplish its task.  I fixed this up when I applied the patch.

> +Note: these are not the same colors/attributes that the
> +rest of git supports, but are specific to git-add--interactive.

This is a problem in my opinion.  Why can't it match the same
names that the C code recognizes?  What if we one day were to
see git-add--interactive.perl converted to C?  How would we then
reconcile the color handling at that point in time?

-- 
Shawn.

Re: revised: [PATCH] Color support added to git-add--interactive.

[Dan Zwell]

On 10/16/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Dan Zwell <dzwell@gmail.com> wrote:
> > Adds color to the prompts and output of git-add--interactive.
>
> 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.

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.

>
> > +color.interactive.<slot>::
> > +        Use customized color for add--interactive output. `<slot>`
>
> You probably should talk about `git add --interactive` as that
> is what the git-add documentation calls it.  Many end-users don't
> even know that `git add -i` is exec()'ing into another program to
> accomplish its task.  I fixed this up when I applied the patch.
>
> > +Note: these are not the same colors/attributes that the
> > +rest of git supports, but are specific to git-add--interactive.
>
> This is a problem in my opinion.  Why can't it match the same
> names that the C code recognizes?  What if we one day were to
> see git-add--interactive.perl converted to C?  How would we then
> reconcile the color handling at that point in time?
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

>
> --
> Shawn.
>

Re: revised: [PATCH] Color support added to git-add--interactive.

[Shawn O. Pearce]

Dan Zwell <dzwell@gmail.com> wrote:
> 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.

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.
 
> > > +Note: these are not the same colors/attributes that the
> > > +rest of git supports, but are specific to git-add--interactive.
> >
> > This is a problem in my opinion.  Why can't it match the same
> > names that the C code recognizes?  What if we one day were to
> > see git-add--interactive.perl converted to C?  How would we then
> > reconcile the color handling at that point in time?
> 
> 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,

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.

[PATCH 1/2] Added basic color support to git add --interactive

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   37 +++++++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 971fd9f..c795a35 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..c66ed4d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

[PATCH 2/2] Let git-add--interactive read colors from git-config

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c795a35..75a976a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index c66ed4d..d85ec92 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -6,10 +6,78 @@ 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";
+	# Grab the 3 main colors in git color string format:
+	my @git_prompt_color =
+		split(/\s+/, qx(git config --get color.interactive.prompt));
+	my @git_header_color =
+		split(/\s+/, qx(git config --get color.interactive.header));
+	my @git_help_color =
+		split(/\s+/, qx(git config --get color.interactive.help));
+
+	# Sane (visible) defaults:
+	if (! @git_prompt_color) {
+		@git_prompt_color = ("blue", "bold");
+	}
+	if (! @git_header_color) {
+		@git_header_color = ("bold");
+	}
+	if (! @git_help_color) {
+		@git_help_color = ("red", "bold");
+	}
+
+	# Parse the git colors into perl colors:
+	my %attrib_mappings = (
+		"bold"    => "bold",
+		"ul"      => "underline",
+		"blink"   => "blink",
+		# not supported:
+		#"dim"     => "",
+		"reverse" => "reverse"
+	);
+
+	my @tmp_perl_colors;
+	my $color_list;
+	# Loop over the array of (arrays of) git-style colors
+	foreach $color_list ([@git_prompt_color], [@git_header_color],
+	                     [@git_help_color]) {
+		my $fg_done;
+		my @perl_attribs;
+		my $word;
+		foreach $word (@{$color_list}) {
+			if ($word =~ /normal/) {
+				$fg_done = "true";
+			}
+			elsif ($word =~ /black|red|green|yellow/ ||
+			       $word =~ /blue|magenta|cyan|white/) {
+				# is a color.
+				if ($fg_done) {
+					# this is the background
+					push @perl_attribs, "on_" . $word;
+				}
+				else {
+					# this is foreground
+					$fg_done = "true";
+					push @perl_attribs, $word;
+				}
+			}
+			else {
+				# this is an attribute, not a color.
+				if ($attrib_mappings{$word}) {
+					push(@perl_attribs,
+						 $attrib_mappings{$word});
+				}
+			}
+		}
+		if (@perl_attribs) {
+			push @tmp_perl_colors, join(" ", @perl_attribs);
+		}
+		else {
+			#@perl_attribs is empty, need a placeholder
+			push @tmp_perl_colors, "reset";
+		}
+	}
+	($prompt_color, $header_color, $help_color) =
+		@tmp_perl_colors;
 
 	require Term::ANSIColor;
 }
-- 
1.5.3.4.207.gc0ee

Re: [PATCH] resend of git-add--interactive color patch against spearce/pu

[Dan Zwell]

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

[PATCH] Let git-add--interactive read "git colors" from git-config

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 99b3817..d06f55f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 37be4b0..ca1ca28 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -6,12 +6,78 @@ 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";
+	# Grab the 3 main colors in git color string format:
+	my @git_prompt_color =
+		split(/\s+/, qx(git config --get color.interactive.prompt));
+	my @git_header_color =
+		split(/\s+/, qx(git config --get color.interactive.header));
+	my @git_help_color =
+		split(/\s+/, qx(git config --get color.interactive.help));
+
+	# Sane (visible) defaults:
+	if (! @git_prompt_color) {
+		@git_prompt_color = ("blue", "bold");
+	}
+	if (! @git_header_color) {
+		@git_header_color = ("bold");
+	}
+	if (! @git_help_color) {
+		@git_help_color = ("red", "bold");
+	}
+
+	# Parse the git colors into perl colors:
+	my %attrib_mappings = (
+		"bold"    => "bold",
+		"ul"      => "underline",
+		"blink"   => "blink",
+		# not supported:
+		#"dim"     => "",
+		"reverse" => "reverse"
+	);
+
+	my @tmp_perl_colors;
+	my $color_list;
+	# Loop over the array of (arrays of) git-style colors
+	foreach $color_list ([@git_prompt_color], [@git_header_color],
+	                     [@git_help_color]) {
+		my $fg_done;
+		my @perl_attribs;
+		my $word;
+		foreach $word (@{$color_list}) {
+			if ($word =~ /normal/) {
+				$fg_done = "true";
+			}
+			elsif ($word =~ /black|red|green|yellow/ ||
+			       $word =~ /blue|magenta|cyan|white/) {
+				# is a color.
+				if ($fg_done) {
+					# this is the background
+					push @perl_attribs, "on_" . $word;
+				}
+				else {
+					# this is foreground
+					$fg_done = "true";
+					push @perl_attribs, $word;
+				}
+			}
+			else {
+				# this is an attribute, not a color.
+				if ($attrib_mappings{$word}) {
+					push(@perl_attribs,
+						 $attrib_mappings{$word});
+				}
+			}
+		}
+		if (@perl_attribs) {
+			push @tmp_perl_colors, join(" ", @perl_attribs);
+		}
+		else {
+			#@perl_attribs is empty, need a placeholder
+			push @tmp_perl_colors, "reset";
+		}
+	}
+	($prompt_color, $header_color, $help_color) =
+		@tmp_perl_colors;
 
 	require Term::ANSIColor;
 }
-- 
1.5.3.4.207.gc0ee

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Jeff King]

On Mon, Oct 22, 2007 at 04:32:44PM -0500, Dan Zwell wrote:

> +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";

Bad indentation?

> +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;
> +	}
> +}

This does nothing for embedded newlines in the strings, which means that
you can end up with ${COLOR}text\n${RESET}, which fouls up changed
backgrounds. See commit 50f575fc. Since the strings you are printing are
small, I don't see any problem with making a copy, using a regex to
insert the color coding, and printing that (I think I even posted
example code in a previous thread on this subject).

-Peff

Re: [PATCH 2/2] Let git-add--interactive read colors from git-config

[Jeff King]

On Mon, Oct 22, 2007 at 04:40:48PM -0500, Dan Zwell wrote:

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

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

> +	# Sane (visible) defaults:
> +	if (! @git_prompt_color) {
> +		@git_prompt_color = ("blue", "bold");
> +	}

I think it might be a bit more readable to keep the assignment and
defaults together:

  my @git_prompt_color = split /\s+/,
    qx(git config --get color.interactive.prompt) || 'blue bold';

Though I wonder why we are splitting here at all, since we just end up
converting the list into a scalar below. And if we just turned that into
a function, we could get a nice:

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

-Peff

Re: [PATCH] Let git-add--interactive read "git colors" from git-config

[Jeff King]

On Mon, Oct 22, 2007 at 09:19:58PM -0500, Dan Zwell wrote:

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

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

Re: [PATCH] Let git-add--interactive read "git colors" from git-config

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> On Mon, Oct 22, 2007 at 09:19:58PM -0500, Dan Zwell wrote:
> 
> > This patch is againts Shawn Pearce's "pu" branch.
> 
> 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.

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.

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Wincent Colaiuta]

El 23/10/2007, a las 6:03, Jeff King escribió:

> This does nothing for embedded newlines in the strings, which means  
> that
> you can end up with ${COLOR}text\n${RESET}, which fouls up changed
> backgrounds. See commit 50f575fc. Since the strings you are  
> printing are
> small, I don't see any problem with making a copy, using a regex to
> insert the color coding, and printing that (I think I even posted
> example code in a previous thread on this subject).

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

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Jeff King]

On Tue, Oct 23, 2007 at 08:28:28AM +0200, Wincent Colaiuta wrote:

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

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.

> Having said that, I think this kind of function belongs in Git.pm, and the 

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

> dependency on Term::ANSIColor should be replaced with dependency-free code 
> that generates the colors itself; this should be easy because the number of 

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

-Peff

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Wincent Colaiuta]

El 23/10/2007, a las 8:41, Jeff King escribió:

> On Tue, Oct 23, 2007 at 08:28:28AM +0200, Wincent Colaiuta wrote:
>
>> 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.
>
> 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, 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

Re: [PATCH 2/2] Let git-add--interactive read colors from git-config

[Dan Zwell]

On Tue, 23 Oct 2007 00:27:02 -0400
Jeff King <peff@peff.net> wrote:

> On Mon, Oct 22, 2007 at 04:40:48PM -0500, Dan Zwell wrote:
> 
> > 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".
> 
> Why not do it as part of this patch, then?
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.

> 
> > +	# Sane (visible) defaults:
> > +	if (! @git_prompt_color) {
> > +		@git_prompt_color = ("blue", "bold");
> > +	}
> 
> I think it might be a bit more readable to keep the assignment and
> defaults together:
> 
>   my @git_prompt_color = split /\s+/,
>     qx(git config --get color.interactive.prompt) || 'blue bold';
> 
> Though I wonder why we are splitting here at all, since we just end up
> converting the list into a scalar below. And if we just turned that
> into a function, we could get a nice:
> 
>   my $prompt_color = git_color_to_ansicolor(
>     qx(git config --get color.interactive.prompt) || 'blue bold');
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

[PATCH 1/2] Added basic color support to git add --interactive

[Dan Zwell]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

From 29b34bb32846921c9432bf1b74c93d06a0667a44 Mon Sep 17 00:00:00 2001
From: Dan Zwell <dzwell@zwell.net>
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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index edf50cd..2fd783f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..16dc7b0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,36 @@
 
 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/) {
+	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");
+}
+
+sub print_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$/;
+	}
+	print $string;
+}
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -175,7 +205,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 +235,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 +574,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 +649,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 +703,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 +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.5.474.g3e4bb


[-- Attachment #2: 0001-Added-basic-color-support-to-git-add-interactive.patch --]
[-- Type: text/x-patch, Size: 4377 bytes --]

>From 29b34bb32846921c9432bf1b74c93d06a0667a44 Mon Sep 17 00:00:00 2001
From: Dan Zwell <dzwell@zwell.net>
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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index edf50cd..2fd783f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..16dc7b0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,36 @@
 
 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/) {
+	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");
+}
+
+sub print_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$/;
+	}
+	print $string;
+}
+
 sub run_cmd_pipe {
 	if ($^O eq 'MSWin32') {
 		my @invalid = grep {m/[":*]/} @_;
@@ -175,7 +205,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 +235,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 +574,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 +649,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 +703,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 +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.5.474.g3e4bb

[PATCH 2/2] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

>From ad3c6a2f015197a72d85039783a63a24c19f7017 Mon Sep 17 00:00:00 2001
From: Dan Zwell <dzwell@zwell.net>
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).

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2fd783f..ade3399 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 16dc7b0..2bce5a1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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 {
diff --git a/perl/Git.pm b/perl/Git.pm
index 3f4080c..9100e0b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -515,7 +515,6 @@ sub config {
 	};
 }
 
-
 =item config_bool ( VARIABLE )
 
 Retrieve the bool configuration C<VARIABLE>. The return value
@@ -550,6 +549,61 @@ sub config_bool {
 }
 
 
+=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");
+
+=cut
+
+sub color_to_ansi_code {
+	my ($git_string) = @_;
+	my @ansi_words;
+	my %attrib_mappings = (
+		"bold"    => "bold",
+		"ul"      => "underline",
+		"blink"   => "blink",
+		# not supported:
+		#"dim"     => "",
+		"reverse" => "reverse"
+	);
+	my ($fg_done, $word);
+
+	foreach $word (split /\s+/, $git_string) {
+		if ($word =~ /normal/) {
+			$fg_done = "true";
+		}
+		elsif ($word =~ /black|red|green|yellow/ ||
+			   $word =~ /blue|magenta|cyan|white/) {
+			# is a color.
+			if ($fg_done) {
+				# this is the background
+				push @ansi_words, "on_" . $word;
+			}
+			else {
+				# this is foreground
+				$fg_done = "true";
+				push @ansi_words, $word;
+			}
+		}
+		else {
+			# this is an attribute, not a color.
+			if ($attrib_mappings{$word}) {
+				push(@ansi_words,
+					 $attrib_mappings{$word});
+			}
+		}
+	}
+	require Term::ANSIColor;
+	return Term::ANSIColor::color(join(" ", @ansi_words)||"reset");
+}
+
 =item ident ( TYPE | IDENTSTR )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
-- 
1.5.3.5.474.g3e4bb

Re: *[PATCH 2/2] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

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

Re: *[PATCH 2/2] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

On Fri, 02 Nov 2007 22:06:08 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Dan Zwell <dzwell@zwell.net> writes:
> 
> 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.
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>
> 
> 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.
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.)

> 
> ---
> 
>  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 {
> 
In a previous incantation of this thread, coloring the diff output was
discussed. Your patch works, I tested it, but it does not highlight
whitespace at the end of lines or space/tab errors. If this is the only
case that more than one color may appear per line, it should not be
hard to match it as a special case (assuming this check isn't disabled
in .gitconfig), and print the rest of the line as we otherwise would.

Dan

Re: *[PATCH 2/2] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> In addition to a small matter of testing, a more practical issue
>> would be to add PAGER support there, I think.
>
> 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.

> ... but it does not highlight
> whitespace at the end of lines or space/tab errors.

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.

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Jeff King]

On Fri, Nov 02, 2007 at 10:41:00PM -0500, Dan Zwell wrote:

> +sub print_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$/;
> +	}
> +	print $string;
> +}

This would probably be a bit more readable by marking the regex as
multline using /m. Something like:

  $string =~ s/^/$color/mg;
  $string =~ s/.$/$&$normal_color/mg;

which covers both the "start/end of line" and "start/end" of string
cases.

Also, if there is to be pager support for showing diffs, perhaps
print_colored needs to take a filehandle argument (or, even simpler,
change "print_colored(...)" to "print color(...), so the caller can use
print as usual).

-Peff

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Nov 02, 2007 at 10:41:00PM -0500, Dan Zwell wrote:
>
>> +sub print_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$/;
>> +	}
>> +	print $string;
>> +}
>
> This would probably be a bit more readable by marking the regex as
> multline using /m. Something like:
>
>   $string =~ s/^/$color/mg;
>   $string =~ s/.$/$&$normal_color/mg;
>
> which covers both the "start/end of line" and "start/end" of string
> cases.

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.

> Also, if there is to be pager support for showing diffs, perhaps
> print_colored needs to take a filehandle argument (or, even simpler,
> change "print_colored(...)" to "print color(...), so the caller can use
> print as usual).

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);
	}

Re: [PATCH 1/2] Added basic color support to git add --interactive

[Jeff King]

On Sat, Nov 03, 2007 at 10:36:00PM -0700, Junio C Hamano wrote:

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

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

[PATCH 0/3] Adding colors to git-add--interactive

[Dan Zwell]

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

[PATCH 1/3] Added basic color support to git add --interactive

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    6 ++++++
 git-add--interactive.perl |   44
++++++++++++++++++++++++++++++++++++++------ 2 files changed, 44
insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8d5d200..3712d6a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..f2b0e56 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

[PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    7 +++++
 git-add--interactive.perl |   19 ++++++++++-----
 perl/Git.pm               |   55
++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 74
insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3712d6a..47c1ab2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f2b0e56..508531f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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");
 	}
 }
 
diff --git a/perl/Git.pm b/perl/Git.pm
index dca92c8..c9e661a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -515,7 +515,6 @@ sub config {
 	};
 }
 
-
 =item config_bool ( VARIABLE )
 
 Retrieve the bool configuration C<VARIABLE>. The return value
@@ -550,6 +549,60 @@ sub config_bool {
 }
 
 
+=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");
+
+=cut
+
+sub color_to_ansi_code {
+	my ($git_string) = @_;
+	my @ansi_words;
+	my %attrib_mappings = (
+		"bold"    => "bold",
+		"ul"      => "underline",
+		"blink"   => "blink",
+		# not supported:
+		#"dim"     => "",
+		"reverse" => "reverse"
+	);
+	my ($fg_done, $word);
+
+	foreach $word (split /\s+/, $git_string) {
+		if ($word =~ /normal/) {
+			$fg_done = "true";
+		}
+		elsif ($word =~ /black|red|green|yellow/ ||
+			   $word =~ /blue|magenta|cyan|white/) {
+			# is a color.
+			if ($fg_done) {
+				# this is the background
+				push @ansi_words, "on_" . $word;
+			}
+			else {
+				# this is foreground
+				$fg_done = "true";
+				push @ansi_words, $word;
+			}
+		}
+		else {
+			# this is an attribute, not a color.
+			if ($attrib_mappings{$word}) {
+				push(@ansi_words,
+					 $attrib_mappings{$word});
+			}
+		}
+	}
+	return Term::ANSIColor::color(join(" ", @ansi_words)||"reset");
+}
+
 =item ident ( TYPE | IDENTSTR )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
-- 
1.5.3.5.565.gf0b83-dirty

[PATCH 3/3] Added diff hunk coloring to git-add--interactive

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 git-add--interactive.perl |   58 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 508531f..d92e8ed 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

[PATCH 0/3] Adding colors to git-add--interactive

[Dan Zwell]

[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

Subject: [PATCH 1/3] Added basic color support to git add --interactive

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8d5d200..3712d6a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..f2b0e56 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3712d6a..47c1ab2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f2b0e56..508531f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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");
 	}
 }
 
diff --git a/perl/Git.pm b/perl/Git.pm
index dca92c8..c9e661a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -515,7 +515,6 @@ sub config {
 	};
 }
 
-
 =item config_bool ( VARIABLE )
 
 Retrieve the bool configuration C<VARIABLE>. The return value
@@ -550,6 +549,60 @@ sub config_bool {
 }
 
 
+=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");
+
+=cut
+
+sub color_to_ansi_code {
+	my ($git_string) = @_;
+	my @ansi_words;
+	my %attrib_mappings = (
+		"bold"    => "bold",
+		"ul"      => "underline",
+		"blink"   => "blink",
+		# not supported:
+		#"dim"     => "",
+		"reverse" => "reverse"
+	);
+	my ($fg_done, $word);
+
+	foreach $word (split /\s+/, $git_string) {
+		if ($word =~ /normal/) {
+			$fg_done = "true";
+		}
+		elsif ($word =~ /black|red|green|yellow/ ||
+			   $word =~ /blue|magenta|cyan|white/) {
+			# is a color.
+			if ($fg_done) {
+				# this is the background
+				push @ansi_words, "on_" . $word;
+			}
+			else {
+				# this is foreground
+				$fg_done = "true";
+				push @ansi_words, $word;
+			}
+		}
+		else {
+			# this is an attribute, not a color.
+			if ($attrib_mappings{$word}) {
+				push(@ansi_words,
+					 $attrib_mappings{$word});
+			}
+		}
+	}
+	return Term::ANSIColor::color(join(" ", @ansi_words)||"reset");
+}
+
 =item ident ( TYPE | IDENTSTR )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
-- 
1.5.3.5.565.gf0b83-dirty

Re: [PATCH 0/3] Adding colors to git-add--interactive

[Jeff King]

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

Re: [PATCH 0/3] Adding colors to git-add--interactive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 0/3] Adding colors to git-add--interactive

[Dan Zwell]

Junio C Hamano wrote:
> 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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

> @@ -8,12 +9,18 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
>  	eval { require Term::ANSIColor; };
>  	if (!$@) {
>  		$use_color = 1;
> +		# 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");

Makes me wonder if you are better off with two new helper
functions defined in Git.pm, as in:

	$prompt_color = $repo->config_color("interactive.prompt") || "bold blue")
	$normal_color = Git::color_to_ansi_code("normal");

> +sub color_to_ansi_code {
> +	my ($git_string) = @_;
> +	my @ansi_words;
> +	my %attrib_mappings = (
> +		"bold"    => "bold",
> +		"ul"      => "underline",
> +		"blink"   => "blink",
> +		# not supported:
> +		#"dim"     => "",
> +		"reverse" => "reverse"
> +	);

I do not like a hash variable name that says it is a "mapping".
It being a hash is enough indication that it is a mapping.

You are better off naming such a mapping as if it is a function
that takes one parameter (in this case, git name) and returns a
single value (perl name).  So I'd probably say:

	my %perl_attrib = ( ... );

(or "git_attrib_to_perl").  A use site of such a hash would look
like this:

	push @perl_words, $perl_attrib{'ul'};
        push @perl_names, $git_attrib_to_perl{'blink'};

Maybe it is just me, but aren't these easier to read?

> +	my ($fg_done, $word);
> +
> +	foreach $word (split /\s+/, $git_string) {
> +		if ($word =~ /normal/) {

	$word eq 'normal' ?

> +			$fg_done = "true";
> +		}
> +		elsif ($word =~ /black|red|green|yellow/ ||
> +			   $word =~ /blue|magenta|cyan|white/) {

	exists $color_name{$word}

with

	my %color_name = map { $_ => 1 } qw(black red ... white);

at the beginning?

> +			# is a color.
> +			if ($fg_done) {
> +				# this is the background
> +				push @ansi_words, "on_" . $word;
> +			}
> +			else {
> +				# this is foreground
> +				$fg_done = "true";
> +				push @ansi_words, $word;
> +			}
> +		}
> +		else {
> +			# this is an attribute, not a color.
> +			if ($attrib_mappings{$word}) {

	exists $git_attrib_to_perl{$word}

?

Re: [PATCH 3/3] Added diff hunk coloring to git-add--interactive

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

> +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;
> +		}

It would be better to do the "if (!$diff_use_color)" part
upfront before entering the loop, wouldn't it?

	sub colored_diff_hunk {
        	my ($text) = @_;
		if (!$diff_use_color) {
	                return @$text;
		}

                my @ret;
                for (@$text) {
                	...
		}
	}

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Makes me wonder if you are better off with two new helper
> functions defined in Git.pm, as in:
>
> 	$prompt_color = $repo->config_color("interactive.prompt") || "bold blue")
> 	$normal_color = Git::color_to_ansi_code("normal");

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.

Re: Subject: [PATCH 1/3] Added basic color support to git add --interactive

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

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

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.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8d5d200..3712d6a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -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.
> +

I think "auto" should disable colors even when the output is "-t
STDOUT" if $TERM eq "dumb" (see color.c::git_config_colorbool()).

> @@ -175,7 +207,7 @@ sub list_and_choose {
>  			if (!$opts->{LIST_FLAT}) {
>  				print "     ";
>  			}
> -			print "$opts->{HEADER}\n";
> +			print colored $header_color, "$opts->{HEADER}\n";

I agree with Jeff's suggestion to stick to more C-ish style to
always use parentheses around function arguments.

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

Junio C Hamano wrote:
>> +			$fg_done = "true";
>> +		}
>> +		elsif ($word =~ /black|red|green|yellow/ ||
>> +			   $word =~ /blue|magenta|cyan|white/) {
> 
> 	exists $color_name{$word}
> 
> with
> 
> 	my %color_name = map { $_ => 1 } qw(black red ... white);
> 
> at the beginning?
> 
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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Dan Zwell <dzwell@gmail.com> writes:

> Junio C Hamano wrote:
>>> +			$fg_done = "true";
>>> +		}
>>> +		elsif ($word =~ /black|red|green|yellow/ ||
>>> +			   $word =~ /blue|magenta|cyan|white/) {
>>
>> 	exists $color_name{$word}
>>
>> with
>>
>> 	my %color_name = map { $_ => 1 } qw(black red ... white);
>>
>> at the beginning?
>>
> 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.

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?

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

Junio C Hamano wrote:
> 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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Jeff King]

On Mon, Nov 12, 2007 at 08:55:13PM -0600, Dan Zwell wrote:

> 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 

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.

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

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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Junio C Hamano]

Dan Zwell <dzwell@gmail.com> writes:

> ... I wrote this with the intent
> that it should ignore invalid color names, but it would probably be
> more useful to print a warning.

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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Dan Zwell]

Junio C Hamano wrote:
> 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

Re: Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig

[Jakub Narebski]

Dan Zwell wrote:

> Junio C Hamano wrote:
>> 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),

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

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH 0/5] Colors for git-add--interactive

[Dan Zwell]

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

[PATCH 1/5] Added basic color support to git add --interactive

[Dan Zwell]

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.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    6 ++++
 git-add--interactive.perl |   66 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8d5d200..3712d6a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index ac598f8..2b5559f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,6 +1,58 @@
 #!/usr/bin/perl -w
 
 use strict;
+use Git;
+
+my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
+
+{
+	# set color options:
+	my $repo = Git->repository();
+	my $color_config = $repo->config('color.interactive');
+	$use_color = 0;
+	if (!defined $color_config) {
+		$use_color = 0;
+	}
+	elsif ($color_config =~ /true|always/) {
+		$use_color = 1;
+	}
+	elsif ($color_config eq 'auto' && -t STDOUT &&
+		   $ENV{'TERM'} ne 'dumb') {
+		$use_color = 1;
+	}
+
+	if ($use_color) {
+		eval { require Term::ANSIColor; };
+		if ($@) {
+			# library did not load.
+			$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');
+		}
+	}
+}
+
+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') {
@@ -175,7 +227,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 +257,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 +596,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 +671,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,8 +725,8 @@ sub patch_update_cmd {
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT});
 				if (1 < @split) {
-					print "Split into ",
-					scalar(@split), " hunks.\n";
+					print colored($header_color, "Split into ",
+						scalar(@split), " hunks.\n");
 				}
 				splice(@hunk, $ix, 1,
 				       map { +{ TEXT => $_, USE => undef } }
@@ -766,7 +818,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

[PATCH 2/5] Don't return 'undef' in case called in a vector context.

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
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(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index dca92c8..6603762 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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

[PATCH 3/5] Added config_default($key, $default) to Git.pm

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 perl/Git.pm |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 6603762..7327300 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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

[PATCH 4/5] Let git-add--interactive read colors from configuration

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 Documentation/config.txt  |    7 ++++
 git-add--interactive.perl |   20 ++++++++---
 perl/Git.pm               |   80 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3712d6a..47c1ab2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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).
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 2b5559f..f76f008 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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');
 		}
 	}
 }
diff --git a/perl/Git.pm b/perl/Git.pm
index 7327300..18ef6b4 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -515,7 +515,6 @@ sub config {
 	};
 }
 
-
 =item config_bool ( VARIABLE )
 
 Retrieve the bool configuration C<VARIABLE>. The return value
@@ -578,6 +577,85 @@ sub config_default {
 	}
 }
 
+=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");
+
+color_to_ansi_code('') returns the empty string, and should do
+nothing when printed.
+
+=cut
+
+sub color_to_ansi_code {
+	my ($git_string) = @_;
+	my @ansi_words;
+	my %git_to_perl_color = (
+		'bold'    => 'bold',
+		'ul'      => 'underline',
+		'blink'   => 'blink',
+		'reverse' => 'reverse'
+		# not supported by Term::ANSIColor:
+		#'dim'     => ''
+	);
+	my %valid_color = map { $_ => 1 } qw(black red green yellow
+					    blue magenta cyan white);
+
+	my ($fg_done, $token);
+	foreach $token (split /\s+/, $git_string) {
+		$token = lc($token);
+
+		if ($token eq 'normal') {
+			$fg_done = 1;
+		}
+		elsif (exists $valid_color{$token}) {
+			# is a color.
+			if ($fg_done) {
+				# this is the background
+				push @ansi_words, 'on_' . $token;
+			}
+			else {
+				# this is foreground
+				$fg_done = 1;
+				push @ansi_words, $token;
+			}
+		}
+		else {
+			# this is an attribute, not a color.
+			if ($git_to_perl_color{$token}) {
+				push(@ansi_words,
+					 $git_to_perl_color{$token});
+			}
+			else {
+				print STDERR 'Warning: bad color or attribute: ';
+				print STDERR "\"$token\". Check git configuration.\n";
+			}
+		}
+	}
+
+	# decide what to return--return color codes, 'clear' code, or
+	# the empty string, depending on the input we were passed /
+	# what we have processed:
+	if (@ansi_words) {
+		return Term::ANSIColor::color(join(' ', @ansi_words));
+	}
+	else {
+		if ($fg_done) {
+			# the git attrib 'normal' was processed
+			return Term::ANSIColor::color('clear');
+		}
+		else {
+			return '';
+		}
+	}
+}
+
 =item ident ( TYPE | IDENTSTR )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
-- 
1.5.3.5.565.gf0b83-dirty

[PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Dan Zwell]

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

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 git-add--interactive.perl |   93 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 72 insertions(+), 21 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f76f008..ba9430c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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

Re: [PATCH 0/5] Colors for git-add--interactive

[Jeff King]

On Thu, Nov 22, 2007 at 04:54:37AM -0600, Dan Zwell wrote:

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

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

Re: [PATCH 2/5] Don't return 'undef' in case called in a vector context.

[Jeff King]

On Thu, Nov 22, 2007 at 04:55:34AM -0600, Dan Zwell wrote:

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

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

Re: [PATCH 3/5] Added config_default($key, $default) to Git.pm

[Jeff King]

On Thu, Nov 22, 2007 at 04:55:52AM -0600, Dan Zwell wrote:

> 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

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?

> +in situations where the empty string is an acceptable return value.
> +This method may also be called in a vector context, when expecting

The term "vector context" is not commonly used. Most of the perl
documentation calls it "list context" (try googling for each and seeing
the hit numbers).

-Peff

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Jeff King]

On Thu, Nov 22, 2007 at 04:56:06AM -0600, Dan Zwell wrote:

> +			# 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'));

And by the same token as the last message, given that config_* take only
two arguments, is there a reason not to extend them so that

  $repo->config_bool('my.key', 0);

handles the default. Then I think you could simplify this to just:

  $repo->config_color('color.interactive.prompt', 'bold blue');

and hide the color_to_ansi_code messiness from the script altogether.

-Peff

Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Jeff King]

On Thu, Nov 22, 2007 at 04:56:24AM -0600, Dan Zwell wrote:

> -		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'));

These were just added in the last patch. I know sometimes it is worth
showing the progression of work as the patches go, but in this case, I
think it is simpler for the reviewers if the first patch which adds a
chunk of code does it in the final way (even if you need to just say "I
did it this way because there will be reasons later on.").

> +	sub get_color {
> +		my ($key, $default) = @_;
> +		return Git::color_to_ansi_code(
> +			scalar $repo->config_default($key, $default));
> +	}

Ah, so you agree that this is a good route. I think this should probably
be Git::config_color.

There is also a subtle issue, which is that it pulls the "$repo"
variable from the outer lexical scope (as Git::config_color, it would
take it as the first parameter).

> +		$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');

Yeah, much nicer to read.

> +	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');

Unfortunately, there is a historical wart that probably still needs
supporting, which is that the original names were diff.color.*. Or have
we officially removed support for that yet?

-Peff

Re: [PATCH 0/5] Colors for git-add--interactive

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

> My impression is that diff --cc is called any time
> that HEAD has two parents. Is this correct?

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.

Re: [PATCH 2/5] Don't return 'undef' in case called in a vector context.

[Junio C Hamano]

Dan Zwell <dzwell@zwell.net> writes:

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

Shouldn't the same fix made to config_bool as well?

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 22, 2007 at 04:56:06AM -0600, Dan Zwell wrote:
>
>> +			# 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'));
>
> And by the same token as the last message, given that config_* take only
> two arguments, is there a reason not to extend them so that
>
>   $repo->config_bool('my.key', 0);
>
> handles the default. Then I think you could simplify this to just:
>
>   $repo->config_color('color.interactive.prompt', 'bold blue');
>
> and hide the color_to_ansi_code messiness from the script altogether.

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

Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +	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');
>
> Unfortunately, there is a historical wart that probably still needs
> supporting, which is that the original names were diff.color.*. Or have
> we officially removed support for that yet?

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.

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Jeff King]

On Thu, Nov 22, 2007 at 01:28:34PM -0800, Junio C Hamano wrote:

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

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.

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

Yes, this is obviously the most important thing.

>  (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"?

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

Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 22, 2007 at 04:56:24AM -0600, Dan Zwell wrote:
>
>> -		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'));
>
> These were just added in the last patch. I know sometimes it is worth
> showing the progression of work as the patches go, but in this case, I
> think it is simpler for the reviewers if the first patch which adds a
> chunk of code does it in the final way (even if you need to just say "I
> did it this way because there will be reasons later on.").

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.

Re: [PATCH 2/5] Don't return 'undef' in case called in a vector context.

[Dan Zwell]

Junio C Hamano wrote:
> Dan Zwell <dzwell@zwell.net> writes:
> 
>> diff --git a/perl/Git.pm b/perl/Git.pm
>> index dca92c8..6603762 100644
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -508,7 +508,7 @@ sub config {
>>  		my $E = shift;
>>  		if ($E->value() == 1) {
>>  			# Key not found.
>> -			return undef;
>> +			return;
>>  		} else {
>>  			throw $E;
>>  		}
> 
> Shouldn't the same fix made to config_bool as well?
> 

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

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Dan Zwell]

Jeff King wrote:
>>  (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"?
> 
> 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).
> 

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

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Jeff King]

On Thu, Nov 22, 2007 at 11:32:16PM -0600, Dan Zwell wrote:

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

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

Re: [PATCH 4/5] Let git-add--interactive read colors from configuration

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

[Jeff King]

On Thu, Nov 22, 2007 at 01:37:55PM -0800, Junio C Hamano wrote:

> > Unfortunately, there is a historical wart that probably still needs
> > supporting, which is that the original names were diff.color.*. Or have
> > we officially removed support for that yet?
> 
> 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.

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.

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

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

-Peff