Page MenuHomePhabricator

Add user preference for diffs in emails
Closed, WontfixPublic


Fancy web diffs are nice, and great because of their commenting features. However, getting notified just with abbreviated mails means less eyes on any given commit because the threshold for viewing the actual commit is higher. Being able to include the actual commit diffs has a number of benefits:

  • I'll casually look at some the more important commits (or just some of the shorter, easy to read ones) that are going on in our project, and passively learn/observe other people's code.
  • My boss can (and will) read commit diffs while he's on a plane, without internet access.
  • There will be useful comments or fixes from people who weren't reviewers but ended up reading the commit diffs anyway.
  • Including the commit diff means including the commit message, which is educational even for people who decide not to bother with the code this time around.

Ideally there would be a setting in the Herald commit notification email rule that lets the user specify how large of a commit diff should maximally be included in the mail, to avoid potential data volume issues but still get the gist of it. The email format could be the same as now, with diffs being (optionally) appended to the commit summary.

Event Timeline

jpetso raised the priority of this task from to Needs Triage.
jpetso updated the task description. (Show Details)
jpetso added a project: Diffusion.
jpetso updated the task description. (Show Details)
jpetso added a subscriber: jpetso.

There are config settings for this already. Take a look at metamta.diffusion.inline-patches (which sets the maximum patch length inlined).

To be honest i was looking for the setting in the mail category first too.

Good point, I actually seem to have set that setting to 1000 before. However, none of our commit emails ever seems to include patches even below that number of lines. Maybe this has been fixed since I last updated our Phabricator instance?

Oh wait, I set it for Differential but not Diffusion. Let me verify that it works and I'll close the bug after. Thanks!

Yep. try setting both.

Yeah I think this should be moved to personal settings, not sure if there is a technical reason not to.

Hm, I just pushed a couple of small changes to master and the commit email still only showed the summary. I had metamta.diffusion.attach-patches set to true and and metameta.diffusion.inline-patches set to 5000 (which should be more than enough for the few lines that were changed). The ones for metamta.differential were also set accordingly, although I guess that shouldn't matter as it didn't go through a code review in this case.

I'll try updating our Phabricator instance tonight to confirm that this is still the case with the latest version. Unless there's something else that I could have forgotten to set up?

Changing config values should prompt you to restart the webserver/daemons.

Restarting Phabricator hasn't made a difference. Will play around with it more tonight to see where things are still going wrong. Thanks for all your input, it's much appreciated :)

I remember having to do something else for this to work. Unfortunately i can't remember what :(
The feature works fine for me.

I agree that this might be better in personal settings. However we cannot do this until T6367 is fixed.
Currenty the email is sent with the user initiating the actions settings/locales.

Okay, so... I updated and that didn't help, but what did help was the realization that diffs are only included in (Herald rule) "Commit Hook: Commit Content" emails, never in "Commits" (i.e. push) emails even with the inlined-patches setting enabled. That means I'll have to send out multiple emails for pushes with multiple commits, but at least it works now. Thanks for helping out!

Would you like to keep this task around for the setting to be moved into either the Herald rule action or user settings?

chad renamed this task from Include commit diff in emails to Add user preference for diffs in emails.Feb 4 2015, 5:04 PM
chad triaged this task as Wishlist priority.
chad added projects: Mail, Differential.
chad added a subscriber: metagriffin.

Adding T7378 here, any resolution of this task should include "diffs" in other applications as well (like Phriction).

not sure if there is a technical reason not to.

  • On many installs we send one email to all recipients (per, so we can not customize the email per user in any way (subject, headers, body, etc).
  • As @fabe mentions, we also can not build separate bodies for each user until after T6367.
eadler added a project: Restricted Project.Jan 9 2016, 12:37 AM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

Does "Restricted project" now mean that this feature is no longer considered for the open source Phabricator?

Restricted Project means you don't have permissions to view that specific Project.

No. Several organizations maintain private projects on this install which they use to express interest in various features to the Phabricator upstream and keep track of relevant changes and progress. One such organization is interested in this feature. It is now more likely that this feature will see the light of day, as it has a powerful enterprise backing it in some capacity.

Can anyone interested in this explain the use case for making it per-Herald or per-user instead of global? Are half your users bitterly opposed to inline diffs, even after we fixed the Differential ones to not be terrible (T10694)?

eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jul 4 2016, 9:08 PM
epriestley claimed this task.

Per T7047#180833, no idea what the use case is for per-user/per-Herald/per-Mail.

  • Inline comment context diffs are now always on for Differential and can not be disabled. We haven't seen negative feedback about them. They will come to Diffusion in T10978.
  • Prose diffs are now always on. We haven't seen negative feedback about them, but see T7643 for planned improvements.
  • Inline diff content has always been globally configurable. It now works well in Differential. It will work better in Diffusion after T11029.
  • It's unclear that any use case exists for granular (e.g., per-user) settings here. See T8227 for why new settings face a high barrier to coming upstream.