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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2016, 18:04 UTC
Message-ID: <xmqqziwh4669.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAOLa=ZQY09yqDVELb9ObTnWfU-9nRyxJiV=_2tHbZPk_oe8sGQ@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

>>> +static void contents_atom_parser(struct used_atom *atom)
>>> +{
>>> +     const char * buf;

const char *buf;

Show 14 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?