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...
[ message continues ]

Hi,
[Cc'ed Wincent correctly]
Good.  If you do not have color enabled, it does not require that package 
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
-

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

-

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/

-

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

+			...
[ message continues ]

Hi,
FWIW, this is what Documentation/config.txt has to say about it:
color.diff.<slot>::

        Use customized color for diff colorization.  `<slot>` specifies

        which part of the patch to use the specified color, and is one

        of `plain` (context text), `meta` (metainformation), `frag`

        (hunk header), `old` (removed lines), `new` (added lines),

        `commit` (commit headers), or `whitespace` (highlighting dubious

        whitespace).  The values of these variables may be specified as

        in color.branch.<slot>.
Hth,

Dscho
-

Hi!
I really like the idea of colorizing git add -i, especially the

prompt.  Here are my two cents.
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) = @_;
Why use printf when you could directly use print here?  It's only
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
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

-

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  
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  
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  
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  
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 p...
[ message continues ]

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

-

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

-

I believe there are other places where the diff output is parsed, and

the colors will mess that up, too (e.g., split_hunk). All of those

regexes need to be changed, too. I am a bit concerned that we are

putting intimate knowledge of the colorization scheme here. As much as

it pains me to have two diff colorizers, I wonder if that would be a

better solution than having a diff colorizer, and a colorized diff

parser.
-Peff

-

Oops, I see you actually dealt with those already (I just responded to

your cover letter first). Though I am still concerned about the

robustness of the re-parsing scheme.
-Peff

-

The importance of the diff coloring pales in comparison to the prompt

coloring. Diff coloring is useful, but prompt coloring is a basic

usability concern (if people can't easily tell where a hunk begins, the

tool becomes annoying). Perhaps we could split this into two patches,

merging the first after a few small changes can be taken care of, while

the second may need more discussion and testing. The coloring of the

prompts is relatively low risk. It just needs to be modified to take

color settings from .git/config. I was thinking that this might be the

example that I would take settings from:
[color]

         add-interactive = auto

[color "add-interactive"]

         prompt = bold blue

         header = bold

         help = blue
For the sake of a unified interface, the "Stage this hunk?" prompt

should be colored the same as the other prompts. I will give a bit of

thought to the default colors, though it's important to avoid red and

green (as those will look like diff output when the second patch is

applied).
Also needed is some command line parsing so that "--color" can be

specified on the command line (very small change), and all of this

should be added to the documentation.
Obviously, the suggestions/fixes from other parts of this thread must be

taken into account, as well. I can probably do all this tomorrow (and

send low-risk/high-risk patches), unless someone takes it before me.
Dan

-

Or could you just piggy-back on the settings for color.diff.<slot>?
And if a separate group for git-add is necessary, perhaps "add" would

be enough, rather than "add-interactive".
Wincent
-

I think color.add is better, because git-add--interactive goes beyond

coloring diffs. When this is complete, it should probably use

color.diff.<slot> for the actual diff output, and color.add.<slot> for

colored prompts/commands.
Dan

-

Or maybe rather color.interactive.<slot>, where <slot> could be

'prompt', 'header', etc.  It's better to give it a name that

describes what it is for, instead of one that describes which tool

uses it.  This way it could possibly be used for other potential

interactive tools in the future.
jlh

-

Yes, I think it is worth splitting into two patches, here. There seems

to be orthogonal discussion on (colorizing and configuration of prompts versus

how to handle colorized diffs).
-Peff

-

Adds color to the prompts and output of git-add--interactive.
-Reads config color.interactive, respects "auto", "true",

"always", and anything else.

-Uses the library Term::ANSIColor, which is included with modern

 versions of perl. This is optional, and should not need to be

 present if color.interactive is not on.

-Reads color.interactive.<slot>, where slot is "header", "prompt",

 or "help", colorizing output accordingly.
Documentation/config.txt is updated to reflect the new keys.

I cannot test this or see how it looks in manpages, however,

as I cannot install the documentation build tools.
Unfortunately, I think the default colors are ugly, but all colors that

are readable on both black and white backgrounds are probably ugly.
This patch does not colorize the diffs, because that is a larger

job, and very distinct from this (simple) task.
Dan
gzipped patch is also attached, just in case.
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_...
[ message continues ]

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

-

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

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,

-

I really should have pointed you to Documentation/SubmittingPatches

when I responded to your email in the first place.  Sorry I didn't
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.

-

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 = ...
[ message continues ]

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

-

On Tue, 23 Oct 2007 00:27:02 -0400

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

I agree, now that you mention it. Eventually the string must be split

(parsing it left to right by word makes more sense than trying to

mutate it with regular expressions, if only because it's a lot harder

to make mistakes), but there's no reason not to split the string inside

