Record new commits in the push log

Summary:
Ref T4195. Like the previous diffs, these both create a useful log and give us an object to hand off to Herald.

Surface this information in Diffusion, too, and clean things up a little bit.

Test Plan: {F87565}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7718

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -403,8 +403,10 @@ final class DiffusionCommitController extends DiffusionController {
     array $audit_requests) {
 
     assert_instances_of($parents, 'PhabricatorRepositoryCommit');
-    $user = $this->getRequest()->getUser();
+    $viewer = $this->getRequest()->getUser();
     $commit_phid = $commit->getPHID();
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
 
     $edge_query = id(new PhabricatorEdgeQuery())
       ->withSourcePHIDs(array($commit_phid))
@@ -440,6 +442,29 @@ final class DiffusionCommitController extends DiffusionController {
       }
     }
 
+    // NOTE: We should never normally have more than a single push log, but
+    // it can occur naturally if a commit is pushed, then the branch it was
+    // on is deleted, then the commit is pushed again (or through other similar
+    // chains of events). This should be rare, but does not indicate a bug
+    // or data issue.
+
+    // NOTE: We never query push logs in SVN because the commiter is always
+    // the pusher and the commit time is always the push time; the push log
+    // is redundant and we save a query by skipping it.
+
+    $push_logs = array();
+    if ($repository->isHosted() && !$repository->isSVN()) {
+      $push_logs = id(new PhabricatorRepositoryPushLogQuery())
+        ->setViewer($viewer)
+        ->withRepositoryPHIDs(array($repository->getPHID()))
+        ->withNewRefs(array($commit->getCommitIdentifier()))
+        ->withRefTypes(array(PhabricatorRepositoryPushLog::REFTYPE_COMMIT))
+        ->execute();
+      foreach ($push_logs as $log) {
+        $phids[] = $log->getPusherPHID();
+      }
+    }
+
     $handles = array();
     if ($phids) {
       $handles = $this->loadViewerHandles($phids);
@@ -494,13 +519,52 @@ final class DiffusionCommitController extends DiffusionController {
       }
     }
 
