From: Karthik Nayak <karthik.188@gmail.com>
Date: Sun, 13 Dec 2015 18:46:41 GMT
Subject: Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()
Message-ID: <CAOLa=ZQhFXUYvYjJjWDzN6z+HDiXrhJqRbd3CWKTGmDtXn3oJw@mail.gmail.com>
In-Reply-To: <CAPig+cSC8fP1KGM1wCXzcfj2QHgvoUUjfBncRH0jKLv1EnYKCg@mail.gmail.com>

On Sun, Dec 13, 2015 at 11:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>>> @@ -833,11 +846,10 @@ static void populate_value(struct ref_array_item *ref)
>>>>                         refname = branch_get_push(branch, NULL);
>>>>                         if (!refname)
>>>>                                 continue;
>>>> -               } else if (match_atom_name(name, "color", &valp)) {
>>>> +               } else if (starts_with(name, "color")) {
>>>
>>> Hmm, so this will also match "colorize". Is that desirable?
>>
>> Well the error checking is done when we parse the atom in color_atom_parser()
>> so here we don't need to worry about something like this.
>
> I'm not sure that I understand your response. Let's say that, in the
> future, someone adds a new atom named "colorize" (which may or may not
> have its own parser in the valid_atom[] table). color_atom_parser()
> will never see that atom, thus error checking in color_atom_parser()
> is not relevant to this case. What is relevant is that the original
> code:
>
>     } if (match_atom_name(name, "color", &valp)) {
>
> only matched %(color) or %(color:whatever). It did not match
> %(colorize). However, the new code:
>
>     } else if (starts_with(name, "color")) {
>
> is far looser and will match %(colorize) and %(color) and
> %(color:whatever) and %(coloranything), which is potentially
> undesirable. It's true that the person adding %(colorize) could be
> careful and ensure that the if/else chain checks %(colorize) first:
>
>     } else if (!strcmp(name, "colorize") {
>         ...
>     } else if (starts_with(name, "color")) {
>         ...
>     } else ...
>
> but that places a certain extra burden on that person. Alternately,
> you can tighten the matching so that it is as strict as the original:
>
>     } else if (!strcmp(name, "color") || starts_with(name, "color:")) {
>         ...
>     } else ...
>
> Or perhaps upgrade match_atom_name() to make the 'val' argument
> optional, in which case you might be able to do something like this:
>
>     } else if (match_atom_name(name, "color", NULL) {
>
> (However, if you introduce an 'enum atom_type' as suggested in my
> response to the cover letter, then this problem goes away because
> you'd be switching on the enum value, which is determinate, rather
> than on a partial string, which may be ambiguous, as illustrated.)

Ah, Thanks for putting it out. I understand, I'll work on the enum atom_type
now. that should take care of this :)

-- 
Regards,
Karthik Nayak