Re: What's in git.git (stable frozen)

From: Jeff King <peff@peff.net>
Date: Jan 6, 2008, 20:59 UTC
Message-ID: <20080106205946.GA17482@coredump.intra.peff.net>
In-Reply-To: <7vy7b33zjk.fsf@gitster.siamese.dyndns.org>

On Sun, Jan 06, 2008 at 04:32:15AM -0800, Junio C Hamano wrote:

> Yeah, I like that much better, especially the renaming of
> $use_color to more descriptive (but is it really about "menu", I
> wonder?).

I thought that, too, but I didn't know what better word to use (something like "display" doesn't make it clear that it doesn't involve the diff).

Show 20 quoted lines

> I might consider rewriting this part
> 
> > +my $menu_use_color = $repo->get_colorbool('color.interactive');
> > +my ($prompt_color, $header_color, $help_color) =
> > +	$menu_use_color ? (
> > +		$repo->get_color('color.interactive.prompt', 'bold blue'),
> > +		$repo->get_color('color.interactive.header', 'bold'),
> > +		$repo->get_color('color.interactive.help', 'red bold'),
> > +	) : ();
> 
> like this:
> 
> 	my ($prompt_color, $header_color, $help_color, $fraginfo_color);
>         if ($colored_prompt) {
>         	$prompt_color = ...;
>                 $header_color = ...;
>         }
> 	if ($colored_diff) {
>         	$fraginfo_color = ...;
> 	}

Actually, that's more or less how it's written before my patch (in fact, you could eliminate that part of my patch and just move $normal_color outside of the conditional). However I didn't like having the declaration and the assignment so far apart (and duplicated). But I will admit my version is a little nested. Please feel free to switch it when you apply.

Show 16 quoted lines

> or even like this:
> 
> 	my (%palette);
>         if ($colored_prompt) {
> 		my %default = (
>                 	prompt => 'bold blue',
>                         header => 'bold',
>                         ...
> 		);
>                 while (my ($k,$v) = each %default) {
>                     $palette{$k} = $repo->get_color("color.interactive.$k",$v);
> 		}
> 	}
> 
> But I realize that's going overboard.  Certainly the last one is
> doing unnecessary generalization for generalization's sake.

Yes, I considered making a %palette, as well, but it just seemed a little gratuitous (and the nice thing about using variables is that they catch typos better).

-Peff