Closed Bug 71794 Opened 20 years ago Closed 18 years ago

processmail shouldn't bother checking dependencies unless state changes.

Categories

(Bugzilla :: Email Notifications, defect, P1)

2.13
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: justdave, Assigned: gerv)

References

Details

Attachments

(1 file, 2 obsolete files)

I believe mail is not sent to any dependent bugs unless the bug being changed has 
a state change.  Bearing this in mind, processmail should not even bother with 
the "Checking dependencies for changes" bit unless the change being made is 
worthy of notifying the dependent bugs anyway.  Right now you get a box on the 
"Processed Bug" screen for each dependent bug telling you it didn't send any 
mail.  What's the point?  :-)
Yes this is quite annoying, but probably not enough of a performance hit to
bother for 2.12.
Target Milestone: --- → Bugzilla 2.16
Priority: -- → P1
*** Bug 93404 has been marked as a duplicate of this bug. ***
Mass moving to new product Bugzilla...
Assignee: tara → jake
Component: Bugzilla → Email
Product: Webtools → Bugzilla
Version: other → 2.13
CCing myk for his views on the perf. question.

Gerv
This is now on the "we really want this for 2.16, but won't hold the release for
it if it's not done by then" list.
*** Bug 111319 has been marked as a duplicate of this bug. ***
no patch, not a blocker, 2.16 is now in freeze mode.

-> 2.18
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Changing default owner of Email Notifications component to JayPee, a.k.a.
J. Paul Reed (preed@sigkill.com).  Jake will be offline for a few months.
Assignee: jake → preed
*** Bug 134899 has been marked as a duplicate of this bug. ***
Attached patch Patch v.1 (obsolete) — Splinter Review
This really needs fixing; I'm sure it'll be a significant perf win, and the fix
is simple.

Gerv
Comment on attachment 104287 [details] [diff] [review]
Patch v.1

What if I add/remove a dep? You need to make sure that that bug gets mail
recording the change, then.
Attachment #104287 - Flags: review-
Taking.

Gerv
Assignee: preed → gerv
Attached patch Patch v.2 (obsolete) — Splinter Review
v.2 - fixes important point raised by bbaetz :-)

It's just dep changes and status/resolution changes, right?

Gerv
Attachment #104287 - Attachment is obsolete: true
Given bug 178157, I wonder if processmail is the better place ot be doing this.
That probably requires a redesign, though

Also, we only send out status notifications in one way, so you could optimise
that check, too...

There is no way I will get to review that patch this week, though
Comment on attachment 105123 [details] [diff] [review]
Patch v.2

>Index: process_bug.cgi

>-
>+    

Out of curiousity, are you using kate nowadays?


>+            if ($col eq 'bug_status' &&
>+                IsOpenedState($old) ne IsOpenedState($new))
>+            {
>+                $check_dep_bugs = 1;
>+            }

Nit: per style guidelines, && on succeeding line, i.e.:


>+            if ($col eq 'bug_status'
>+                && IsOpenedState($old) ne IsOpenedState($new))


>+            open(PMAIL, "-|") or 
>+              exec('./processmail', $k, $::COOKIE{'Bugzilla_login'});

Nit: per style guidelines, "or" on succeeding line.

r=myk
Attachment #105123 - Flags: review+
Attached patch Patch v.3Splinter Review
Nits fixed; this is the patch that will be checked in.

Myk: I'm still using NEdit, as it happens. I haven't had a chance to look at
kate.

Gerv
Attachment #105123 - Attachment is obsolete: true
Fixed.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.164; previous revision: 1.163
done


Gerv
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
a=justdave

next time get it before you check in. :)
Sorry, Dave - completely forgot that you were doing that. Can you please send
round an official mail to say how a= works on Bugzilla?

Thanks :-)

Gerv
Myk already did.  It was posted to both developers at and reviewers at
bugzilla.org early this afternoon.  Did you read it? :-)
> Myk already did.  It was posted to both developers at and reviewers at
> bugzilla.org early this afternoon.  Did you read it? :-)

Not until later this morning. It was at the bottom of a mail giving a list of
bugs to review for the b.m.o. upgrade, so I ignored it, because I already have
more than enough to do for that :-) 

But you still haven't said whether you are a=ing bugs, or particular patches. If
it's bugs, please go through the b.m.o. upgrade blocker list and a= them all :-)

Gerv
>Myk: I'm still using NEdit, as it happens. I haven't had a chance to look at
>kate.

I just wondered because kate has the tendency to leave lines with indented
spaces around.  nedit, as far as I remember, didn't do that.

>Not until later this morning. It was at the bottom of a mail giving a list of
>bugs to review for the b.m.o. upgrade, so I ignored it, because I already have
>more than enough to do for that :-)

Please read the end of the email if you haven't yet.  I could really use a blurb
about report.cgi. :-)

This is a possible suspect in bug 179351.  Please see that bug.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.