From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 11 Nov 2007 19:56:29 GMT
Subject: Re: Subject: [PATCH 1/3] Added basic color support to git add --interactive
Message-ID: <7vve88d09e.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071110202319.1f89a755@paradox.zwell.net>

Dan Zwell <dzwell@zwell.net> writes:

> Added function "colored()" that prints text with a color that
> is passed in. Converted many calls to "print" to being calls to
> "print colored".

You said "Let me know if other things need correction."  I think
most of the comments you recieved were about improvements not
correction, and I think I am going to say in this message will
also mostly fall into that category.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8d5d200..3712d6a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -382,6 +382,12 @@ color.diff.<slot>::
>  	whitespace).  The values of these variables may be specified as
>  	in color.branch.<slot>.
>  
> +color.interactive::
> +	When true (or `always`), always use colors in `git add
> +	--interactive`.  When false (or `never`), never.  When set to
> +	`auto`, use colors only when the output is to the
> +	terminal. Defaults to false.
> +

I think "auto" should disable colors even when the output is "-t
STDOUT" if $TERM eq "dumb" (see color.c::git_config_colorbool()).

> @@ -175,7 +207,7 @@ sub list_and_choose {
>  			if (!$opts->{LIST_FLAT}) {
>  				print "     ";
>  			}
> -			print "$opts->{HEADER}\n";
> +			print colored $header_color, "$opts->{HEADER}\n";

I agree with Jeff's suggestion to stick to more C-ish style to
always use parentheses around function arguments.