On Fri, Feb 5, 2016 at 5:52 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@xxxxxxxxx> 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.
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> 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.
>> + 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.
>> + 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.
>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html