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