-    $props['Committed'] = phabricator_datetime($commit->getEpoch(), $user);
+    if (!$repository->isSVN()) {
+      $authored_info = id(new PHUIStatusItemView());
+      // TODO: In Git, a distinct authorship date is available. When present,
+      // we should show it here.
 
-    $author_phid = $data->getCommitDetail('authorPHID');
-    if ($data->getCommitDetail('authorPHID')) {
-      $props['Author'] = $handles[$author_phid]->renderLink();
-    } else {
-      $props['Author'] = $data->getAuthorName();
+      $author_phid = $data->getCommitDetail('authorPHID');
+      $author_name = $data->getAuthorName();
+      if ($author_phid) {
+        $authored_info->setTarget($handles[$author_phid]->renderLink());
+      } else if (strlen($author_name)) {
+        $authored_info->setTarget($author_name);
+      }
+
+      $props['Authored'] = id(new PHUIStatusListView())
+        ->addItem($authored_info);
+    }
+
+    $committed_info = id(new PHUIStatusItemView())
+      ->setNote(phabricator_datetime($commit->getEpoch(), $viewer));
+
+    $committer_phid = $data->getCommitDetail('committerPHID');
+    $committer_name = $data->getCommitDetail('committer');
+    if ($committer_phid) {
+      $committed_info->setTarget($handles[$committer_phid]->renderLink());
+    } else if (strlen($committer_name)) {
+      $committed_info->setTarget($committer_name);
+    } else if ($author_phid) {
+      $committed_info->setTarget($handles[$author_phid]->renderLink());
+    } else if (strlen($author_name)) {
+      $committed_info->setTarget($author_name);
+    }
+
+    $props['Comitted'] = id(new PHUIStatusListView())
+      ->addItem($committed_info);
+
+    if ($push_logs) {
+      $pushed_list = new PHUIStatusListView();
+
+      foreach ($push_logs as $push_log) {
+        $pushed_item = id(new PHUIStatusItemView())
+          ->setTarget($handles[$push_log->getPusherPHID()]->renderLink())
+          ->setNote(phabricator_datetime($push_log->getEpoch(), $viewer));
+        $pushed_list->addItem($pushed_item);
+      }
+
+      $props['Pushed'] = $pushed_list;
     }
 
     $reviewer_phid = $data->getCommitDetail('reviewerPHID');
@@ -508,16 +572,6 @@ final class DiffusionCommitController extends DiffusionController {
       $props['Reviewer'] = $handles[$reviewer_phid]->renderLink();
     }
 
-    $committer = $data->getCommitDetail('committer');
-    if ($committer) {
-      $committer_phid = $data->getCommitDetail('committerPHID');
-      if ($data->getCommitDetail('committerPHID')) {
-        $props['Committer'] = $handles[$committer_phid]->renderLink();
-      } else {
-        $props['Committer'] = $committer;
-      }
-    }
-
     if ($revision_phid) {
       $props['Differential Revision'] = $handles[$revision_phid]->renderLink();
     }
@@ -530,8 +584,6 @@ final class DiffusionCommitController extends DiffusionController {
       $props['Parents'] = phutil_implode_html(" \xC2\xB7 ", $parent_links);
     }
 
-    $request = $this->getDiffusionRequest();
-
     $props['Branches'] = phutil_tag(
       'span',
       array(
@@ -545,7 +597,7 @@ final class DiffusionCommitController extends DiffusionController {
       ),
       pht('Unknown'));
 
-    $callsign = $request->getRepository()->getCallsign();
+    $callsign = $repository->getCallsign();
     $root = '/diffusion/'.$callsign.'/commit/'.$commit->getCommitIdentifier();
     Javelin::initBehavior(
       'diffusion-commit-branches',
@@ -554,7 +606,7 @@ final class DiffusionCommitController extends DiffusionController {
         $root.'/tags/' => 'commit-tags',
       ));
 
-    $refs = $this->buildRefs($request);
+    $refs = $this->buildRefs($drequest);
     if ($refs) {
       $props['References'] = $refs;
     }

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -18,6 +18,7 @@ final class DiffusionCommitHookEngine extends Phobject {
   private $subversionRepository;
   private $remoteAddress;
   private $remoteProtocol;
+  private $transactionKey;
 
   public function setRemoteProtocol($remote_protocol) {
     $this->remoteProtocol = $remote_protocol;
@@ -37,6 +38,23 @@ final class DiffusionCommitHookEngine extends Phobject {
     return $this->remoteAddress;
   }
 
+  private function getRemoteAddressForLog() {
+    // If whatever we have here isn't a valid IPv4 address, just store `null`.
+    // Older versions of PHP return `-1` on failure instead of `false`.
+    $remote_address = $this->getRemoteAddress();
+    $remote_address = max(0, ip2long($remote_address));
+    $remote_address = nonempty($remote_address, null);
+    return $remote_address;
+  }
+
+  private function getTransactionKey() {
+    if (!$this->transactionKey) {
+      $entropy = Filesystem::readRandomBytes(64);
+      $this->transactionKey = PhabricatorHash::digestForIndex($entropy);
+    }
+    return $this->transactionKey;
+  }
+
   public function setSubversionTransactionInfo($transaction, $repository) {
     $this->subversionTransaction = $transaction;
     $this->subversionRepository = $repository;
@@ -89,6 +107,18 @@ final class DiffusionCommitHookEngine extends Phobject {
     return $err;
   }
 
+  private function newPushLog() {
+    return PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer())
+      ->setRepositoryPHID($this->getRepository()->getPHID())
+      ->setEpoch(time())
+      ->setRemoteAddress($this->getRemoteAddressForLog())
+      ->setRemoteProtocol($this->getRemoteProtocol())
+      ->setTransactionKey($this->getTransactionKey())
+      ->setRejectCode(PhabricatorRepositoryPushLog::REJECT_ACCEPT)
+      ->setRejectDetails(null);
+  }
+
+
   /**
    * @task git
    */
@@ -106,34 +136,17 @@ final class DiffusionCommitHookEngine extends Phobject {
     // TODO: Now, do content checks.
 
     // TODO: Generalize this; just getting some data in the database for now.
-    $transaction_key = PhabricatorHash::digestForIndex(
-      Filesystem::readRandomBytes(64));
-
-    // If whatever we have here isn't a valid IPv4 address, just store `null`.
-    // Older versions of PHP return `-1` on failure instead of `false`.
-    $remote_address = $this->getRemoteAddress();
-    $remote_address = max(0, ip2long($remote_address));
-    $remote_address = nonempty($remote_address, null);
-
-    $remote_protocol = $this->getRemoteProtocol();
 
     $logs = array();
     foreach ($updates as $update) {
-      $log = PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer())
-        ->setRepositoryPHID($this->getRepository()->getPHID())
-        ->setEpoch(time())
-        ->setRemoteAddress($remote_address)
-        ->setRemoteProtocol($remote_protocol)
-        ->setTransactionKey($transaction_key)
+      $log = $this->newPushLog()
         ->setRefType($update['type'])
         ->setRefNameHash(PhabricatorHash::digestForIndex($update['ref']))
         ->setRefNameRaw($update['ref'])
         ->setRefNameEncoding(phutil_is_utf8($update['ref']) ? 'utf8' : null)
         ->setRefOld($update['old'])
         ->setRefNew($update['new'])
-        ->setMergeBase(idx($update, 'merge-base'))
-        ->setRejectCode(PhabricatorRepositoryPushLog::REJECT_ACCEPT)
-        ->setRejectDetails(null);
+        ->setMergeBase(idx($update, 'merge-base'));
 
       $flags = 0;
       if ($update['operation'] == 'create') {
@@ -151,6 +164,18 @@ final class DiffusionCommitHookEngine extends Phobject {
       $logs[] = $log;
     }
 
+    // Now, build logs for all the commits.
+    // TODO: Generalize this, too.
+    $commits = array_mergev(ipull($updates, 'commits'));
+    $commits = array_unique($commits);
+    foreach ($commits as $commit) {
+      $log = $this->newPushLog()
+        ->setRefType(PhabricatorRepositoryPushLog::REFTYPE_COMMIT)
+        ->setRefNew($commit)
+        ->setChangeFlags(PhabricatorRepositoryPushLog::CHANGEFLAG_ADD);
+      $logs[] = $log;
+    }
+
     foreach ($logs as $log) {
       $log->save();
     }

src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php

@@ -6,6 +6,8 @@ final class PhabricatorRepositoryPushLogQuery
   private $ids;
   private $repositoryPHIDs;
   private $pusherPHIDs;
+  private $refTypes;
+  private $newRefs;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -22,6 +24,16 @@ final class PhabricatorRepositoryPushLogQuery
     return $this;
   }
 
+  public function withRefTypes(array $ref_types) {
+    $this->refTypes = $ref_types;
+    return $this;
+  }
+
+  public function withNewRefs(array $new_refs) {
+    $this->newRefs = $new_refs;
+    return $this;
+  }
+
   protected function loadPage() {
     $table = new PhabricatorRepositoryPushLog();
     $conn_r = $table->establishConnection('r');
@@ -86,6 +98,20 @@ final class PhabricatorRepositoryPushLogQuery
         $this->pusherPHIDs);
     }
 
+    if ($this->refTypes) {
+      $where[] = qsprintf(
+        $conn_r,
+        'refType IN (%Ls)',
+        $this->refTypes);
+    }
+
+    if ($this->newRefs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'refNew IN (%Ls)',
+        $this->newRefs);
+    }
+
     $where[] = $this->buildPagingClause($conn_r);
 
     return $this->formatWhereClause($where);

src/view/phui/PHUIStatusListView.php

@@ -6,6 +6,7 @@ final class PHUIStatusListView extends AphrontTagView {
 
   public function addItem(PHUIStatusItemView $item) {
     $this->items[] = $item;
+    return $this;
   }
 
   protected function canAppendChild() {