Remove all the multi-pass autoclose-branch separate-cache / seenOnBranches junk

Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.

In particular:

  - As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
  - Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
  - We no longer need to do a separate discovery step on closable branches.
  - We no longer need to keep track of `seenOnBranches`.

Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4327

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

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -348,10 +348,8 @@ final class DiffusionCommitController extends DiffusionController {
       $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/');
       $change_list->setRepository($repository);
       $change_list->setUser($user);
-      // pick the first branch for "Browse in Diffusion" View Option
-      $branches     = $commit_data->getCommitDetail('seenOnBranches', array());
-      $first_branch = reset($branches);
-      $change_list->setBranch($first_branch);
+
+      // TODO: Try to setBranch() to something reasonable here?
 
       $change_list->setStandaloneURI(
         '/diffusion/'.$callsign.'/diff/');

src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php

@@ -246,7 +246,7 @@ final class PhabricatorRepositoryPullLocalDaemon
           $repository,
           $ref->getIdentifier(),
           $ref->getEpoch(),
-          $ref->getBranch());
+          $close_immediately = true);
       }
     }
 
@@ -319,56 +319,21 @@ final class PhabricatorRepositoryPullLocalDaemon
     return true;
   }
 
-  private function isKnownCommitOnAnyAutocloseBranch(
-    PhabricatorRepository $repository,
-    $target) {
-
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'repositoryID = %d AND commitIdentifier = %s',
-      $repository->getID(),
-      $target);
-
-    if (!$commit) {
-      $callsign = $repository->getCallsign();
-
-      $console = PhutilConsole::getConsole();
-      $console->writeErr(
-        "WARNING: Repository '%s' is missing commits ('%s' is missing from ".
-        "history). Run '%s' to repair the repository.\n",
-        $callsign,
-        $target,
-        "bin/repository discover --repair {$callsign}");
-
-      return false;
-    }
-
-    $data = $commit->loadCommitData();
-    if (!$data) {
-      return false;
-    }
-
-    if ($repository->shouldAutocloseCommit($commit, $data)) {
-      return true;
-    }
-
-    return false;
-  }
-
   private function recordCommit(
     PhabricatorRepository $repository,
     $commit_identifier,
     $epoch,
-    $branch = null) {
+    $close_immediately) {
 
     $commit = new PhabricatorRepositoryCommit();
     $commit->setRepositoryID($repository->getID());
     $commit->setCommitIdentifier($commit_identifier);
     $commit->setEpoch($epoch);
+    if ($close_immediately) {
+      $commit->setImportStatus(PhabricatorRepositoryCommit::IMPORTED_CLOSEABLE);
+    }
 
     $data = new PhabricatorRepositoryCommitData();
-    if ($branch) {
-      $data->setCommitDetail('seenOnBranches', array($branch));
-    }
 
     try {
       $commit->openTransaction();
@@ -419,43 +384,6 @@ final class PhabricatorRepositoryPullLocalDaemon
     }
   }
 
-  private function updateCommit(
-    PhabricatorRepository $repository,
-    $commit_identifier,
-    $branch) {
-
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'repositoryID = %d AND commitIdentifier = %s',
-      $repository->getID(),
-      $commit_identifier);
-
-    if (!$commit) {
-      // This can happen if the phabricator DB doesn't have the commit info,
-      // or the commit is so big that phabricator couldn't parse it. In this
-      // case we just ignore it.
-      return;
-    }
-
-    $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
-      'commitID = %d',
-      $commit->getID());
-    if (!$data) {
-      $data = new PhabricatorRepositoryCommitData();
-      $data->setCommitID($commit->getID());
-    }
-    $branches = $data->getCommitDetail('seenOnBranches', array());
-    $branches[] = $branch;
-    $data->setCommitDetail('seenOnBranches', $branches);
-    $data->save();
-
-    $this->insertTask(
-      $repository,
-      $commit,
-      array(
-        'only' => true
-      ));
-  }
-
   private function insertTask(
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit,
@@ -566,6 +494,8 @@ final class PhabricatorRepositoryPullLocalDaemon
       return;
     }
 
