1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 08:52:39 +01:00

Make repository discovery partially testable

Summary:
Ref T2784. Begins pulling discovery into Engines and covering it with tests. In particular:

  - Discovery is currently a one-shot process where we find all the new commits and write them to the database in one go. Split it apart so we find and return the new commits first, then write them to the database separately. This makes things simpler and more testable.
  - This diff only brings SVN into an engine (and only the "find the commits" part), since it's simpler than Git or Mercurial.
  - Creates a base Engine class and moves common functionality there.
  - Restores the `--verbose` flag to `repository pull`.

Test Plan: Added unit tests. Ran `bin/repository discover`. Ran `bin/phd debug pulllocal`.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2784

Differential Revision: https://secure.phabricator.com/D5906
This commit is contained in:
epriestley 2013-05-12 19:08:37 -07:00
parent 105dd1899c
commit e7fde9a77c
11 changed files with 357 additions and 138 deletions

View file

@ -1283,13 +1283,16 @@ phutil_register_library_map(array(
'PhabricatorRepositoryCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php',
'PhabricatorRepositoryCommitOwnersWorker' => 'applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php',
'PhabricatorRepositoryCommitParserWorker' => 'applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php',
'PhabricatorRepositoryCommitRef' => 'applications/repository/engine/PhabricatorRepositoryCommitRef.php',
'PhabricatorRepositoryCommitSearchIndexer' => 'applications/repository/search/PhabricatorRepositoryCommitSearchIndexer.php',
'PhabricatorRepositoryConfigOptions' => 'applications/repository/PhabricatorRepositoryConfigOptions.php',
'PhabricatorRepositoryController' => 'applications/repository/controller/PhabricatorRepositoryController.php',
'PhabricatorRepositoryCreateController' => 'applications/repository/controller/PhabricatorRepositoryCreateController.php',
'PhabricatorRepositoryDAO' => 'applications/repository/storage/PhabricatorRepositoryDAO.php',
'PhabricatorRepositoryDeleteController' => 'applications/repository/controller/PhabricatorRepositoryDeleteController.php',
'PhabricatorRepositoryDiscoveryEngine' => 'applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php',
'PhabricatorRepositoryEditController' => 'applications/repository/controller/PhabricatorRepositoryEditController.php',
'PhabricatorRepositoryEngine' => 'applications/repository/engine/PhabricatorRepositoryEngine.php',
'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php',
'PhabricatorRepositoryGitCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php',
'PhabricatorRepositoryListController' => 'applications/repository/controller/PhabricatorRepositoryListController.php',
@ -1302,7 +1305,6 @@ phutil_register_library_map(array(
'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryMercurialCommitMessageParserWorker.php',
'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php',
'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php',
'PhabricatorRepositoryPullLocalDaemonTestCase' => 'applications/repository/daemon/__tests__/PhabricatorRepositoryPullLocalDaemonTestCase.php',
'PhabricatorRepositoryQuery' => 'applications/repository/query/PhabricatorRepositoryQuery.php',
'PhabricatorRepositoryShortcut' => 'applications/repository/storage/PhabricatorRepositoryShortcut.php',
'PhabricatorRepositorySvnCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositorySvnCommitChangeParserWorker.php',
@ -1488,6 +1490,7 @@ phutil_register_library_map(array(
'PhabricatorWorkerTaskDetailController' => 'applications/daemon/controller/PhabricatorWorkerTaskDetailController.php',
'PhabricatorWorkerTaskUpdateController' => 'applications/daemon/controller/PhabricatorWorkerTaskUpdateController.php',
'PhabricatorWorkerTestCase' => 'infrastructure/daemon/workers/__tests__/PhabricatorWorkerTestCase.php',
'PhabricatorWorkingCopyDiscoveryTestCase' => 'applications/repository/engine/__tests__/PhabricatorWorkingCopyDiscoveryTestCase.php',
'PhabricatorWorkingCopyPullTestCase' => 'applications/repository/engine/__tests__/PhabricatorWorkingCopyPullTestCase.php',
'PhabricatorWorkingCopyTestCase' => 'applications/repository/engine/__tests__/PhabricatorWorkingCopyTestCase.php',
'PhabricatorWorkpanelView' => 'view/layout/PhabricatorWorkpanelView.php',
@ -3014,6 +3017,7 @@ phutil_register_library_map(array(
'PhabricatorRepositoryCreateController' => 'PhabricatorRepositoryController',
'PhabricatorRepositoryDAO' => 'PhabricatorLiskDAO',
'PhabricatorRepositoryDeleteController' => 'PhabricatorRepositoryController',
'PhabricatorRepositoryDiscoveryEngine' => 'PhabricatorRepositoryEngine',
'PhabricatorRepositoryEditController' => 'PhabricatorRepositoryController',
'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
'PhabricatorRepositoryGitCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
@ -3025,8 +3029,8 @@ phutil_register_library_map(array(
'PhabricatorRepositoryManagementWorkflow' => 'PhutilArgumentWorkflow',
'PhabricatorRepositoryMercurialCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
'PhabricatorRepositoryMercurialCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker',
'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine',
'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon',
'PhabricatorRepositoryPullLocalDaemonTestCase' => 'PhabricatorTestCase',
'PhabricatorRepositoryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'PhabricatorRepositoryShortcut' => 'PhabricatorRepositoryDAO',
'PhabricatorRepositorySvnCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker',
@ -3197,6 +3201,7 @@ phutil_register_library_map(array(
'PhabricatorWorkerTaskDetailController' => 'PhabricatorDaemonController',
'PhabricatorWorkerTaskUpdateController' => 'PhabricatorDaemonController',
'PhabricatorWorkerTestCase' => 'PhabricatorTestCase',
'PhabricatorWorkingCopyDiscoveryTestCase' => 'PhabricatorWorkingCopyTestCase',
'PhabricatorWorkingCopyPullTestCase' => 'PhabricatorWorkingCopyTestCase',
'PhabricatorWorkingCopyTestCase' => 'PhabricatorTestCase',
'PhabricatorWorkpanelView' => 'AphrontView',

View file

@ -31,6 +31,7 @@ final class PhabricatorRepositoryPullLocalDaemon
private $commitCache = array();
private $repair;
private $discoveryEngines = array();
public function setRepair($repair) {
$this->repair = $repair;
@ -183,14 +184,38 @@ final class PhabricatorRepositoryPullLocalDaemon
case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
return $this->executeGitDiscover($repository);
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
return $this->executeSvnDiscover($repository);
$refs = $this->getDiscoveryEngine($repository)
->discoverCommits();
break;
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
return $this->executeHgDiscover($repository);
default:
throw new Exception("Unknown VCS '{$vcs}'!");
}
foreach ($refs as $ref) {
$this->recordCommit(
$repository,
$ref->getIdentifier(),
$ref->getEpoch(),
$ref->getBranch());
}
return (bool)count($refs);
}
private function getDiscoveryEngine(PhabricatorRepository $repository) {
$id = $repository->getID();
if (empty($this->discoveryEngines[$id])) {
$engine = id(new PhabricatorRepositoryDiscoveryEngine())
->setRepository($repository)
->setVerbose($this->getVerbose())
->setRepairMode($this->repair);
$this->discoveryEngines[$id] = $engine;
}
return $this->discoveryEngines[$id];
}
private function isKnownCommit(
PhabricatorRepository $repository,
@ -681,109 +706,4 @@ final class PhabricatorRepositoryPullLocalDaemon
}
}
/* -( Subversion Implementation )------------------------------------------ */
private function executeSvnDiscover(
PhabricatorRepository $repository) {
$uri = $this->executeSvnGetBaseSVNLogURI($repository);
list($xml) = $repository->execxRemoteCommand(
'log --xml --quiet --limit 1 %s@HEAD',
$uri);
$results = $this->executeSvnParseLogXML($xml);
$commit = head_key($results);
$epoch = head($results);
if ($this->isKnownCommit($repository, $commit)) {
return false;
}
$this->executeSvnDiscoverCommit($repository, $commit, $epoch);
return true;
}
private function executeSvnDiscoverCommit(
PhabricatorRepository $repository,
$commit,
$epoch) {
$uri = $this->executeSvnGetBaseSVNLogURI($repository);
$discover = array(
$commit => $epoch,
);
$upper_bound = $commit;
$limit = 1;
while ($upper_bound > 1 &&
!$this->isKnownCommit($repository, $upper_bound)) {
// Find all the unknown commits on this path. Note that we permit
// importing an SVN subdirectory rather than the entire repository, so
// commits may be nonsequential.
list($err, $xml, $stderr) = $repository->execRemoteCommand(
' log --xml --quiet --limit %d %s@%d',
$limit,
$uri,
$upper_bound - 1);
if ($err) {
if (preg_match('/(path|File) not found/', $stderr)) {
// We've gone all the way back through history and this path was not
// affected by earlier commits.
break;
} else {
throw new Exception("svn log error #{$err}: {$stderr}");
}
}
$discover += $this->executeSvnParseLogXML($xml);
$upper_bound = min(array_keys($discover));
// Discover 2, 4, 8, ... 256 logs at a time. This allows us to initially
// import large repositories fairly quickly, while pulling only as much
// data as we need in the common case (when we've already imported the
// repository and are just grabbing one commit at a time).
$limit = min($limit * 2, 256);
}
// NOTE: We do writes only after discovering all the commits so that we're
// never left in a state where we've missed commits -- if the discovery
// script terminates it can always resume and restore the import to a good
// state. This is also why we sort the discovered commits so we can do
// writes forward from the smallest one.
ksort($discover);
foreach ($discover as $commit => $epoch) {
$this->recordCommit($repository, $commit, $epoch);
}
}
private function executeSvnParseLogXML($xml) {
$xml = phutil_utf8ize($xml);
$result = array();
$log = new SimpleXMLElement($xml);
foreach ($log->logentry as $entry) {
$commit = (int)$entry['revision'];
$epoch = (int)strtotime((string)$entry->date[0]);
$result[$commit] = $epoch;
}
return $result;
}
private function executeSvnGetBaseSVNLogURI(
PhabricatorRepository $repository) {
$uri = $repository->getDetail('remote-uri');
$subpath = $repository->getDetail('svn-subpath');
return $uri.$subpath;
}
}

View file

@ -0,0 +1,36 @@
<?php
final class PhabricatorRepositoryCommitRef {
private $identifier;
private $epoch;
private $branch;
public function setIdentifier($identifier) {
$this->identifier = $identifier;
return $this;
}
public function getIdentifier() {
return $this->identifier;
}
public function setEpoch($epoch) {
$this->epoch = $epoch;
return $this;
}
public function getEpoch() {
return $this->epoch;
}
public function setBranch($branch) {
$this->branch = $branch;
return $this;
}
public function getBranch() {
return $this->branch;
}
}

View file

@ -0,0 +1,173 @@
<?php
/**
* @task discover Discovering Repositories
* @task svn Discovering Subversion Repositories
* @task git Discovering Git Repositories
* @task hg Discovering Mercurial Repositories
* @task internal Internals
*/
final class PhabricatorRepositoryDiscoveryEngine
extends PhabricatorRepositoryEngine {
private $repairMode;
private $commitCache = array();
const MAX_COMMIT_CACHE_SIZE = 2048;
/* -( Discovering Repositories )------------------------------------------- */
public function setRepairMode($repair_mode) {
$this->repairMode = $repair_mode;
return $this;
}
public function getRepairMode() {
return $this->repairMode;
}
/**
* @task discovery
*/
public function discoverCommits() {
$repository = $this->getRepository();
$vcs = $repository->getVersionControlSystem();
switch ($vcs) {
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
$refs = $this->discoverSubversionCommits();
break;
/*
TODO: Implement these!
case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
$refs = $this->executeGitDiscovery();
break;
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
$refs = $this->executeMercurialDiscovery();
break;
*/
default:
throw new Exception("Unknown VCS '{$vcs}'!");
}
// Mark discovered commits in the cache.
foreach ($refs as $ref) {
$this->commitCache[$ref->getIdentifier()] = true;
}
return $refs;
}
/* -( Discovering Subversion Repositories )-------------------------------- */
/**
* @task svn
*/
private function discoverSubversionCommits() {
$repository = $this->getRepository();
$upper_bound = null;
$limit = 1;
$refs = array();
do {
// Find all the unknown commits on this path. Note that we permit
// importing an SVN subdirectory rather than the entire repository, so
// commits may be nonsequential.
if ($upper_bound === null) {
$at_rev = 'HEAD';
} else {
$at_rev = ($upper_bound - 1);
}
try {
list($xml, $stderr) = $repository->execxRemoteCommand(
'log --xml --quiet --limit %d %s@%s',
$limit,
$repository->getSubversionBaseURI(),
$at_rev);
} catch (CommandException $ex) {
$stderr = $ex->getStdErr();
if (preg_match('/(path|File) not found/', $stderr)) {
// We've gone all the way back through history and this path was not
// affected by earlier commits.
break;
}
throw $ex;
}
$xml = phutil_utf8ize($xml);
$log = new SimpleXMLElement($xml);
foreach ($log->logentry as $entry) {
$identifier = (int)$entry['revision'];
$epoch = (int)strtotime((string)$entry->date[0]);
$refs[$identifier] = id(new PhabricatorRepositoryCommitRef())
->setIdentifier($identifier)
->setEpoch($epoch);
if ($upper_bound === null) {
$upper_bound = $identifier;
} else {
$upper_bound = min($upper_bound, $identifier);
}
}
// Discover 2, 4, 8, ... 256 logs at a time. This allows us to initially
// import large repositories fairly quickly, while pulling only as much
// data as we need in the common case (when we've already imported the
// repository and are just grabbing one commit at a time).
$limit = min($limit * 2, 256);
} while ($upper_bound > 1 && !$this->isKnownCommit($upper_bound));
krsort($refs);
while ($refs && $this->isKnownCommit(last($refs)->getIdentifier())) {
array_pop($refs);
}
$refs = array_reverse($refs);
return $refs;
}
/* -( Internals )---------------------------------------------------------- */
private function isKnownCommit($identifier) {
if (isset($this->commitCache[$identifier])) {
return true;
}
if ($this->repairMode) {
// 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',
$this->getRepository()->getID(),
$identifier);
if (!$commit) {
return false;
}
$this->commitCache[$identifier] = true;
while (count($this->commitCache) > self::MAX_COMMIT_CACHE_SIZE) {
array_shift($this->commitCache);
}
return true;
}
}

View file

@ -0,0 +1,62 @@
<?php
/**
* @task config Configuring Repository Engines
* @task internal Internals
*/
abstract class PhabricatorRepositoryEngine {
private $repository;
private $verbose;
/**
* @task config
*/
public function setRepository(PhabricatorRepository $repository) {
$this->repository = $repository;
return $this;
}
/**
* @task config
*/
protected function getRepository() {
if ($this->repository === null) {
throw new Exception("Call setRepository() to provide a repository!");
}
return $this->repository;
}
/**
* @task config
*/
public function setVerbose($verbose) {
$this->verbose = $verbose;
return $this;
}
/**
* @task config
*/
public function getVerbose() {
return $this->verbose;
}
/**
* @task internal
*/
protected function log($pattern /* ... */) {
if ($this->getVerbose()) {
$console = PhutilConsole::getConsole();
$argv = func_get_args();
call_user_func_array(array($console, 'writeLog'), $argv);
}
return $this;
}
}

View file

@ -10,37 +10,28 @@
* @task hg Pulling Mercurial Working Copies
* @task internal Internals
*/
final class PhabricatorRepositoryPullEngine {
private $repository;
final class PhabricatorRepositoryPullEngine
extends PhabricatorRepositoryEngine {
/* -( Pulling Working Copies )--------------------------------------------- */
public function setRepository(PhabricatorRepository $repository) {
$this->repository = $repository;
return $this;
}
private function getRepository() {
return $this->repository;
}
public function pullRepository() {
$repository = $this->getRepository();
if (!$repository) {
throw new Exception("Call setRepository() before pullRepository()!");
}
$is_hg = false;
$is_git = false;
$vcs = $repository->getVersionControlSystem();
$callsign = $repository->getCallsign();
switch ($vcs) {
case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN:
// We never pull a local copy of Subversion repositories.
$this->log(
"Repository '%s' is a Subversion repository, which does not require ".
"a local working copy to be pulled.",
$callsign);
return;
case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT:
$is_git = true;
@ -65,12 +56,18 @@ final class PhabricatorRepositoryPullEngine {
}
if (!Filesystem::pathExists($local_path)) {
$this->log(
"Creating a new working copy for repository '%s'.",
$callsign);
if ($is_git) {
$this->executeGitCreate();
} else {
$this->executeMercurialCreate();
}
} else {
$this->log(
"Updating the working copy for repository '%s'.",
$callsign);
if ($is_git) {
$this->executeGitUpdate();
} else {

View file

@ -1,7 +1,28 @@
<?php
final class PhabricatorRepositoryPullLocalDaemonTestCase
extends PhabricatorTestCase {
final class PhabricatorWorkingCopyDiscoveryTestCase
extends PhabricatorWorkingCopyTestCase {
public function testSubversionCommitDiscovery() {
$repo = $this->buildPulledRepository('ST');
$engine = id(new PhabricatorRepositoryDiscoveryEngine())
->setRepository($repo);
$refs = $engine->discoverCommits($repo);
$this->assertEqual(
array(
1368319433,
1368319448,
),
mpull($refs, 'getEpoch'),
'Commit Epochs');
// The next time through, these should be cached as already discovered.
$refs = $engine->discoverCommits($repo);
$this->assertEqual(array(), $refs);
}
public function testExecuteGitVerifySameOrigin() {
$cases = array(

View file

@ -3,8 +3,6 @@
abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase {
private $dirs = array();
private $repos = array();
private $pulled = array();
protected function getPhabricatorTestCaseConfiguration() {
return array(
@ -13,10 +11,6 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase {
}
protected function buildBareRepository($callsign) {
if (isset($this->repos[$callsign])) {
return $this->repos[$callsign];
}
$data_dir = dirname(__FILE__).'/data/';
$types = array(
@ -67,8 +61,6 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase {
$this->dirs[] = $dir;
$this->dirs[] = $local;
$this->repos[$callsign] = $repo;
return $repo;
}
@ -79,10 +71,6 @@ abstract class PhabricatorWorkingCopyTestCase extends PhabricatorTestCase {
protected function buildPulledRepository($callsign) {
$repository = $this->buildBareRepository($callsign);
if (isset($this->pulled[$callsign])) {
return $repository;
}
id(new PhabricatorRepositoryPullEngine())
->setRepository($repository)
->pullRepository();

View file

@ -10,6 +10,10 @@ final class PhabricatorRepositoryManagementPullWorkflow
->setSynopsis('Pull __repository__, named by callsign or PHID.')
->setArguments(
array(
array(
'name' => 'verbose',
'help' => 'Show additional debugging information.',
),
array(
'name' => 'repos',
'wildcard' => true,
@ -32,6 +36,7 @@ final class PhabricatorRepositoryManagementPullWorkflow
id(new PhabricatorRepositoryPullEngine())
->setRepository($repo)
->setVerbose($args->getArg('verbose'))
->pullRepository();
}

View file

@ -90,6 +90,18 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
return $this->getDetail('local-path');
}
public function getSubversionBaseURI() {
$vcs = $this->getVersionControlSystem();
if ($vcs != PhabricatorRepositoryType::REPOSITORY_TYPE_SVN) {
throw new Exception("Not a subversion repository!");
}
$uri = $this->getDetail('remote-uri');
$subpath = $this->getDetail('svn-subpath');
return $uri.$subpath;
}
public function execRemoteCommand($pattern /* , $arg, ... */) {
$args = func_get_args();
$args = $this->formatRemoteCommand($args);