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

RESOLVED FIXED in Bugzilla 2.18

Status

()

RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: mkanat, Assigned: nb+bz)

Tracking

2.18
Bugzilla 2.18
Bug Flags:
approval +
approval2.18 +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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 '%,%'
(Reporter)

Comment 1

14 years ago
(Actually, this technically first shows up in 2.18.)
Version: 2.19.1 → 2.18
(Reporter)

Comment 2

14 years ago
Created attachment 170799 [details] [diff] [review]
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.
Attachment #170799 - Flags: review?

Updated

14 years ago
Attachment #170799 - Flags: review? → review+

Updated

14 years ago
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval?
Flags: approval2.18+
Flags: approval+
Target Milestone: --- → Bugzilla 2.18
(Assignee)

Comment 3

14 years ago
(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.
(Assignee)

Comment 4

14 years ago
Maybe we should have a comment explaining *why* we collect all the flagtype names.
(Assignee)

Comment 5

14 years ago
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").
(Assignee)

Comment 6

14 years ago
Created attachment 170811 [details] [diff] [review]
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.
Attachment #170811 - Flags: review?
(Assignee)

Comment 7

14 years ago
Created attachment 170812 [details] [diff] [review]
Fixed patch

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

Updated

14 years ago
Attachment #170811 - Attachment is obsolete: true
Attachment #170812 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #170811 - Flags: review? → review+
(Assignee)

Updated

14 years ago
Attachment #170811 - Flags: review+ → review-
(Assignee)

Comment 8

14 years ago
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.
Flags: approval2.18+
Flags: approval+

Comment 9

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

Canceling my review after reading the comments.
Attachment #170799 - Flags: review+
Summary: checksetup uses slow code to check for spaces and commas in flags → Add comments why checksetup uses slow code to check for spaces and commas in flags

Comment 10

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

You cannot deny your own patches. ;)
Attachment #170811 - Flags: review-
Attachment #170799 - Attachment is obsolete: true
Comment on attachment 170812 [details] [diff] [review]
Fixed patch

r=wurblzap by inspection.
Attachment #170812 - Flags: review? → review+
Flags: approval?
Flags: approval2.18?
(Reporter)

Updated

14 years ago
Assignee: mkanat → Nick.Barnes
Status: ASSIGNED → NEW
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+

Comment 12

14 years ago
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
(Assignee)

Comment 13

14 years ago
Created attachment 173839 [details] [diff] [review]
trunk patch

Patch updated so it cleanly applies to trunk tip.
Attachment #170812 - Attachment is obsolete: true
Attachment #173839 - Flags: review?(wurblzap)
(Assignee)

Comment 14

14 years ago
Created attachment 173840 [details] [diff] [review]
2.18 patch

2.18 branch patch (the trunk patch works for me, but just for completeness).
Attachment #173840 - Flags: review?(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 :)
Attachment #173839 - Flags: review?(wurblzap) → review+
Attachment #173840 - Flags: review?(wurblzap) → review+
Whiteboard: patch awaiting checkin

Comment 16

14 years ago
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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Whiteboard: patch awaiting checkin
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.