Re: [PATCH v3 08/15] ref-filter: introduce color_atom_parser()

[Karthik Nayak <karthik.188@gmail.com>]

On Wed, Jan 6, 2016 at 2:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "used_atom" structure for further usage in
>> populate_value().
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 29 ++++++++++++++++++++---------
>>  1 file changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index b54c872..9708d67 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>  static struct used_atom {
>>       const char *name;
>>       cmp_type type;
>> +     union {
>> +             char *color;
>> +     } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>> @@ -53,6 +56,18 @@ static int match_atom_name(const char *name, const char *atom_name, const char *
>>       return 1;
>>  }
>>
>> +static void color_atom_parser(struct used_atom *atom)
>> +{
>> +     if (!match_atom_name(atom->name, "color", (const char **)&atom->u.color))
>> +             die("BUG: parsing non-'color'");
>> +     if (!atom->u.color)
>> +             die(_("expected format: %%(color:<color>)"));
>> +     /* atom->u.color points to part of atom->name */
>> +     atom->u.color = xstrdup(atom->u.color);
>> +     if (color_parse(atom->u.color, atom->u.color) < 0)
>> +             die(_("invalid color value: %s"), atom->u.color);
>
> Is this calling color_parse() from color.c?
>

Yes it is!

> The function wants the destination to be at least COLOR_MAXLEN, but
> I do not see where the piece memory pointed by atom->u.color is
> guaranteed to be that long in the new code.  Looking at the code
> removed by this patch, it used to correctly use a buffer that is
> COLOR_MAXLEN bytes long.  So...
>
>         const char *color_value;
>
>         if (!match_atom_name(atom->name, "color", color_value))
>                 die("BUG: parsing non-'color'");
>         if (!color_value)
>                 die(_("expected format: %%(color:<color>)"));
>         atom->u.color = xmalloc(COLOR_MAXLEN);
>         if (color_parse(color_value, atom->u.color) < 0)
>                 die(_("invalid color value: %s"), color_value);
>
> or even define it in the union, i.e.
>
>         union {
>                 char color[COLOR_MAXLEN];
>         } u;
>
> and then use atom->u.color[] in-place?

I like the in-place suggestion, I wasn't wary of the Minimum length requirement
of the dest in color_parser(). Thanks for bringing it up. Will change.

-- 
Regards,
Karthik Nayak