the loop, where it would look nicer/more contained. I will make this

change.
Dan

-

From 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...
[ message continues ]

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

        ...
[ message continues ]

On Fri, 02 Nov 2007 22:06:08 -0700

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

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

-

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

-

=46rom 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>.

=20

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

=20

 use strict;

=20

+my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);

+my $color_config =3D qx(git config --get color.interactive);

+if ($color_config=3D~/tru...
[ message continues ]

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

-

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,
Making it take a FH would be useful.  With that, my

proof-of-concept patch to add print_diff_hunk would become:
	sub print_diff_hunk {

        	my ($text) = @_;

		my $pager;
                if ($use_pager) {

	                open($pager, "| less");

		} else {

                	$pager = \*STDOUT;

		}

                for (@$text) {

			print_colored $pager $color ...

		}

                close($pager);

	}

-

Yes, though I wonder if the former is "more correct" in the sense that

we don't know what the attributes are doing, and maybe it matters for

them to apply to each line, whether it has text or not.
But I don't think it's possible for a blank line to actually do anything

with the attributes we're currently allowing, and we don't have any

plans to allow arbitrary terminal control codes in the color specs, so

it probably doesn't matter.
-Peff

-

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...
[ message continues ]

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")
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'};
	exists $color_name{$word}
with
	my %color_name = map { $_ => 1 } qw(black red ... white);
	exists $git_attrib_to_perl{$word}
?

-

Sorry, but please disregard.  "bold blue" part was also

parameter to the string-to-ansi-color-escape function, so the

above does not make much sense.
-

I don't see the advantage of doing it that way. After all, we're pattern

matching. Does using a hash, an array, and a call to map() gain us

something? I think a regular expression is clearer. Of course, as Jeff

pointed out, I should have used a whitespace-agnostic regular expression.

+		elsif ($word =~ /black|red|green|yellow|

+			blue|magenta|cyan|white/x ) {
I agreed with the rest of your suggestions, and will implement them in

the next round of changes, later this week.
Dan
-

I suggested the hash approach only because (1) it is easier to

read than two regexp matches that are split only to keep the

line less than 80-chars long, and (2) a misconfiguration like

"color.foo = fred" can be caught more easily.
I do not quite understand the "after all, we're pattern

matching" part, though.  Are you talking about "split(/\s+/, $str)"

your for-loop iterates over?
-

I think we're talking about the same thing. I was referring to the split

regular expression, and the question is, "for the current element of

split(/\s+/, $str), does it match a color?"
Anyway, I preferred the regex version for readability, though I should

have used the /x modifier--it would still take two lines, but it would

not need to attempt two matches. As for misconfigured color

configurations, should we catch that? I wrote this with the intent that

it should ignore invalid color names, but it would probably be more

useful to print a warning.
Dan

-

Your regex is wrong, because it doesn't anchor at the beginning and end

of the string (so /red/ will match "supercaliredfragilistic", which is

probably not what you want). So you probably want /^red$/, which is

equivalent to using 'eq' with 'red'. Or, as Junio noted, you are overall

trying to say "is element $word in this list"; the canonical perl way of
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

-

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::ANSICol...
[ message continues ]

[Note: I'm resending this because it looks like this e-mail didn't go

out properly. Sorry for duplicates.]
A bit of a recap--this feature was

requested by a user a few weeks ago, and I wrote a short patch that

implemented partial coloring. The main criticisms of this patch were:

- The name of the configuration key to turn on interactive coloring

was not well chosen.

- The color attribute synonyms "clear" and "reset" were used

interchangeably, though "reset" is what the rest of git uses.

- The colors were not user-settable.
When the above were fixed, the new patch garnered the following

criticism:

- The color names accepted in .gitconfig were perl color names (to be

fed to Term::ANSIColor), and this was not consistent with the rest

of git. For example, "red blue ul" would have to be written, "red

on_blue underline".
Fixing this (the colors could not be converted by a regex, but had to

be parsed) was very libraryish, and also a little confusing. I was

given a suggestion or two about how to make it more readable. This

parsing also needed to be added to Git.pm instead of

git-add--interactive.perl.
In the next iteration, all of the above were fixed, but as Jeff King

pointed out, I was printing $color before the beginning of a string,

and printing $clear at the end, which allowed ${COLOR}text\n${RESET},

which looks bad on some terminals.
Last iteration, it was pointed out that print_colored must take a

file handle to pave the way for pager support, and Junio Hamano and Jeff

King both gave suggestions as to how, and Junio sent me few changes

that allow printing colored diffs in addition to prompts. I ended up

making changes so that the two color functions can be used with perl's

print():

print colored($color, "some text")

print color_diff_hunk($hunk)
This way, file handles can be added directly to the print calls, when

support for $PAGER is added.
Let me know if other things need correction.
Dan

-

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

 }
...
[ message continues ]

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

                	...

		}

	}
