Re: [PATCH 5/5] Added diff hunk coloring to git-add--interactive

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2007, 22:35 UTC
Message-ID: <7v1wah3o3w.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071122122540.GH12913@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

> On Thu, Nov 22, 2007 at 04:56:24AM -0600, Dan Zwell wrote:
>
>> -		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'));
>
> 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
> did it this way because there will be reasons later on.").

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.