Page MenuHomePhabricator

klimek (Manuel Klimek)
User

Projects

Today

  • Clear sailing ahead.

Tomorrow

  • Clear sailing ahead.

Sunday

  • Clear sailing ahead.

User Details

User Since
Jul 3 2012, 3:29 PM (612 w, 2 d)
Availability
Available

Recent Activity

Jan 3 2019

klimek added a comment to T4926: When emails are sent to new people (or lists) they should contain the change description and patch.

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.

Jan 3 2019, 11:18 AM · llvm, Differential, Mail

Oct 6 2016

klimek added a comment to T10694: Improve behavior of context for inline comments in mail.

@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 :)

Oct 6 2016, 10:39 AM · Prioritized, Restricted Project, Differential, Mail
klimek added a comment to T10694: Improve behavior of context for inline comments in mail.

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

Oct 6 2016, 10:28 AM · Prioritized, Restricted Project, Differential, Mail

Apr 13 2016

klimek added a comment to T5181: Parse in-line replies in email responses.

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"):

Apr 13 2016, 2:15 PM · llvm, Restricted Project, Mail
klimek added a comment to T5181: Parse in-line replies in email responses.

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.

Apr 13 2016, 12:57 PM · llvm, Restricted Project, Mail

Aug 10 2015

klimek added a comment to T5181: Parse in-line replies in email responses.

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

Aug 10 2015, 10:59 AM · llvm, Restricted Project, Mail

Jul 3 2015

klimek abandoned D13526: Special-case text sections with empty headers in emails..
Jul 3 2015, 4:55 PM

Jul 2 2015

klimek retitled D13527: Add List-Id header to mails sent as user. from to Add List-Id header to mails sent as user..
Jul 2 2015, 1:06 PM
klimek retitled D13526: Special-case text sections with empty headers in emails. from to Special-case text sections with empty headers in emails..
Jul 2 2015, 12:21 PM
klimek added a comment to T8336: Make a workflow that integrates nicely with list based email/patch reviews.

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 SettingsEmail 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)
Jul 2 2015, 10:19 AM · llvm, FreeBSD, Herald
klimek added a comment to T8336: Make a workflow that integrates nicely with list based email/patch reviews.

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 SettingsEmail 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.

Jul 2 2015, 8:13 AM · llvm, FreeBSD, Herald

Jul 1 2015

klimek updated subscribers of T8730: Special-case text sections with empty headers.
Jul 1 2015, 2:43 PM · Mail
klimek created T8730: Special-case text sections with empty headers.
Jul 1 2015, 2:42 PM · Mail
klimek added a comment to T1735: Improve implementation of "Re:" prefix by dropping it from messages which are unambiguously first-in-thread.

Awesome docs, thanks!

Jul 1 2015, 12:06 PM · llvm, Restricted Project, Mail
klimek updated subscribers of T1735: Improve implementation of "Re:" prefix by dropping it from messages which are unambiguously first-in-thread.

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

Jul 1 2015, 7:57 AM · llvm, Restricted Project, Mail
klimek added a comment to T1837: Implement correct send-as-user behavior..

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

Jul 1 2015, 7:56 AM · llvm, Restricted Project, Mail

Jun 30 2015

klimek added a comment to T5234: Make sending email replies in error cases optional.

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:

Jun 30 2015, 4:47 PM
klimek merged T8724: Add List-Id field to emails to mailing lists, otherwise most mail programs will reject the email when sent as-user into T1837: Implement correct send-as-user behavior..
Jun 30 2015, 4:27 PM · llvm, Restricted Project, Mail
klimek merged task T8724: Add List-Id field to emails to mailing lists, otherwise most mail programs will reject the email when sent as-user into T1837: Implement correct send-as-user behavior..
Jun 30 2015, 4:27 PM · Mail
klimek changed the visibility for T1837: Implement correct send-as-user behavior..
Jun 30 2015, 4:26 PM · llvm, Restricted Project, Mail
klimek created T8725: Allow use of Re: only for non-first emails.
Jun 30 2015, 4:18 PM · Mail
klimek created T8724: Add List-Id field to emails to mailing lists, otherwise most mail programs will reject the email when sent as-user.
Jun 30 2015, 4:15 PM · Mail

Jun 3 2015

klimek added a comment to T8387: Convert Mailing Lists into special users, similar to bot users.

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

