Distinguish between empty and unparsed commits in Diffusion

Summary:
Fixes T3416. Fixes T1733.

  - Adds a flag to the commit table showing whether or not we have parsed it.
  - The flag is set to `0` initially when the commit is discovered.
  - The flag is set to `1` when the changes are parsed.
  - The UI can now use the flag to distinguish between "empty commit" and "commit which we haven't imported changes for yet".
  - Simplify rendering code a little bit.
  - Fix an issue with the Message parser for empty commits.
  - There's a key on the flag so we can do `SELECT * FROM repository_commit WHERE repositoryID = %d AND importStatus = 0 LIMIT 1` soon, to determine if a repository is fully imported or not. This will let us improve the UI (Ref T776, Ref T3217).

Test Plan:
  - Ran `bin/storage upgrade -f`.
  - Created an empty commit.
  - Without the daemons running, ran `bin/repository pull GTEST` and `bin/repository discover GTEST`.
  - Viewed web UI to get the first screenshot ("Still Importing...").
  - Ran the message and change steps with `scripts/repository/reparse.php`.
  - Viewed web UI to get the second screenshot ("Empty Commit").

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T776, T1733, T3416, T3217

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

resources/sql/patches/20131026.commitstatus.sql

@@ -0,0 +1,8 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_commit
+  ADD COLUMN importStatus INT UNSIGNED NOT NULL COLLATE utf8_bin;
+
+UPDATE {$NAMESPACE}_repository.repository_commit
+  SET importStatus = 15;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_commit
+  ADD KEY (repositoryID, importStatus);

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -76,7 +76,6 @@ final class DiffusionCommitController extends DiffusionController {
     $this->auditAuthorityPHIDs =
       PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($user);
 
-
     $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub');
     $changesets = null;
     if ($is_foreign) {
@@ -154,10 +153,14 @@ final class DiffusionCommitController extends DiffusionController {
 
     $hard_limit = 1000;
 
-    $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest(
+    if ($commit->isImported()) {
-      $drequest);
+      $change_query = DiffusionPathChangeQuery::newFromDiffusionRequest(
-    $change_query->setLimit($hard_limit + 1);
+        $drequest);
-    $changes = $change_query->loadChanges();
+      $change_query->setLimit($hard_limit + 1);
+      $changes = $change_query->loadChanges();
+    } else {
+      $changes = array();
+    }
 
     $was_limited = (count($changes) > $hard_limit);
     if ($was_limited) {
@@ -206,37 +209,29 @@ final class DiffusionCommitController extends DiffusionController {
     }
 
     if ($bad_commit) {
-      $error_panel = new AphrontErrorView();
+      $content[] = $this->renderStatusMessage(
-      $error_panel->setTitle(pht('Bad Commit'));
+        pht('Bad Commit'),
-      $error_panel->appendChild($bad_commit['description']);
+        $bad_commit['description']);
-
-      $content[] = $error_panel;
     } else if ($is_foreign) {
       // Don't render anything else.
+    } else if (!$commit->isImported()) {
+      $content[] = $this->renderStatusMessage(
+        pht('Still Importing...'),
+        pht(
+          'This commit is still importing. Changes will be visible once '.
+          'the import finishes.'));
     } else if (!count($changes)) {
-      $no_changes = new AphrontErrorView();
+      $content[] = $this->renderStatusMessage(
-      $no_changes->setSeverity(AphrontErrorView::SEVERITY_WARNING);
+        pht('Empty Commit'),
-      $no_changes->setTitle(pht('Not Yet Parsed'));
+        pht(
-      // TODO: This can also happen with weird SVN changes that don't do
+          'This commit is empty and does not affect any paths.'));
-      // anything (or only alter properties?), although the real no-changes case
-      // is extremely rare and might be impossible to produce organically. We
-      // should probably write some kind of "Nothing Happened!" change into the
-      // DB once we parse these changes so we can distinguish between
-      // "not parsed yet" and "no changes".
-      $no_changes->appendChild(
-        pht("This commit hasn't been fully parsed yet (or doesn't affect any ".
-        "paths)."));
-      $content[] = $no_changes;
     } else if ($was_limited) {
-      $huge_commit = new AphrontErrorView();
+      $content[] = $this->renderStatusMessage(
-      $huge_commit->setSeverity(AphrontErrorView::SEVERITY_WARNING);
+        pht('Enormous Commit'),
-      $huge_commit->setTitle(pht('Enormous Commit'));
-      $huge_commit->appendChild(
         pht(
           'This commit is enormous, and affects more than %d files. '.
           'Changes are not shown.',
           $hard_limit));
-      $content[] = $huge_commit;
     } else {
       // The user has clicked "Show All Changes", and we should show all the
       // changes inline even if there are more than the soft limit.
@@ -253,6 +248,7 @@ final class DiffusionCommitController extends DiffusionController {
             'href'    => '?show_all=true',
           ),
           pht('Show All Changes'));
+
         $warning_view = id(new AphrontErrorView())
           ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
           ->setTitle('Very Large Commit')
@@ -1084,4 +1080,11 @@ final class DiffusionCommitController extends DiffusionController {
     return $parser->processCorpus($corpus);
   }
 
+  private function renderStatusMessage($title, $body) {
+    return id(new AphrontErrorView())
+      ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
+      ->setTitle($title)
+      ->appendChild($body);
+  }
+
 }

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -15,6 +15,13 @@ final class PhabricatorRepositoryCommit
   protected $authorPHID;
   protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE;
   protected $summary = '';
+  protected $importStatus = 0;
+
+  const IMPORTED_MESSAGE = 1;
+  const IMPORTED_CHANGE = 2;
+  const IMPORTED_OWNERS = 4;
+  const IMPORTED_HERALD = 8;
+  const IMPORTED_ALL = 15;
 
   private $commitData = self::ATTACHABLE;
   private $audits;
@@ -39,6 +46,20 @@ final class PhabricatorRepositoryCommit
     return $this->isUnparsed;
   }
 
+  public function isImported() {
+    return ($this->getImportStatus() == self::IMPORTED_ALL);
+  }
+
+  public function writeImportStatusFlag($flag) {
+    queryfx(
+      $this->establishConnection('w'),
+      'UPDATE %T SET importStatus = (importStatus | %d) WHERE id = %d',
+      $this->getTableName(),
+      $flag,
+      $this->getID());
+    return $this;
+  }
+
   public function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID   => true,

src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php

@@ -12,6 +12,18 @@ final class PhabricatorRepositoryCommitHeraldWorker
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit) {
 
+    $result = $this->applyHeraldRules($repository, $commit);
+
+    $commit->writeImportStatusFlag(
+      PhabricatorRepositoryCommit::IMPORTED_HERALD);
+
+    return $result;
+  }
+
+  private function applyHeraldRules(
+    PhabricatorRepository $repository,
+    PhabricatorRepositoryCommit $commit) {
+
     $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
       'commitID = %d',
       $commit->getID());

src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

@@ -58,6 +58,9 @@ final class PhabricatorRepositoryCommitOwnersWorker
       $commit->save();
     }
 
+    $commit->writeImportStatusFlag(
+      PhabricatorRepositoryCommit::IMPORTED_OWNERS);
+
     if ($this->shouldQueueFollowupTasks()) {
       PhabricatorWorker::scheduleTask(
         'PhabricatorRepositoryCommitHeraldWorker',

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php

@@ -58,6 +58,9 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
   protected function finishParse() {
     $commit = $this->commit;
 
+    $commit->writeImportStatusFlag(
+      PhabricatorRepositoryCommit::IMPORTED_CHANGE);
+
     id(new PhabricatorSearchIndexer())
       ->indexDocumentByPHID($commit->getPHID());
 

src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php

@@ -220,6 +220,9 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
     }
 
     $data->save();
+
+    $commit->writeImportStatusFlag(
+      PhabricatorRepositoryCommit::IMPORTED_MESSAGE);
   }
 
   private function loadUserName($user_phid, $default, PhabricatorUser $actor) {
@@ -249,7 +252,17 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker
       ->loadRawDiff();
 
     // TODO: Support adds, deletes and moves under SVN.
-    $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
+    if (strlen($raw_diff)) {
+      $changes = id(new ArcanistDiffParser())->parseDiff($raw_diff);
+    } else {
+      // This is an empty diff, maybe made with `git commit --allow-empty`.
+      // NOTE: These diffs have the same tree hash as their ancestors, so
+      // they may attach to revisions in an unexpected way. Just let this
+      // happen for now, although it might make sense to special case it
+      // eventually.
+      $changes = array();
+    }
+
     $diff = DifferentialDiff::newFromRawChanges($changes)
       ->setRevisionID($revision->getID())
       ->setAuthorPHID($actor_phid)

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -1708,6 +1708,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type' => 'sql',
         'name' => $this->getPatchPath('20131025.repopush.sql'),
       ),
+      '20131026.commitstatus.sql' => array(
+        'type' => 'sql',
+        'name' => $this->getPatchPath('20131026.commitstatus.sql'),
+      ),
     );
   }
 }