Page MenuHomePhabricator

show merged commits in herald emails for merge commits

Authored by fabe on May 25 2015, 10:31 AM.



Includes a new Block in Herad emails which tells the user about which commits were merged in a merge commit
Otherwise the email would just say "Merge branch XYZ". Ref T8295

Test Plan

imported various commits (and merges) and watched resulting herald emails for all of them

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fabe updated this revision to Diff 31326.May 25 2015, 10:31 AM
fabe retitled this revision from to show merged commits in herald emails for merge commits.
fabe updated this object.
fabe edited the test plan for this revision. (Show Details)
fabe updated this revision to Diff 31330.May 25 2015, 10:59 AM
fabe edited edge metadata.
  • add diffusion.fields config and disable mergedcommits by default & celerity map fix
epriestley requested changes to this revision.May 25 2015, 11:22 AM
epriestley added a reviewer: epriestley.

Some minor stuff inline, this generally looks good to me.


This should be the actual viewer -- I think $this->getViewer() will work.


I think this is no longer used for anything and has no effect (we determine it automatically in all cases, now).


Using ConduitCall directly (as in PhabricatorCommitBranchesField) might be a little simpler than building a DiffusionRequest.


Use real viewer.


Use pht().


Maybe simpler with foreach ($merges as $merge).


Maybe put $ex->getMessage() in the mail body? So we end up with something like:

Error generating merged commits: blah blah blah useful error message.

That seems more obvious/helpful than just dumping it to the webserver error log.

This revision now requires changes to proceed.May 25 2015, 11:22 AM
fabe updated this revision to Diff 31340.May 25 2015, 11:47 AM
fabe edited edge metadata.
  • fix inlines
epriestley accepted this revision.May 25 2015, 11:48 AM
epriestley edited edge metadata.
This revision is now accepted and ready to land.May 25 2015, 11:48 AM
This revision was automatically updated to reflect the committed changes.