Add an "importing" state to repositories and clean up the UI

Summary:
Fixes T3217. Ref T776. Ref T1493. Broadly, this introduces a mechanism which works like this:

  - When a repository is created, we set an "importing" flag.
  - After discovery completes, we check if a repository has no importing commits. Basically, this is the first time we catch up to HEAD.
  - If we're caught up, clear the "importing" flag.

This flag lets us fix some issues:

  - T3217. Currently, when you import a new repository and users have rules like "Email me on every commit ever" or "trigger an audit on every commit", we take a bunch of publish actions. Instead, implicitly disable publishing during import.
  - An imported but un-pulled repository currently has an incomprehensible error on `/diffusion/X/`. Fix that.
  - Show more cues in the UI about importing.
  - Made some exceptions more specific.

Test Plan:
This is the new screen for a completely new repo, replacing a giant exception:

{F75443}

  - Created a repository, saw it "importing".
  - Pulled and discovered it.
  - Processed its commits.
  - Ran discovery again, saw import flag clear.
  - Also this repository was empty, which hit some of the other code.

This is the new "parsed empty repository" UI, which isn't good, but is less broken:

{F75446}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, hach-que

Maniphest Tasks: T3607, T1493, T776, T3217

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

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -1080,11 +1080,5 @@ 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/diffusion/controller/DiffusionController.php

@@ -442,5 +442,11 @@ abstract class DiffusionController extends PhabricatorController {
     return preg_replace('@^/diffusion/[A-Z]+@', '', $request_path);
   }
 
+  protected function renderStatusMessage($title, $body) {
+    return id(new AphrontErrorView())
+      ->setSeverity(AphrontErrorView::SEVERITY_WARNING)
+      ->setTitle($title)
+      ->appendChild($body);
+  }
 }
 

src/applications/diffusion/controller/DiffusionRepositoryController.php

