Re: [PATCH] color: allow multiple attributes

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2010, 18:16 UTC
Message-ID: <7vhbp1cjkc.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100228122019.GB24247@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

>> +		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?

Yes, I think we are now bounded and don't need this; I just thought it would have an educational value to show how to comment a complex expression in a readable way ;-)

Show 7 quoted lines

>> +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.

True, I don't know if it matters. I don't know if "blue bold bold" would result in bolder blue than "blue bold" on some terminal emulators, either.

I'd suggest that we ignore the issue for now, and when somebody complains with an actual non-working case, we would assess the damage that comes from this reordering to decide what to do next. Parhaps a "non-working case" could be "'blink ul' blinks letter without blinking underline, but 'ul blink' makes both letter and underline blink". At that point we can say "Ok, you found a case the order changes the results. But does that difference matter in practice?" and move forward, either by fixing it, or declaring it doesn't matter in practice.

We were already losing the order by emitting attr then fg then bg even though attr can come before any colors (an undocumented side effect of a sloppy parsing logic, but some of the existing tests insist on kepping it working), by the way.