From: Wincent Colaiuta <win@wincent.com>
Date: Sun, 14 Oct 2007 11:36:32 GMT
Subject: Re: [PATCH] Add color to git-add--interactive diffs (Take 2: now without spurious line break!)
Message-ID: <EFADE863-FC59-4A50-B165-9D30D9648B97@wincent.com>
In-Reply-To: <1192351494.7226.18.camel@athena>

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