Right. It would be useful to see actual examples and discuss them on a
case-by-case basis. I imagine you sent the original email when observing
such a case (or a few) - could you share the review link? I think this
thread discussed a number of ways in which such things could go wrong, and
it would be interesting to see which of those applies to the example you're
presenting.
Eli
Without the right people reviewing proposed patches, problems in the LLVM codebase ifself will never get discovered and fixed.
We shouldn't jeopardize LLVM's own code quality in the name of fixing Phabricator bugs, and that's exactly what's happening right now.
Alp.
I think this is probably a fairly common cause of lost review mail. I
updated the Phab documentation in r211731 to try to clarify this. Feel
free to improve on that if I missed something.
Alp, does r211731 help address your concerns?
Cool, that makes sense.
Is there a container image of the llvm.org Phabricator setup available?
I'd like to have a shot at enforcing this or at least present a hint in the UI of the web app but I couldn't figure out where the container is hosted.
Alp.
PS. I caused a few of these recently, my email in phab was set to
an account that isn't subscribed to llvm-commits, so my reviews
were held for moderation. Fixed on my end.I think this is probably a fairly common cause of lost review mail. I
updated the Phab documentation in r211731 to try to clarify this. Feel free
to improve on that if I missed something.Alp, does r211731 help address your concerns?
Cool, that makes sense.
Is there a container image of the llvm.org Phabricator setup available?
Not sure.
I'd like to have a shot at enforcing this or at least present a hint in the
UI of the web app but I couldn't figure out where the container is hosted.
Just to be clear, the user experience here isn't any worse because of
Phab - a user should get the list mail telling them that their mail
in "[PATCH] cmake: NDEBUG needlessly defined in non-Release builds",
if Reid's theory is correct, would've been exactly the same even if
Phab were not involved at all. User emails <developer> and <mailing
, <developer> receives the mail while the list doesn't as the
mail awaits review. Developer replies and then we're exactly where we
were, every other developer sees a reply without the original mail and
is confused. Eventually the moderator accepts the email and it comes
through.
(I'm still a tad confused, though, as the ML archive seems to show the
original NDEBUG patch mail, but I don't think I've seen it come
through to my inbox - so I'm not sure if there are toher (Phab or
non-Phab) issues at work here)
- David
PS. I caused a few of these recently, my email in phab was set to
an account that isn't subscribed to llvm-commits, so my reviews
were held for moderation. Fixed on my end.I think this is probably a fairly common cause of lost review mail. I
updated the Phab documentation in r211731 to try to clarify this. Feel
free to improve on that if I missed something.Alp, does r211731 help address your concerns?
Cool, that makes sense.
Is there a container image of the llvm.org Phabricator setup available?
I'd like to have a shot at enforcing this or at least present a hint in
the UI of the web app but I couldn't figure out where the container is
hosted.
Help would be highly appreciated, awesome!
Gives you a phab install that basically matches our setup if you replace
r4nt-master with r4nt-staging for the branch of the mirrored phab repos.
Cheers,
/Manuel
In a recent review via Phabricator, I was receiving bounce notifications for mail being sent to llvm-commits because of “Too many recipients to the message”, even though I am a subscriber. I wonder how common is that.
In a recent review via Phabricator, I was receiving bounce notifications for
mail being sent to llvm-commits because of "Too many recipients to the
message", even though I am a subscriber. I wonder how common is that.
Not too common, but it does happen. There is an issue with that. I'm
not sure what the maximum number is so that we can make the maximum
number of people on a phab review smaller.
-eric
Someone else emailed about that to me earlier today. The current limit is set at 10 for llvm-commits. It sounds like that is too low. Does anyone that uses Phabricator have an idea of how much to increase this limit? Would 20 suffice, or are more people than that CC’ed on Phabricator emails? Regards, John Criswell
20 is probably good for now. In general if someone tries to CC more
than that on a single review then something has gone wrong.
-eric
I'm not exactly sure why Phab should need a higher limit. I've seen a
few Phab emails come through with a list as long as my arm - but I
don't know why that happens. Anyone know where those are coming from?
Maybe it's people using Herald to subscribe to changes in particular
parts of the codebase - so when someone starts a code review all the
Herald subscribers end up cc'd on the initial mail? I wonder if the
Herald subscribers should be bcc'd or handled in some other way?
If that's what's happening, and it's the right way to do it, I'm not
sure what the right limit is - as more people use Herald we could get
larger and larger CC lists.
What's the mailing list limit for? What're the tradeoffs of setting it
higher, or removing it entirely?
- David
I use Herald and find it very useful to keep an eye on certain types
of commits, and it seems more people are using it in this way. I don't
see any reason why Herald wouldn't be able to bcc instead of cc, as
long as people use the web-ui or reply to phabricator directly then
they should still receive updates when the review changes. I have no
idea whether Herald supports that though.
Amara
In a recent review via Phabricator, I was receiving bounce notifications for
mail being sent to llvm-commits because of "Too many recipients to the
message", even though I am a subscriber. I wonder how common is that.Someone else emailed about that to me earlier today.
The current limit is set at 10 for llvm-commits. It sounds like that is too
low.Does anyone that uses Phabricator have an idea of how much to increase this
limit? Would 20 suffice, or are more people than that CC'ed on Phabricator
emails?I'm not exactly sure why Phab should need a higher limit. I've seen a
few Phab emails come through with a list as long as my arm - but I
don't know why that happens. Anyone know where those are coming from?
In my case a person that put up a patch put everyone they knew into
the reviewer field to get a lot of eyes on it. It overran the limit 
Maybe it's people using Herald to subscribe to changes in particular
parts of the codebase - so when someone starts a code review all the
Herald subscribers end up cc'd on the initial mail? I wonder if the
Herald subscribers should be bcc'd or handled in some other way?If that's what's happening, and it's the right way to do it, I'm not
sure what the right limit is - as more people use Herald we could get
larger and larger CC lists.What's the mailing list limit for? What're the tradeoffs of setting it
higher, or removing it entirely?
A spam precaution like the others
-eric
I have seen the “Too many recipients to the message” several times.
A limit of 10 includes the patch author and the list leaving just 8 subscribers/reviewers is way too low.
Given that these e-mails can be sent only by a Phab. user I’m not sure that spam is a problem at all:
A potential spammer would first have to subscribe to Phab. then create a proper diff,… far easier just to spam-mail everyone.
Yaron
Just to add another anecdote to the data, yesterday I pushed a review to lldb-commits through Phab. I had one reviewer on the issue (who is a subscriber), and I am also a subscriber to the mailing list, and nobody else was CC’ed on the review. The message showed up on the archive and in my mailbox within seconds, but other people who were next to me, and who also subscribe to lldb-commits, did not get the email for a few hours.
The lists were ridiculously slow yesterday.
This could be a side effect of graylisting or other common spam prevention stuff. Depending on the "triple" - it could easily be fast for one person and bounce (retry) for another. I'm just speculating here and "slow" may not be a result of anything which is easily "fixed".
As I understand, some people legitimately use Phabricator for internal
review, ...
MIPS currently do this for patches that only touch the MIPS backend (details can be found at http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220385.html). Any patch that touches common code is sent to the list. One thing I haven't said on that thread yet is that I'm considering trying the 'everything to llvm-commits' workflow myself in a couple weeks when my urgent work is finished. I have some reservations about that workflow but I'm willing to try it to see if my concerns are justified or not.
Manuel: I'm aware of one other way for a review to be invisible to llvm-commits despite being CC'd. If you forget to CC llvm-commits when creating the differential revision and add it later in the web interface, Phabricator doesn't send an email to the list. Adding a comment (at the same time or afterwards) triggers an email.
The same thing happens when adding reviewers without a comment.
> As I understand, some people legitimately use Phabricator for internal
> review, ...MIPS currently do this for patches that only touch the MIPS backend
(details can be found at
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220385.html).
Any patch that touches common code is sent to the list. One thing I haven't
said on that thread yet is that I'm considering trying the 'everything to
llvm-commits' workflow myself in a couple weeks when my urgent work is
finished. I have some reservations about that workflow but I'm willing to
try it to see if my concerns are justified or not.
Not knowing your concerns, I'd say they are mostly unjustified 
Manuel: I'm aware of one other way for a review to be invisible to
llvm-commits despite being CC'd. If you forget to CC llvm-commits when
creating the differential revision and add it later in the web interface,
Phabricator doesn't send an email to the list. Adding a comment (at the
same time or afterwards) triggers an email.
The same thing happens when adding reviewers without a comment.
That is intentional. We don't want to spam the list with changes in phab -
use phab like you would use email, and everything should be fine.
In a recent review via Phabricator, I was receiving bounce notifications
for mail being sent to llvm-commits because of "Too many recipients to the
message", even though I am a subscriber. I wonder how common is that.Someone else emailed about that to me earlier today.
The current limit is set at 10 for llvm-commits. It sounds like that is
too low.
Wait, is that set on llvm-commits, or is this related to phab?