-

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...
[ message continues ]

A bit of a recap--this feature was requested by a user a few weeks

ago, and I wrote a short patch that implemented partial coloring. The

main criticisms of this patch were:

- The name of the configuration key to turn on interactive coloring

was not well chosen.

- The color attribute synonyms "clear" and "reset" were used

interchangeably, though "reset" is what the rest of git uses.

- The colors were not user-settable.
When the above were fixed, the new patch garnered the following

criticism:

- The color names accepted in .gitconfig were perl color names (to be

fed to Term::ANSIColor), and this was not consistent with the rest

of git. For example, "red blue ul" would have to be written, "red

on_blue underline".
Fixing this (the colors could not be converted by a regex, but had to

be parsed) was very libraryish, and also a little confusing. I was

given a suggestion or two about how to make it more readable. This

parsing also needed to be added to Git.pm instead of

git-add--interactive.perl.
In the next iteration, all of the above were fixed, but as Jeff King

pointed out, I was printing $color before the beginning of a string,

and printing $clear at the end, which allowed ${COLOR}text\n${RESET},

which looks bad on some terminals.
Last iteration, it was pointed out that print_colored must take a

file handle to pave the way for pager support, and Junio Hamano and Jeff

King both gave suggestions as to how, and Junio sent me few changes

that allow printing colored diffs in addition to prompts. I ended up

making changes so that the two color functions can be used with perl's

print():

print colored($color, "some text")

print color_diff_hunk($hunk)
This way, file handles can be added directly to the print calls, when

support for $PAGER is added.
Let me know if other things need correction.
Dan

-

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

-		...
[ message continues ]

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

-

If you are suggesting to reorganize the series like this:
 1/5 Fix to Git.pm for list context;
 2/5 Enhance Git.pm to allow config() methods to take default values;
 3/5 Enhance Git.pm with get_color() method;
 4/5 Teach git-add--interactive to read color settings from the

     config;
 5/5 Paint output from git-add--interactive in colors, including

     prompt, help and diff hunks.
I think that makes a very good sense.  The earlier part of the

series would be independent from colorization of "add -i" and

can go in before everything else to allow other potential users,

e.g. "git remote --color" ;-).  I do not see a strong reason to

have the separate "Basic color support with hardcoded color" at

the beginning, either.
I think it is a matter of taste to either:
 (1) Squash 4 and 5 in the above list into one; or
 (2) Split 5 into separate commits to color different parts.
Perhaps the former would be simpler and more appropriate for

this series.

-

Neither officially or unofficially yet, but we can start the

process of making it official with an early announcement.  I do

not think we would hurt people as long as a long enough advance

notice is given.
I however am wondering if we need to have so many "enable color

support" switches.  color.status, color.diff, and now yet

another color.interactive?  Who sets color.status and/or

color.interactive to auto without setting color.diff to auto as

well?
It may be good that they _can_ be individually controlled, but I

strongly suspect that most people would just want to set a

single variable color.ui to "auto", and have it give the default

value for all the color.$cmd configuration variable that are not

explicitly defined.

-

Andy Parkins added color.* (and removed documentation for *.color)

almost a year ago (in a159ca0c). But I don't think there has been an
Yes, I have often thought this, as well, and a "color.all" would

probably be convenient.
-Peff

-

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 ...
[ message continues ]

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

-

I like the config_color() method.
I think the "config_bool with default" also makes sense but it

needs to be coded a bit carefully.  Issues to consider:
 (1) Non default form "$r->config_bool('key')" should keep the

     original semantics; missing key in the configuration is the

     same as false (i.e. "undef" in scalar, () in list context).
 (2) What should be the second parameter in the form to default

     to true?  '1'?  'true'?  Any kind of "true" value in Perl

     should be accepted?
 (3) Same question as (2) but for defaulting to false.  Any kind

     of "false"?

-

Yes. It is not strictly necessary for this patch series, but I think it

is nice to stake out a claim on the third argument of config_* functions

for consistency sake. But perhaps in the name of avoiding regression, it
Hmm. I am tempted to say "yes, any true or any false value" in that the

point of config_* is to convert git config values to native perl

representations. OTOH, the moral equivalent of
  config_color('my.key', 'bold red');
is probably more appropriately
  config_bool('my.key', 'true');
so I am fine doing it that way, as well (though I think it makes us

duplicate the "translate these strings into bools" code into perl).
-Peff

