[PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[Tom Tobin]

(This is repost; my damned mail client wrapped a line in the patch last
time, and now I've got that under control.  My apologies!)  :(

Seeing the recent discussion and code regarding adding color to
git-add--interactive, I thought I'd throw in my recent attempt at
colorizing the diffs.  (This doesn't handle anything else, such as the
prompts.)

After banging my head against parsing colorized output of git-add-files,
I gave up and implemented internal colorization keying off of the
color.diff configuration.

Hopefully this can be of some use towards fully colorizing
git-add--interactive; I'll admit up front that Perl isn't my primary
language, so I apologize in advance for whatever stupidities I've
introduced.  ;) 

Signed-off-by: Tom Tobin <korpios@korpios.com>
---
 git-add--interactive.perl |  111 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 109 insertions(+), 2 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index be68814..eeb38e6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,5 +1,6 @@
 #!/usr/bin/perl -w
 
+use List::Util qw(first);
 use strict;
 
 sub run_cmd_pipe {
@@ -22,6 +23,112 @@ if (!defined $GIT_DIR) {
 }
 chomp($GIT_DIR);
 
+my ($use_color) = 0;
+my (%term_color_codes) = (
+	"normal", "", "black", "0", "red", "1",
+	"green", "2", "yellow", "3", "blue", "4",
+	"magenta", "5", "cyan", "6", "white", "7"
+);
+my (%term_attr_codes) = (
+	"bold", "1", "dim", "2", "ul", "4", "blink", "5", "reverse", "7"
+);
+my %colorconfig = (
+	'color.diff' => 'never',
+	'color.diff.plain' => '',
+	'color.diff.meta' => 'bold',
+	'color.diff.frag' => 'cyan',
+	'color.diff.old' => 'red',
+	'color.diff.new' => 'green',
+	'color.diff.commit' => 'yellow',
+	'color.diff.whitespace' => 'normal red'
+	);
+for (split("\n", `git-config --get-regexp '^color\.diff'`)) {
+	my ($var, $val) = $_ =~ /^([^\s]+)\s(.*)$/;
+	$colorconfig{$var} = $val;
+}
+if (first { $_ eq $colorconfig{'color.diff'} } ("true", "always", "auto")) {
+	$use_color = 1;
+}
+
+sub parse_color {
+	my ($fg, $bg, $attr, $lookup);
+	my ($fg_code, $bg_code, $attr_code, $output_code) = ("", "", "", "");
+	my (@color) = @_;
+	my (@colorvals) = defined($color[0]) ? split(" ", $color[0]) : ();
+
+	for (@colorvals) {
+		$lookup = $term_color_codes{$_};
+		if (defined($lookup)) {
+			if (!defined($fg)) {
+				$fg = 1;
+				$fg_code = "3$lookup";
+			} elsif (!defined($bg)) {
+				$bg = 1;
+				$bg_code = "4$lookup";
+			} else {
+				die("Color slots only take up to two colors!");
+			}
+			next;
+		}
+		$lookup = $term_attr_codes{$_};
+		if (defined($lookup)) {
+			if (!defined($attr)) {
+				$attr = 1;
+				$attr_code = $lookup;
+			} else {
+				die("Color slots only take a single attribute!");
+			}
+		} else {
+			die("Unrecognized value for color slot!");
+		}
+	}
+	for ($fg_code, $bg_code, $attr_code) {
+		if ($_ eq "") {
+			next;
+		}
+		if ($output_code ne "") {
+			$output_code = $output_code . ";";
+		}
+		$output_code = $output_code . $_;
+	}
+	if (length($output_code)) {
+		return "\e[${output_code}m";
+	} else {
+		return "";
+	}
+}
+
+sub colorize_head_line {
+	my $line = shift @_;
+	if ($use_color) {
+		# git doesn't colorize these by default, soooo
+		# if ($line =~ /^\+/) {
+		#	 return parse_color($colorconfig{'color.diff.new'}) . "$line\e[m";
+		# }
+		# if ($line =~ /^-/) {
+		#	 return parse_color($colorconfig{'color.diff.old'}) . "$line\e[m";
+		# }
+		return parse_color($colorconfig{'color.diff.meta'}) . "$line\e[m";
+	}
+	return $line;
+}
+
+sub colorize_hunk_line {
+	my $line = shift @_;
+	if ($use_color) {
+		if ($line =~ /^\+/) {
+			return parse_color($colorconfig{'color.diff.new'}) . "$line\e[m";
+		}
+		if ($line =~ /^-/) {
+			return parse_color($colorconfig{'color.diff.old'}) . "$line\e[m";
+		}
+		if ($line =~ /^@@ /) {
+			return parse_color($colorconfig{'color.diff.frag'}) . "$line\e[m";
+		}
+	}
+	return $line;
+}
+
 sub refresh {
 	my $fh;
 	open $fh, 'git update-index --refresh |'
@@ -573,7 +680,7 @@ sub patch_update_cmd {
 	my $path = $it->{VALUE};
 	my ($head, @hunk) = parse_diff($path);
 	for (@{$head->{TEXT}}) {
-		print;
+		print colorize_head_line($_);
 	}
 	$num = scalar @hunk;
 	$ix = 0;
@@ -617,7 +724,7 @@ sub patch_update_cmd {
 			$other .= '/s';
 		}
 		for (@{$hunk[$ix]{TEXT}}) {
-			print;
+			print colorize_hunk_line($_);
 		}
 		print "Stage this hunk [y/n/a/d$other/?]? ";
 		my $line = <STDIN>;
-- 
1.5.3.4

Re: [PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[Wincent Colaiuta]

El 14/10/2007, a las 10:44, Tom Tobin escribió:

> After banging my head against parsing colorized output of git-add- 
> files,
> I gave up and implemented internal colorization keying off of the
> color.diff configuration.

Great!

> +sub parse_color {

You could simplify the manual escape sequence construction that  
you're doing here by using Term::ANSIColor like the other patches  
did. I see that git-send-email.perl uses that module too, so I guess  
depending on that module is ok.

I also wonder whether the config code should be using the git.pm  
module like git-send-email.perl and a couple others do (although it  
would be slower than slurping in all the config in one shot like you  
do; perhaps there's justification for a new function in git.pm that  
wraps git-config --get-regexp...).

> +sub colorize_head_line {
> +	my $line = shift @_;
> +	if ($use_color) {
> +		# git doesn't colorize these by default, soooo
> +		# if ($line =~ /^\+/) {
> +		#	 return parse_color($colorconfig{'color.diff.new'}) . "$line\e 
> [m";
> +		# }
> +		# if ($line =~ /^-/) {
> +		#	 return parse_color($colorconfig{'color.diff.old'}) . "$line\e 
> [m";
> +		# }
> +		return parse_color($colorconfig{'color.diff.meta'}) . "$line\e[m";
> +	}
> +	return $line;
> +}
> +
> +sub colorize_hunk_line {
> +	my $line = shift @_;
> +	if ($use_color) {
> +		if ($line =~ /^\+/) {
> +			return parse_color($colorconfig{'color.diff.new'}) . "$line\e[m";
> +		}
> +		if ($line =~ /^-/) {
> +			return parse_color($colorconfig{'color.diff.old'}) . "$line\e[m";
> +		}
> +		if ($line =~ /^@@ /) {
> +			return parse_color($colorconfig{'color.diff.frag'}) . "$line\e[m";
> +		}
> +	}
> +	return $line;
> +}

This is a good start but to completely match the colorized output  
produced by diff it will need some additional logic; for example,  
highlighting spurious whitespace. Search for  
need_highlight_leading_space in diff.c and you'll see that the test  
is basically for any space which precedes a tab in the leading  
whitespace on newly inserted lines. In this case the spaces are  
highlighted using the whitespace color (normally red background).

I don't know when color.diff.commit is ever used in diff output, but  
perhaps that would need to be handled as well.

Cheers,
Wincent

Re: [PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[Johannes Schindelin]

Hi,

On Sun, 14 Oct 2007, Wincent Colaiuta wrote:

> > +sub parse_color {
> 
> You could simplify the manual escape sequence construction that you're 
> doing here by using Term::ANSIColor like the other patches did. I see 
> that git-send-email.perl uses that module too, so I guess depending on 
> that module is ok.

Wrong.  Depending on that module is not correct, you always have to wrap 
it into an "if (<is_color>) {...}".

I use git add -i quite often, and I _never_ use git send-email.  My guess 
is that I am not alone with that.

Ciao,
Dscho

Re: [PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 14 Oct 2007, Wincent Colaiuta wrote:
> 
>>> +sub parse_color {
>> You could simplify the manual escape sequence construction that you're 
>> doing here by using Term::ANSIColor like the other patches did. I see 
>> that git-send-email.perl uses that module too, so I guess depending on 
>> that module is ok.
> 
> Wrong.  Depending on that module is not correct, you always have to wrap 
> it into an "if (<is_color>) {...}".
> 
> I use git add -i quite often, and I _never_ use git send-email.  My guess 
> is that I am not alone with that.
> 

Not by a longshot, no. Personally I find git-send-email so tricky to use I've
rolled my own sender. I circulated it on the list a year or so back, but it's
not nearly so feature-full as git-send-email, so it never got much of an
audience.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)

[Wincent Colaiuta]

El 14/10/2007, a las 19:15, Johannes Schindelin escribió:

> On Sun, 14 Oct 2007, Wincent Colaiuta wrote:
>
>>> +sub parse_color {
>>
>> You could simplify the manual escape sequence construction that  
>> you're
>> doing here by using Term::ANSIColor like the other patches did. I see
>> that git-send-email.perl uses that module too, so I guess  
>> depending on
>> that module is ok.
>
> Wrong.  Depending on that module is not correct, you always have to  
> wrap
> it into an "if (<is_color>) {...}".
>
> I use git add -i quite often, and I _never_ use git send-email.  My  
> guess
> is that I am not alone with that.

In that case I propose factoring out the escape sequence generation  
into git.pm, where it can be used by git-send-email, git-add-- 
interactive, or any other Perl script in Git. Do you think that's a  
good idea? If so I'll try whipping up a patch along those lines.

Wincent

Re: [PATCH] Add color to git-add--interactive diffs (Total different idea to solve the problem)

[Peter Baumann]

On Sun, Oct 14, 2007 at 03:44:54AM -0500, Tom Tobin wrote:
> (This is repost; my damned mail client wrapped a line in the patch last
> time, and now I've got that under control.  My apologies!)  :(
> 
> Seeing the recent discussion and code regarding adding color to
> git-add--interactive, I thought I'd throw in my recent attempt at
> colorizing the diffs.  (This doesn't handle anything else, such as the
> prompts.)
> 
> After banging my head against parsing colorized output of git-add-files,
> I gave up and implemented internal colorization keying off of the
> color.diff configuration.
> 
> Hopefully this can be of some use towards fully colorizing
> git-add--interactive; I'll admit up front that Perl isn't my primary
> language, so I apologize in advance for whatever stupidities I've
> introduced.  ;) 
> 
> Signed-off-by: Tom Tobin <korpios@korpios.com>

[...skiping patch ...]

Tossing around ideas, so feel free to ignore me.

Wouldn't it make more sense to implement the diff coloring inside git apply
so that you could use something like

        diff file1 file2|git apply --color

to make the generated diff with colors [1]? It already implements the
same semantic for generating a diffstat, using

        diff file1 file2|git apply --stat

so we would get a generic diff colorizing tool and you could use inside
git add -i the diff without color and just print it out with the
git apply --color filter. So if someone implements another tool which
needs color handling he could use this output filter.

-Peter

[1]: there is a programm colordiff which does exactly this, but AFAIK git
     colorization has more features.

Re: [PATCH] Add color to git-add--interactive diffs (Total different idea to solve the problem)

[Johannes Schindelin]

Hi,

On Mon, 22 Oct 2007, Peter Baumann wrote:

> Wouldn't it make more sense to implement the diff coloring inside git 
> apply so that you could use something like
> 
>         diff file1 file2|git apply --color
> 
> to make the generated diff with colors [1]? It already implements the
> same semantic for generating a diffstat, using
> 
>         diff file1 file2|git apply --stat

No.  In both cases, "git diff" realises that the output is no terminal, 
and switches off color generation.  (Just try with diff.color=true instead 
of =auto.)

Ciao,
Dscho

Re: [PATCH] Add color to git-add--interactive diffs (Total different idea to solve the problem)

[Peter Baumann]

On Tue, Oct 23, 2007 at 12:55:44AM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 22 Oct 2007, Peter Baumann wrote:
> 
> > Wouldn't it make more sense to implement the diff coloring inside git 
> > apply so that you could use something like
> > 
> >         diff file1 file2|git apply --color
> > 
> > to make the generated diff with colors [1]? It already implements the
> > same semantic for generating a diffstat, using
> > 
> >         diff file1 file2|git apply --stat
> 
> No.  In both cases, "git diff" realises that the output is no terminal, 
> and switches off color generation.  (Just try with diff.color=true instead 
> of =auto.)
> 

I didn't mean git-diff here, instead I meant diff, so no coloring involved
on the diff side. The git-apply would be enhanced to do the coloring on
every diff it gets on its STDIN.

In the git-add -i case, the perl script whould do something along these
lines:

	foreach my $file (@files) {
		# read in the diff of a file *WITHOUT* using color
		@diff = `git-diff-files $file`;

		# ... store it away for later use in hunk selection ...


		# print out a nice colored diff for the user
		`echo @diff | git apply --color`
	}

Instead of handcoding the colorization in the git-add--interactive perl
script, just enhance git-apply to do the colorization *after the fact* for
you on _any_ patch you throw at it in its STDIN.

-Peter

Re: [PATCH] Add color to git-add--interactive diffs (Total different idea to solve the problem)

[Johannes Schindelin]

Hi,

On Tue, 23 Oct 2007, Peter Baumann wrote:

> On Tue, Oct 23, 2007 at 12:55:44AM +0100, Johannes Schindelin wrote:
> 
> > On Mon, 22 Oct 2007, Peter Baumann wrote:
> > 
> > > Wouldn't it make more sense to implement the diff coloring inside 
> > > git apply so that you could use something like
> > > 
> > >         diff file1 file2|git apply --color
> > > 
> > > to make the generated diff with colors [1]? It already implements 
> > > the same semantic for generating a diffstat, using
> > > 
> > >         diff file1 file2|git apply --stat
> > 
> > No.  In both cases, "git diff" realises that the output is no terminal, 
> > and switches off color generation.  (Just try with diff.color=true instead 
> > of =auto.)
> > 
> 
> I didn't mean git-diff here, instead I meant diff, so no coloring involved
> on the diff side. The git-apply would be enhanced to do the coloring on
> every diff it gets on its STDIN.

Ah!  I completely misunderstood indeed.  Clever...

Ciao,
Dscho