+    $branches = $this->sortBranches($repository, $branches);
+
     $callsign = $repository->getCallsign();
 
     $tracked_something = false;
@@ -586,7 +516,10 @@ final class PhabricatorRepositoryPullLocalDaemon
       }
 
       $this->log("Looking for new commits.");
-      $this->executeGitDiscoverCommit($repository, $commit, $name, false);
+      $this->executeGitDiscoverCommit(
+        $repository,
+        $commit,
+        $repository->shouldAutocloseBranch($name));
     }
 
     if (!$tracked_something) {
@@ -596,31 +529,29 @@ final class PhabricatorRepositoryPullLocalDaemon
         "Repository r{$repo_callsign} '{$repo_name}' has no tracked branches! ".
         "Verify that your branch filtering settings are correct.");
     }
-
-
-    $this->log("Discovering commits on autoclose branches...");
-    foreach ($branches as $name => $commit) {
-      $this->log("Examining branch '{$name}', at {$commit}'.");
-      if (!$repository->shouldTrackBranch($name)) {
-        $this->log("Skipping, branch is untracked.");
-        continue;
-      }
-
-      if (!$repository->shouldAutocloseBranch($name)) {
-        $this->log("Skipping, branch is not autoclose.");
-        continue;
-      }
-
-      if ($this->isKnownCommitOnAnyAutocloseBranch($repository, $commit)) {
-        $this->log("Skipping, commit is known on an autoclose branch.");
-        continue;
-      }
-
-      $this->log("Looking for new autoclose commits.");
-      $this->executeGitDiscoverCommit($repository, $commit, $name, 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.
+   */
+  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;
+  }
 
 
   /**
@@ -629,8 +560,7 @@ final class PhabricatorRepositoryPullLocalDaemon
   private function executeGitDiscoverCommit(
     PhabricatorRepository $repository,
     $commit,
-    $branch,
-    $autoclose) {
+    $close_immediately) {
 
     $discover = array($commit);
     $insert = array($commit);
@@ -651,15 +581,9 @@ final class PhabricatorRepositoryPullLocalDaemon
           continue;
         }
         $seen_parent[$parent] = true;
-        if ($autoclose) {
-          $known = $this->isKnownCommitOnAnyAutocloseBranch(
-            $repository,
-            $parent);
-        } else {
-          $known = $this->isKnownCommit($repository, $parent);
-        }
+        $known = $this->isKnownCommit($repository, $parent);
         if (!$known) {
-          $this->log("Discovered commit '{$parent}'.");
+          $this->log(pht('Discovered commit "%s".', $parent));
           $discover[] = $parent;
           $insert[] = $parent;
         }
@@ -670,22 +594,14 @@ final class PhabricatorRepositoryPullLocalDaemon
     }
 
     $n = count($insert);
-    if ($autoclose) {
-      $this->log("Found {$n} new autoclose commits on branch '{$branch}'.");
-    } else {
-      $this->log("Found {$n} new commits on branch '{$branch}'.");
-    }
+    $this->log(pht('Found %d new commits.', new PhutilNumber($n)));
 
     while (true) {
       $target = array_pop($insert);
       $epoch = $stream->getCommitDate($target);
       $epoch = trim($epoch);
 
-      if ($autoclose) {
-        $this->updateCommit($repository, $target, $branch);
-      } else {
-        $this->recordCommit($repository, $target, $epoch, $branch);
-      }
+      $this->recordCommit($repository, $target, $epoch, $close_immediately);
 
       if (empty($insert)) {
         break;

src/applications/repository/storage/PhabricatorRepository.php

@@ -557,6 +557,9 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
       return true;
     }
 
+    // TODO: Remove this eventually, it's no longer written to by the import
+    // pipeline (however, old tasks may still be queued which don't reflect
+    // the new data format).
     $branches = $data->getCommitDetail('seenOnBranches', array());
     foreach ($branches as $branch) {
       if ($this->shouldAutocloseBranch($branch)) {