On Wed, Jan 6, 2016 at 2:19 AM, Junio C Hamano <[email protected]> wrote:
> Karthik Nayak <[email protected]> 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 <[email protected]>
>> Helped-by: Eric Sunshine <[email protected]>
>> Signed-off-by: Karthik Nayak <[email protected]>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
