From 0bf5c3603c9b2eeda1f96399cbcc6957cbad72cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Dec 2012 12:54:08 -0800 Subject: [PATCH] Refactor commit ranges and base commit handling Summary: See D4049, D4096. - Move commit range storage from Mercurial and Git APIs to the base API. - Move caching up to the base level. - Store symbolic name and resolved name separately, so we can re-resolve the correct commit from the symbolic name after dirtying caches. - Rename `supportsRelativeLocalCommit()` to `supportsCommitRanges()` (old name wasn't very good, and not consistent with new terminology like the `--base` flag). - Rename `getRelativeCommit()` and `setRelativeCommit()` to `getBaseCommit()` and `setBaseCommit()`. - Introduce `reloadCommitRange()` and call it from `reloadWorkingCopy()`. I think this fixes the problem in D4049, and provides a general solution for the class of problems we're running into here, with D4096. Specifically: - We no longer get dirty caches, as long as you call reloadWorkingCopy() after changing the working copy (or call a method which calls it for you). - We no longer get order-of-parsing-things problems, because setBaseCommit() reloads the appropriate caches. - We no longer get nasty effects from calling `requireCleanWorkingCopy()` too early. Test Plan: This is pretty far-reaching and hard to test. Unit tests; ran various arc commands. :/ Reviewers: vrana Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D4097 --- src/repository/api/ArcanistGitAPI.php | 332 +++++++++---------- src/repository/api/ArcanistMercurialAPI.php | 224 ++++++------- src/repository/api/ArcanistRepositoryAPI.php | 52 ++- src/repository/api/ArcanistSubversionAPI.php | 6 +- src/workflow/ArcanistBaseWorkflow.php | 39 ++- src/workflow/ArcanistCoverWorkflow.php | 4 +- src/workflow/ArcanistDiffWorkflow.php | 24 +- src/workflow/ArcanistExportWorkflow.php | 6 +- src/workflow/ArcanistLandWorkflow.php | 6 +- src/workflow/ArcanistLintWorkflow.php | 7 +- src/workflow/ArcanistPatchWorkflow.php | 2 +- src/workflow/ArcanistWhichWorkflow.php | 11 +- 12 files changed, 368 insertions(+), 345 deletions(-) diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 87673012..fd1a23d3 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -7,7 +7,6 @@ */ final class ArcanistGitAPI extends ArcanistRepositoryAPI { - private $relativeCommit = null; private $repositoryHasNoCommits = false; const SEARCH_LENGTH_FOR_PARENT_REVISIONS = 16; @@ -43,18 +42,13 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return !$this->repositoryHasNoCommits; } - public function setRelativeCommit($relative_commit) { - $this->relativeCommit = $relative_commit; - return $this; - } - public function getLocalCommitInformation() { if ($this->repositoryHasNoCommits) { // Zero commits. throw new Exception( "You can't get local commit information for a repository with no ". "commits."); - } else if ($this->relativeCommit == self::GIT_MAGIC_ROOT_COMMIT) { + } else if ($this->getBaseCommit() == self::GIT_MAGIC_ROOT_COMMIT) { // One commit. $against = 'HEAD'; } else { @@ -79,7 +73,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // this as being the commits X and Y. If we log "B..Y", we only show // Y. With "Y --not B", we show X and Y. - $against = csprintf('%s --not %s', 'HEAD', $this->getRelativeCommit()); + $against = csprintf('%s --not %s', 'HEAD', $this->getBaseCommit()); } // NOTE: Windows escaping of "%" symbols apparently is inherently broken; @@ -124,139 +118,153 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return $commits; } - public function getRelativeCommit() { - if ($this->relativeCommit === null) { - - // Detect zero-commit or one-commit repositories. There is only one - // relative-commit value that makes any sense in these repositories: the - // empty tree. - list($err) = $this->execManualLocal('rev-parse --verify HEAD^'); - if ($err) { - list($err) = $this->execManualLocal('rev-parse --verify HEAD'); - if ($err) { - $this->repositoryHasNoCommits = true; - } - - $this->relativeCommit = self::GIT_MAGIC_ROOT_COMMIT; - - if ($this->repositoryHasNoCommits) { - $this->setBaseCommitExplanation( - "the repository has no commits."); - } else { - $this->setBaseCommitExplanation( - "the repository has only one commit."); - } - - return $this->relativeCommit; - } - - if ($this->getBaseCommitArgumentRules() || - $this->getWorkingCopyIdentity()->getConfigFromAnySource('base')) { - $base = $this->resolveBaseCommit(); - if (!$base) { - throw new ArcanistUsageException( - "None of the rules in your 'base' configuration matched a valid ". - "commit. Adjust rules or specify which commit you want to use ". - "explicitly."); - } - $this->relativeCommit = $base; - return $this->relativeCommit; - } - - $do_write = false; - $default_relative = null; - $working_copy = $this->getWorkingCopyIdentity(); - if ($working_copy) { - $default_relative = $working_copy->getConfig( - 'git.default-relative-commit'); + protected function buildBaseCommit($symbolic_commit) { + if ($symbolic_commit !== null) { + if ($symbolic_commit == ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { $this->setBaseCommitExplanation( - "it is the merge-base of '{$default_relative}' and HEAD, as ". - "specified in 'git.default-relative-commit' in '.arcconfig'. This ". - "setting overrides other settings."); + "you explicitly specified the empty tree."); + return $symbolic_commit; } - if (!$default_relative) { - list($err, $upstream) = $this->execManualLocal( - "rev-parse --abbrev-ref --symbolic-full-name '@{upstream}'"); - - if (!$err) { - $default_relative = trim($upstream); - $this->setBaseCommitExplanation( - "it is the merge-base of '{$default_relative}' (the Git upstream ". - "of the current branch) HEAD."); - } - } - - if (!$default_relative) { - $default_relative = $this->readScratchFile('default-relative-commit'); - $default_relative = trim($default_relative); - if ($default_relative) { - $this->setBaseCommitExplanation( - "it is the merge-base of '{$default_relative}' and HEAD, as ". - "specified in '.git/arc/default-relative-commit'."); - } - } - - if (!$default_relative) { - - // TODO: Remove the history lesson soon. - - echo phutil_console_format( - "** Select a Default Commit Range **\n\n"); - echo phutil_console_wrap( - "You're running a command which operates on a range of revisions ". - "(usually, from some revision to HEAD) but have not specified the ". - "revision that should determine the start of the range.\n\n". - "Previously, arc assumed you meant 'HEAD^' when you did not specify ". - "a start revision, but this behavior does not make much sense in ". - "most workflows outside of Facebook's historic git-svn workflow.\n\n". - "arc no longer assumes 'HEAD^'. You must specify a relative commit ". - "explicitly when you invoke a command (e.g., `arc diff HEAD^`, not ". - "just `arc diff`) or select a default for this working copy.\n\n". - "In most cases, the best default is 'origin/master'. You can also ". - "select 'HEAD^' to preserve the old behavior, or some other remote ". - "or branch. But you almost certainly want to select ". - "'origin/master'.\n\n". - "(Technically: the merge-base of the selected revision and HEAD is ". - "used to determine the start of the commit range.)"); - - $prompt = "What default do you want to use? [origin/master]"; - $default = phutil_console_prompt($prompt); - - if (!strlen(trim($default))) { - $default = 'origin/master'; - } - - $default_relative = $default; - $do_write = true; - } - - list($object_type) = $this->execxLocal( - 'cat-file -t %s', - $default_relative); - - if (trim($object_type) !== 'commit') { - throw new Exception( - "Relative commit '{$default_relative}' is not the name of a commit!"); - } - - if ($do_write) { - // Don't perform this write until we've verified that the object is a - // valid commit name. - $this->writeScratchFile('default-relative-commit', $default_relative); - $this->setBaseCommitExplanation( - "it is the merge-base of '{$default_relative}' and HEAD, as you ". - "just specified."); - } - - list($merge_base) = $this->execxLocal( + list($err, $merge_base) = $this->execManualLocal( 'merge-base %s HEAD', - $default_relative); + $symbolic_commit); + if ($err) { + throw new ArcanistUsageException( + "Unable to find any git commit named '{$symbolic_commit}' in ". + "this repository."); + } - $this->relativeCommit = trim($merge_base); + $this->setBaseCommitExplanation( + "it is the merge-base of '{$symbolic_commit}' and HEAD, as you ". + "explicitly specified."); + return trim($merge_base); } - return $this->relativeCommit; + // Detect zero-commit or one-commit repositories. There is only one + // relative-commit value that makes any sense in these repositories: the + // empty tree. + list($err) = $this->execManualLocal('rev-parse --verify HEAD^'); + if ($err) { + list($err) = $this->execManualLocal('rev-parse --verify HEAD'); + if ($err) { + $this->repositoryHasNoCommits = true; + } + + if ($this->repositoryHasNoCommits) { + $this->setBaseCommitExplanation( + "the repository has no commits."); + } else { + $this->setBaseCommitExplanation( + "the repository has only one commit."); + } + + return self::GIT_MAGIC_ROOT_COMMIT; + } + + if ($this->getBaseCommitArgumentRules() || + $this->getWorkingCopyIdentity()->getConfigFromAnySource('base')) { + $base = $this->resolveBaseCommit(); + if (!$base) { + throw new ArcanistUsageException( + "None of the rules in your 'base' configuration matched a valid ". + "commit. Adjust rules or specify which commit you want to use ". + "explicitly."); + } + return $base; + } + + $do_write = false; + $default_relative = null; + $working_copy = $this->getWorkingCopyIdentity(); + if ($working_copy) { + $default_relative = $working_copy->getConfig( + 'git.default-relative-commit'); + $this->setBaseCommitExplanation( + "it is the merge-base of '{$default_relative}' and HEAD, as ". + "specified in 'git.default-relative-commit' in '.arcconfig'. This ". + "setting overrides other settings."); + } + + if (!$default_relative) { + list($err, $upstream) = $this->execManualLocal( + "rev-parse --abbrev-ref --symbolic-full-name '@{upstream}'"); + + if (!$err) { + $default_relative = trim($upstream); + $this->setBaseCommitExplanation( + "it is the merge-base of '{$default_relative}' (the Git upstream ". + "of the current branch) HEAD."); + } + } + + if (!$default_relative) { + $default_relative = $this->readScratchFile('default-relative-commit'); + $default_relative = trim($default_relative); + if ($default_relative) { + $this->setBaseCommitExplanation( + "it is the merge-base of '{$default_relative}' and HEAD, as ". + "specified in '.git/arc/default-relative-commit'."); + } + } + + if (!$default_relative) { + + // TODO: Remove the history lesson soon. + + echo phutil_console_format( + "** Select a Default Commit Range **\n\n"); + echo phutil_console_wrap( + "You're running a command which operates on a range of revisions ". + "(usually, from some revision to HEAD) but have not specified the ". + "revision that should determine the start of the range.\n\n". + "Previously, arc assumed you meant 'HEAD^' when you did not specify ". + "a start revision, but this behavior does not make much sense in ". + "most workflows outside of Facebook's historic git-svn workflow.\n\n". + "arc no longer assumes 'HEAD^'. You must specify a relative commit ". + "explicitly when you invoke a command (e.g., `arc diff HEAD^`, not ". + "just `arc diff`) or select a default for this working copy.\n\n". + "In most cases, the best default is 'origin/master'. You can also ". + "select 'HEAD^' to preserve the old behavior, or some other remote ". + "or branch. But you almost certainly want to select ". + "'origin/master'.\n\n". + "(Technically: the merge-base of the selected revision and HEAD is ". + "used to determine the start of the commit range.)"); + + $prompt = "What default do you want to use? [origin/master]"; + $default = phutil_console_prompt($prompt); + + if (!strlen(trim($default))) { + $default = 'origin/master'; + } + + $default_relative = $default; + $do_write = true; + } + + list($object_type) = $this->execxLocal( + 'cat-file -t %s', + $default_relative); + + if (trim($object_type) !== 'commit') { + throw new Exception( + "Relative commit '{$default_relative}' is not the name of a commit!"); + } + + if ($do_write) { + // Don't perform this write until we've verified that the object is a + // valid commit name. + $this->writeScratchFile('default-relative-commit', $default_relative); + $this->setBaseCommitExplanation( + "it is the merge-base of '{$default_relative}' and HEAD, as you ". + "just specified."); + } + + list($merge_base) = $this->execxLocal( + 'merge-base %s HEAD', + $default_relative); + + return trim($merge_base); } private function getDiffFullOptions($detect_moves_and_renames = true) { @@ -294,7 +302,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $options = $this->getDiffFullOptions(); list($stdout) = $this->execxLocal( "diff {$options} %s --", - $this->getRelativeCommit()); + $this->getBaseCommit()); return $stdout; } @@ -308,7 +316,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $options = $this->getDiffFullOptions($detect_moves_and_renames); list($stdout) = $this->execxLocal( "diff {$options} %s -- %s", - $this->getRelativeCommit(), + $this->getBaseCommit(), $path); return $stdout; } @@ -336,7 +344,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { } public function getGitCommitLog() { - $relative = $this->getRelativeCommit(); + $relative = $this->getBaseCommit(); if ($this->repositoryHasNoCommits) { // No commits yet. return ''; @@ -348,7 +356,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // 2..N commits. list($stdout) = $this->execxLocal( 'log --first-parent --format=medium %s..HEAD', - $this->getRelativeCommit()); + $this->getBaseCommit()); } return $stdout; } @@ -357,14 +365,14 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { list($stdout) = $this->execxLocal( 'log --format=medium -n%d %s', self::SEARCH_LENGTH_FOR_PARENT_REVISIONS, - $this->getRelativeCommit()); + $this->getBaseCommit()); return $stdout; } public function getSourceControlBaseRevision() { list($stdout) = $this->execxLocal( 'rev-parse %s', - $this->getRelativeCommit()); + $this->getBaseCommit()); return rtrim($stdout, "\n"); } @@ -455,7 +463,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { list($stdout, $stderr) = $this->execxLocal( 'diff %C --raw %s --', $this->getDiffBaseOptions(), - $this->getRelativeCommit()); + $this->getBaseCommit()); return $this->parseGitStatus($stdout); } @@ -477,6 +485,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $this->execxLocal( 'add -- %Ls', $paths); + $this->reloadWorkingCopy(); + return $this; } public function doCommit($message) { @@ -497,6 +507,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { $this->execxLocal( 'commit --amend --allow-empty -F %s', $tmp_file); + $this->reloadWorkingCopy(); + return $this; } public function getPreReceiveHookStatus($old_ref, $new_ref) { @@ -563,7 +575,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // TODO: 'git blame' supports --porcelain and we should probably use it. list($stdout) = $this->execxLocal( 'blame --date=iso -w -M %s -- %s', - $this->getRelativeCommit(), + $this->getBaseCommit(), $path); $blame = array(); @@ -597,7 +609,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { } public function getOriginalFileData($path) { - return $this->getFileDataAtRevision($path, $this->getRelativeCommit()); + return $this->getFileDataAtRevision($path, $this->getBaseCommit()); } public function getCurrentFileData($path) { @@ -711,7 +723,11 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return true; } - public function supportsRelativeLocalCommits() { + public function supportsCommitRanges() { + return true; + } + + public function supportsLocalCommits() { return true; } @@ -726,33 +742,6 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return true; } - public function parseRelativeLocalCommit(array $argv) { - if (count($argv) == 0) { - return; - } - if (count($argv) != 1) { - throw new ArcanistUsageException("Specify only one commit."); - } - $base = reset($argv); - if ($base == ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { - $merge_base = $base; - $this->setBaseCommitExplanation( - "you explicitly specified the empty tree."); - } else { - list($err, $merge_base) = $this->execManualLocal( - 'merge-base %s HEAD', - $base); - if ($err) { - throw new ArcanistUsageException( - "Unable to find any git commit named '{$base}' in this repository."); - } - $this->setBaseCommitExplanation( - "it is the merge-base of '{$base}' and HEAD, as you explicitly ". - "specified."); - } - $this->setRelativeCommit(trim($merge_base)); - } - public function getAllLocalChanges() { $diff = $this->getFullGitDiff(); if (!strlen(trim($diff))) { @@ -859,6 +848,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { public function updateWorkingCopy() { $this->execxLocal('pull'); + $this->reloadWorkingCopy(); } public function getCommitSummary($commit) { diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index ecadd58d..e3ef70ca 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -7,10 +7,7 @@ */ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { - private $base; - private $relativeCommit; private $branch; - private $workingCopyRevision; private $localCommitInfo; private $includeDirectoryStateInDiffs; private $rawDiffCache = array(); @@ -58,7 +55,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } public function getSourceControlBaseRevision() { - return $this->getCanonicalRevisionName($this->getRelativeCommit()); + return $this->getCanonicalRevisionName($this->getBaseCommit()); } public function getCanonicalRevisionName($string) { @@ -81,107 +78,99 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $this->branch; } - public function setRelativeCommit($commit) { - try { - $commit = $this->getCanonicalRevisionName($commit); - } catch (Exception $ex) { - throw new ArcanistUsageException( - "Commit '{$commit}' is not a valid Mercurial commit identifier."); - } - - $this->relativeCommit = $commit; - $this->status = null; + public function didReloadCommitRange() { $this->localCommitInfo = null; - - return $this; } - public function getRelativeCommit() { - if (empty($this->relativeCommit)) { - - if ($this->getBaseCommitArgumentRules() || - $this->getWorkingCopyIdentity()->getConfigFromAnySource('base')) { - $base = $this->resolveBaseCommit(); - if (!$base) { - throw new ArcanistUsageException( - "None of the rules in your 'base' configuration matched a valid ". - "commit. Adjust rules or specify which commit you want to use ". - "explicitly."); - } - $this->relativeCommit = $base; - return $this->relativeCommit; + protected function buildBaseCommit($symbolic_commit) { + if ($symbolic_commit !== null) { + try { + $commit = $this->getCanonicalRevisionName($commit); + } catch (Exception $ex) { + throw new ArcanistUsageException( + "Commit '{$commit}' is not a valid Mercurial commit identifier."); } - - list($err, $stdout) = $this->execManualLocal( - 'outgoing --branch %s --style default', - $this->getBranchName()); - - if (!$err) { - $logs = ArcanistMercurialParser::parseMercurialLog($stdout); - } else { - // Mercurial (in some versions?) raises an error when there's nothing - // outgoing. - $logs = array(); - } - - if (!$logs) { - - $this->setBaseCommitExplanation( - "you have no outgoing commits, so arc assumes you intend to submit ". - "uncommitted changes in the working copy."); - // In Mercurial, we support operations against uncommitted changes. - $this->setRelativeCommit($this->getWorkingCopyRevision()); - return $this->relativeCommit; - } - - $outgoing_revs = ipull($logs, 'rev'); - - // This is essentially an implementation of a theoretical `hg merge-base` - // command. - $against = $this->getWorkingCopyRevision(); - while (true) { - // NOTE: The "^" and "~" syntaxes were only added in hg 1.9, which is - // new as of July 2011, so do this in a compatible way. Also, "hg log" - // and "hg outgoing" don't necessarily show parents (even if given an - // explicit template consisting of just the parents token) so we need - // to separately execute "hg parents". - - list($stdout) = $this->execxLocal( - 'parents --style default --rev %s', - $against); - $parents_logs = ArcanistMercurialParser::parseMercurialLog($stdout); - - list($p1, $p2) = array_merge($parents_logs, array(null, null)); - - if ($p1 && !in_array($p1['rev'], $outgoing_revs)) { - $against = $p1['rev']; - break; - } else if ($p2 && !in_array($p2['rev'], $outgoing_revs)) { - $against = $p2['rev']; - break; - } else if ($p1) { - $against = $p1['rev']; - } else { - // This is the case where you have a new repository and the entire - // thing is outgoing; Mercurial literally accepts "--rev null" as - // meaning "diff against the empty state". - $against = 'null'; - break; - } - } - - if ($against == 'null') { - $this->setBaseCommitExplanation( - "this is a new repository (all changes are outgoing)."); - } else { - $this->setBaseCommitExplanation( - "it is the first commit reachable from the working copy state ". - "which is not outgoing."); - } - - $this->setRelativeCommit($against); + $this->setBaseCommitExplanation("you specified it explicitly."); + return $commit; } - return $this->relativeCommit; + + if ($this->getBaseCommitArgumentRules() || + $this->getWorkingCopyIdentity()->getConfigFromAnySource('base')) { + $base = $this->resolveBaseCommit(); + if (!$base) { + throw new ArcanistUsageException( + "None of the rules in your 'base' configuration matched a valid ". + "commit. Adjust rules or specify which commit you want to use ". + "explicitly."); + } + return $base; + } + + list($err, $stdout) = $this->execManualLocal( + 'outgoing --branch %s --style default', + $this->getBranchName()); + + if (!$err) { + $logs = ArcanistMercurialParser::parseMercurialLog($stdout); + } else { + // Mercurial (in some versions?) raises an error when there's nothing + // outgoing. + $logs = array(); + } + + if (!$logs) { + $this->setBaseCommitExplanation( + "you have no outgoing commits, so arc assumes you intend to submit ". + "uncommitted changes in the working copy."); + return $this->getWorkingCopyRevision(); + } + + $outgoing_revs = ipull($logs, 'rev'); + + // This is essentially an implementation of a theoretical `hg merge-base` + // command. + $against = $this->getWorkingCopyRevision(); + while (true) { + // NOTE: The "^" and "~" syntaxes were only added in hg 1.9, which is + // new as of July 2011, so do this in a compatible way. Also, "hg log" + // and "hg outgoing" don't necessarily show parents (even if given an + // explicit template consisting of just the parents token) so we need + // to separately execute "hg parents". + + list($stdout) = $this->execxLocal( + 'parents --style default --rev %s', + $against); + $parents_logs = ArcanistMercurialParser::parseMercurialLog($stdout); + + list($p1, $p2) = array_merge($parents_logs, array(null, null)); + + if ($p1 && !in_array($p1['rev'], $outgoing_revs)) { + $against = $p1['rev']; + break; + } else if ($p2 && !in_array($p2['rev'], $outgoing_revs)) { + $against = $p2['rev']; + break; + } else if ($p1) { + $against = $p1['rev']; + } else { + // This is the case where you have a new repository and the entire + // thing is outgoing; Mercurial literally accepts "--rev null" as + // meaning "diff against the empty state". + $against = 'null'; + break; + } + } + + if ($against == 'null') { + $this->setBaseCommitExplanation( + "this is a new repository (all changes are outgoing)."); + } else { + $this->setBaseCommitExplanation( + "it is the first commit reachable from the working copy state ". + "which is not outgoing."); + } + + return $against; } public function getLocalCommitInformation() { @@ -190,7 +179,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { "log --template '%C' --rev %s --branch %s --", "{node}\1{rev}\1{author}\1{date|rfc822date}\1". "{branch}\1{tag}\1{parents}\1{desc}\2", - '(ancestors(.) - ancestors('.$this->getRelativeCommit().'))', + '(ancestors(.) - ancestors('.$this->getBaseCommit().'))', $this->getBranchName()); $logs = array_filter(explode("\2", $info)); @@ -271,7 +260,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { public function getBlame($path) { list($stdout) = $this->execxLocal( 'annotate -u -v -c --rev %s -- %s', - $this->getRelativeCommit(), + $this->getBaseCommit(), $path); $blame = array(); @@ -355,6 +344,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { // Diffs are against ".", so we need to drop the cache if we change the // working copy. $this->rawDiffCache = array(); + $this->branch = null; } private function getDiffOptions() { @@ -373,7 +363,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { // copy commit" (i.e., from 'x' to '.'). The latter excludes any dirty // changes in the working copy. - $range = $this->getRelativeCommit(); + $range = $this->getBaseCommit(); if (!$this->includeDirectoryStateInDiffs) { $range .= '...'; } @@ -399,7 +389,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } public function getOriginalFileData($path) { - return $this->getFileDataAtRevision($path, $this->getRelativeCommit()); + return $this->getFileDataAtRevision($path, $this->getBaseCommit()); } public function getCurrentFileData($path) { @@ -438,7 +428,11 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { } } - public function supportsRelativeLocalCommits() { + public function supportsCommitRanges() { + return true; + } + + public function supportsLocalCommits() { return true; } @@ -458,21 +452,6 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $message; } - public function parseRelativeLocalCommit(array $argv) { - if (count($argv) == 0) { - return; - } - if (count($argv) != 1) { - throw new ArcanistUsageException("Specify only one commit."); - } - - $this->setBaseCommitExplanation("you explicitly specified it."); - - // This does the "hg id" call we need to normalize/validate the revision - // identifier. - $this->setRelativeCommit(reset($argv)); - } - public function getAllLocalChanges() { $diff = $this->getFullMercurialDiff(); if (!strlen(trim($diff))) { @@ -513,7 +492,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { public function getCommitMessageLog() { list($stdout) = $this->execxLocal( "log --template '{node}\\2{desc}\\1' --rev %s --branch %s --", - 'ancestors(.) - ancestors('.$this->getRelativeCommit().')', + 'ancestors(.) - ancestors('.$this->getBaseCommit().')', $this->getBranchName()); $map = array(); @@ -593,6 +572,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { public function updateWorkingCopy() { $this->execxLocal('up'); + $this->reloadWorkingCopy(); } private function getMercurialConfig($key, $default = null) { @@ -611,6 +591,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $this->execxLocal( 'add -- %Ls', $paths); + $this->reloadWorkingCopy(); } public function doCommit($message) { @@ -628,6 +609,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $this->execxLocal( 'commit --amend -l %s', $tmp_file); + $this->reloadWorkingCopy(); } public function setIncludeDirectoryStateInDiffs($include) { diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index c7a7e405..89f037dc 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -34,6 +34,9 @@ abstract class ArcanistRepositoryAPI { private $uncommittedStatusCache; private $commitRangeStatusCache; + private $symbolicBaseCommit; + private $resolvedBaseCommit; + abstract public function getSourceControlSystemName(); public function getDiffLinesOfContext() { @@ -257,6 +260,7 @@ abstract class ArcanistRepositoryAPI { $this->commitRangeStatusCache = null; $this->didReloadWorkingCopy(); + $this->reloadCommitRange(); return $this; } @@ -310,7 +314,6 @@ abstract class ArcanistRepositoryAPI { abstract public function getSourceControlPath(); abstract public function isHistoryDefaultImmutable(); abstract public function supportsAmend(); - abstract public function supportsRelativeLocalCommits(); abstract public function getWorkingCopyRevision(); abstract public function updateWorkingCopy(); abstract public function getMetadataPath(); @@ -334,6 +337,8 @@ abstract class ArcanistRepositoryAPI { throw new ArcanistCapabilityNotSupportedException($this); } + abstract public function supportsLocalCommits(); + public function doCommit($message) { throw new ArcanistCapabilityNotSupportedException($this); } @@ -355,10 +360,6 @@ abstract class ArcanistRepositoryAPI { throw new ArcanistCapabilityNotSupportedException($this); } - public function parseRelativeLocalCommit(array $argv) { - throw new ArcanistCapabilityNotSupportedException($this); - } - public function getCommitSummary($commit) { throw new ArcanistCapabilityNotSupportedException($this); } @@ -539,6 +540,47 @@ abstract class ArcanistRepositoryAPI { /* -( Base Commits )------------------------------------------------------- */ + abstract public function supportsCommitRanges(); + + final public function setBaseCommit($symbolic_commit) { + if (!$this->supportsCommitRanges()) { + throw new ArcanistCapabilityNotSupportedException($this); + } + + $this->symbolicBaseCommit = $symbolic_commit; + $this->reloadCommitRange(); + return $this; + } + + final public function getBaseCommit() { + if (!$this->supportsCommitRanges()) { + throw new ArcanistCapabilityNotSupportedException($this); + } + + if ($this->resolvedBaseCommit === null) { + $commit = $this->buildBaseCommit($this->symbolicBaseCommit); + $this->resolvedBaseCommit = $commit; + } + + return $this->resolvedBaseCommit; + } + + final public function reloadCommitRange() { + $this->resolvedBaseCommit = null; + $this->baseCommitExplanation = null; + + $this->didReloadCommitRange(); + + return $this; + } + + protected function didReloadCommitRange() { + return; + } + + protected function buildBaseCommit($symbolic_commit) { + throw new ArcanistCapabilityNotSupportedException($this); + } public function getBaseCommitExplanation() { return $this->baseCommitExplanation; diff --git a/src/repository/api/ArcanistSubversionAPI.php b/src/repository/api/ArcanistSubversionAPI.php index 7fae4703..2d46e9f6 100644 --- a/src/repository/api/ArcanistSubversionAPI.php +++ b/src/repository/api/ArcanistSubversionAPI.php @@ -553,7 +553,11 @@ EODIFF; return false; } - public function supportsRelativeLocalCommits() { + public function supportsCommitRanges() { + return false; + } + + public function supportsLocalCommits() { return false; } diff --git a/src/workflow/ArcanistBaseWorkflow.php b/src/workflow/ArcanistBaseWorkflow.php index bd787cea..fa62c6e6 100644 --- a/src/workflow/ArcanistBaseWorkflow.php +++ b/src/workflow/ArcanistBaseWorkflow.php @@ -828,7 +828,7 @@ abstract class ArcanistBaseWorkflow { if ($this->shouldAmend) { $commit = head($api->getLocalCommitInformation()); $api->amendCommit($commit['message']); - } else if ($api->supportsRelativeLocalCommits()) { + } else if ($api->supportsLocalCommits()) { $api->doCommit(self::AUTO_COMMIT_TITLE); } } @@ -982,7 +982,12 @@ abstract class ArcanistBaseWorkflow { protected function getChange($path) { $repository_api = $this->getRepositoryAPI(); - if ($repository_api instanceof ArcanistSubversionAPI) { + // TODO: Very gross + $is_git = ($repository_api instanceof ArcanistGitAPI); + $is_hg = ($repository_api instanceof ArcanistMercurialAPI); + $is_svn = ($repository_api instanceof ArcanistSubversionAPI); + + if ($is_svn) { // NOTE: In SVN, we don't currently support a "get all local changes" // operation, so special case it. if (empty($this->changeCache[$path])) { @@ -994,7 +999,7 @@ abstract class ArcanistBaseWorkflow { } $this->changeCache[$path] = reset($changes); } - } else if ($repository_api->supportsRelativeLocalCommits()) { + } else if ($is_git || $is_hg) { if (empty($this->changeCache)) { $changes = $repository_api->getAllLocalChanges(); foreach ($changes as $change) { @@ -1006,7 +1011,7 @@ abstract class ArcanistBaseWorkflow { } if (empty($this->changeCache[$path])) { - if ($repository_api instanceof ArcanistGitAPI) { + if ($is_git) { // This can legitimately occur under git if you make a change, "git // commit" it, and then revert the change in the working copy and run // "arc lint". @@ -1257,9 +1262,7 @@ abstract class ArcanistBaseWorkflow { } } else { $repository_api = $this->getRepositoryAPI(); - if ($rev) { - $repository_api->parseRelativeLocalCommit(array($rev)); - } + $this->parseBaseCommitArgument(array($rev)); $paths = $repository_api->getWorkingCopyStatus(); foreach ($paths as $path => $flags) { @@ -1492,4 +1495,26 @@ abstract class ArcanistBaseWorkflow { return $event; } + public function parseBaseCommitArgument(array $argv) { + if (!count($argv)) { + return; + } + + $api = $this->getRepositoryAPI(); + if (!$api->supportsCommitRanges()) { + throw new ArcanistUsageException( + "This version control system does not support commit ranges."); + } + + if (count($argv) > 1) { + throw new ArcanistUsageException( + "Specify exactly one base commit. The end of the commit range is ". + "always the working copy state."); + } + + $api->setBaseCommit(head($argv)); + + return $this; + } + } diff --git a/src/workflow/ArcanistCoverWorkflow.php b/src/workflow/ArcanistCoverWorkflow.php index 3ed4170e..0fa642c6 100644 --- a/src/workflow/ArcanistCoverWorkflow.php +++ b/src/workflow/ArcanistCoverWorkflow.php @@ -70,9 +70,7 @@ EOTEXT $in_rev = $this->getArgument('rev'); if ($in_rev) { - // Although selectPathsForWorkflow() may set this, we want to set it - // explicitly so we blame against the correct relative commit. - $repository_api->parseRelativeLocalCommit(array($in_rev)); + $this->parseBaseCommitArgument(array($in_rev)); } $paths = $this->selectPathsForWorkflow( diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index b184a99c..62e89b1c 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -605,16 +605,8 @@ EOTEXT $repository_api->setBaseCommitArgumentRules( $this->getArgument('base', '')); - if ($repository_api->supportsRelativeLocalCommits()) { - - // Parse the relative commit as soon as we can, to avoid generating - // caches we need to drop later and expensive discovery operations - // (particularly in Mercurial). - - $relative = $this->getArgument('paths'); - if ($relative) { - $repository_api->parseRelativeLocalCommit($relative); - } + if ($repository_api->supportsCommitRanges()) { + $this->parseBaseCommitArgument($this->getArgument('paths')); } } @@ -821,10 +813,8 @@ EOTEXT } } - } else if ($repository_api->supportsRelativeLocalCommits()) { - $paths = $repository_api->getWorkingCopyStatus(); } else { - throw new Exception("Unknown VCS!"); + $paths = $repository_api->getWorkingCopyStatus(); } foreach ($paths as $path => $mask) { @@ -1275,9 +1265,9 @@ EOTEXT $this->console->writeOut("Linting...\n"); try { $argv = $this->getPassthruArgumentsAsArgv('lint'); - if ($repository_api->supportsRelativeLocalCommits()) { + if ($repository_api->supportsCommitRanges()) { $argv[] = '--rev'; - $argv[] = $repository_api->getRelativeCommit(); + $argv[] = $repository_api->getBaseCommit(); } $lint_workflow = $this->buildChildWorkflow('lint', $argv); @@ -1356,9 +1346,9 @@ EOTEXT $this->console->writeOut("Running unit tests...\n"); try { $argv = $this->getPassthruArgumentsAsArgv('unit'); - if ($repository_api->supportsRelativeLocalCommits()) { + if ($repository_api->supportsCommitRanges()) { $argv[] = '--rev'; - $argv[] = $repository_api->getRelativeCommit(); + $argv[] = $repository_api->getBaseCommit(); } $unit_workflow = $this->buildChildWorkflow('unit', $argv); $unit_result = $unit_workflow->run(); diff --git a/src/workflow/ArcanistExportWorkflow.php b/src/workflow/ArcanistExportWorkflow.php index b08d3309..fcf8ce09 100644 --- a/src/workflow/ArcanistExportWorkflow.php +++ b/src/workflow/ArcanistExportWorkflow.php @@ -177,8 +177,7 @@ EOTEXT $parser->setRepositoryAPI($repository_api); if ($repository_api instanceof ArcanistGitAPI) { - $repository_api->parseRelativeLocalCommit( - $this->getArgument('paths')); + $this->parseBaseCommitArgument($this->getArgument('paths')); $diff = $repository_api->getFullGitDiff(); $changes = $parser->parseDiff($diff); $authors = $this->getConduit()->callMethodSynchronous( @@ -191,8 +190,7 @@ EOTEXT $author_dict['realName'], $repository_api->execxLocal('config user.email')); } else if ($repository_api instanceof ArcanistMercurialAPI) { - $repository_api->parseRelativeLocalCommit( - $this->getArgument('paths')); + $repository_api->parseBaseCommitArgument($this->getArgument('paths')); $diff = $repository_api->getFullMercurialDiff(); $changes = $parser->parseDiff($diff); $authors = $this->getConduit()->callMethodSynchronous( diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 23fd01b7..55c282a6 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -301,7 +301,7 @@ EOTEXT private function findRevision() { $repository_api = $this->getRepositoryAPI(); - $repository_api->parseRelativeLocalCommit(array($this->ontoRemoteBranch)); + $this->parseBaseCommitArgument(array($this->ontoRemoteBranch)); $revision_id = $this->getArgument('revision'); if ($revision_id) { @@ -484,9 +484,7 @@ EOTEXT } } - // Now that we've rebased, the merge-base of origin/master and HEAD may - // be different. Reparse the relative commit. - $repository_api->parseRelativeLocalCommit(array($this->ontoRemoteBranch)); + $repository_api->reloadWorkingCopy(); } private function squash() { diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php index e83a1fc3..78b92ddb 100644 --- a/src/workflow/ArcanistLintWorkflow.php +++ b/src/workflow/ArcanistLintWorkflow.php @@ -240,8 +240,9 @@ EOTEXT if ($this->getArgument('only-new')) { $conduit = $this->getConduit(); $api = $this->getRepositoryAPI(); - $relative_commit = ($rev ? $rev : $api->resolveBaseCommit()); - $api->setRelativeCommit($relative_commit); + if ($rev) { + $api->setBaseCommit($rev); + } $svn_root = id(new PhutilURI($api->getSourceControlPath()))->getPath(); $all_paths = array(); @@ -267,7 +268,7 @@ EOTEXT $lint_future = $conduit->callMethod('diffusion.getlintmessages', array( 'arcanistProject' => $this->getWorkingCopy()->getProjectID(), 'branch' => '', // TODO: Tracking branch. - 'commit' => $api->getRelativeCommit(), + 'commit' => $api->getBaseCommit(), 'files' => array_keys($all_paths), )); } diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php index d6b45646..2d9c44fd 100644 --- a/src/workflow/ArcanistPatchWorkflow.php +++ b/src/workflow/ArcanistPatchWorkflow.php @@ -885,7 +885,7 @@ EOTEXT // Check to see if the bundle's base revision matches the working copy // base revision - if ($repository_api->supportsRelativeLocalCommits()) { + if ($repository_api->supportsLocalCommits()) { $bundle_base_rev = $bundle->getBaseRevision(); if (empty($bundle_base_rev)) { // this means $source is SOURCE_PATCH || SOURCE_BUNDLE w/ $version < 2 diff --git a/src/workflow/ArcanistWhichWorkflow.php b/src/workflow/ArcanistWhichWorkflow.php index 7717087f..18030ad4 100644 --- a/src/workflow/ArcanistWhichWorkflow.php +++ b/src/workflow/ArcanistWhichWorkflow.php @@ -73,20 +73,15 @@ EOTEXT $arg_commit = $this->getArgument('commit'); if (count($arg_commit)) { - if (!$repository_api->supportsRelativeLocalCommits()) { - throw new ArcanistUsageException( - "This version control system does not support relative commits."); - } else { - $repository_api->parseRelativeLocalCommit($arg_commit); - } + $this->parseBaseCommitArgument($arg_commit); } $arg = $arg_commit ? ' '.head($arg_commit) : ''; $repository_api->setBaseCommitArgumentRules( $this->getArgument('base', '')); - if ($repository_api->supportsRelativeLocalCommits()) { - $relative = $repository_api->getRelativeCommit(); + if ($repository_api->supportsCommitRanges()) { + $relative = $repository_api->getBaseCommit(); if ($this->getArgument('show-base')) { echo $relative."\n";