From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 13 Dec 2015 06:05:57 GMT
Subject: Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()
Message-ID: <CAPig+cSC8fP1KGM1wCXzcfj2QHgvoUUjfBncRH0jKLv1EnYKCg@mail.gmail.com>
In-Reply-To: <CAOLa=ZQY3PMtfemVAsbHydtguQG9pc2uVTiJ6Qh+jLf6mY4dFw@mail.gmail.com>

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.)