Re: [PATCH] color: allow multiple attributes

From: Jeff King <peff@peff.net>
Date: Feb 28, 2010, 12:20 UTC
Message-ID: <20100228122019.GB24247@coredump.intra.peff.net>
In-Reply-To: <1267325798-8280-1-git-send-email-gitster@pobox.com>

On Sat, Feb 27, 2010 at 06:56:38PM -0800, Junio C Hamano wrote:

Show 7 quoted lines

>   >>> but wouldn't it be more user friendly for us
>   >>> to support "red blink bold ul italic"?
>   >>
>   >> Yes, I think this should be done whether or not the patch in question
>   >> is accepted.
> 
>   This time with a bit of test updates as well for real inclusion.

Looks OK to me, but...

>   Also I realized that we can stuff them in an unsigned flag word as
>   bitfields ("red bold" and "red bold bold bold" would give the same
>   boldness anyway) to lift the artificial limit of number of attribute
>   words.

I also had this thought, but shouldn't that mean:

Show 13 quoted lines

> +		int i;
> +		int num_attrs = count_bits(attr);
> +
> +		if (COLOR_MAXLEN <=
> +		    /* Number of bytes to denote colors and attributes */
> +		    num_attrs
> +		    + (fg < 0 ? 0 : (fg < 8) ? 2 : 8) /* "3x" or "38;5;xxx" */
> +		    + (bg < 0 ? 0 : (bg < 8) ? 2 : 8) /* "4x" or "48;5;xxx" */
> +		    /* Number of semicolons between the above elements */
> +		    + (num_attrs + (0 <= fg) + (0 <= bg) - 1)
> +		    /* ESC '[', terminating 'm' and NUL */
> +		    + 4)
> +			goto bad;

We don't need this, because the length of what can be specified is bounded, and we simply need to set COLOR_MAXLEN high enough to handle the longest case? Though I suppose it doesn't hurt to be paranoid.

> +test_expect_success 'fg bg attr...' '
> +	color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
> +'

Hmm. Just a thought on the bit-setting approach, but does the order of attributes ever matter? We are going to lose the ordering information the user specifies, obviously.

-Peff