threads / patch / 10270

patchAdd color to git-add--interactive diffs (Take 2: now without spurious line break!)

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

## tl;dr

9 messages between Oct 14, 2007 and Oct 23, 2007. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Tom Tobin· Oct 14, 2007, 08:44 UTC · lore

(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(-)
Show changes to git-add--interactive.perl +109 −2
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
Wincent Colaiuta· Oct 14, 2007, 11:36 UTC · re: Tom Tobin · lore

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

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

Show 32 quoted lines
> +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

Johannes Schindelin· Oct 14, 2007, 17:15 UTC · re: Wincent Colaiuta · lore

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

Hi,
On Sun, 14 Oct 2007, Wincent Colaiuta wrote:
Show 6 quoted lines
> > +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

Andreas Ericsson· Oct 14, 2007, 17:55 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 16 quoted lines
> 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
Wincent Colaiuta· Oct 14, 2007, 21:01 UTC · re: Johannes Schindelin · lore

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

El 14/10/2007, a las 19:15, Johannes Schindelin escribió:
Show 18 quoted lines
> 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
Peter Baumann· Oct 22, 2007, 20:47 UTC · re: Tom Tobin · lore

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

On Sun, Oct 14, 2007 at 03:44:54AM -0500, Tom Tobin wrote:
Show 18 quoted lines
> (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.
Johannes Schindelin· Oct 22, 2007, 23:55 UTC · re: Peter Baumann · lore

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

Hi,
On Mon, 22 Oct 2007, Peter Baumann wrote:
Show 9 quoted lines
> 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

Peter Baumann· Oct 23, 2007, 05:34 UTC · re: Johannes Schindelin · lore

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

On Tue, Oct 23, 2007 at 12:55:44AM +0100, Johannes Schindelin wrote:
Show 18 quoted lines
> 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
Johannes Schindelin· Oct 23, 2007, 11:13 UTC · re: Peter Baumann · lore

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

Hi,
On Tue, 23 Oct 2007, Peter Baumann wrote:
Show 22 quoted lines
> 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

← back to recent threads