Re: [PATCH v17 08/14] ref-filter: add support for %(contents:lines=X)

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7d2732a..b098b16 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -56,6 +56,7 @@ static struct {
>>       { "color" },
>>       { "align" },
>>       { "end" },
>> +     { "contents:lines" },
>
> Do we even need "contents:lines" and existing other "contents:blah"
> in this list in the first place?  If they are needed, group them
> together, not append at the end.
>
> I wonder how this code sensibly can parse "%(contents:lines=6)".
> After splitting the format string at %( and closing ), the code
> calls parse_ref_filter_atom() and the rule that helper function uses
> to figure out the atom-name proper (which is to be checked against
> the valid_atom[] array) is to find the first colon, so
>
>     %(contents:lines=6)
>
> would cause "contents:lines=6" to be fed parse_ref_filter_atom(),
> it cheks if "contents" is in the valid_atom[] array (it is), and
> stores the whole thing in used_atom[].
>
> So in that sense, match_atom_name() would do the right thing, but
> that would make any reader of this code realize that she never saw
> "contents:lines" entry in valid_atom[] array being used during this
> process.