For the record: I'd like us to get away from maintaining our own phab fork, and I agree that means probably migrating to a different system at some point.

@epriestley - makes sense; I've been approached by some folks about our setup because they wanted kernel devs to get an actual code review system, who apparently have a similar mindset to compiler developers, but that's probably a long shot anyway :)

Note that this feature (text mode unified diff) is crucial for our deployment (requirement for text-only mails to contain reasonable diff snippets).

Jay: ok then, while I'll not argue about whether breaking is correct or not, gmail does break citations correctly.
That is, if I open an email and write (break before "unrelated newline"):

In T5181#170236, @jayvdb wrote:

In T5181#136123, @epriestley wrote:

In particular, I think we're going to hit cases like this:

> quack quack quack
bark bark bark

...and have no idea if "bark bark bark" is the first line getting wrapped or an inline reply, and every time we get it wrong someone will file a task complaining that we're the worst software in existence.

It would be better to reject incoming emails with inline comments from such email clients (such as gmail), as opposed to get it wrong.

@adrelanos: yes, that's what this ticket is about.

In T8336#124234, @klimek wrote:

In T8336#116857, @epriestley wrote:

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]

Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing Settings → Email Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

---

we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.

These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.

I'm currently playing around with this (as integrating broke our minimal email mode in a non-trivial way). We're getting very close to being able to get rid of most of our customizations - there are currently 2 things that I can't switch off (or I'm missing something):

• the actions at the top of the mail (never want to see those)
• the email prefs at the bottom (for people on the mailing list who don't have a phab account those are unnecessary)

In T8336#116857, @epriestley wrote:

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]

Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing Settings → Email Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

---

we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.

These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.

Awesome docs, thanks!

Any chance to get this in? Perhaps under and option? Eitan, how are you guys in FreeBSD land handling this?

T8724 has an implementation idea on how to fix this. Would there be interest in a full patch?

Ok, just tested replying to the email inline, and I get the following error:
"""Your email to Phabricator was not processed, because an error occurred while
trying to handle it:

@epriestly - thx for the heads up. /me fears the next integrate ;)

In T8336#116857, @epriestley wrote:

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]

Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing Settings → Email Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

In T8336#116839, @epriestley wrote:

ability to enforce selecting a mailing list to cc

I think this probably remains too niche and complex to have a place in the upstream.

replies to review comments and the patch should be in the same email

This might be implementable as a Custom Field (i.e., an extension instead of a patch) now, which could reduce the cost of maintaining it. It's possible we could upstream it after T6030.

control over what gets sent - sending everything somebody does in the app that is not relevant to the review is not adding value

T5791 and T4103 are the paths forward for this in the upstream, depending on how much control you want.

Actually works quite nice (I have worded the text to suggest to create a new revision). Now I have to find out where I can make the warning field screaming red ;)

Ah, this works, but only after-the-fact. The problem is when we add a mailing list to cc' after the fact, the mailing list doesn't get a mail that looks like the "request for review" mail, so people on the list are missing context (which is another thing we want to fix but haven't gotten around to).

Another reason is that the email parsing doesn't work for inline replies, and we basically always do inline replies.
So if we replied only to phab, we'd basically restrict how users are able to write emails, which they will simply not accept.

Ah, sorry, just read the commit comment...

I just wanted to try it - any reason why this was reverted again?

The problem is that many people only use the mailing list and don't do
reviews via phab themselves (but still want to be part of other people's
review threads). I think having different flows for phab vs non phab is not
going to fly for us.
btrahan added a subscriber: btrahan.
btrahan added a comment.

That definitely sounds like a very good first step. We would reevaluate how often unintentional private reviews happen afterwards. Thanks!

http://reviews.llvm.org/differential/diff/9189/

I tried to reply this via email, but email delivery failed
"""
Delivery to the following recipient failed permanently:

We have a setup where:

• all phab reviews also go to a mailing list as well as to the specified reviewers (if any)
• people reply on the mailing list to phab review emails via reply-all
• we do not want phab to be in the critical path for reviews, thus a reply-on-to-phab setup is not an option

I think getting to an 95% solution shouldn't be too hard (mainly effort, which is why I haven't done it myself yet), and just including most of the email for the 5% where we don't get it right seems better than not having the comments in the phab thread at all. Then I'd guess it's 1. wait for bug filed, 2. add regexp, 3. redo from start.

This is getting more and more of an issue - now the reverse case happened - we had a user set up a phabricator user name that (by chance) matched the subversion user name of a different user.

The message exists, and looks exactly normal:
PROPERTIES
ID: 30951
Status:
Related PHID:
Author PHID:
Message ID Hash: BvDfx.DmqDTw

Ah, I think with one of the recent integrates I'm back at the point where I get an error due to an empty message body. I remember inserting code to switch that error off, but I'm unsure of where I had to pull the lever...

Btw, this is becoming more and more annoying - if there are any tips on how I can debug this, that would be highly appreciated :) (the problem might well be related to my local changes...)

Ok, that worked... what's the best way to put the email up without it
getting indexed by search engines (the content is all public, as it goes to
a public mailing list anyway, but I don't want the full mail addresses to
show up indexed in search engines).

I by now had multiple people reporting this problem. Do you still have
admin access to our phab instance (and can you get to the stuff there?)
Otherwise I'll dig for the original...

Ah, sorry - I overlooked the notice because it was in the middle of the comments section (which was already large) and the file section.