On Thu, Dec 3, 2015 at 8:35 AM, Karthik Nayak <[email protected]> wrote:
> On Wed, Dec 2, 2015 at 4:57 AM, Eric Sunshine <[email protected]> wrote:
>> On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <[email protected]> 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.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
