277723 - Add comments why checksetup uses slow code to check for spaces and commas in flags

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Max Kanat-Alexander]

Instead of what we do now in checksetup to check for spaces/commas in flags, we
should just use SQL to do it:

WHERE name LIKE '% %' OR name LIKE '%,%'

Comment 1

[Max Kanat-Alexander]

(Actually, this technically first shows up in 2.18.)

Comment 2

[Max Kanat-Alexander]

Attachment: Faster Flags Check

OK, here's a version that does a faster check on the flags. It's just the
minimal changes required to make this happen.

Comment 3

[Nick Barnes]

(In reply to comment #2)

> OK, here's a version that does a faster check on the flags. It's just the
> minimal changes required to make this happen.

No! No! This isn't going to work.  The subsequent code for renaming flags needs
to know all the flagtype names, in order to prevent collisions.  That's why I
did it this way.  Consider what happens if you have a flag called "a b" and
another flag called "a_b": your code will rename "a b" as "a_b" which will then
collide.

We could do a select like the one you suggest in order to determine *whether* we
need to rename any flags, but then we would need to go on to select all the
flagtypes to make the rest of the code work.  Or rewrite it to be much more
complex and do a load more SQL queries when actually renaming flags.

When I wrote this, I did consider the speed question, but I estimated that
nobody would have enough flag types for a delay here to be noticeable.  Any
reasonable server will run this code over thousands of flags in a second or so.
 I tested it on my five-year-old desktop box with *ten thousand* flag types in
my database and it took much less than a second.  So is there actually a defect
here?  Does anyone actually have a problem with this part of checksetup.pl
taking too long to run?  I doubt it.

Comment 4

[Nick Barnes]

Maybe we should have a comment explaining *why* we collect all the flagtype names.

Comment 5

[Nick Barnes]

To show how this patch breaks things, here's the output before this patch (this
is on my ten-thousand flagtypes test database, which has some bad flagtype names):

Checking flag type names for spaces and commas...
  Bad flag type name "a b" ...
  ... can't rename as "a_b" ...
  renamed flag type "a b" as "a_b'"
  Bad flag type name "b,c" ...
  ... can't rename as "b_c" ...
  ... can't rename as "b_c'" ...
  ... can't rename as "b_c''" ...
  renamed flag type "b,c" as "b_c'''"
  Bad flag type name "123456789 123456789 123456789 123456789 1234567890" ...
  ... can't rename as "123456789_123456789_123456789_123456789_1234567890" ...
  renamed flag type "123456789 123456789 123456789 123456789 1234567890" as
"123456789_123456789_123456789_123456789_1234567..."
  Bad flag type name "foo, bar baz" ...
  renamed flag type "foo, bar baz" as "foo__bar_baz"
... done.

and here's the output after this patch:

Checking flag type names for spaces and commas...
  Bad flag type name "a b" ...
  renamed flag type "a b" as "a_b"
  Bad flag type name "b,c" ...
  renamed flag type "b,c" as "b_c"
  Bad flag type name "123456789 123456789 123456789 123456789 1234567890" ...
  renamed flag type "123456789 123456789 123456789 123456789 1234567890" as
"123456789_123456789_123456789_123456789_1234567890"
  Bad flag type name "foo, bar baz" ...
  renamed flag type "foo, bar baz" as "foo__bar_baz"
... done.

Notice that all the "can't rename" lines have disappeared.  And as you can see
here, the resulting database has multiple flagtypes with the same name:

mysql> select id, name from flagtypes where name like 'a%';
+----+------+
| id | name |
+----+------+
|  1 | a_b  |
|  2 | a_b  |
+----+------+
2 rows in set (0.04 sec)

mysql> 

(I also end up with two flagtypes called "b_c" and two called
"123456789_123456789_123456789_123456789_1234567890").

Comment 6

[Nick Barnes]

Attachment: Improve comments to show why we do this select.

Here's a patch to improve the comments of this section, so that developers can
see why we need all the flagtype names.

Comment 7

[Nick Barnes]

Attachment: Fixed patch

Whoops.  Do NOT apply that last patch.	This one is better.

Comment 8

[Nick Barnes]

Note: I did not mean to grant review+ on my obsolete patch there.  I was trying
to remove the review? flag and either pilot error or a Bugzilla bug made it
review+.  If I can reproduce that I will raise a bug.

Comment 9

[Vlad Dascalu]

Comment on attachment 170799 [details] [diff] [review]
Faster Flags Check

Canceling my review after reading the comments.

Comment 10

[Frédéric Buclin]

Comment on attachment 170811 [details] [diff] [review]
Improve comments to show why we do this select.

You cannot deny your own patches. ;)

Comment 11

[Marc Schumann [:Wurblzap]]

Comment on attachment 170812 [details] [diff] [review]
Fixed patch

r=wurblzap by inspection.

Comment 12

[Shane H. W. Travis]

Patch does not apply to either 2.18 or tip version of checksetup.

patch -p1 < ~travis/277723.txt
patching file checksetup.pl
Hunk #1 FAILED at 3761.
Hunk #2 FAILED at 3778.
2 out of 2 hunks FAILED -- saving rejects to file checksetup.pl.rej

Comment 13

[Nick Barnes]

Attachment: trunk patch

Patch updated so it cleanly applies to trunk tip.

Comment 14

[Nick Barnes]

Attachment: 2.18 patch

2.18 branch patch (the trunk patch works for me, but just for completeness).

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 173839 [details] [diff] [review]
trunk patch

r=wurblzap, again by inspection.
For this bug, feel free to carry r+ forward for uncritical unrotting :)

Comment 16

[Shane H. W. Travis]

Thanks for spinning new patches, Nick!

2.18:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.25; previous revision: 1.289.2.24
done

Tip:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.337; previous revision: 1.336
done