Re: [PATCH 2/2] Let git-add--interactive read colors from git-config

[Dan Zwell <dzwell@zwell.net>]

On Tue, 23 Oct 2007 00:27:02 -0400
Jeff King <peff@peff.net> wrote:

> On Mon, Oct 22, 2007 at 04:40:48PM -0500, Dan Zwell wrote:
> 
> > Note: the code to parse git-style color strings to perl-style color
> > strings should eventually be added to Git.pm so that other (perl)
> > parts of git can be configured to read colors from .gitconfig in
> > a nicer way. A git-style string is "ul red black", while perl 
> > likes strings like "underline red on_black".
> 
> Why not do it as part of this patch, then?
Will do. I didn't include it in the patch because I need to learn more
about perl before I can make this change, though I can probably just
find enough examples in the other scripts that use Git.pm.

> 
> > +	# Sane (visible) defaults:
> > +	if (! @git_prompt_color) {
> > +		@git_prompt_color = ("blue", "bold");
> > +	}
> 
> I think it might be a bit more readable to keep the assignment and
> defaults together:
> 
>   my @git_prompt_color = split /\s+/,
>     qx(git config --get color.interactive.prompt) || 'blue bold';
> 
> Though I wonder why we are splitting here at all, since we just end up
> converting the list into a scalar below. And if we just turned that
> into a function, we could get a nice:
> 
>   my $prompt_color = git_color_to_ansicolor(
>     qx(git config --get color.interactive.prompt) || 'blue bold');
I agree, now that you mention it. Eventually the string must be split
(parsing it left to right by word makes more sense than trying to
mutate it with regular expressions, if only because it's a lot harder
to make mistakes), but there's no reason not to split the string inside
the loop, where it would look nicer/more contained. I will make this
change.

Dan