@@ -7,7 +7,8 @@ final class DiffusionRepositoryController extends DiffusionController {
   }
 
   public function processRequest() {
-    $drequest = $this->diffusionRequest;
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
 
     $content = array();
 
@@ -15,106 +16,108 @@ final class DiffusionRepositoryController extends DiffusionController {
     $content[] = $crumbs;
 
     $content[] = $this->buildPropertiesTable($drequest->getRepository());
-
-    $history_results = $this->callConduitWithDiffusionRequest(
-      'diffusion.historyquery',
-      array(
-        'commit' => $drequest->getCommit(),
-        'path' => $drequest->getPath(),
-        'offset' => 0,
-        'limit' => 15));
-    $history = DiffusionPathChange::newFromConduit(
-      $history_results['pathChanges']);
-
-    $browse_results = DiffusionBrowseResultSet::newFromConduit(
-      $this->callConduitWithDiffusionRequest(
-        'diffusion.browsequery',
-        array(
-          'path' => $drequest->getPath(),
-          'commit' => $drequest->getCommit(),
-        )));
-    $browse_paths = $browse_results->getPaths();
-
     $phids = array();
-    foreach ($history as $item) {
+
-      $data = $item->getCommitData();
+    try {
-      if ($data) {
+      $history_results = $this->callConduitWithDiffusionRequest(
-        if ($data->getCommitDetail('authorPHID')) {
+        'diffusion.historyquery',
-          $phids[$data->getCommitDetail('authorPHID')] = true;
+        array(
-        }
+          'commit' => $drequest->getCommit(),
-        if ($data->getCommitDetail('committerPHID')) {
+          'path' => $drequest->getPath(),
-          $phids[$data->getCommitDetail('committerPHID')] = true;
+          'offset' => 0,
+          'limit' => 15));
+      $history = DiffusionPathChange::newFromConduit(
+        $history_results['pathChanges']);
+
+      foreach ($history as $item) {
+        $data = $item->getCommitData();
+        if ($data) {
+          if ($data->getCommitDetail('authorPHID')) {
+            $phids[$data->getCommitDetail('authorPHID')] = true;
+          }
+          if ($data->getCommitDetail('committerPHID')) {
+            $phids[$data->getCommitDetail('committerPHID')] = true;
+          }
         }
       }
+    } catch (Exception $ex) {
+      $history_results = null;
+      $history = null;
+      $history_exception = $ex;
     }
 
-    foreach ($browse_paths as $item) {
+    try {
-      $data = $item->getLastCommitData();
+      $browse_results = DiffusionBrowseResultSet::newFromConduit(
-      if ($data) {
+        $this->callConduitWithDiffusionRequest(
-        if ($data->getCommitDetail('authorPHID')) {
+          'diffusion.browsequery',
-          $phids[$data->getCommitDetail('authorPHID')] = true;
+          array(
-        }
+            'path' => $drequest->getPath(),
-        if ($data->getCommitDetail('committerPHID')) {
+            'commit' => $drequest->getCommit(),
-          $phids[$data->getCommitDetail('committerPHID')] = true;
+          )));
+      $browse_paths = $browse_results->getPaths();
+
+      foreach ($browse_paths as $item) {
+        $data = $item->getLastCommitData();
+        if ($data) {
+          if ($data->getCommitDetail('authorPHID')) {
+            $phids[$data->getCommitDetail('authorPHID')] = true;
+          }
+          if ($data->getCommitDetail('committerPHID')) {
+            $phids[$data->getCommitDetail('committerPHID')] = true;
+          }
         }
       }
+
+    } catch (Exception $ex) {
+      $browse_results = null;
+      $browse_paths = null;
+      $browse_exception = $ex;
     }
+
     $phids = array_keys($phids);
     $handles = $this->loadViewerHandles($phids);
 
-    $readme = $this->callConduitWithDiffusionRequest(
+    if ($browse_results) {
-      'diffusion.readmequery',
+      $readme = $this->callConduitWithDiffusionRequest(
-      array(
+        'diffusion.readmequery',
-       'paths' => $browse_results->getPathDicts()
+        array(
+         'paths' => $browse_results->getPathDicts()
         ));
+    } else {
+      $readme = null;
+    }
 
-    $history_table = new DiffusionHistoryTableView();
+    $content[] = $this->buildHistoryTable(
-    $history_table->setUser($this->getRequest()->getUser());
+      $history_results,
-    $history_table->setDiffusionRequest($drequest);
+      $history,
-    $history_table->setHandles($handles);
+      $history_exception,
-    $history_table->setHistory($history);
+      $handles);
-    $history_table->loadRevisions();
-    $history_table->setParents($history_results['parents']);
-    $history_table->setIsHead(true);
 
-    $callsign = $drequest->getRepository()->getCallsign();
+    $content[] = $this->buildBrowseTable(
-    $all = phutil_tag(
+      $browse_results,
-      'a',
+      $browse_paths,
-      array(
+      $browse_exception,
-        'href' => $drequest->generateURI(
+      $handles);
-          array(
-            'action' => 'history',
-          )),
-      ),
-      pht('View Full Commit History'));
 
-    $panel = new AphrontPanelView();
+    try {
-    $panel->setHeader(pht("Recent Commits · %s", $all));
+      $content[] = $this->buildTagListTable($drequest);
-    $panel->appendChild($history_table);
+    } catch (Exception $ex) {
-    $panel->setNoBackground();
+      if (!$repository->isImporting()) {
+        $content[] = $this->renderStatusMessage(
+          pht('Unable to Load Tags'),
+          $ex->getMessage());
+      }
+    }
 
-    $content[] = $panel;
+    try {
-
+      $content[] = $this->buildBranchListTable($drequest);
-
+    } catch (Exception $ex) {
-    $browse_table = new DiffusionBrowseTableView();
+      if (!$repository->isImporting()) {
-    $browse_table->setDiffusionRequest($drequest);
+        $content[] = $this->renderStatusMessage(
-    $browse_table->setHandles($handles);
+          pht('Unable to Load Branches'),
-    $browse_table->setPaths($browse_paths);
+          $ex->getMessage());
-    $browse_table->setUser($this->getRequest()->getUser());
+      }
-
+    }
-    $browse_panel = new AphrontPanelView();
-    $browse_panel->setHeader(phutil_tag(
-      'a',
-      array('href' => $drequest->generateURI(array('action' => 'browse'))),
-      pht('Browse Repository')));
-    $browse_panel->appendChild($browse_table);
-    $browse_panel->setNoBackground();
-
-    $content[] = $browse_panel;
-
-    $content[] = $this->buildTagListTable($drequest);
-
-    $content[] = $this->buildBranchListTable($drequest);
 
     if ($readme) {
       $box = new PHUIBoxView();
@@ -145,6 +148,15 @@ final class DiffusionRepositoryController extends DiffusionController {
       ->setUser($user)
       ->setPolicyObject($repository);
 
+    if (!$repository->isTracked()) {
+      $header->setStatus('policy-noone', '', pht('Inactive'));
+    } else if ($repository->isImporting()) {
+      $header->setStatus('time', 'red', pht('Importing...'));
+    } else {
+      $header->setStatus('oh-ok', '', pht('Active'));
+    }
+
+
     $actions = $this->buildActionList($repository);
 
     $view = id(new PHUIPropertyListView())
@@ -333,4 +345,109 @@ final class DiffusionRepositoryController extends DiffusionController {
     return $view;
   }
 
+  private function buildHistoryTable(
+    $history_results,
+    $history,
+    $history_exception,
+    array $handles) {
+
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+
+    if ($history_exception) {
+      if ($repository->isImporting()) {
+        return $this->renderStatusMessage(
+          pht('Still Importing...'),
+          pht(
+            'This repository is still importing. History is not yet '.
+            'available.'));
+      } else {
+        return $this->renderStatusMessage(
+          pht('Unable to Retrieve History'),
+          $history_exception->getMessage());
+      }
+    }
+
+    $history_table = id(new DiffusionHistoryTableView())
+      ->setUser($viewer)
+      ->setDiffusionRequest($drequest)
+      ->setHandles($handles)
+      ->setHistory($history);
+
+    // TODO: Super sketchy.
+    $history_table->loadRevisions();
+
+    if ($history_results) {
+      $history_table->setParents($history_results['parents']);
+    }
+
+    $history_table->setIsHead(true);
+
+    $callsign = $drequest->getRepository()->getCallsign();
+    $all = phutil_tag(
+      'a',
+      array(
+        'href' => $drequest->generateURI(
+          array(
+            'action' => 'history',
+          )),
+      ),
+      pht('View Full Commit History'));
+
+    $panel = new AphrontPanelView();
+    $panel->setHeader(pht("Recent Commits · %s", $all));
+    $panel->appendChild($history_table);
+    $panel->setNoBackground();
+
+    return $panel;
+  }
+
+  private function buildBrowseTable(
+    $browse_results,
+    $browse_paths,
+    $browse_exception,
+    array $handles) {
+
+    $request = $this->getRequest();
+    $viewer = $request->getUser();
+    $drequest = $this->getDiffusionRequest();
+    $repository = $drequest->getRepository();
+
+    if ($browse_exception) {
+      if ($repository->isImporting()) {
+        // The history table renders a useful message.
+        return null;
+      } else {
+        return $this->renderStatusMessage(
+          pht('Unable to Retrieve Paths'),
+          $browse_exception->getMessage());
+      }
+    }
+
+    $browse_table = id(new DiffusionBrowseTableView())
+      ->setUser($viewer)
+      ->setDiffusionRequest($drequest)
+      ->setHandles($handles);
+    if ($browse_paths) {
+      $browse_table->setPaths($browse_paths);
+    } else {
+      $browse_table->setPaths(array());
+    }
+
+    $browse_uri = $drequest->generateURI(array('action' => 'browse'));
+
+    $browse_panel = new AphrontPanelView();
+    $browse_panel->setHeader(
+      phutil_tag(
+        'a',
+        array('href' => $browse_uri),
+        pht('Browse Repository')));
+    $browse_panel->appendChild($browse_table);
+    $browse_panel->setNoBackground();
+
+    return $browse_panel;
+  }
+
 }

src/applications/diffusion/controller/DiffusionRepositoryCreateController.php

@@ -95,6 +95,10 @@ final class DiffusionRepositoryCreateController
           // transactions to apply this change.
           $repository->setCallsign($callsign);
 
+          // Put the repository in "Importing" mode until we finish
+          // parsing it.
+          $repository->setDetail('importing', true);
+
           $xactions[] = id(clone $template)
             ->setTransactionType($type_name)
             ->setNewValue(

src/applications/diffusion/view/DiffusionHistoryTableView.php

@@ -21,6 +21,8 @@ final class DiffusionHistoryTableView extends DiffusionView {
         $commit_phids[] = $item->getCommit()->getPHID();
       }
     }
+
+    // TODO: Get rid of this.
     $this->revisions = id(new DifferentialRevision())
       ->loadIDsByCommitPHIDs($commit_phids);
     return $this;

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -201,28 +201,41 @@ final class PhabricatorRepositoryPullLocalDaemon
 
   public function discoverRepository(PhabricatorRepository $repository) {
     $vcs = $repository->getVersionControlSystem();
+
+    $result = null;
+    $refs = null;
     switch ($vcs) {
       case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-        return $this->executeGitDiscover($repository);
+        $result = $this->executeGitDiscover($repository);
+        break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
         $refs = $this->getDiscoveryEngine($repository)
           ->discoverCommits();
         break;
       case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
-        return $this->executeHgDiscover($repository);
+        $result = $this->executeHgDiscover($repository);
+        break;
       default:
         throw new Exception("Unknown VCS '{$vcs}'!");
     }
 
-    foreach ($refs as $ref) {
+    if ($refs !== null) {
-      $this->recordCommit(
+      foreach ($refs as $ref) {
-        $repository,
+        $this->recordCommit(
-        $ref->getIdentifier(),
+          $repository,
-        $ref->getEpoch(),
+          $ref->getIdentifier(),
-        $ref->getBranch());
+          $ref->getEpoch(),
+          $ref->getBranch());
+      }
     }
 
-    return (bool)count($refs);
+    $this->checkIfRepositoryIsFullyImported($repository);
+
+    if ($refs !== null) {
+      return (bool)count($refs);
+    } else {
+      return $result;
+    }
   }
 
   private function getDiscoveryEngine(PhabricatorRepository $repository) {
@@ -456,7 +469,39 @@ final class PhabricatorRepositoryPullLocalDaemon
     return $repository->getID().':'.$commit_identifier;
   }
 
+  private function checkIfRepositoryIsFullyImported(
+    PhabricatorRepository $repository) {
 
+    // Check if the repository has the "Importing" flag set. We want to clear
+    // the flag if we can.
+    $importing = $repository->getDetail('importing');
+    if (!$importing) {
+      // This repository isn't marked as "Importing", so we're done.
+      return;
+    }
+
+    // Look for any commit which hasn't imported.
+    $unparsed_commit = queryfx_one(
+      $repository->establishConnection('r'),
+      'SELECT * FROM %T WHERE repositoryID = %d AND importStatus != %d',
+      id(new PhabricatorRepositoryCommit())->getTableName(),
+      $repository->getID(),
+      PhabricatorRepositoryCommit::IMPORTED_ALL);
+    if ($unparsed_commit) {
+      // We found a commit which still needs to import, so we can't clear the
+      // flag.
+      return;
+    }
+
+    // Clear the "importing" flag.
+    $repository->openTransaction();
+      $repository->beginReadLocking();
+        $repository = $repository->reload();
+        $repository->setDetail('importing', false);
+        $repository->save();
+      $repository->endReadLocking();
+    $repository->saveTransaction();
+  }
 
 /* -(  Git Implementation  )------------------------------------------------- */
 
@@ -488,6 +533,12 @@ final class PhabricatorRepositoryPullLocalDaemon
       $stdout,
       $only_this_remote = DiffusionBranchInformation::DEFAULT_GIT_REMOTE);
 
+    if (!$branches) {
+      // This repository has no branches at all, so we don't need to do
+      // anything. Generally, this means the repository is empty.
+      return;
+    }
+
     $callsign = $repository->getCallsign();
 
     $tracked_something = false;

src/applications/repository/storage/PhabricatorRepository.php

@@ -192,24 +192,32 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public function execLocalCommand($pattern /* , $arg, ... */) {
+    $this->assertLocalExists();
+
     $args = func_get_args();
     $args = $this->formatLocalCommand($args);
     return call_user_func_array('exec_manual', $args);
   }
 
   public function execxLocalCommand($pattern /* , $arg, ... */) {
+    $this->assertLocalExists();
+
     $args = func_get_args();
     $args = $this->formatLocalCommand($args);
     return call_user_func_array('execx', $args);
   }
 
   public function getLocalCommandFuture($pattern /* , $arg, ... */) {
+    $this->assertLocalExists();
+
     $args = func_get_args();
     $args = $this->formatLocalCommand($args);
     return newv('ExecFuture', $args);
   }
 
   public function passthruLocalCommand($pattern /* , $arg, ... */) {
+    $this->assertLocalExists();
+
     $args = func_get_args();
     $args = $this->formatLocalCommand($args);
     return call_user_func_array('phutil_passthru', $args);
@@ -433,6 +441,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
   public function shouldAutocloseBranch($branch) {
+    if ($this->isImporting()) {
+      return false;
+    }
+
     if ($this->getDetail('disable-autoclose', false)) {
       return false;
     }
@@ -486,6 +498,10 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
     return 'r'.$this->getCallsign().$short_identifier;
   }
 
+  public function isImporting() {
+    return (bool)$this->getDetail('importing', false);
+  }
+
 /* -(  Repository URI Management  )------------------------------------------ */
 
 
@@ -750,6 +766,27 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
 
+  /**
+   * Raise more useful errors when there are basic filesystem problems.
+   */
+  private function assertLocalExists() {
+    switch ($this->getVersionControlSystem()) {
+      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
+        if (!$this->isHosted()) {
+          // For non-hosted SVN repositories, we don't expect a local directory
+          // to exist.
+          return;
+        }
+        break;
+    }
+
+    $local = $this->getLocalPath();
+    Filesystem::assertExists($local);
+    Filesystem::assertIsDirectory($local);
+    Filesystem::assertReadable($local);
+  }
+
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 

src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php

@@ -24,6 +24,13 @@ final class PhabricatorRepositoryCommitHeraldWorker
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit) {
 
+    // Don't take any actions on an importing repository. Principally, this
+    // avoids generating thousands of audits or emails when you import an
+    // established repository on an existing install.
+    if ($repository->isImporting()) {
+      return;
+    }
+
     $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
       'commitID = %d',
       $commit->getID());