From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 07 Jan 2016 14:25:41 GMT
Subject: Re: [PATCH v3 00/15] ref-filter: use parsing functions
Message-ID: <CAOLa=ZSi0c61G8oXsCC0UfprDhf3hESaqKw8eGNRZJ7MTvNY4g@mail.gmail.com>
In-Reply-To: <CAPig+cRjXHmBnnmHbbibeV-gcvwMFQc7-5ce4s2OHu+wQwMy4Q@mail.gmail.com>

On Thu, Jan 7, 2016 at 2:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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

Definitely not a difficulty per se. Just that it seems like something
match_atom_name()
seems to be fit for. As the function name suggests that we're matching
the atom name
and the check for '!buf' indicates that no options are to be included
for that particular atom.

Also after Junio's suggestion[1], I think It looks better now[2]. But
either ways, I'm not
strongly against what you're saying, so my opinion on this matter is
quite flexible.

[1]: http://article.gmane.org/gmane.comp.version-control.git/283404
[2]: http://article.gmane.org/gmane.comp.version-control.git/283449

-- 
Regards,
Karthik Nayak