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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 7, 2016, 04:58 UTC
Message-ID: <CAOLa=ZQksDyyiukDZVZFX6113nOr=V_yCaBswtm5xDrLe5vrig@mail.gmail.com>
In-Reply-To: <CAPig+cQ+jsAdJSB6J_P8h+zDbiGpr4JGv=Vj1vEMSLdHERi0pw@mail.gmail.com>

On Fri, Feb 5, 2016 at 5:52 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 11 quoted lines

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

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.

Show 14 quoted lines

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

Will change.

Show 10 quoted lines

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

Show 15 quoted lines

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

Show 20 quoted lines

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