From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 27 Jul 2015 16:02:21 GMT
Subject: Re: [PATCH v5 02/11] ref-filter: make `color` use `ref_formatting_state`
Message-ID: <CAOLa=ZS2xEegWfoObXub8KMrvjw_7K-Wd9x8E7kKAZtdt8M+=Q@mail.gmail.com>
In-Reply-To: <vpqa8uhaht3.fsf@anie.imag.fr>

On Mon, Jul 27, 2015 at 6:17 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Make the `color` atom a pseudo atom and ensure that it uses
>> `ref_formatting_state`.
>
> Actually, I think this is an incorrect change.
>
> Previously, %(color:...) was taking effect immediately, and after this
> patch, it takes effect starting from the next atom.
>
> Try this:
>
> git for-each-ref --format '%(color:red)red %(color:reset)normal'
>
> Before your patch, I get 'red' as red, and 'normal' as normal. After
> your patch, I get no color at all, since %(color:...) just stores
> information that is never used because I have no real atom.
>
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -19,11 +19,13 @@
>>  struct atom_value {
>>       const char *s;
>>       unsigned long ul; /* used for sorting when not FIELD_STR */
>> -     unsigned int pseudo_atom : 1; /*  atoms which aren't placeholders for ref attributes */
>> +     unsigned int pseudo_atom : 1, /*  atoms which aren't placeholders for ref attributes */
>> +             color : 1;
>>  };
>
> As a consequence of the remark above, I think the name and comment of
> the field are misleading. There are 3 kinds of atoms:
>
> * Placeholders for ref attributes
>
> * Atoms that take action immediately, but that are not ref attributes
>   like %(color)
>
> * Atoms that only act as modifiers for the next atom. Perhaps they could
>   be called "prefix atoms" or "modifier atoms".
>

Thats right, Junio and I missed out the second part and overlapped it
over with the
third part. Thanks for seeing it through.

-- 
Regards,
Karthik Nayak