Jun 3 2015, 12:57 PM · FreeBSD, Mailing Lists, Mail

May 28 2015

klimek added a comment to T8336: Make a workflow that integrates nicely with list based email/patch reviews.

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 SettingsEmail 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".

May 28 2015, 3:42 PM · llvm, FreeBSD, Herald
klimek added a comment to T8336: Make a workflow that integrates nicely with list based email/patch reviews.

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.

May 28 2015, 3:08 PM · llvm, FreeBSD, Herald
klimek created T8336: Make a workflow that integrates nicely with list based email/patch reviews.
May 28 2015, 8:23 AM · llvm, FreeBSD, Herald

Jan 20 2015

klimek created T6993: Sort names in name completion by probability.
Jan 20 2015, 3:10 PM

Aug 20 2014

klimek added a comment to D10278: Fix sending email as user / via sendgrid..

Awesome. So what about the rest? I'm happy with keeping this in our branch (I think it's never led to a merge problem), but I also think it's super useful to at least have documented somewhere if we don't want to put this into the main branch (it took me a day to figure out what the right solution is (after talking to some of my colleagues), so if somebody else hits this they'll probably also be wasting a lot of time on it; figuring out why mail is rejected is about the least fun thing one can work on).

Aug 20 2014, 1:10 AM

Aug 15 2014

klimek added a comment to D10278: Fix sending email as user / via sendgrid..
  • Is disabling metamta.can-send-as-user a reasonable alternative to "List-Id"?
Aug 15 2014, 8:36 PM
klimek retitled D10278: Fix sending email as user / via sendgrid. from to Fix sending email as user / via sendgrid..
Aug 15 2014, 5:54 PM
klimek added inline comments to D10146: Add an option to put comment context into emails..
Aug 15 2014, 5:13 PM
klimek updated the diff for D10146: Add an option to put comment context into emails..
  • Address review comments
  • Merge to head
Aug 15 2014, 5:12 PM

Aug 13 2014

klimek added a comment to D10146: Add an option to put comment context into emails..

Regarding your effort to maintain this, a few thoughts:

  1. Thank you so much for doing it; no matter whether you take this in the end, I really appreciate your feedback and time; I understand the trade-off between features and maintenance headaches, so I fully appreciate if we have to keep our own fork.
  2. That said, keeping our own fork often feels very very frustrating; I know that's not your problem, but I think that 99.9% of the time I've spent countless hours in syncing and resolving merge conflicts would have cost no time if the person writing the feature (-> you ;) would have changed the code with the refactoring (I love the refactorings, just not the merging ...); that would save so much of my time that you dropping me an email whenever there is a problem with this code so I can resolve this would be immediately prioritized for me, based on that I would lose so much if I have to keep our own fork
  3. <<</me opens the bag and pulls out a carrot; the carrot looks somewhat translucent, so you can't really be sure whether it exists>>>: we would very much like not having to maintain our own instance, and we'd like to become paying customers (the countless hours I throw at maintaining our own GCE instance with this stuff are beyond reasonable); not sure whether you still aim at providing such a service though...
  4. I think the feature might not be used very much until now because it's not feature complete yet; I think the comments in the diff actually make the whole email diff thingy much more useful, and if we add parsing in-line replies and adding them to comment hunks as replies, that would make it even more useful (it would mean I could just do reviews completely by email if I have enough context); but of course I would say that, so probably not a very convincing argument on its own :P
Aug 13 2014, 8:30 AM
klimek updated the diff for D10146: Add an option to put comment context into emails..

Address review comments.

Aug 13 2014, 8:20 AM

Aug 12 2014

klimek added a comment to D10146: Add an option to put comment context into emails..

Ping.

Aug 12 2014, 11:50 AM

Aug 5 2014

klimek retitled D10146: Add an option to put comment context into emails. from to Add an option to put comment context into emails..
Aug 5 2014, 1:00 PM
klimek added a comment to D9994: Rename `PhutilRemarkupRule` subclasses.
In D9994#22, @klimek wrote:

I think this broke my install - told me something about running "arc liberate", but running that didn't fix the issue.

Aug 5 2014, 12:38 PM
klimek added a comment to D9994: Rename `PhutilRemarkupRule` subclasses.

I think this broke my install - told me something about running "arc liberate", but running that didn't fix the issue.

Aug 5 2014, 12:12 PM

Aug 4 2014

klimek created T5786: Separate Differential reviewer warnings from the reviewers field.
Aug 4 2014, 3:52 PM · Custom Fields, Differential
klimek added a comment to T5495: Configurable warning when not subscribing a mailing list.

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 ;)

