Re: [PATCH v3 00/15] ref-filter: use parsing functions

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2016, 21:14 UTC
Message-ID: <CAPig+cRjXHmBnnmHbbibeV-gcvwMFQc7-5ce4s2OHu+wQwMy4Q@mail.gmail.com>
In-Reply-To: <1451980994-26865-1-git-send-email-Karthik.188@gmail.com>

On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 6 quoted lines

> Eric suggested that I make match_atom_name() not return a value [0]. I
> haven't done that as we use match_atom_name() in [14/15] for matching
> 'subject' and 'body' in contents_atom_parser() and although Eric
> suggested I use strcmp() instead, this would not work as we need to
> check for derefernced 'subject' and 'body' atoms.
> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701

I don't understand the difficulty. It should be easy to manually skip the 'deref' for this one particular case:

    const char *name = atom->name;
    if (*name == '*')
        name++;

Which would allow this unnecessarily complicated code from patch 14/15:

    if (match_atom_name(atom->name, "subject", &buf) && !buf) {
        ...
        return;
    } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
        ...
        return;
    } if (!match_atom_name(atom->name, "contents", &buf))
        die("BUG: parsing non-'contents'");

to be simplified to the more easily understood form suggested during review[1] of v2:

    if (!strcmp(name, "subject")) {
        ...
        return;
    } else if (!strcmp(name, "body")) {
        ...
        return;
    } else if (!match_atom_name(name,"contents", &buf))
        die("BUG: expected 'contents' or 'contents:'");

You could also just use (!strcmp("body") || !strcmp("*body")) rather than skipping "*" manually, but the repetition makes that a bit noisier and uglier.

[1]: http://article.gmane.org/gmane.comp.version-control.git/282645