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

On Thu, Dec 17, 2015 at 3:09 PM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak [off-list ref] wrote:

quoted

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 [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---

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.

quoted

+               } 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.

quoted

+               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