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...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";
+ ...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...
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_...
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 = ...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...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 ...
...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...
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...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...[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;
}
...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...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(
- ...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 ...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
-