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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Jan 31, 2016 at 12:42 PM, 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. 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).

> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> 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

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

> +       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;

>                 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.)

>                 if (!subpos)
>                         find_subpos(buf, sz,