On Thu, Dec 17, 2015 at 3:09 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html