1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-25 16:22:42 +01:00

Refactor getWorkingCopyStatus() into getUncommittedStatus() and getCommitRangeStatus()

Summary:
See discussion in D4049.

The getWorkingCopyStatus() method gets called from requireCleanWorkingCopy() in a lot of places, which triggers resolution of the base of the commit range. This is unnecessary; we do not need to examine the base commit in order to determine whether the working copy is dirty or not. This causes problems, in D4049 and elsewhere (we currently have a lot of fluff calls to setDefaultBaseCommit() in workflows which need to call requireCleanWorkingCopy() but do not ever use commit ranges, such as `arc patch`). This is mostly an artifact of SVN, where the "commit range" and "uncommitted stuff in the working copy" are always the same.

  - Split the method into two status methods: getUncommittedStatus() (uncommitted stuff in the working copy, required by requireCleanWorkingCopy()) and getCommitRangeStatus() (committed stuff in the commit range).
  - Lift caching out of the implementations into the base class.
  - Dirty the cache after we commit.

This doesn't do anything useful on its own and creates one caching problem (`commitRangeStatusCache` is not invalidated when the commit range changes because of `setBaseCommit()` or similar) but I wanted to break things apart here. I won't land it until there's a more complete picture.

This creates a minor performance regression in git and hg (we run less stuff in parallel than previously) but all the commands should be disk-bound anyway and the regression should be minor. It prevents a larger regression in `hg` in D4049, and lets us do less work to arrive at common error states (dirty working copy). We can examine perf at the end of this change sequence.

Test Plan: Ran unit tests, various `arc` commands.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Differential Revision: https://secure.phabricator.com/D4095
This commit is contained in:
epriestley 2012-12-17 12:53:28 -08:00
parent 5f5392df8a
commit 0e27dbfd17
9 changed files with 394 additions and 160 deletions

View file

