Re: [PATCH/RFC 05/10] ref-filter: introduce color_atom_parser()

On Sun, Dec 13, 2015 at 11:35 AM, Eric Sunshine [off-list ref] wrote:

On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak [off-list ref] wrote:

quoted

On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak [off-list ref] wrote:

quoted

@@ -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