Aug 4 2014, 3:43 PM · Custom Fields, Differential
klimek added a comment to T5495: Configurable warning when not subscribing a mailing list.

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).

Aug 4 2014, 3:24 PM · Custom Fields, Differential
klimek added a comment to T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.

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.

Aug 4 2014, 3:06 PM
klimek added a comment to T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.

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

Aug 4 2014, 3:01 PM
klimek added a comment to T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.

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

Aug 4 2014, 3:00 PM

Jul 11 2014

klimek added a comment to T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.

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.

Jul 11 2014, 1:15 PM

Jun 27 2014

klimek added a comment to T5495: Configurable warning when not subscribing a mailing list.

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

Jun 27 2014, 9:34 AM · Custom Fields, Differential
klimek created T5495: Configurable warning when not subscribing a mailing list.
Jun 27 2014, 7:35 AM · Custom Fields, Differential

Jun 10 2014

klimek added a comment to T5303: Differential is set to public access (without login), but accessing diffs requires login.

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

Jun 10 2014, 12:51 PM · Differential
klimek created T5303: Differential is set to public access (without login), but accessing diffs requires login.
Jun 10 2014, 10:30 AM · Differential

Jun 5 2014

klimek added a comment to T5234: Make sending email replies in error cases optional.

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

Jun 5 2014, 10:58 AM

May 31 2014

klimek created T5234: Make sending email replies in error cases optional.
May 31 2014, 6:58 AM
klimek created T5233: PHPMailer email verification incorrect.
May 31 2014, 6:55 AM

May 26 2014

klimek added a comment to T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.

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
May 26 2014, 5:42 PM
klimek created T5185: Comment emails generated from parsing incoming email replies should not go to people already on the To/CC of the incoming email.
May 26 2014, 12:38 PM
klimek added a comment to T5181: Parse in-line replies in email responses.

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.

May 26 2014, 11:50 AM · llvm, Restricted Project, Mail
klimek created T5181: Parse in-line replies in email responses.
May 26 2014, 6:29 AM · llvm, Restricted Project, Mail

Apr 30 2014

klimek created T4926: When emails are sent to new people (or lists) they should contain the change description and patch.
Apr 30 2014, 8:15 AM · llvm, Differential, Mail

Jan 7 2014

klimek added a comment to T1731: Allow users to set their VCS names.

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.

Jan 7 2014, 10:13 AM · FreeBSD, Diffusion

Dec 11 2013

klimek added a comment to T4107: Add explicit logs for Mail and SSH.

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

Dec 11 2013, 4:38 PM · Mail
klimek added a comment to T4107: Add explicit logs for Mail and SSH.

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...

Dec 11 2013, 4:14 PM · Mail
klimek added a comment to T4107: Add explicit logs for Mail and SSH.

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...)

Dec 11 2013, 2:49 PM · Mail

Dec 6 2013

klimek raised the priority of T4208: Allow using arbitrary commits as top for arc diff from to Needs Triage.
Dec 6 2013, 2:50 PM

Nov 20 2013

klimek raised the priority of T4135: Possible display bug due to unicode (?) from to Needs Triage.
Nov 20 2013, 2:28 PM

Nov 18 2013

klimek added a comment to T4107: Add explicit logs for Mail and SSH.

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).

Nov 18 2013, 5:33 PM · Mail
klimek added a comment to T4107: Add explicit logs for Mail and SSH.

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...

Nov 18 2013, 5:10 PM · Mail

Nov 15 2013

klimek raised the priority of T4107: Add explicit logs for Mail and SSH from to Needs Triage.
Nov 15 2013, 9:42 AM · Mail

Oct 11 2013

klimek added a comment to T3942: Make messaging for 100+ click-to-load changeset behavior more clear.

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

Oct 11 2013, 6:38 PM · Design, Differential
klimek raised the priority of T3942: Make messaging for 100+ click-to-load changeset behavior more clear from to Needs Triage.
Oct 11 2013, 4:55 PM · Design, Differential