Re: [PATCH v3 10/13] pretty: add %C(auto) for auto-coloring

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 17, 2013, 09:55 UTC
Message-ID: <CACsJy8DLj90aynu376O0LZKpQqvcmyoA_8gMNoek3WK6HTRtLQ@mail.gmail.com>
In-Reply-To: <7vd2tujh3o.fsf@alter.siamese.dyndns.org>

On Wed, Apr 17, 2013 at 7:33 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

>> @@ -1005,7 +1006,15 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>>       /* these are independent of the commit */
>>       switch (placeholder[0]) {
>>       case 'C':
>> -             return parse_color(sb, placeholder, c);
>> +             if (!prefixcmp(placeholder + 1, "(auto)")) {
>> +                     c->auto_color_next = 1;
>> +                     return 7;
>> +             } else {
>> +                     int ret = parse_color(sb, placeholder, c);
>> +                     if (ret)
>> +                             c->auto_color_next = 0;
>> +                     return ret;
>> +             }
>
> This is to handle a corrupt input, e.g. "%C(auto)%Cbleu%H" where
> (perhaps deprecated) "%Cblue" is misspelled, and parse_color()
> returns 0 without consuming any byte.
>
> Does it make sense not to turn auto off in such a case?

We don't have any mechanism to report invalid %C. Instead we let them through as literals. If they are literals, they should not have any side effects. So I think it makes sense not to turn off things.

Show 11 quoted lines

> Otherwise the above would become
>
>         if (!prefixcmp(placeholder + 1, "(auto)")) {
>                 c->auto_color_next = 1;
>                 return 7; /* consumed 7 bytes, "C(auto)" */
>         }
>         c->auto_color_next = 0;
>         return parse_color(sb, placeholder, c);
>
> which may be simpler.  When we see %C, previous %C(auto) is
> cancelled.

If we do this, maybe we could show invalid %C with blinking. Quite catchy and might make the user wonder why. Of course it won't work without coloring. But who would add %C in that case. -- Duy