@ -103,6 +103,7 @@ phutil_register_library_map(array(
'ArcanistPyFlakesLinter' => 'lint/linter/ArcanistPyFlakesLinter.php',
'ArcanistPyLintLinter' => 'lint/linter/ArcanistPyLintLinter.php',
'ArcanistRepositoryAPI' => 'repository/api/ArcanistRepositoryAPI.php',
'ArcanistRepositoryAPIStateTestCase' => 'repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php',
'ArcanistRubyLinter' => 'lint/linter/ArcanistRubyLinter.php',
'ArcanistRubyLinterTestCase' => 'lint/linter/__tests__/ArcanistRubyLinterTestCase.php',
'ArcanistScalaSBTLinter' => 'lint/linter/ArcanistScalaSBTLinter.php',
@ -216,6 +217,7 @@ phutil_register_library_map(array(
'ArcanistPhutilTestTerminatedException' => 'Exception',
'ArcanistPyFlakesLinter' => 'ArcanistLinter',
'ArcanistPyLintLinter' => 'ArcanistLinter',
'ArcanistRepositoryAPIStateTestCase' => 'ArcanistTestCase',
'ArcanistRubyLinter' => 'ArcanistLinter',
'ArcanistRubyLinterTestCase' => 'ArcanistArcanistLinterTestCase',
'ArcanistScalaSBTLinter' => 'ArcanistLinter',

View file

@ -7,14 +7,13 @@
*/
final class ArcanistGitAPI extends ArcanistRepositoryAPI {
private $status;
private $relativeCommit = null;
private $repositoryHasNoCommits = false;
const SEARCH_LENGTH_FOR_PARENT_REVISIONS = 16;
/**
* For the repository's initial commit, 'git diff HEAD^' and similar do
* not work. Using this instead does work.
* not work. Using this instead does work; it is the hash of the empty tree.
*/
const GIT_MAGIC_ROOT_COMMIT = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
@ -384,92 +383,81 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
return rtrim($stdout);
}
public function getWorkingCopyStatus() {
if (!isset($this->status)) {
protected function buildUncommittedStatus() {
$diff_options = $this->getDiffBaseOptions();
$options = $this->getDiffBaseOptions();
// -- parallelize these slow cpu bound git calls.
// Find committed changes.
$committed_future = $this->buildLocalFuture(
array(
"diff {$options} --raw %s --",
$this->getRelativeCommit(),
));
// Find uncommitted changes.
$uncommitted_future = $this->buildLocalFuture(
array(
"diff {$options} --raw %s --",
$this->repositoryHasNoCommits
? self::GIT_MAGIC_ROOT_COMMIT
: 'HEAD',
));
// Untracked files
$untracked_future = $this->buildLocalFuture(
array(
'ls-files --others --exclude-standard',
));
// TODO: This doesn't list unstaged adds. It's not clear how to get that
// list other than "git status --porcelain" and then parsing it. :/
// Unstaged changes
$unstaged_future = $this->buildLocalFuture(
array(
'ls-files -m',
));
$futures = array(
$committed_future,
$uncommitted_future,
$untracked_future,
$unstaged_future
);
Futures($futures)->resolveAll();
// -- read back and process the results
list($stdout, $stderr) = $committed_future->resolvex();
$files = $this->parseGitStatus($stdout);
list($stdout, $stderr) = $uncommitted_future->resolvex();
$uncommitted_files = $this->parseGitStatus($stdout);
foreach ($uncommitted_files as $path => $mask) {
$mask |= self::FLAG_UNCOMMITTED;
if (!isset($files[$path])) {
$files[$path] = 0;
}
$files[$path] |= $mask;
}
list($stdout, $stderr) = $untracked_future->resolvex();
$stdout = rtrim($stdout, "\n");
if (strlen($stdout)) {
$stdout = explode("\n", $stdout);
foreach ($stdout as $file) {
$files[$file] = self::FLAG_UNTRACKED;
}
}
list($stdout, $stderr) = $unstaged_future->resolvex();
$stdout = rtrim($stdout, "\n");
if (strlen($stdout)) {
$stdout = explode("\n", $stdout);
foreach ($stdout as $file) {
$files[$file] = isset($files[$file])
? ($files[$file] | self::FLAG_UNSTAGED)
: self::FLAG_UNSTAGED;
}
}
$this->status = $files;
if ($this->repositoryHasNoCommits) {
$diff_base = self::GIT_MAGIC_ROOT_COMMIT;
} else {
$diff_base = 'HEAD';
}
return $this->status;
// Find uncommitted changes.
$uncommitted_future = $this->buildLocalFuture(
array(
'diff %C --raw %s --',
$diff_options,
$diff_base,
));
$untracked_future = $this->buildLocalFuture(
array(
'ls-files --others --exclude-standard',
));
// TODO: This doesn't list unstaged adds. It's not clear how to get that
// list other than "git status --porcelain" and then parsing it. :/
// Unstaged changes
$unstaged_future = $this->buildLocalFuture(
array(
'ls-files -m',
));
$futures = array(
$uncommitted_future,
$untracked_future,
$unstaged_future,
);
Futures($futures)->resolveAll();
$result = new PhutilArrayWithDefaultValue();
list($stdout) = $uncommitted_future->resolvex();
$uncommitted_files = $this->parseGitStatus($stdout);
foreach ($uncommitted_files as $path => $mask) {
$result[$path] |= ($mask | self::FLAG_UNCOMMITTED);
}
list($stdout) = $untracked_future->resolvex();
$stdout = rtrim($stdout, "\n");
if (strlen($stdout)) {
$stdout = explode("\n", $stdout);
foreach ($stdout as $path) {
$result[$path] |= self::FLAG_UNTRACKED;
}
}
list($stdout, $stderr) = $unstaged_future->resolvex();
$stdout = rtrim($stdout, "\n");
if (strlen($stdout)) {
$stdout = explode("\n", $stdout);
foreach ($stdout as $path) {
$result[$path] |= self::FLAG_UNSTAGED;
}
}
return $result->toArray();
}
protected function buildCommitRangeStatus() {
list($stdout, $stderr) = $this->execxLocal(
'diff %C --raw %s --',
$this->getDiffBaseOptions(),
$this->getRelativeCommit());
return $this->parseGitStatus($stdout);
}
public function getGitConfig($key, $default = null) {
@ -497,6 +485,10 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
$this->execxLocal(
'commit --allow-empty-message -F %s',
$tmp_file);
$this->reloadWorkingCopy();
return $this;
}
public function amendCommit($message) {

View file

@ -7,7 +7,6 @@
*/
final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
private $status;
private $base;
private $relativeCommit;
private $branch;
@ -296,70 +295,66 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
return $blame;
}
public function getWorkingCopyStatus() {
protected function buildUncommittedStatus() {
list($stdout) = $this->execxLocal('status');
if (!isset($this->status)) {
// A reviewable revision spans multiple local commits in Mercurial, but
// there is no way to get file change status across multiple commits, so
// just take the entire diff and parse it to figure out what's changed.
$results = new PhutilArrayWithDefaultValue();
// Execute status in the background
$status_future = $this->buildLocalFuture(array('status'));
$status_future->start();
$diff = $this->getFullMercurialDiff();
if (!$diff) {
$this->status = array();
return $this->status;
$working_status = ArcanistMercurialParser::parseMercurialStatus($stdout);
foreach ($working_status as $path => $mask) {
if (!($mask & ArcanistRepositoryAPI::FLAG_UNTRACKED)) {
// Mark tracked files as uncommitted.
$mask |= self::FLAG_UNCOMMITTED;
}
$parser = new ArcanistDiffParser();
$changes = $parser->parseDiff($diff);
$status_map = array();
foreach ($changes as $change) {
$flags = 0;
switch ($change->getType()) {
case ArcanistDiffChangeType::TYPE_ADD:
case ArcanistDiffChangeType::TYPE_MOVE_HERE:
case ArcanistDiffChangeType::TYPE_COPY_HERE:
$flags |= self::FLAG_ADDED;
break;
case ArcanistDiffChangeType::TYPE_CHANGE:
case ArcanistDiffChangeType::TYPE_COPY_AWAY: // Check for changes?
$flags |= self::FLAG_MODIFIED;
break;
case ArcanistDiffChangeType::TYPE_DELETE:
case ArcanistDiffChangeType::TYPE_MOVE_AWAY:
case ArcanistDiffChangeType::TYPE_MULTICOPY:
$flags |= self::FLAG_DELETED;
break;
}
$status_map[$change->getCurrentPath()] = $flags;
}
list($stdout) = $status_future->resolvex();
$working_status = ArcanistMercurialParser::parseMercurialStatus($stdout);
foreach ($working_status as $path => $status) {
if ($status & ArcanistRepositoryAPI::FLAG_UNTRACKED) {
// If the file is untracked, don't mark it uncommitted.
continue;
}
$status |= self::FLAG_UNCOMMITTED;
if (!empty($status_map[$path])) {
$status_map[$path] |= $status;
} else {
$status_map[$path] = $status;
}
}
$this->status = $status_map;
$results[$path] |= $mask;
}
return $this->status;
return $results->toArray();
}
protected function buildCommitRangeStatus() {
// TODO: Possibly we should use "hg status --rev X --rev ." for this
// instead, but we must run "hg diff" later anyway in most cases, so
// building and caching it shouldn't hurt us.
$diff = $this->getFullMercurialDiff();
if (!$diff) {
return array();
}
$parser = new ArcanistDiffParser();
$changes = $parser->parseDiff($diff);
$status_map = array();
foreach ($changes as $change) {
$flags = 0;
switch ($change->getType()) {
case ArcanistDiffChangeType::TYPE_ADD:
case ArcanistDiffChangeType::TYPE_MOVE_HERE:
case ArcanistDiffChangeType::TYPE_COPY_HERE:
$flags |= self::FLAG_ADDED;
break;
case ArcanistDiffChangeType::TYPE_CHANGE:
case ArcanistDiffChangeType::TYPE_COPY_AWAY: // Check for changes?
$flags |= self::FLAG_MODIFIED;
break;
case ArcanistDiffChangeType::TYPE_DELETE:
case ArcanistDiffChangeType::TYPE_MOVE_AWAY:
case ArcanistDiffChangeType::TYPE_MULTICOPY:
$flags |= self::FLAG_DELETED;
break;
}
$status_map[$change->getCurrentPath()] = $flags;
}
return $status_map;
}
protected function didReloadWorkingCopy() {
// Diffs are against ".", so we need to drop the cache if we change the
// working copy.
$this->rawDiffCache = array();
}
private function getDiffOptions() {
@ -629,6 +624,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
$this->execxLocal(
'commit -l %s',
$tmp_file);
$this->reloadWorkingCopy();
}
public function amendCommit($message) {

View file

@ -3,6 +3,7 @@
/**
* Interfaces with the VCS in the working copy.
*
* @task status Path Status
* @group workingcopy
*/
abstract class ArcanistRepositoryAPI {
@ -30,6 +31,9 @@ abstract class ArcanistRepositoryAPI {
private $workingCopyIdentity;
private $baseCommitArgumentRules;
private $uncommittedStatusCache;
private $commitRangeStatusCache;
abstract public function getSourceControlSystemName();
public function getDiffLinesOfContext() {
@ -76,7 +80,9 @@ abstract class ArcanistRepositoryAPI {
}
// check if we're in an svn working copy
list($err) = exec_manual('svn info');
list($err) = id(new ExecFuture('svn info'))
->setCWD($root)
->resolve();
if (!$err) {
$api = new ArcanistSubversionAPI($root);
$api->workingCopyIdentity = $working_copy;
@ -101,36 +107,174 @@ abstract class ArcanistRepositoryAPI {
}
}
public function getUntrackedChanges() {
return $this->getWorkingCopyFilesWithMask(self::FLAG_UNTRACKED);
/* -( Path Status )-------------------------------------------------------- */
abstract protected function buildUncommittedStatus();
abstract protected function buildCommitRangeStatus();
/**
* Get a list of uncommitted paths in the working copy that have been changed
* or are affected by other status effects, like conflicts or untracked
* files.
*
* Convenience methods @{method:getUntrackedChanges},
* @{method:getUnstagedChanges}, @{method:getUncommittedChanges},
* @{method:getMergeConflicts}, and @{method:getIncompleteChanges} allow
* simpler selection of paths in a specific state.
*
* This method returns a map of paths to bitmasks with status, using
* `FLAG_` constants. For example:
*
* array(
* 'some/uncommitted/file.txt' => ArcanistRepositoryAPI::FLAG_UNSTAGED,
* );
*
* A file may be in several states. Not all states are possible with all
* version control systems.
*
* @return map<string, bitmask> Map of paths, see above.
* @task status
*/
final public function getUncommittedStatus() {
if ($this->uncommittedStatusCache === null) {
$status = $this->buildUncommittedStatus();;
ksort($status);
$this->uncommittedStatusCache = $status;
}
return $this->uncommittedStatusCache;
}
public function getUnstagedChanges() {
return $this->getWorkingCopyFilesWithMask(self::FLAG_UNSTAGED);
/**
* @task status
*/
final public function getUntrackedChanges() {
return $this->getUncommittedPathsWithMask(self::FLAG_UNTRACKED);
}
public function getUncommittedChanges() {
return $this->getWorkingCopyFilesWithMask(self::FLAG_UNCOMMITTED);
/**
* @task status
*/
final public function getUnstagedChanges() {
return $this->getUncommittedPathsWithMask(self::FLAG_UNSTAGED);
}
public function getMergeConflicts() {
return $this->getWorkingCopyFilesWithMask(self::FLAG_CONFLICT);
/**
* @task status
*/
final public function getUncommittedChanges() {
return $this->getUncommittedPathsWithMask(self::FLAG_UNCOMMITTED);
}
public function getIncompleteChanges() {
return $this->getWorkingCopyFilesWithMask(self::FLAG_INCOMPLETE);
/**
* @task status
*/
final public function getMergeConflicts() {
return $this->getUncommittedPathsWithMask(self::FLAG_CONFLICT);
}
private function getWorkingCopyFilesWithMask($mask) {
/**
* @task status
*/
final public function getIncompleteChanges() {
return $this->getUncommittedPathsWithMask(self::FLAG_INCOMPLETE);
}
/**
* @task status
*/
private function getUncommittedPathsWithMask($mask) {
$match = array();
foreach ($this->getWorkingCopyStatus() as $file => $flags) {
foreach ($this->getUncommittedStatus() as $path => $flags) {
if ($flags & $mask) {
$match[] = $file;
$match[] = $path;
}
}
return $match;
}
/**
* Get a list of paths affected by the commits in the current commit range.
*
* See @{method:getUncommittedStatus} for a description of the return value.
*
* @return map<string, bitmask> Map from paths to status.
* @task status
*/
final public function getCommitRangeStatus() {
if ($this->commitRangeStatusCache === null) {
$status = $this->buildCommitRangeStatus();
ksort($status);
$this->commitRangeStatusCache = $status;
}
return $this->commitRangeStatusCache;
}
/**
* Get a list of paths affected by commits in the current commit range, or
* uncommitted changes in the working copy. See @{method:getUncommittedStatus}
* or @{method:getCommitRangeStatus} to retreive smaller parts of the status.
*
* See @{method:getUncommittedStatus} for a description of the return value.
*
* @return map<string, bitmask> Map from paths to status.
* @task status
*/
final public function getWorkingCopyStatus() {
$range_status = $this->getCommitRangeStatus();
$uncommitted_status = $this->getUncommittedStatus();
$result = new PhutilArrayWithDefaultValue($range_status);
foreach ($uncommitted_status as $path => $mask) {
$result[$path] |= $mask;
}
$result = $result->toArray();
ksort($result);
return $result;
}
/**
* Drops caches after changes to the working copy. By default, some queries
* against the working copy are cached. They
*
* @return this
* @task status
*/
final public function reloadWorkingCopy() {
$this->uncommittedStatusCache = null;
$this->commitRangeStatusCache = null;
$this->didReloadWorkingCopy();
return $this;
}
/**
* Hook for implementations to dirty working copy caches after the working
* copy has been updated.
*
* @return this
* @task status
*/
protected function didReloadWorkingCopy() {
return;
}
private static function discoverGitBaseDirectory($root) {
try {
@ -148,13 +292,14 @@ abstract class ArcanistRepositoryAPI {
}
}
/**
* @return Traversable
*/
abstract public function getAllFiles();
abstract public function getBlame($path);
abstract public function getWorkingCopyStatus();
abstract public function getRawDiffText($path);
abstract public function getOriginalFileData($path);
abstract public function getCurrentFileData($path);

View file

@ -46,7 +46,13 @@ final class ArcanistSubversionAPI extends ArcanistRepositoryAPI {
return $future;
}
public function getWorkingCopyStatus() {
protected function buildCommitRangeStatus() {
// In SVN, the commit range is always "uncommitted changes", so these
// statuses are equivalent.
return $this->getUncommittedStatus();
}
protected function buildUncommittedStatus() {
return $this->getSVNStatus();
}

View file

@ -0,0 +1,93 @@
<?php
final class ArcanistRepositoryAPIStateTestCase extends ArcanistTestCase {
public function testStateParsing() {
$dir = dirname(__FILE__).'/state/';
$tests = Filesystem::listDirectory($dir, $include_hidden = false);
foreach ($tests as $test) {
$fixture = PhutilDirectoryFixture::newFromArchive($dir.'/'.$test);
$fixture_path = $fixture->getPath();
$working_copy = ArcanistWorkingCopyIdentity::newFromPath($fixture_path);
$api = ArcanistRepositoryAPI::newAPIFromWorkingCopyIdentity(
$working_copy);
if ($api->supportsRelativeLocalCommits()) {
$api->setDefaultBaseCommit();
}
$this->assertCorrectState($test, $api);
}
}
private function assertCorrectState($test, ArcanistRepositoryAPI $api) {
$f_mod = ArcanistRepositoryAPI::FLAG_MODIFIED;
$f_add = ArcanistRepositoryAPI::FLAG_ADDED;
$f_del = ArcanistRepositoryAPI::FLAG_DELETED;
$f_unt = ArcanistRepositoryAPI::FLAG_UNTRACKED;
$f_con = ArcanistRepositoryAPI::FLAG_CONFLICT;
$f_mis = ArcanistRepositoryAPI::FLAG_MISSING;
$f_uns = ArcanistRepositoryAPI::FLAG_UNSTAGED;
$f_unc = ArcanistRepositoryAPI::FLAG_UNCOMMITTED;
$f_ext = ArcanistRepositoryAPI::FLAG_EXTERNALS;
$f_obs = ArcanistRepositoryAPI::FLAG_OBSTRUCTED;
$f_inc = ArcanistRepositoryAPI::FLAG_INCOMPLETE;
switch ($test) {
case 'svn_basic.svn.tgz':
$expect = array(
'ADDED' => $f_add,
'COPIED_TO' => $f_add,
'DELETED' => $f_del,
'MODIFIED' => $f_mod,
'MOVED' => $f_del,
'MOVED_TO' => $f_add,
'PROPCHANGE' => $f_mod,
'UNTRACKED' => $f_unt,
);
$this->assertEqual($expect, $api->getUncommittedStatus());
$this->assertEqual($expect, $api->getCommitRangeStatus());
break;
case 'git_basic.git.tgz':
$expect_uncommitted = array(
'UNCOMMITTED' => $f_add | $f_unc,
'UNSTAGED' => $f_mod | $f_uns | $f_unc,
'UNTRACKED' => $f_unt,
);
$this->assertEqual($expect_uncommitted, $api->getUncommittedStatus());
$expect_range = array(
'ADDED' => $f_add,
'DELETED' => $f_del,
'MODIFIED' => $f_mod,
'UNCOMMITTED' => $f_add,
'UNSTAGED' => $f_add,
);
$this->assertEqual($expect_range, $api->getCommitRangeStatus());
break;
case 'hg_basic.hg.tgz':
$expect_uncommitted = array(
'UNCOMMITTED' => $f_mod | $f_unc,
'UNTRACKED' => $f_unt,
);
$this->assertEqual($expect_uncommitted, $api->getUncommittedStatus());
$expect_range = array(
'ADDED' => $f_add,
'DELETED' => $f_del,
'MODIFIED' => $f_mod,
'UNCOMMITTED' => $f_add,
);
$this->assertEqual($expect_range, $api->getCommitRangeStatus());
break;
default:
throw new Exception(
"No test cases for working copy '{$test}'!");
}
}
}

Binary file not shown.

Binary file not shown.

Binary file not shown.