From 3116d3656ad017783e75df7e863368053d753e7f Mon Sep 17 00:00:00 2001 From: Howard Chan Date: Tue, 14 May 2013 11:00:56 -0700 Subject: [PATCH] Adding arc revert command[] Summary: Arc Revert does the following: 1. Git revert 2. Go to the differential of the rev you are reverting and either repoen it or set it to a reverted state 3. File a hipri task to orig author [Preview] Creating Arc Revert workflow Porting arc revert from FB4A to phabricator for general usage. This is my first stab, so totally appreciate feedback and assistance. I'm currently focused on making this work for git. However, I built out the functions through the GitAPI so this could be easily extendable to Mercurial later on. Stuck on the following (help): 1. Creating a task for FB internal. I tried building on top of existing arc listeners but getting errors on failures to load the TaskCreator (and other) tasks. 2. I'm using a hacky way to grab the diff revision id from the newly created revert diff. (see line 204) I'm looking for a way to just fetch the diff ID from arc after the diff is created; is this possible? Test Plan: - 1. Ran arc revert on a www and fbcode diff 2. Confirmed that revert was run on the diffs and a proper diff filed - Reviewers: royw, sdwilsh, nh, epriestley Reviewed By: epriestley CC: aran, epriestley, Korvin, pti, keir Maniphest Tasks: T1751 Differential Revision: https://secure.phabricator.com/D5553 --- src/__phutil_library_map__.php | 4 + src/repository/api/ArcanistGitAPI.php | 50 +++++- src/repository/api/ArcanistMercurialAPI.php | 45 ++++- src/workflow/ArcanistBackoutWorkflow.php | 184 ++++++++++++++++++++ src/workflow/ArcanistRevertWorkflow.php | 38 ++++ 5 files changed, 315 insertions(+), 6 deletions(-) create mode 100644 src/workflow/ArcanistBackoutWorkflow.php create mode 100644 src/workflow/ArcanistRevertWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 15337967..a4963e2b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -15,6 +15,7 @@ phutil_register_library_map(array( 'ArcanistAnoidWorkflow' => 'workflow/ArcanistAnoidWorkflow.php', 'ArcanistApacheLicenseLinter' => 'lint/linter/ArcanistApacheLicenseLinter.php', 'ArcanistArcanistLinterTestCase' => 'lint/linter/__tests__/ArcanistArcanistLinterTestCase.php', + 'ArcanistBackoutWorkflow' => 'workflow/ArcanistBackoutWorkflow.php', 'ArcanistBaseCommitParser' => 'parser/ArcanistBaseCommitParser.php', 'ArcanistBaseCommitParserTestCase' => 'parser/__tests__/ArcanistBaseCommitParserTestCase.php', 'ArcanistBaseTestResultParser' => 'unit/engine/ArcanistBaseTestResultParser.php', @@ -119,6 +120,7 @@ phutil_register_library_map(array( 'ArcanistRepositoryAPI' => 'repository/api/ArcanistRepositoryAPI.php', 'ArcanistRepositoryAPIMiscTestCase' => 'repository/api/__tests__/ArcanistRepositoryAPIMiscTestCase.php', 'ArcanistRepositoryAPIStateTestCase' => 'repository/api/__tests__/ArcanistRepositoryAPIStateTestCase.php', + 'ArcanistRevertWorkflow' => 'workflow/ArcanistRevertWorkflow.php', 'ArcanistRubyLinter' => 'lint/linter/ArcanistRubyLinter.php', 'ArcanistRubyLinterTestCase' => 'lint/linter/__tests__/ArcanistRubyLinterTestCase.php', 'ArcanistScalaSBTLinter' => 'lint/linter/ArcanistScalaSBTLinter.php', @@ -178,6 +180,7 @@ phutil_register_library_map(array( 'ArcanistAnoidWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistApacheLicenseLinter' => 'ArcanistLicenseLinter', 'ArcanistArcanistLinterTestCase' => 'ArcanistLinterTestCase', + 'ArcanistBackoutWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistBaseCommitParserTestCase' => 'ArcanistTestCase', 'ArcanistBaseWorkflow' => 'Phobject', 'ArcanistBaseXHPASTLinter' => 'ArcanistFutureLinter', @@ -255,6 +258,7 @@ phutil_register_library_map(array( 'ArcanistPyLintLinter' => 'ArcanistLinter', 'ArcanistRepositoryAPIMiscTestCase' => 'ArcanistTestCase', 'ArcanistRepositoryAPIStateTestCase' => 'ArcanistTestCase', + 'ArcanistRevertWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistRubyLinter' => 'ArcanistLinter', 'ArcanistRubyLinterTestCase' => 'ArcanistArcanistLinterTestCase', 'ArcanistScalaSBTLinter' => 'ArcanistLinter', diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 4367a035..3ddb6470 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -392,9 +392,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { public function getCanonicalRevisionName($string) { $match = null; if (preg_match('/@([0-9]+)$/', $string, $match)) { - list($stdout) = $this->execxLocal( - 'svn find-rev r%d', - $match[1]); + $stdout = $this->getHashFromFromSVNRevisionNumber($match[1]); } else { list($stdout) = $this->execxLocal( 'show -s --format=%C %s', @@ -404,6 +402,34 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return rtrim($stdout); } + private function executeSVNFindRev($input, $vcs) { + $match = array(); + list($stdout) = $this->execxLocal( + 'svn find-rev %s', + $input); + if (!$stdout) { + throw new ArcanistUsageException("Cannot find the {$vcs} equivalent " + ."of {$input}."); + } + // When git performs a partial-rebuild during svn + // look-up, we need to parse the final line + $lines = explode("\n", $stdout); + $stdout = $lines[count($lines) - 2]; + return rtrim($stdout); + } + + // Convert svn revision number to git hash + public function getHashFromFromSVNRevisionNumber($revision_id) { + return $this->executeSVNFindRev("r".$revision_id, "Git"); + } + + + // Convert a git hash to svn revision number + public function getSVNRevisionNumberFromHash($hash) { + return $this->executeSVNFindRev($hash, "SVN"); + } + + protected function buildUncommittedStatus() { $diff_options = $this->getDiffBaseOptions(); @@ -882,6 +908,24 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return trim($summary); } + public function backoutCommit($commit_hash) { + $this->execxLocal( + 'revert %s -n --no-edit', $commit_hash); + $this->reloadWorkingCopy(); + if (!$this->getUncommittedStatus()) { + throw new ArcanistUsageException( + "{$commit_hash} has already been reverted."); + } + } + + public function getBackoutMessage($commit_hash) { + return "This reverts commit ".$commit_hash."."; + } + + public function isGitSubversionRepo() { + return Filesystem::pathExists($this->getPath('.git/svn')); + } + public function resolveBaseCommitRule($rule, $source) { list($type, $name) = explode(':', $rule, 2); diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index f555b413..1c80dc59 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -74,6 +74,32 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $stdout; } + public function getHashFromFromSVNRevisionNumber($revision_id) { + $matches = array(); + $string = hgsprintf('svnrev(%s)', $revision_id); + list($stdout) = $this->execxLocal( + 'log -l 1 --template %s -r %s --', + '{node}', + $string); + if (!$stdout) { + throw new ArcanistUsageException("Cannot find the HG equivalent " + ."of {$revision_id} given."); + } + return $stdout; + } + + + public function getSVNRevisionNumberFromHash($hash) { + $matches = array(); + list($stdout) = $this->execxLocal( + 'log -r %s --template {svnrev}', $hash); + if (!$stdout) { + throw new ArcanistUsageException("Cannot find the SVN equivalent " + ."of {$hash} given."); + } + return $stdout; + } + public function getSourceControlPath() { return '/'; } @@ -102,8 +128,8 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { hgsprintf('ancestor(%R,.)', $symbolic_commit)); } catch (Exception $ex) { throw new ArcanistUsageException( - "Commit '{$symbolic_commit}' is not a valid Mercurial commit ". - "identifier."); + "Commit '{$symbolic_commit}' is not a valid Mercurial commit ". + "identifier."); } } @@ -742,6 +768,20 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return trim($summary); } + public function backoutCommit($commit_hash) { + $this->execxLocal( + 'backout -r %s', $commit_hash); + $this->reloadWorkingCopy(); + if (!$this->getUncommittedStatus()) { + throw new ArcanistUsageException( + "{$commit_hash} has already been reverted."); + } + } + + public function getBackoutMessage($commit_hash) { + return "Backed out changeset ".$commit_hash."."; + } + public function resolveBaseCommitRule($rule, $source) { list($type, $name) = explode(':', $rule, 2); @@ -774,7 +814,6 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { 'log --template {node} --rev %s', $name); } - if (!$err) { $this->setBaseCommitExplanation( "it is specified by '{$rule}' in your {$source} 'base' ". diff --git a/src/workflow/ArcanistBackoutWorkflow.php b/src/workflow/ArcanistBackoutWorkflow.php new file mode 100644 index 00000000..ac7c73d2 --- /dev/null +++ b/src/workflow/ArcanistBackoutWorkflow.php @@ -0,0 +1,184 @@ + | + Entering a differential revision will only work if there is only one commit + associated with the revision. This requires your working copy is up to date + and that the commit exists in the working copy. +EOTEXT + ); + } + + public function getArguments() { + return array( + '*' => 'input', + ); + } + + public function requiresWorkingCopy() { + return true; + } + + public function requiresRepositoryAPI() { + return true; + } + + public function requiresAuthentication() { + return true; + } + + // Given a differential revision ID, fetches the commit ID + private function getCommitIDFromRevisionID($revision_id) { + $conduit = $this->getConduit(); + $revisions = $conduit->callMethodSynchronous( + 'differential.query', + array( + 'ids' => array($revision_id), + )); + if (!$revisions) { + throw new ArcanistUsageException( + 'The revision you provided does not exist!'); + } + $revision = $revisions[0]; + $commits = $revision["commits"]; + if (!$commits) { + throw new ArcanistUsageException( + 'This revision has not been committed yet!'); + } + elseif (count($commits) > 1) { + throw new ArcanistUsageException( + 'The revision you provided has multiple commits!'); + } + $commit_phid = $commits[0]; + $commit = $conduit->callMethodSynchronous( + 'phid.query', + array( + 'phids' => array($commit_phid), + )); + $commit_id = $commit[$commit_phid]["name"]; + return $commit_id; + } + + // Fetches an array of commit info provided a Commit_id + // in the form of rE123456 (not local commit hash) + private function getDiffusionCommit($commit_id) { + $result = $this->getConduit()->callMethodSynchronous( + 'diffusion.getcommits', + array( + 'commits' => array($commit_id), + )); + $commit = $result[$commit_id]; + // This commit was not found in Diffusion + if (array_key_exists("error", $commit)) { + return null; + } + return $commit; + } + + // Retrieves default template from differential and prefills info + private function buildCommitMessage($commit_hash) { + $conduit = $this->getConduit(); + $repository_api = $this->getRepositoryAPI(); + + $summary = $repository_api->getBackoutMessage($commit_hash); + $fields = array('summary' => $summary, + 'testPlan' => 'revert-hammer', + ); + $template = $conduit->callMethodSynchronous( + 'differential.getcommitmessage', + array( + 'revision_id' => null, + 'edit' => 'create', + 'fields' => $fields + )); + $template = $this->newInteractiveEditor($template) + ->setName('new-commit') + ->editInteractively(); + return $template; + } + + // Performs the backout/revert of a revision and creates a commit + public function run() { + $console = PhutilConsole::getConsole(); + $conduit = $this->getConduit(); + $repository_api = $this->getRepositoryAPI(); + $repository = $this->loadProjectRepository(); + $callsign = $repository["callsign"]; + $is_git_svn = $repository_api instanceof ArcanistGitAPI && + $repository_api->isGitSubversionRepo(); + $is_hg_svn = $repository_api instanceof ArcanistMercurialAPI && + $repository_api->isHgSubversionRepo(); + $revision_id = null; + + if (!($repository_api instanceof ArcanistGitAPI) && + !($repository_api instanceof ArcanistMercurialAPI)) { + throw new ArcanistUsageException( + 'Backout currently only supports Git and Mercurial' + ); + } + + $console->writeOut("Starting backout\n"); + $input = $this->getArgument('input'); + if (!$input || count($input) != 1) { + throw new ArcanistUsageException( + 'You must specify one commit to backout!'); + } + + // Input looks like a Differential revision, so + // we try to find the commit attached to it + $matches = array(); + if (preg_match('/^D(\d+)$/i', $input[0], $matches)) { + $revision_id = $matches[1]; + $commit_id = $this->getCommitIDFromRevisionID($revision_id); + $commit = $this->getDiffusionCommit($commit_id); + $commit_hash = $commit['commitIdentifier']; + // Convert commit hash from SVN to Git/HG (for FB case) + if ($is_git_svn || $is_hg_svn) { + $commit_hash = $repository_api-> + getHashFromFromSVNRevisionNumber($commit_hash); + } + } else { + // Assume input is a commit hash + $commit_hash = $input[0]; + } + if (!$repository_api->hasLocalCommit($commit_hash)) { + throw new ArcanistUsageException('Invalid commit provided or does not'. + 'exist in the working copy!'); + } + + // Run 'backout'. + $subject = $repository_api->getCommitSummary($commit_hash); + $console->writeOut("Backing out commit {$commit_hash} {$subject} \n"); + + $repository_api->backoutCommit($commit_hash); + + // Create commit message and execute the commit + $message = $this->buildCommitMessage($commit_hash); + $repository_api->doCommit($message); + $console->writeOut("Double-check the commit and push when ready\n"); + + } +} + diff --git a/src/workflow/ArcanistRevertWorkflow.php b/src/workflow/ArcanistRevertWorkflow.php new file mode 100644 index 00000000..ee7bc743 --- /dev/null +++ b/src/workflow/ArcanistRevertWorkflow.php @@ -0,0 +1,38 @@ + 'input', + ); + } + + public function run() { + $console = PhutilConsole::getConsole(); + $console->writeOut("Please use arc backout instead.\n"); + } +} +