Re: [PATCH 1/3] pretty: add conditional %C?colorname placeholders

From: Will Palmer <wmpalmer@gmail.com>
Date: Apr 26, 2010, 08:20 UTC
Message-ID: <x2u5b9751661004260120t46494b18yea74c3f908b5742e@mail.gmail.com>
In-Reply-To: <r2o5b9751661004260113m7b6c387bm8467a063c13e5a0f@mail.gmail.com>

On Mon, Apr 26, 2010 at 7:30 AM, Alex Riesen <raa.lkml@gmail.com> wrote:

Show 18 quoted lines

> On Sun, Apr 25, 2010 at 17:42, Will Palmer <wmpalmer@gmail.com> wrote:
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index 1686a54..53eb903 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -132,6 +132,7 @@ The placeholders are:
>>  - '%Cblue': switch color to blue
>>  - '%Creset': reset color
>>  - '%C(...)': color specification, as described in color.branch.* config option
>> +- '%C?...: switch to specified color, if relevant color.* config option specifies that color is ok
>
> You missed the closing quote character. Besides, how do you think to
> distinguish between "%C?diff.color" and "%C?diff.colorcontinuation text"?
> The "%C(...)" has a placeholde termination rule (the closing bracket), yours
> does not seem to have one (unless something is missing in the documentation).
>
> Ah... I see. Definitely something missing. The spec should be: '%C?(...)'.
>

It's meant to handle both %C?green and %C?(green), perhaps I should list those out on separate lines? Missing quote noted, thanks.