From: Wincent Colaiuta <win@wincent.com>
Date: Thu, 06 Dec 2007 19:59:48 GMT
Subject: Re: [PATCH 3/3] Color support for "git-add -i"
Message-ID: <5055E3DF-E01D-41B5-9F59-DAD69885CAE8@wincent.com>
In-Reply-To: <1196906706-11170-3-git-send-email-gitster@pobox.com>

El 6/12/2007, a las 3:05, Junio C Hamano escribió:

> +sub colored_diff_hunk {
> +	my ($text) = @_;
> +	# return the text, so that it can be passed to print()
> +	my @ret;
> +	for (@$text) {
> +		if (!$diff_use_color) {
> +			push @ret, $_;
> +			next;
> +		}
> +
> +		if (/^\+/) {
> +			push @ret, colored($new_color, $_);
> +		} elsif (/^\-/) {
> +			push @ret, colored($old_color, $_);
> +		} elsif (/^\@/) {
> +			push @ret, colored($fraginfo_color, $_);
> +		} elsif (/^ /) {
> +			push @ret, colored($normal_color, $_);
> +		} else {
> +			push @ret, colored($metainfo_color, $_);
> +		}
> +	}
> +	return @ret;
> +}


My one concern here is that as Git's awareness of whitespace problems  
becomes more sophisticated it will be harder and harder to do diff  
colorization in a way that matches that which is performed by the  
builtin diff tools. Here I'm talking about the whole core.whitespace  
series which allows the user to define per-path attributes specifying  
what kinds of things are to be considered whitespace errors; so far we  
have three classes of error proposed as far as I know: trailing  
whitespace, spaces before tabs, and indents with non-tabs.

I think it's very important that "git add --interactive" be 100%  
consistent with the other tools here because in many cases the  
previewing you do during an interactive session is what you rely upon  
to review whether a change should be committed. In other words, you  
don't want to think stuff is ok because "git add --interactive" leads  
you to believe that it's ok when it really isn't, and you don't want  
to have to run "git diff" as a separate step either just to double  
check.

We can replicate the core.whitespace logic here but it's likely to be  
an error prone process as it involves repeating the same logic in two  
different places using two different implementations in two different  
languages.

What are the other options?

- Make git-add--interactive part of builtin-add so as to be able to  
use the core.whitespace code directly? (ideally yes and at some point  
in the future it seems inevitable that this will happen, but it will  
require a fair bit of work)

- Fork a second "git diff-files" process to capture the colorized  
version of the output? (may set off the "kludge" alarm)

- Something else?

Cheers,
Wincent