128469 - seriously clean up email filtering code

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The code that filters emails in processmail to determine who should and should
not get notification of certain bug changes is in serious need of clean up both
for performance/efficiency reasons and to make it much easier to understand what
is going on in that code and how to extend it.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: clean up, reorganization, and documentation

This patch cleans up, reorganizes, and documents the code that determines which
users should and shouldn't receive notification on bug changes.  The resulting
code is smaller (despite extensive documentation), faster (fewer SQL queries),
simpler (fewer blocks, less nesting), easier to understand (documentation,
variable renaming), and easier to extend.

All code changes are confined to a single function in processmail with no
change in API, making this a drop-in replacement for the current code.

Comment 2

[Andreas Franke (gone)]

Are you sure that the new code behaves exactly the same as the old code? Can you
break down the changes into simpler, easy-to-understand steps? What are the
non-cosmetic changes (i.e. except comment-only-changes, variable-renamings,
formatting etc.)? Would it help to have the plain text of both versions of this
function for comparison.

Long ago, I had a look at this function, and I found it extremely complicated
and hard to understand. I think the rewrite is really necessary, and should go
in soon, but since the old stuff is so hard, it may need careful analysis to not
change the observed behaviour.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I expect this code to behave exactly the same as the old code, but of course
review needs to include testing.  I don't think I can break down the changes;
this code is already broken off of bug 122900 and constitutes the rewrite of a
single function.

Besides cosmetic changes, the new code runs a single query to grab the list of
watchers (the old code ran a query for each watched person), checks the specific
user preference pertaining to each bug change (the old code looped over all user
preferences for each change), and adds users to the recipients or "excluded"
list only once by correctly ordering the conditions that determine on which list
to place them (the old code sometimes added users to one list and then removed
them later).

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: fixes discrepancies between old and new behavior

This patch fixes discrepancies between the behaviors of the old and new
versions of the code.  In patch one, users were sometimes not added to the
excluded addresses list when they should have been.  This version of the code
fixes that problem.  It should now behave exactly the same as the old code. 
I'll attach a test that demonstrates this shortly.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: test: verifies results of new function match results of old function

This Perl script contains an exact copy of the old and new functions.  It runs
every combination of role, reason, and user through the functions and records
whether the new function succeeded or failed to reproduce the results of the
old function.  On my copy of the landfill database, this test finds that the
new function succeeds in reproducing the results of the old function in 100% of
cases.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 83983 [details]
test: verifies results of new function match results of old function

Changing content type of Perl script to text/plain so it is easy to look at.

Comment 7

[Bradley Baetz (:bbaetz)]

This is going to colide with my processmail -> Bugmail.pm patch, but hopefully
just in the file name

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Yes, this patch is a drop-in replacement for a single function in processmail,
so it'll be easy to integrate with your modulification patch, no matter which
one goes in first.  bbaetz- Can you review?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I ran the test on a copy of the b.m.o database, and the new code correctly
replicates the old code:

Total of 1821400 successes and 0 failures.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: fixes typo bug in watchers code

The previous patch treated an array as a scalar and thus added the size of the
array to the list of users to filter instead of the login name of the user. 
This patch fixes that problem.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: test v2: corresponds to patch v3

This test script contains the code from patch v3.

Comment 12

[Andreas Franke (gone)]

Comment on attachment 84695 [details] [diff] [review]
patch v3: fixes typo bug in watchers code

Ok, test v2 works on my installation, too:
Total of 1610 successes and 0 failures.

Thus I installed this patch. Note that I patched the new Bugmail.pm instead of
processmail, without any problems.

r=afranke due to successful testing.

Comment 13

[Bradley Baetz (:bbaetz)]

So, if patch v2 was wrong, why did the tests pass? Was this only discovered when
testing on bmo's database?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Andreas discovered the test failures when testing against his database.  I
should have found them testing against b.m.o's database, but I tested the
database with a copy of Bugzilla that had watchers turned off in the parameters,
so the code that adds watchers to the list of potential recipients didn't run.

When I tested b.m.o's database with watchers turned on, I saw the same failures
Andreas noticed and tracked down their cause.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: fixes typo: "excludeself" -> "ExcludeSelf"

Comment 16

[Jouni Heikniemi]

Note that bug 122900 now has a patch which contains the code from this bug's
patches as well.

Comment 17

[Andreas Franke (gone)]

For the record: Myk had asked me whether this patch needs a second review, in
the light of the new checkin policy for the bugzilla trunk. When I got the
request, I had a look a the patch and basically found the same problem that
jouni found independently. Thus with this problem fixed in the patch in bug
122900, I think it had enough eyes on it to go on the trunk now. Jouni, do you
agree?

> Subject: Re: second review needed for bug 128469?
> From: Andreas Franke <[e-mail]>
> Date: Tue, 11 Jun 2002 15:38:14 +0200
> To: Myk Melez <[e-mail]>
> 
> > >It seems to say "if I have turned off any of the reasons in
> > >my prefs, then I will be excluded". I would expect something
> > >like "if I have turned *on* any of the reasons in my prefs, 
> > >then I will be *included*".
> > >
> > That's right, you will be included in all email unless you specifically 
> > request to be excluded by turning off a preference in your email.
> 
> No, I don't think that's right. You should be included in all email
> unless you specifically request to be excluded by turning off the
> preferences for *all* reasons in your email.
> [...]
> Your tests seem to be only calling the function with _one_ reason
> at a time, instead of arbitrary subsets of all possible reasons.

Comment 18

[Jouni Heikniemi]

I refuse to take a stand in that issue until I see something official about the
"new checkin policy", of which I have only heard rumors about so far. If there's
a message in reviewers@ I've missed, please forward it to me via mail.

Myk, do you still plan to checkin these as separate fixes even though bug 122900
swallowed the patch for this?

Comment 19

[Myk Melez [:myk] [@mykmelez]]

I might as well check in a single patch.  I originally separated the patches to
make it easier to review, but once the fix for bug 122900 included code in the
middle of the changes for this bug it became easier to bundle the patches
together.  I doesn't matter much to me either way, really.

Comment 20

[Jouni Heikniemi]

I'm fine with checking in just one patch; if they're both going to be checked in
one after the another anyway, I don't think there's a lot sense in making this a
two-part thing anymore. So, I'd say that once bug 122900 has the second r=
(which afranke will probably give it, if he's confident enough), let's get this
in and start waiting for regressions.

Comment 21

[Andreas Franke (gone)]

Well, I don't like the idea of reviewing the "unconfirmed" patch mixed together
with the cleanup code. Equivalent code transformations and new features don't
mix well IMO. Still you have my r=afranke on the cleanup part of the patch in
bug 122900, and since the whole patch there has r=jouni and is also live on
b.m.o, it should get in right now. I have asked for an official confirmation on
[e-mail] .

Comment 22

[Jouni Heikniemi]

I tend to agree (with all of it). Let's wait for the policy being set in
reviewers@ and proceed then.

Comment 23

[Jouni Heikniemi]

I have given the patch on bug 122900 second-review, so these two should get
fixed the moment Myk has the time to check this stuff in. 

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Fixed by the check-in for bug 122900.