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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 7, 2016, 20:03 UTC
Message-ID: <CAOLa=ZR=id7826YQZ4+Gk-6RzWVKrVomSFvzPqToeLcgt8Jr+Q@mail.gmail.com>
In-Reply-To: <xmqqziwh4669.fsf@gitster.mtv.corp.google.com>

On Thu, Jan 7, 2016 at 11:34 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 8 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>>> +static void contents_atom_parser(struct used_atom *atom)
>>>> +{
>>>> +     const char * buf;
>
> const char *buf;
>

will change that.

Show 23 quoted lines

>>>> +
>>>> +     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.
>
> I am not sure if it is "Y would be better even though X and Y both
> would work".  It looks to me "X and Y behave differently, X is a bug
> and Y is correct".
>
> The above would behave differently between "if" and "else if" (and
> by the way, the code layout suggests it is "else if"; otherwise you
> would be starting "if" on its own line) when you feed "subject:foo",
> no?

It is indeed an "else if". What I was referring to was that since its like

if (cond_a) { ... return; } else if (cond_b) { ... return; } if (cond_c) { ... }

cond_c is only checked if cond_a and cond_b don't hold good. Similar to how 'else if' would work, because cond_a and cond_b return. Sorry for the confusion.

-- 
Regards,
Karthik Nayak