Re: [PATCH v3 14/15] ref-filter: introduce contents_atom_parser()

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 6, 2016, 18:22 UTC
Message-ID: <CAOLa=ZQY09yqDVELb9ObTnWfU-9nRyxJiV=_2tHbZPk_oe8sGQ@mail.gmail.com>
In-Reply-To: <xmqqio37iuvj.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 6, 2016 at 2:52 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 47 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>>  ref-filter.c | 74 +++++++++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 46 insertions(+), 28 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 32b4674..9e61530 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -39,6 +39,10 @@ static struct used_atom {
>>               struct align align;
>>               enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>>                       remote_ref;
>> +             struct {
>> +                     enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> +                     unsigned int nlines;
>> +             } contents;
>>       } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -96,6 +100,35 @@ static void remote_ref_atom_parser(struct used_atom *atom)
>>               die(_("unrecognized format: %%(%s)"), atom->name);
>>  }
>>
>> +static void contents_atom_parser(struct used_atom *atom)
>> +{
>> +     const char * buf;
>> +
>> +     if (match_atom_name(atom->name, "subject", &buf) && !buf) {
>> +             atom->u.contents.option = C_SUB;
>> +             return;
>> +     } else if (match_atom_name(atom->name, "body", &buf) && !buf) {
>> +             atom->u.contents.option = C_BODY_DEP;
>> +             return;
>> +     } if (!match_atom_name(atom->name, "contents", &buf))
>> +               die("BUG: parsing non-'contents'");
>
> Did you really intend to say "if" here, not "else if"?
>

Not that it makes a difference here since both the previous condition return. I think "else if" would be better.

Show 5 quoted lines

> I also wonder if the "&& !buf" in the first two are correct.  What
> should happen to "%(subject:foo)", which gives you a non-empty buf?
> It may or may not be an error, but it should not fall thru and cause
> "BUG:parsing non-contents", should it?
>

I think It would be better to add specific messages there

if (match_atom_name(atom->name, "subject", &buf)) {
    if (buf)
        die("%%(subject) atom does not take arguments");
    atom->u.contents.option = C_SUB;
    return;
} else if (match_atom_name(atom->name, "body", &buf)) {
    if (buf)
        die("%%(body) atom does not take arguments");
    atom->u.contents.option = C_BODY_DEP;
    return;
} else if (!match_atom_name(atom->name, "contents", &buf))
     die("BUG: parsing non-'contents'");

-- 
Regards,
Karthik Nayak