Re: [PATCH v2 10/11] ref-filter: introduce contents_atom_parser()

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

On Thu, Dec 17, 2015 at 3:09 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce contents_atom_parser() which will parse the '%(contents)'
>> atom and store information into the 'used_atom' structure based on the
>> modifiers used along with the atom.
>>
>> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -39,6 +39,10 @@ static struct used_atom {
>>                 struct align align;
>>                 enum { RR_SHORTEN, RR_TRACK, RR_TRACKSHORT, RR_NORMAL }
>>                         remote_ref;
>> +               struct {
>> +                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> +                       unsigned int no_lines;
>
> 'no_lines' sounds like "no lines!". How about 'nlines' instead?
>

Sure, will do.

>> +               } contents;
>>         } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -90,6 +94,36 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>> +static void contents_atom_parser(struct used_atom *atom)
>> +{
>> +       const char * buf;
>> +
>> +       if (match_atom_name(atom->str, "contents", &buf))
>> +               atom->u.contents.option = C_BARE;
>> +       else if (match_atom_name(atom->str, "subject", &buf)) {
>
> The original code used strcmp() and matched only "subject", however
> the new code will incorrectly match both "subject" and
> "subject:whatever". Therefore, you should be using strcmp() here
> rather than match_atom_name().
>
> Ditto for "body".

Will change.

>
>> +               atom->u.contents.option = C_SUB;
>> +               return;
>> +       } else if (match_atom_name(atom->str, "body", &buf)) {
>> +               atom->u.contents.option = C_BODY_DEP;
>> +               return;
>> +       }
>> +       if (!buf)
>> +               return;
>
> It's not easy to see that this 'if (!buf)' check relates to the
> "contents" check at the very top of the if/else if/ chain since there
> are entirely unrelated checks in between. Reorganizing it can improve
> clarity:
>
>     if (!strcmp("subject")) {
>         ...
>         return;
>     } else if (!strcmp("body")) {
>         ...
>         return;
>     } else if (!match_atom_name(...,"contents", &buf))
>         die("BUG: expected 'contents' or 'contents:'");
>
>     if (!buf) {
>         atom->u.contents.option = C_BARE;
>         return;
>     }
>

This looks good. Thanks.

-- 
Regards,
Karthik Nayak