-

As you said, config_* converts git values to perl values. However, that

conversion needs only be done for strings in .gitconfig. Is there any

reason why the caller of the function would need to pass a string

"false"? I just don't see the need for conversion of any kind.
Further, I think that we could return the default variable directly,

without parsing it at all. It would be much simpler, and there would

need to be no special cases for dealing with undef or 'false'. It's a

perl function, being called with perl arguments, so a user should not be

that surprised when 'false' does what perl says it should do.
Dan

-

I think that is more elegant for config_bool, but it means that

config_bool and config_color have slightly different behaviors (the

difference being that it is easy to feed a native perl value as the

default to config_bool, but to get the same behavior for config_color,

you would call Git::color_to_ansi_code manually, which is a pain).
In this instance, I am inclined to sacrifice consistency for convenience

and make it:
   my $bool = config_bool('my.key', 0);

   my $color = config_color('my.key', 'bold red');
Note that there is one tricky part of config_bool, which is what

config_bool('my.key', undef) should do (is it "default false" or "no

default"?).
-Peff

-

I am glad somebody finally got to the trick question I posed

earlier ;-)
But config_bool('key') and config_bool('key', undef) would both

return undef to say "The value is false" when key does not

exist, so it was not much of a trick.  It does not make a

difference if the undef came because the default parameter was

undef, or because there was no default parameter given and the

built-in behaviour of config_bool() was to return undef for a

missing key.
-

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
-

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

-

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
-

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

-

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

-

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;

+	}

...
[ message continues ]

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

-

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.

-

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

-

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.
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
This also seems like a candidate for lib-ification in Git.pm, alongside
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.
It is much more common (and proper OO, in the face of inheritance) to

use
Yay, documentation!  It would also be nice to have a test script that
Why not? I don't especially care about "dim" support, but if there is a
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
Style: whitespace around ||
Personally I would have just excluded the parsing, since it isn't
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
Now this was a surprise after reading the commit message.
-Peff

-

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

-

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

+		prin...
[ message continues ]

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

-

I did too, where you add a third, optional "trailer" parameter to the

function where you pass the newline if there is one (following the

style of the functions in color.c). Pasting it below.
Having said that, I think this kind of function belongs in Git.pm,

and the dependency on Term::ANSIColor should be replaced with

dependency-free code that generates the colors itself; this should be

easy because the number of possible colors is small, Git thus far

only uses a subset of the possible ANSI colors, and the C code for

doing it is already there in color.c and just needs to be translated

into Perl.
sub print_ansi_color {

	my $color = shift;

	my $string = shift;

	my $trailer = shift;

	if ($use_color) {

		print Term::ANSIColor::color($color), $string,

		    Term::ANSIColor::color('clear');

	} else {

		print $string;

	}

	if ($trailer) {

		print $trailer;

	}

}
Cheers,

Wincent
-

The problem with that approach is that you can only send in a single

line at a time (with the newline detached!), so it makes life harder for

the caller. E.g., there is at least one spot that uses a here-doc with

many lines; splitting that into a bunch of print_ansi_color calls would
Yes! Most of this is obviously library-ish code, and should go into the
Out of curiosity, are people really running perl < 5.6?  Term::ANSIColor

has been in the base distribution for 7 years now.
-Peff

-

Yes, I agree that it complicates things for the caller. I was just

copying the model found in color.c; but seeing as this is Perl

splitting into lines inside the printing function would be

straightforward.
Cheers,

Wincent
-

Colors are specified in color.interactive.{prompt,header,help}.

They are specified as git color strings as described in the

documentation, then parsed into perl color strings (slightly

different). Ugly but visible defaults are still used.
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 confi...
[ message continues ]

Don't do that. The code in 'pu' is a mess of half-working features. If

your patch is accepted, then it has to be picked apart from those

half-working features that aren't being accepted (which hopefully isn't

hard if nobody has been working in the same area, but can be quite

ugly).  Base your work on 'master' if possible, or 'next' if it relies

on features only in next. If it relies on some topic branch that is

_only_ in pu, then mention explicitly which topic.
-Peff

-

And even when you base work on things in next, don't base it on the

tip of next.  Base it on a specific topic that is merged into next.

Next is also a mess of features, but they are more likely to be in

a working state than the features in pu.
Topics in next will merge to master at different times.  If your

changes depend on more than one topic that may make it more difficult

for the maintainer to merge your topic to master.
Fortunately In Dan's case the only topic in pu that impacted

git-add--interactive was his dz/color-addi topic, so this probably

applies to the tip of it just as well as it does to the tip of pu.
--

Shawn.

-

I 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

-

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