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

[Eric Sunshine <sunshine@sunshineco.com>]

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?

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

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

> +       if (!strcmp(buf, "body"))
> +               atom->u.contents.option = C_BODY;
> +       else if (!strcmp(buf, "signature"))
> +               atom->u.contents.option = C_SIG;
> +       else if (!strcmp(buf, "subject"))
> +               atom->u.contents.option = C_SUB;
> +       else if (skip_prefix(buf, "lines=", &buf)) {
> +               atom->u.contents.option = C_LINES;
> +               if (strtoul_ui(buf, 10, &atom->u.contents.no_lines))
> +                       die(_("positive value expected contents:lines=%s"), buf);
> +       } else
> +               die(_("unrecognized %%(contents) argument: %s"), buf);
> +}
> @@ -777,28 +801,23 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj
>                                     &bodypos, &bodylen, &nonsiglen,
>                                     &sigpos, &siglen);
>
> -               if (!strcmp(name, "subject"))
> +               if (atom->u.contents.option == C_SUB)
>                         v->s = copy_subject(subpos, sublen);
> -               else if (!strcmp(name, "contents:subject"))
> -                       v->s = copy_subject(subpos, sublen);
> -               else if (!strcmp(name, "body"))
> +               else if (atom->u.contents.option == C_BODY_DEP)
>                         v->s = xmemdupz(bodypos, bodylen);
> -               else if (!strcmp(name, "contents:body"))
> +               else if (atom->u.contents.option == C_BODY)
>                         v->s = xmemdupz(bodypos, nonsiglen);
> -               else if (!strcmp(name, "contents:signature"))
> +               else if (atom->u.contents.option == C_SIG)
>                         v->s = xmemdupz(sigpos, siglen);
> -               else if (!strcmp(name, "contents"))
> -                       v->s = xstrdup(subpos);
> -               else if (skip_prefix(name, "contents:lines=", &valp)) {
> +               else if (atom->u.contents.option == C_LINES) {
>                         struct strbuf s = STRBUF_INIT;
>                         const char *contents_end = bodylen + bodypos - siglen;
>
> -                       if (strtoul_ui(valp, 10, &v->u.contents.lines))
> -                               die(_("positive value expected contents:lines=%s"), valp);
>                         /*  Size is the length of the message after removing the signature */
> -                       append_lines(&s, subpos, contents_end - subpos, v->u.contents.lines);
> +                       append_lines(&s, subpos, contents_end - subpos, atom->u.contents.no_lines);
>                         v->s = strbuf_detach(&s, NULL);
> -               }
> +               } else if (atom->u.contents.option == C_BARE)
> +                       v->s = xstrdup(subpos);
>         }
>  }
>
> --
> 2.6.4