Move Git discovery into DiscoveryEngine

Summary:
Ref T4327. Consolidates and simplifies infrastructure:

  - Moves Git discovery into DiscoveryEngine.
  - Collapses a bunch of the Git and Mercurial code related to stream discovery.
  - Removes all cach code from PullLocal daemon (it's no longer called).
  - Adds basic unit tests for Git and Mercurial discovery.

Various cleanup:

  - Makes GitStream and MercurialStream extend a common base.
  - Improves performance of MercurialStream in some cases, by requiring fewer commits be output and parsed.
  - Makes mirroring exceptions easier to debug.
  - Fixes discovery of Mercurial repositories with multiple branch heads.
  - Adds some missing `pht()`.

Test Plan:
I tested this fairly throughly because I think this phase is complete:

  - Made new repositories in multiple VCSes and did full imports.
    - Particularly, I reimported Arcanist to make sure that TODO was resolved. I think it was related to the toposort stuff.
  - Pushed commits to multiple VCSes.
  - Pushed commits to a non-close branch, then pushed a merge commit. Observed commits import initially as non-close, then get flagged for close.
  - Started full daemons and resolved various minor issues that showed up in the daemon log until everything ran cleanly.
  - Basically spent about 30 minutes banging on this in every way I could think of to try to break it. I found and fixed some minor stuff, but it seems solid.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4327

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

src/__phutil_library_map__.php

@@ -1847,6 +1847,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryEngine' => 'applications/repository/engine/PhabricatorRepositoryEngine.php',
     'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php',
     'PhabricatorRepositoryGitCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php',
+    'PhabricatorRepositoryGraphStream' => 'applications/repository/daemon/PhabricatorRepositoryGraphStream.php',
     'PhabricatorRepositoryListController' => 'applications/repository/controller/PhabricatorRepositoryListController.php',
     'PhabricatorRepositoryManagementDeleteWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementDeleteWorkflow.php',
     'PhabricatorRepositoryManagementDiscoverWorkflow' => 'applications/repository/management/PhabricatorRepositoryManagementDiscoverWorkflow.php',
@@ -4154,6 +4155,7 @@ phutil_register_library_map(array(
     'PhabricatorGarbageCollectorConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorGarbageCollectorDaemon' => 'PhabricatorDaemon',
     'PhabricatorGestureExample' => 'PhabricatorUIExample',
+    'PhabricatorGitGraphStream' => 'PhabricatorRepositoryGraphStream',
     'PhabricatorGlobalLock' => 'PhutilLock',
     'PhabricatorGlobalUploadTargetView' => 'AphrontView',
     'PhabricatorHandleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
@@ -4241,6 +4243,7 @@ phutil_register_library_map(array(
     'PhabricatorMarkupCache' => 'PhabricatorCacheDAO',
     'PhabricatorMarkupOneOff' => 'PhabricatorMarkupInterface',
     'PhabricatorMarkupPreviewController' => 'PhabricatorController',
+    'PhabricatorMercurialGraphStream' => 'PhabricatorRepositoryGraphStream',
     'PhabricatorMetaMTAActorQuery' => 'PhabricatorQuery',
     'PhabricatorMetaMTAConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorMetaMTAController' => 'PhabricatorController',
@@ -4516,6 +4519,7 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
     'PhabricatorRepositoryGitCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
+    'PhabricatorRepositoryGraphStream' => 'Phobject',
     'PhabricatorRepositoryListController' => 'PhabricatorRepositoryController',
     'PhabricatorRepositoryManagementDeleteWorkflow' => 'PhabricatorRepositoryManagementWorkflow',
     'PhabricatorRepositoryManagementDiscoverWorkflow' => 'PhabricatorRepositoryManagementWorkflow',

src/applications/repository/daemon/PhabricatorGitGraphStream.php

@@ -1,6 +1,7 @@
 <?php
 
-final class PhabricatorGitGraphStream {
+final class PhabricatorGitGraphStream
+  extends PhabricatorRepositoryGraphStream {
 
   private $repository;
   private $iterator;

src/applications/repository/daemon/PhabricatorMercurialGraphStream.php

@@ -5,7 +5,8 @@
  * the Mercurial commit graph with one nonblocking invocation of "hg". See
  * @{class:PhabricatorRepositoryPullLocalDaemon}.
  */
-final class PhabricatorMercurialGraphStream {
+final class PhabricatorMercurialGraphStream
+  extends PhabricatorRepositoryGraphStream {
 
   private $repository;
   private $iterator;
@@ -15,11 +16,13 @@ final class PhabricatorMercurialGraphStream {
   private $local          = array();
   private $localParents   = array();
 
-  public function __construct(PhabricatorRepository $repository) {
+  public function __construct(PhabricatorRepository $repository, $commit) {
     $this->repository = $repository;
 
     $future = $repository->getLocalCommandFuture(
-      "log --template '{rev}\1{node}\1{date}\1{parents}\2'");
+      'log --template %s --rev %s',
+      '{rev}\1{node}\1{date}\1{parents}\2',
+      hgsprintf('reverse(ancestors(%s))', $commit));
 
     $this->iterator = new LinesOfALargeExecFuture($future);
     $this->iterator->setDelimiter("\2");

src/applications/repository/daemon/PhabricatorRepositoryGraphStream.php

@@ -0,0 +1,8 @@
+<?php
+
+abstract class PhabricatorRepositoryGraphStream extends Phobject {
+
+  abstract public function getParents($commit);
+  abstract public function getCommitDate($commit);
+
+}

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -29,7 +29,6 @@
 final class PhabricatorRepositoryPullLocalDaemon
   extends PhabricatorDaemon {
 
-  private $commitCache = array();
   private $repair;
   private $discoveryEngines = array();
 
@@ -223,31 +222,15 @@ final class PhabricatorRepositoryPullLocalDaemon
   }
 
   public function discoverRepository(PhabricatorRepository $repository) {
-    $vcs = $repository->getVersionControlSystem();
-
-    $result = null;
-    $refs = null;
-    switch ($vcs) {
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
-        $result = $this->executeGitDiscover($repository);
-        break;
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
-      case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
     $refs = $this->getDiscoveryEngine($repository)
       ->discoverCommits();
-        break;
-      default:
-        throw new Exception("Unknown VCS '{$vcs}'!");
-    }
 
-    if ($refs !== null) {
     foreach ($refs as $ref) {
       $this->recordCommit(
         $repository,
         $ref->getIdentifier(),
         $ref->getEpoch(),
-          $close_immediately = true);
+        $ref->getCanCloseImmediately());
-      }
     }
 
     $this->checkIfRepositoryIsFullyImported($repository);
@@ -258,14 +241,15 @@ final class PhabricatorRepositoryPullLocalDaemon
       // TODO: We should report these into the UI properly, but for
       // now just complain. These errors are much less severe than
       // pull errors.
-      phlog($ex);
+      $proxy = new PhutilProxyException(
+        pht(
+          'Error while pushing "%s" repository to a mirror.',
+          $repository->getCallsign()),
+        $ex);
+      phlog($proxy);
     }
 
-    if ($refs !== null) {
     return (bool)count($refs);
-    } else {
-      return $result;
-    }
   }
 
   private function updateRepositoryRefs(PhabricatorRepository $repository) {
@@ -287,38 +271,6 @@ final class PhabricatorRepositoryPullLocalDaemon
     return $this->discoveryEngines[$id];
   }
 
-  private function isKnownCommit(
-    PhabricatorRepository $repository,
-    $target) {
-
-    if ($this->getCache($repository, $target)) {
-      return true;
-    }
-
-    if ($this->repair) {
-      // In repair mode, rediscover the entire repository, ignoring the
-      // database state. We can hit the local cache above, but if we miss it
-      // stop the script from going to the database cache.
-      return false;
-    }
-
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'repositoryID = %d AND commitIdentifier = %s',
-      $repository->getID(),
-      $target);
-
-    if (!$commit) {
-      return false;
-    }
-
-    $this->setCache($repository, $target);
-    while (count($this->commitCache) > 2048) {
-      array_shift($this->commitCache);
-    }
-
-    return true;
-  }
-
   private function recordCommit(
     PhabricatorRepository $repository,
     $commit_identifier,
@@ -364,8 +316,6 @@ final class PhabricatorRepositoryPullLocalDaemon
         $this->log("Repaired commit '{$commit_identifier}'.");
       }
 
-      $this->setCache($repository, $commit_identifier);
-
       PhutilEventEngine::dispatchEvent(
         new PhabricatorEvent(
           PhabricatorEventType::TYPE_DIFFUSION_DIDDISCOVERCOMMIT,
@@ -380,7 +330,6 @@ final class PhabricatorRepositoryPullLocalDaemon
       // more than once when looking at history, or because of races or
       // data inconsistency or cosmic radiation; in any case, we're still
       // in a good state if we ignore the failure.
-      $this->setCache($repository, $commit_identifier);
     }
   }
 
@@ -409,30 +358,6 @@ final class PhabricatorRepositoryPullLocalDaemon
     PhabricatorWorker::scheduleTask($class, $data);
   }
 
-
-  private function setCache(
-    PhabricatorRepository $repository,
-    $commit_identifier) {
-
-    $key = $this->getCacheKey($repository, $commit_identifier);
-    $this->commitCache[$key] = true;
-  }
-
-  private function getCache(
-    PhabricatorRepository $repository,
-    $commit_identifier) {
-
-    $key = $this->getCacheKey($repository, $commit_identifier);
-    return idx($this->commitCache, $key, false);
-  }
-
-  private function getCacheKey(
-    PhabricatorRepository $repository,
-    $commit_identifier) {
-
-    return $repository->getID().':'.$commit_identifier;
-  }
-
   private function checkIfRepositoryIsFullyImported(
     PhabricatorRepository $repository) {
 
@@ -468,239 +393,6 @@ final class PhabricatorRepositoryPullLocalDaemon
     $repository->saveTransaction();
   }
 
-/* -(  Git Implementation  )------------------------------------------------- */
-
-
-  /**
-   * @task git
-   */
-  private function executeGitDiscover(
-    PhabricatorRepository $repository) {
-
-    if (!$repository->isHosted()) {
-      $this->verifyOrigin($repository);
-    }
-
-    $refs = id(new DiffusionLowLevelGitRefQuery())
-      ->setRepository($repository)
-      ->withIsOriginBranch(true)
-      ->execute();
-
-    $branches = mpull($refs, 'getCommitIdentifier', 'getShortName');
-
-    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;
-    }
-
-    $branches = $this->sortBranches($repository, $branches);
-
-    $callsign = $repository->getCallsign();
-
-    $tracked_something = false;
-
-    $this->log("Discovering commits in repository '{$callsign}'...");
-    foreach ($branches as $name => $commit) {
-      $this->log("Examining branch '{$name}', at {$commit}.");
-      if (!$repository->shouldTrackBranch($name)) {
-        $this->log("Skipping, branch is untracked.");
-        continue;
-      }
-
-      $tracked_something = true;
-
-      if ($this->isKnownCommit($repository, $commit)) {
-        $this->log("Skipping, HEAD is known.");
-        continue;
-      }
-
-      $this->log("Looking for new commits.");
-      $this->executeGitDiscoverCommit(
-        $repository,
-        $commit,
-        $repository->shouldAutocloseBranch($name));
-    }
-
-    if (!$tracked_something) {
-      $repo_name = $repository->getName();
-      $repo_callsign = $repository->getCallsign();
-      throw new Exception(
-        "Repository r{$repo_callsign} '{$repo_name}' has no tracked branches! ".
-        "Verify that your branch filtering settings are correct.");
-    }
-  }
-
-  /**
-   * Sort branches so we process closeable branches first. This makes the
-   * whole import process a little cheaper, since we can close these commits
-   * the first time through rather than catching them in the refs step.
-   */
-  private function sortBranches(
-    PhabricatorRepository $repository,
-    array $branches) {
-
-    $head_branches = array();
-    $tail_branches = array();
-    foreach ($branches as $name => $commit) {
-      if ($repository->shouldAutocloseBranch($name)) {
-        $head_branches[$name] = $commit;
-      } else {
-        $tail_branches[$name] = $commit;
-      }
-    }
-
-    return $head_branches + $tail_branches;
-  }
-
-
-  /**
-   * @task git
-   */
-  private function executeGitDiscoverCommit(
-    PhabricatorRepository $repository,
-    $commit,
-    $close_immediately) {
-
-    $discover = array($commit);
-    $insert = array($commit);
-
-    $seen_parent = array();
-
-    $stream = new PhabricatorGitGraphStream($repository, $commit);
-
-    while (true) {
-      $target = array_pop($discover);
-      $parents = $stream->getParents($target);
-      foreach ($parents as $parent) {
-        if (isset($seen_parent[$parent])) {
-          // We end up in a loop here somehow when we parse Arcanist if we
-          // don't do this. TODO: Figure out why and draw a pretty diagram
-          // since it's not evident how parsing a DAG with this causes the
-          // loop to stop terminating.
-          continue;
-        }
-        $seen_parent[$parent] = true;
-        $known = $this->isKnownCommit($repository, $parent);
-        if (!$known) {
-          $this->log(pht('Discovered commit "%s".', $parent));
-          $discover[] = $parent;
-          $insert[] = $parent;
-        }
-      }
-      if (empty($discover)) {
-        break;
-      }
-    }
-
-    $n = count($insert);
-    $this->log(pht('Found %d new commits.', new PhutilNumber($n)));
-
-    while (true) {
-      $target = array_pop($insert);
-      $epoch = $stream->getCommitDate($target);
-      $epoch = trim($epoch);
-
-      $this->recordCommit($repository, $target, $epoch, $close_immediately);
-
-      if (empty($insert)) {
-        break;
-      }
-    }
-  }
-
-
-  /**
-   * Verify that the "origin" remote exists, and points at the correct URI.
-   *
-   * This catches or corrects some types of misconfiguration, and also repairs
-   * an issue where Git 1.7.1 does not create an "origin" for `--bare` clones.
-   * See T4041.
-   *
-   * @param   PhabricatorRepository Repository to verify.
-   * @return  void
-   */
-  private function verifyOrigin(PhabricatorRepository $repository) {
-    list($remotes) = $repository->execxLocalCommand(
-      'remote show -n origin');
-
-    $matches = null;
-    if (!preg_match('/^\s*Fetch URL:\s*(.*?)\s*$/m', $remotes, $matches)) {
-      throw new Exception(
-        "Expected 'Fetch URL' in 'git remote show -n origin'.");
-    }
-
-    $remote_uri = $matches[1];
-    $expect_remote = $repository->getRemoteURI();
-
-    if ($remote_uri == "origin") {
-      // If a remote does not exist, git pretends it does and prints out a
-      // made up remote where the URI is the same as the remote name. This is
-      // definitely not correct.
-
-      // Possibly, we should use `git remote --verbose` instead, which does not
-      // suffer from this problem (but is a little more complicated to parse).
-      $valid = false;
-      $exists = false;
-    } else {
-      $normal_type_git = PhabricatorRepositoryURINormalizer::TYPE_GIT;
-
-      $remote_normal = id(new PhabricatorRepositoryURINormalizer(
-        $normal_type_git,
-        $remote_uri))->getNormalizedPath();
-
-      $expect_normal = id(new PhabricatorRepositoryURINormalizer(
-        $normal_type_git,
-        $expect_remote))->getNormalizedPath();
-
-      $valid = ($remote_normal == $expect_normal);
-      $exists = true;
-    }
-
-    if (!$valid) {
-      if (!$exists) {
-        // If there's no "origin" remote, just create it regardless of how
-        // strongly we own the working copy. There is almost no conceivable
-        // scenario in which this could do damage.
-        $this->log(
-          pht(
-            'Remote "origin" does not exist. Creating "origin", with '.
-            'URI "%s".',
-            $expect_remote));
-        $repository->execxLocalCommand(
-          'remote add origin %P',
-          $repository->getRemoteURIEnvelope());
-
-        // NOTE: This doesn't fetch the origin (it just creates it), so we won't
-        // know about origin branches until the next "pull" happens. That's fine
-        // for our purposes, but might impact things in the future.
-      } else {
-        if ($repository->canDestroyWorkingCopy()) {
-          // Bad remote, but we can try to repair it.
-          $this->log(
-            pht(
-              'Remote "origin" exists, but is pointed at the wrong URI, "%s". '.
-              'Resetting origin URI to "%s.',
-              $remote_uri,
-              $expect_remote));
-          $repository->execxLocalCommand(
-            'remote set-url origin %P',
-            $repository->getRemoteURIEnvelope());
-        } else {
-          // Bad remote and we aren't comfortable repairing it.
-          $message = pht(
-            'Working copy at "%s" has a mismatched origin URI, "%s". '.
-            'The expected origin URI is "%s". Fix your configuration, or '.
-            'set the remote URI correctly. To avoid breaking anything, '.
-            'Phabricator will not automatically fix this.',
-            $repository->getLocalPath(),
-            $remote_uri,
-            $expect_remote);
-          throw new Exception($message);
-        }
-      }
-    }
-  }
 
   private function pushToMirrors(PhabricatorRepository $repository) {
     if (!$repository->canMirror()) {

src/applications/repository/engine/PhabricatorRepositoryCommitRef.php

@@ -5,6 +5,7 @@ final class PhabricatorRepositoryCommitRef {
   private $identifier;
   private $epoch;
   private $branch;
+  private $canCloseImmediately;
 
   public function setIdentifier($identifier) {
     $this->identifier = $identifier;
@@ -33,4 +34,13 @@ final class PhabricatorRepositoryCommitRef {
     return $this->branch;
   }
 
+  public function setCanCloseImmediately($can_close_immediately) {
+    $this->canCloseImmediately = $can_close_immediately;
+    return $this;
+  }
+
+  public function getCanCloseImmediately() {
+    return $this->canCloseImmediately;
+  }
+
 }

src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php

@@ -43,13 +43,9 @@ final class PhabricatorRepositoryDiscoveryEngine
       case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
         $refs = $this->discoverMercurialCommits();
         break;
-/*
-  TODO: Implement this!
-
       case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
         $refs = $this->discoverGitCommits();
         break;
-*/
       default:
         throw new Exception("Unknown VCS '{$vcs}'!");
     }
@@ -63,6 +59,157 @@ final class PhabricatorRepositoryDiscoveryEngine
   }
 
 
+/* -(  Discovering Git Repositories  )--------------------------------------- */
+
+
+  /**
+   * @task git
+   */
+  private function discoverGitCommits() {
+    $repository = $this->getRepository();
+
+    if (!$repository->isHosted()) {
+      $this->verifyGitOrigin($repository);
+    }
+
+    $branches = id(new DiffusionLowLevelGitRefQuery())
+      ->setRepository($repository)
+      ->withIsOriginBranch(true)
+      ->execute();
+
+    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 array();
+    }
+
+    $branches = $this->sortBranches($branches);
+    $branches = mpull($branches, 'getCommitIdentifier', 'getShortName');
+
+    $this->log(
+      pht(
+        'Discovering commits in repository %s.',
+        $repository->getCallsign()));
+
+    $refs = array();
+    foreach ($branches as $name => $commit) {
+      $this->log(pht('Examining branch "%s", at "%s".', $name, $commit));
+
+      if (!$repository->shouldTrackBranch($name)) {
+        $this->log(pht("Skipping, branch is untracked."));
+        continue;
+      }
+
+      if ($this->isKnownCommit($commit)) {
+        $this->log(pht("Skipping, HEAD is known."));
+        continue;
+      }
+
+      $this->log(pht("Looking for new commits."));
+
+      $refs[] = $this->discoverStreamAncestry(
+        new PhabricatorGitGraphStream($repository, $commit),
+        $commit,
+        $repository->shouldAutocloseBranch($name));
+    }
+
+    return array_mergev($refs);
+  }
+
+
+  /**
+   * Verify that the "origin" remote exists, and points at the correct URI.
+   *
+   * This catches or corrects some types of misconfiguration, and also repairs
+   * an issue where Git 1.7.1 does not create an "origin" for `--bare` clones.
+   * See T4041.
+   *
+   * @param   PhabricatorRepository Repository to verify.
+   * @return  void
+   */
+  private function verifyGitOrigin(PhabricatorRepository $repository) {
+    list($remotes) = $repository->execxLocalCommand(
+      'remote show -n origin');
+
+    $matches = null;
+    if (!preg_match('/^\s*Fetch URL:\s*(.*?)\s*$/m', $remotes, $matches)) {
+      throw new Exception(
+        "Expected 'Fetch URL' in 'git remote show -n origin'.");
+    }
+
+    $remote_uri = $matches[1];
+    $expect_remote = $repository->getRemoteURI();
+
+    if ($remote_uri == "origin") {
+      // If a remote does not exist, git pretends it does and prints out a
+      // made up remote where the URI is the same as the remote name. This is
+      // definitely not correct.
+
+      // Possibly, we should use `git remote --verbose` instead, which does not
+      // suffer from this problem (but is a little more complicated to parse).
+      $valid = false;
+      $exists = false;
+    } else {
+      $normal_type_git = PhabricatorRepositoryURINormalizer::TYPE_GIT;
+
+      $remote_normal = id(new PhabricatorRepositoryURINormalizer(
+        $normal_type_git,
+        $remote_uri))->getNormalizedPath();
+
+      $expect_normal = id(new PhabricatorRepositoryURINormalizer(
+        $normal_type_git,
+        $expect_remote))->getNormalizedPath();
+
+      $valid = ($remote_normal == $expect_normal);
+      $exists = true;
+    }
+
+    if (!$valid) {
+      if (!$exists) {
+        // If there's no "origin" remote, just create it regardless of how
+        // strongly we own the working copy. There is almost no conceivable
+        // scenario in which this could do damage.
+        $this->log(
+          pht(
+            'Remote "origin" does not exist. Creating "origin", with '.
+            'URI "%s".',
+            $expect_remote));
+        $repository->execxLocalCommand(
+          'remote add origin %P',
+          $repository->getRemoteURIEnvelope());
+
+        // NOTE: This doesn't fetch the origin (it just creates it), so we won't
+        // know about origin branches until the next "pull" happens. That's fine
+        // for our purposes, but might impact things in the future.
+      } else {
+        if ($repository->canDestroyWorkingCopy()) {
+          // Bad remote, but we can try to repair it.
+          $this->log(
+            pht(
+              'Remote "origin" exists, but is pointed at the wrong URI, "%s". '.
+              'Resetting origin URI to "%s.',
+              $remote_uri,
+              $expect_remote));
+          $repository->execxLocalCommand(
+            'remote set-url origin %P',
+            $repository->getRemoteURIEnvelope());
+        } else {
+          // Bad remote and we aren't comfortable repairing it.
+          $message = pht(
+            'Working copy at "%s" has a mismatched origin URI, "%s". '.
+            'The expected origin URI is "%s". Fix your configuration, or '.
+            'set the remote URI correctly. To avoid breaking anything, '.
+            'Phabricator will not automatically fix this.',
+            $repository->getLocalPath(),
+            $remote_uri,
+            $expect_remote);
+          throw new Exception($message);
+        }
+      }
+    }
+  }
+
+
 /* -(  Discovering Subversion Repositories  )-------------------------------- */
 
 
@@ -108,7 +255,8 @@ final class PhabricatorRepositoryDiscoveryEngine
         $epoch = (int)strtotime((string)$entry->date[0]);
         $refs[$identifier] = id(new PhabricatorRepositoryCommitRef())
           ->setIdentifier($identifier)
-          ->setEpoch($epoch);
+          ->setEpoch($epoch)
+          ->setCanCloseImmediately(true);
 
         if ($upper_bound === null) {
           $upper_bound = $identifier;
@@ -147,42 +295,49 @@ final class PhabricatorRepositoryDiscoveryEngine
     $branches = id(new DiffusionLowLevelMercurialBranchesQuery())
       ->setRepository($repository)
       ->execute();
-    $branches = mpull($branches, 'getHeadCommitIdentifier', 'getName');
 
     $refs = array();
-    foreach ($branches as $name => $commit) {
+    foreach ($branches as $branch) {
-      $this->log("Examining branch '{$name}', at {$commit}'.");
+      // NOTE: Mercurial branches may have multiple heads, so the names may
+      // not be unique.
+      $name = $branch->getShortName();
+      $commit = $branch->getCommitIdentifier();
+
+      $this->log(pht('Examining branch "%s" head "%s".', $name, $commit));
       if (!$repository->shouldTrackBranch($name)) {
-        $this->log("Skipping, branch is untracked.");
+        $this->log(pht("Skipping, branch is untracked."));
         continue;
       }
 
       if ($this->isKnownCommit($commit)) {
-        $this->log("Skipping, tip is a known commit.");
+        $this->log(pht("Skipping, this head is a known commit."));
         continue;
       }
 
-      $this->log("Looking for new commits.");
+      $this->log(pht("Looking for new commits."));
-      $refs[] = $this->discoverMercurialAncestry($repository, $commit);
+
+      $refs[] = $this->discoverStreamAncestry(
+        new PhabricatorMercurialGraphStream($repository, $commit),
+        $commit,
+        $close_immediately = true);
     }
 
     return array_mergev($refs);
   }
 
 
-  /**
+/* -(  Internals  )---------------------------------------------------------- */
-   * @task hg
+
-   */
+
-  private function discoverMercurialAncestry(
+  private function discoverStreamAncestry(
-    PhabricatorRepository $repository,
+    PhabricatorRepositoryGraphStream $stream,
-    $commit) {
+    $commit,
+    $close_immediately) {
 
     $discover = array($commit);
     $graph = array();
     $seen = array();
 
-    $stream = new PhabricatorMercurialGraphStream($repository);
-
     // Find all the reachable, undiscovered commits. Build a graph of the
     // edges.
     while ($discover) {
@@ -214,16 +369,14 @@ final class PhabricatorRepositoryDiscoveryEngine
     foreach ($commits as $commit) {
       $refs[] = id(new PhabricatorRepositoryCommitRef())
         ->setIdentifier($commit)
-        ->setEpoch($stream->getCommitDate($commit));
+        ->setEpoch($stream->getCommitDate($commit))
+        ->setCanCloseImmediately($close_immediately);
     }
 
     return $refs;
   }
 
 
-/* -(  Internals  )---------------------------------------------------------- */
-
-
   private function reduceGraph(array $edges) {
     foreach ($edges as $commit => $parents) {
       $edges[$commit] = array_keys($parents);
@@ -271,4 +424,33 @@ final class PhabricatorRepositoryDiscoveryEngine
     return true;
   }
 
+
+  /**
+   * Sort branches so we process closeable branches first. This makes the
+   * whole import process a little cheaper, since we can close these commits
+   * the first time through rather than catching them in the refs step.
+   *
+   * @task internal
+   *
+   * @param   list<DiffusionRepositoryRef> List of branch heads.
+   * @return  list<DiffusionRepositoryRef> Sorted list of branch heads.
+   */
+  private function sortBranches(array $branches) {
+    $repository = $this->getRepository();
+
+    $head_branches = array();
+    $tail_branches = array();
+    foreach ($branches as $branch) {
+      $name = $branch->getShortName();
+
+      if ($repository->shouldAutocloseBranch($name)) {
+        $head_branches[] = $branch;
+      } else {
+        $tail_branches[] = $branch;
+      }
+    }
+
+    return array_merge($head_branches, $tail_branches);
+  }
+
 }

src/applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php

@@ -4,12 +4,7 @@ final class PhabricatorWorkingCopyDiscoveryTestCase
   extends PhabricatorWorkingCopyTestCase {
 
   public function testSubversionCommitDiscovery() {
-    $repo = $this->buildPulledRepository('ST');
+    $refs = $this->discoverRefs('ST');
-
-    $engine = id(new PhabricatorRepositoryDiscoveryEngine())
-      ->setRepository($repo);
-
-    $refs = $engine->discoverCommits($repo);
     $this->assertEqual(
       array(
         1368319433,
@@ -17,11 +12,41 @@ final class PhabricatorWorkingCopyDiscoveryTestCase
       ),
       mpull($refs, 'getEpoch'),
       'Commit Epochs');
+  }
+
+  public function testMercurialCommitDiscovery() {
+    $refs = $this->discoverRefs('HT');
+    $this->assertEqual(
+      array(
+        '4a110ae879f473f2e82ffd032475caedd6cdba91',
+      ),
+      mpull($refs, 'getIdentifier'));
+  }
+
+  public function testGitCommitDiscovery() {
+    $refs = $this->discoverRefs('GT');
+    $this->assertEqual(
+      array(
+        '763d4ab372445551c95fb5cccd1a7a223f5b2ac8',
+      ),
+      mpull($refs, 'getIdentifier'));
+  }
+
+  private function discoverRefs($callsign) {
+    $repo = $this->buildPulledRepository($callsign);
+
+    $engine = id(new PhabricatorRepositoryDiscoveryEngine())
+      ->setRepository($repo);
+
+    $refs = $engine->discoverCommits($repo);
 
     // The next time through, these should be cached as already discovered.
 
-    $refs = $engine->discoverCommits($repo);
+    $new_refs = $engine->discoverCommits($repo);
-    $this->assertEqual(array(), $refs);
+    $this->assertEqual(array(), $new_refs);
+
+    return $refs;
   }
 
+
 }