Re: [PATCH v4 11/12] ref-filter: introduce contents_atom_parser()

On Fri, Feb 5, 2016 at 5:52 AM, Eric Sunshine [off-list ref] wrote:

On Sun, Jan 31, 2016 at 12:42 PM, 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. Also introduce body_atom_parser()
and subject_atom_parser() for parsing atoms '%(body)' and '%(subject)'
respectively.

These latter two parsers are conceptually distinct from introduction
of the %(contents) parser, thus could be done in a follow-on patch or
two (though I don't care strongly enough to insist upon it).

I think they go together, considering they use the same contents structure in
used_atom, Introducing separate patches would leave us in a grey area where
%(contents:...) uses used_atom->contents whereas body and subject don't.
So I'd prefer them being together.

quoted

Signed-off-by: Karthik Nayak <redacted>
---

diff --git a/ref-filter.c b/ref-filter.c

@@ -66,6 +70,38 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
+static void body_atom_parser(struct used_atom *atom, const char *arg)
+{
+       if (arg)
+               die("%%(body) atom does not take arguments");

None of the other error messages bothers saying "atom" literally
following a %(foo). For consistency, this likely should say merely:

    %(body) does not take arguments

Will change.

quoted

+       atom->u.contents.option = C_BODY_DEP;
+}
+
+static void subject_atom_parser(struct used_atom *atom, const char *arg)
+{
+       if (arg)
+               die("%%(subject) atom does not take arguments");

Ditto.

Will change.

quoted

+       atom->u.contents.option = C_SUB;
+}

@@ -733,19 +763,15 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj

        for (i = 0; i < used_atom_cnt; i++) {
                const char *name = used_atom[i].name;
+               struct used_atom *atom = &used_atom[i];

Not a big deal, but if you re-order these two lines, then the second,
which extracts name, could do so from the variable declared by the
first:

    struct used_atom *atom = &used_atom[i];
    const char *name = atom->name;

Seems good, will incorporate.

quoted

                struct atom_value *v = &val[i];
-               const char *valp = NULL;
                if (!!deref != (*name == '*'))
                        continue;
                if (deref)
                        name++;
                if (strcmp(name, "subject") &&
                    strcmp(name, "body") &&
-                   strcmp(name, "contents") &&
-                   strcmp(name, "contents:subject") &&
-                   strcmp(name, "contents:body") &&
-                   strcmp(name, "contents:signature") &&
-                   !starts_with(name, "contents:lines="))
+                   !starts_with(name, "contents"))
                        continue;

This changes behavior in that it will also now match
"contentsanything", whereas the original was much more strict. Is that
desirable? (Genuine question.)

Well, we wont have something like that come through because
parse_ref_filter_atom()
would throw an error for %(contentsanything), but if in the future
sometime if we
introduce an atom %(contentsfoo) this might end up being a problem.

-- 
Regards,
Karthik Nayak