Improve some diffusioney things.

Summary: Test Plan: Reviewers: CC:
2024-11-26 00:32:42 +01:00 · 2011-03-12 22:51:40 -08:00 · 2011-03-12 22:51:40 -08:00 · 383b3d740c
commit 383b3d740c
parent 485b5e5ded
17 changed files with 233 additions and 52 deletions
--- a/resources/sql/patches/006.repository.sql
+++ b/resources/sql/patches/006.repository.sql
@ -42,3 +42,13 @@ create table phabricator_repository.repository_filesystem (
 );

 alter table repository_filesystem add key (repositoryID, svnCommit);
+
+truncate phabricator_repository.repository_commit;
+alter table phabricator_repository.repository_commit
+  change repositoryPHID repositoryID int unsigned not null;
+alter table phabricator_repository.repository_commit drop key repositoryPHID;
+alter table phabricator_repository.repository_commit add unique key
+  (repositoryID, commitIdentifier(16));
+alter table phabricator_repository.repository_commit add key
+  (repositoryID, epoch);
+
--- a/src/applications/diffusion/controller/home/DiffusionHomeController.php
+++ b/src/applications/diffusion/controller/home/DiffusionHomeController.php
@ -27,24 +27,49 @@ class DiffusionHomeController extends DiffusionController {
    $commit = new PhabricatorRepositoryCommit();
    $conn_r = $commit->establishConnection('r');

-    // TODO: Both these queries are basically bogus and have total trash for
-    // query plans, and don't return the right results. Build a cache instead.
-    // These are just pulling data with approximately the right look to it.
-    $commits = $commit->loadAllWhere(
-      '1 = 1 GROUP BY repositoryPHID');
-    $commits = mpull($commits, null, 'getRepositoryPHID');
+    // TODO: These queries are pretty bogus.

-    $commit_counts = queryfx_all(
-      $conn_r,
-      'SELECT repositoryPHID, count(*) N FROM %T
-        GROUP BY repositoryPHID',
+    $commits = array();
+    $commit_counts = array();
+
+    $max_epoch = queryfx_all(
+      $commit->establishConnection('r'),
+      'SELECT repositoryID, MAX(epoch) maxEpoch FROM %T GROUP BY repositoryID',
      $commit->getTableName());
-    $commit_counts = ipull($commit_counts, 'N', 'repositoryPHID');
+
+    if ($max_epoch) {
+      $sql = array();
+      foreach ($max_epoch as $head) {
+        $sql[] = '('.(int)$head['repositoryID'].', '.(int)$head['maxEpoch'].')';
+      }
+
+      // NOTE: It's possible we'll pull multiple commits for some repository
+      // here but it reduces query cost around 3x to unique them in PHP rather
+      // than apply GROUP BY in MySQL.
+      $commits = $commit->loadAllWhere(
+        '(repositoryID, epoch) IN (%Q)',
+        implode(', ', $sql));
+      $commits = mpull($commits, null, 'getRepositoryID');
+
+      $commit_counts = queryfx_all(
+        $conn_r,
+        'SELECT repositoryID, count(*) N FROM %T
+          GROUP BY repositoryID',
+        $commit->getTableName());
+      $commit_counts = ipull($commit_counts, 'N', 'repositoryID');
+    }

    $rows = array();
    foreach ($repositories as $repository) {
-      $phid = $repository->getPHID();
-      $commit = idx($commits, $phid);
+      $id = $repository->getID();
+      $commit = idx($commits, $id);
+      $date = null;
+      $time = null;
+      if ($commit) {
+        $date = date('M j, Y', $commit->getEpoch());
+        $time = date('g:i A', $commit->getEpoch());
+      }
+
      $rows[] = array(
        phutil_render_tag(
          'a',
@ -53,13 +78,14 @@ class DiffusionHomeController extends DiffusionController {
          ),
          phutil_escape_html($repository->getName())),
        $repository->getVersionControlSystem(),
-        idx($commit_counts, $phid, 0),
+        idx($commit_counts, $id, 0),
        $commit
-          ? $commit->getCommitIdentifier()
-          : null, // TODO: Link/format
-        $commit
-          ? phabricator_format_timestamp($commit->getEpoch())
+          ? DiffusionView::linkCommit(
+              $repository,
+              $commit->getCommitIdentifier())
          : null,
+        $date,
+        $time,
      );
    }

@ -70,11 +96,17 @@ class DiffusionHomeController extends DiffusionController {
        'VCS',
        'Size',
        'Last',
-        'Committed',
+        'Date',
+        'Time',
      ));
    $table->setColumnClasses(
      array(
        'wide',
+        '',
+        'n',
+        'n',
+        '',
+        'right',
      ));

    $panel = new AphrontPanelView();
--- a/src/applications/diffusion/controller/repository/DiffusionRepositoryController.php
+++ b/src/applications/diffusion/controller/repository/DiffusionRepositoryController.php
@ -35,8 +35,16 @@ class DiffusionRepositoryController extends DiffusionController {
    $history_table->setDiffusionRequest($drequest);
    $history_table->setHistory($history);

+    $callsign = $drequest->getRepository()->getCallsign();
+    $all = phutil_render_tag(
+      'a',
+      array(
+        'href' => "/diffusion/{$callsign}/history/",
+      ),
+      'View Full Commit History');
+
    $panel = new AphrontPanelView();
-    $panel->setHeader('Recent Commits');
+    $panel->setHeader("Recent Commits &middot; {$all}");
    $panel->appendChild($history_table);

    $content[] = $panel;
--- a/src/applications/diffusion/data/pathchange/DiffusionPathChange.php
+++ b/src/applications/diffusion/data/pathchange/DiffusionPathChange.php
@ -19,6 +19,8 @@
 final class DiffusionPathChange {

  private $commitIdentifier;
+  private $commit;
+  private $commitData;

  final public function setCommitIdentifier($commit) {
    $this->commitIdentifier = $commit;
@ -29,5 +31,49 @@ final class DiffusionPathChange {
    return $this->commitIdentifier;
  }

+  final public function setCommit($commit) {
+    $this->commit = $commit;
+    return $this;
+  }
+
+  final public function getCommit() {
+    return $this->commit;
+  }
+
+  final public function setCommitData($commit_data) {
+    $this->commitData = $commit_data;
+    return $this;
+  }
+
+  final public function getCommitData() {
+    return $this->commitData;
+  }
+
+
+  final public function getEpoch() {
+    if ($this->getCommit()) {
+      return $this->getCommit()->getEpoch();
+    }
+    return null;
+  }
+
+  final public function getAuthorName() {
+    if ($this->getCommitData()) {
+      return $this->getCommitData()->getAuthorName();
+    }
+    return null;
+  }
+
+  final public function getSummary() {
+    if (!$this->getCommitData()) {
+      return null;
+    }
+    $message = $this->getCommitData()->getCommitMessage();
+    $first = idx(explode("\n", $message), 0);
+    return substr($first, 0, 80);
+  }
+
+
+

 }
--- a/src/applications/diffusion/data/pathchange/init.php
+++ b/src/applications/diffusion/data/pathchange/init.php
@ -6,5 +6,7 @@



+phutil_require_module('phutil', 'utils');
+

 phutil_require_source('DiffusionPathChange.php');
--- a/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php
+++ b/src/applications/diffusion/query/browse/svn/DiffusionSvnBrowseQuery.php
@ -37,6 +37,12 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery {
    $paths = ipull($paths, 'id', 'path');
    $path_id = $paths[$path_normal];

+    if ($commit) {
+      $slice_clause = 'AND svnCommit <= '.(int)$commit;
+    } else {
+      $slice_clause = '';
+    }
+
    $index = queryfx_all(
      $conn_r,
      'SELECT pathID, max(svnCommit) maxCommit FROM %T WHERE
@ -45,7 +51,7 @@ final class DiffusionSvnBrowseQuery extends DiffusionBrowseQuery {
      PhabricatorRepository::TABLE_FILESYSTEM,
      $repository->getID(),
      $path_id,
-      '');
+      $slice_clause);

    if (!$index) {
      // TODO: !
--- a/src/applications/diffusion/query/history/base/DiffusionHistoryQuery.php
+++ b/src/applications/diffusion/query/history/base/DiffusionHistoryQuery.php
@ -20,6 +20,7 @@ abstract class DiffusionHistoryQuery {

  private $request;
  private $limit = 100;
+  private $offset = 0;

  final private function __construct() {
    // <private>
@ -66,5 +67,14 @@ abstract class DiffusionHistoryQuery {
    return $this->limit;
  }

+  final public function setOffset($offset) {
+    $this->offset = $offset;
+    return $this;
+  }
+
+  final public function getOffset() {
+    return $this->offset;
+  }
+
  abstract protected function executeQuery();
 }
--- a/src/applications/diffusion/query/history/git/DiffusionGitHistoryQuery.php
+++ b/src/applications/diffusion/query/history/git/DiffusionGitHistoryQuery.php
@ -23,7 +23,7 @@ final class DiffusionGitHistoryQuery extends DiffusionHistoryQuery {

    $repository = $drequest->getRepository();
    $path = $drequest->getPath();
-    $commit = $drequest->getCommit();
+    $commit_hash = $drequest->getCommit();

    $local_path = $repository->getDetail('local-path');
    $git = $drequest->getPathToGitBinary();
@ -32,31 +32,49 @@ final class DiffusionGitHistoryQuery extends DiffusionHistoryQuery {
      '(cd %s && %s log '.
        '--skip=%d '.
        '-n %d '.
-        '-M '.
-        '-C '.
-        '-B '.
-        '--find-copies-harder '.
-        '--raw '.
        '-t '.
        '--abbrev=40 '.
-        '--pretty=format:%%x1c%%H%%x1d '.
+        '--pretty=format:%%H '.
        '%s -- %s)',
      $local_path,
      $git,
-      $offset = 0,
+      $this->getOffset(),
      $this->getLimit(),
-      $commit,
+      $commit_hash,
      $path);

-    $commits = explode("\x1c", $stdout);
-    array_shift($commits); // \x1c character is first, remove empty record
+    $hashes = explode("\n", $stdout);
+    $hashes = array_filter($hashes);
+
+    $commits = array();
+    $commit_data = array();
+
+    if ($hashes) {
+      $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere(
+        'repositoryID = %d AND commitIdentifier IN (%Ls)',
+          $repository->getID(),
+        $hashes);
+      $commits = mpull($commits, null, 'getCommitIdentifier');
+      if ($commits) {
+        $commit_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere(
+          'commitID in (%Ld)',
+          mpull($commits, 'getID'));
+        $commit_data = mpull($commit_data, null, 'getCommitID');
+      }
+    }

    $history = array();
-    foreach ($commits as $commit) {
-      list($hash, $raw) = explode("\x1d", $commit);
-
+    foreach ($hashes as $hash) {
      $item = new DiffusionPathChange();
      $item->setCommitIdentifier($hash);
+      $commit = idx($commits, $hash);
+      if ($commit) {
+        $item->setCommit($commit);
+        $data = idx($commit_data, $commit->getID());
+        if ($data) {
+          $item->setCommitData($data);
+        }
+      }
      $history[] = $item;
    }

--- a/src/applications/diffusion/query/history/git/init.php
+++ b/src/applications/diffusion/query/history/git/init.php
@ -8,8 +8,11 @@

 phutil_require_module('phabricator', 'applications/diffusion/data/pathchange');
 phutil_require_module('phabricator', 'applications/diffusion/query/history/base');
+phutil_require_module('phabricator', 'applications/repository/storage/commit');
+phutil_require_module('phabricator', 'applications/repository/storage/commitdata');

 phutil_require_module('phutil', 'future/exec');
+phutil_require_module('phutil', 'utils');


 phutil_require_source('DiffusionGitHistoryQuery.php');
--- a/src/applications/diffusion/query/history/svn/DiffusionSvnHistoryQuery.php
+++ b/src/applications/diffusion/query/history/svn/DiffusionSvnHistoryQuery.php
@ -47,10 +47,36 @@ final class DiffusionSvnHistoryQuery extends DiffusionHistoryQuery {
      $commit ? $commit : 0x7FFFFFFF,
      $this->getLimit());

+    $commits = array();
+    $commit_data = array();
+
+    $commit_ids = ipull($history_data, 'commitID');
+    if ($commit_ids) {
+      $commits = id(new PhabricatorRepositoryCommit())->loadAllWhere(
+        'id IN (%Ld)',
+        $commit_ids);
+      if ($commits) {
+        $commit_data = id(new PhabricatorRepositoryCommitData())->loadAllWhere(
+          'commitID in (%Ld)',
+          $commit_ids);
+        $commit_data = mpull($commit_data, null, 'getCommitID');
+      }
+    }
+
+
+
    $history = array();
    foreach ($history_data as $row) {
      $item = new DiffusionPathChange();
-      $item->setCommitIdentifier($row['commitID']);
+      $commit = idx($commits, $row['commitID']);
+      if ($commit) {
+        $item->setCommit($commit);
+        $item->setCommitIdentifier($commit->getCommitIdentifier());
+        $data = idx($commit_data, $commit->getID());
+        if ($data) {
+          $item->setCommitData($data);
+        }
+      }
      $history[] = $item;
    }

--- a/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php
+++ b/src/applications/diffusion/view/historytable/DiffusionHistoryTableView.php
@ -30,6 +30,16 @@ final class DiffusionHistoryTableView extends DiffusionView {

    $rows = array();
    foreach ($this->history as $history) {
+      $epoch = $history->getEpoch();
+
+      if ($epoch) {
+        $date = date('M j, Y', $epoch);
+        $time = date('g:i A', $epoch);
+      } else {
+        $date = null;
+        $time = null;
+      }
+
      $rows[] = array(
        $this->linkBrowse(
          $drequest->getPath(),
@ -39,10 +49,11 @@ final class DiffusionHistoryTableView extends DiffusionView {
        self::linkCommit(
          $drequest->getRepository(),
          $history->getCommitIdentifier()),
-        '?',
-        '?',
-        '',
-        '',
+        '-',
+        $date,
+        $time,
+        phutil_escape_html($history->getAuthorName()),
+        phutil_escape_html($history->getSummary()),
        // TODO: etc etc
      );
    }
@ -54,6 +65,7 @@ final class DiffusionHistoryTableView extends DiffusionView {
        'Commit',
        'Change',
        'Date',
+        'Time',
        'Author',
        'Details',
      ));
@ -63,6 +75,7 @@ final class DiffusionHistoryTableView extends DiffusionView {
        'n',
        '',
        '',
+        'right',
        '',
        'wide wrap',
      ));
--- a/src/applications/diffusion/view/historytable/init.php
+++ b/src/applications/diffusion/view/historytable/init.php
@ -8,6 +8,9 @@

 phutil_require_module('phabricator', 'applications/diffusion/view/base');
 phutil_require_module('phabricator', 'view/control/table');
+phutil_require_module('phabricator', 'view/utils');
+
+phutil_require_module('phutil', 'markup');


 phutil_require_source('DiffusionHistoryTableView.php');
--- a/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php
+++ b/src/applications/repository/daemon/commitdiscovery/base/PhabricatorRepositoryCommitDiscoveryDaemon.php
@ -47,8 +47,8 @@ abstract class PhabricatorRepositoryCommitDiscoveryDaemon
    }

    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'repositoryPHID = %s AND commitIdentifier = %s',
-      $this->getRepository()->getPHID(),
+      'repositoryID = %s AND commitIdentifier = %s',
+      $this->getRepository()->getID(),
      $target);

    if (!$commit) {
@ -67,7 +67,7 @@ abstract class PhabricatorRepositoryCommitDiscoveryDaemon
    $repository = $this->getRepository();

    $commit = new PhabricatorRepositoryCommit();
-    $commit->setRepositoryPHID($repository->getPHID());
+    $commit->setRepositoryID($repository->getID());
    $commit->setCommitIdentifier($commit_identifier);
    $commit->setEpoch($epoch);

--- a/src/applications/repository/daemon/committask/PhabricatorRepositoryCommitTaskDaemon.php
+++ b/src/applications/repository/daemon/committask/PhabricatorRepositoryCommitTaskDaemon.php
@ -38,9 +38,8 @@ class PhabricatorRepositoryCommitTaskDaemon
        }

        // TODO: Cache these.
-        $repository = id(new PhabricatorRepository())->loadOneWhere(
-          'phid = %s',
-          $commit->getRepositoryPHID());
+        $repository = id(new PhabricatorRepository())->load(
+          $commit->getRepositoryID());

        $vcs = $repository->getVersionControlSystem();
        switch ($vcs) {
--- a/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php
+++ b/src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php
@ -18,7 +18,7 @@

 class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO {

-  protected $repositoryPHID;
+  protected $repositoryID;
  protected $phid;
  protected $commitIdentifier;
  protected $epoch;
--- a/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php
+++ b/src/applications/repository/worker/base/PhabricatorRepositoryCommitParserWorker.php
@ -36,9 +36,8 @@ abstract class PhabricatorRepositoryCommitParserWorker

    $this->commit = $commit;

-    $repository = id(new PhabricatorRepository())->loadOneWhere(
-      'phid = %s',
-      $commit->getRepositoryPHID());
+    $repository = id(new PhabricatorRepository())->load(
+      $commit->getRepositoryID());

    if (!$repository) {
      return;
--- a/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php
+++ b/src/applications/repository/worker/commitchangeparser/base/PhabricatorRepositoryCommitChangeParserWorker.php
@ -19,6 +19,12 @@
 abstract class PhabricatorRepositoryCommitChangeParserWorker
  extends PhabricatorRepositoryCommitParserWorker {

+  public function getRequiredLeaseTime() {
+    // It can take a very long time to parse commits; some commits in the
+    // Facebook repository affect many millions of paths. Acquire 24h leases.
+    return 60 * 60 * 24;
+  }
+
  protected function lookupOrCreatePaths(array $paths) {
    $repository = new PhabricatorRepository();
    $conn_w = $repository->establishConnection('w');
@ -37,7 +43,7 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
        }
        queryfx(
          $conn_w,
-          'INSERT INTO %T (path) VALUES %Q',
+          'INSERT IGNORE INTO %T (path) VALUES %Q',
          PhabricatorRepository::TABLE_PATH,
          implode(', ', $sql));
      }