From 268de6428c4a2483a3590090064276dd556dd5a6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Aug 2011 09:00:29 -0700 Subject: [PATCH] Basic Mercurial support for Arcanist Summary: There's a lot of ground left to cover but this makes "arc diff" work (on one trivial diff) in my sandbox, at least, and supports parsing of Mercurial native diffs (which are unified + a custom header). Piles of missing features, still. Some of this is blocked by me not understanding the mercurial model well yet. This is also a really good opportunity for cleanup (especially, reducing the level of "instanceof" in the diff workflow), I'll try to do a bunch of that in followup diffs. Test Plan: Ran "arc diff" in a mercurial repository, got a diff out of it. Reviewed By: aran Reviewers: Makinde, jungejason, tuomaspelkonen, aran, codeblock CC: aran, epriestley, codeblock, fratrik Differential Revision: 792 --- src/__phutil_library_map__.php | 2 + src/parser/diff/ArcanistDiffParser.php | 58 +++++-- .../api/base/ArcanistRepositoryAPI.php | 24 ++- src/repository/api/base/__init__.php | 2 + src/repository/api/git/ArcanistGitAPI.php | 1 - .../api/mercurial/ArcanistMercurialAPI.php | 142 ++++++++++++++++++ src/repository/api/mercurial/__init__.php | 14 ++ src/workflow/diff/ArcanistDiffWorkflow.php | 27 +++- 8 files changed, 249 insertions(+), 21 deletions(-) create mode 100644 src/repository/api/mercurial/ArcanistMercurialAPI.php create mode 100644 src/repository/api/mercurial/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2d7d4f02..b5e28bac 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -55,6 +55,7 @@ phutil_register_library_map(array( 'ArcanistLinterTestCase' => 'lint/linter/base/test', 'ArcanistListWorkflow' => 'workflow/list', 'ArcanistMarkCommittedWorkflow' => 'workflow/mark-committed', + 'ArcanistMercurialAPI' => 'repository/api/mercurial', 'ArcanistNoEffectException' => 'exception/usage/noeffect', 'ArcanistNoEngineException' => 'exception/usage/noengine', 'ArcanistNoLintLinter' => 'lint/linter/nolint', @@ -118,6 +119,7 @@ phutil_register_library_map(array( 'ArcanistLinterTestCase' => 'ArcanistPhutilTestCase', 'ArcanistListWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistMarkCommittedWorkflow' => 'ArcanistBaseWorkflow', + 'ArcanistMercurialAPI' => 'ArcanistRepositoryAPI', 'ArcanistNoEffectException' => 'ArcanistUsageException', 'ArcanistNoEngineException' => 'ArcanistUsageException', 'ArcanistNoLintLinter' => 'ArcanistLinter', diff --git a/src/parser/diff/ArcanistDiffParser.php b/src/parser/diff/ArcanistDiffParser.php index cd57f107..f7fece8f 100644 --- a/src/parser/diff/ArcanistDiffParser.php +++ b/src/parser/diff/ArcanistDiffParser.php @@ -27,6 +27,7 @@ class ArcanistDiffParser { protected $text; protected $line; protected $isGit; + protected $isMercurial; protected $detectBinaryFiles = false; protected $changes = array(); @@ -209,6 +210,9 @@ class ArcanistDiffParser { '(?PBinary) files '. '(?P.+)\s+\d{4}-\d{2}-\d{2} and '. '(?P.+)\s+\d{4}-\d{2}-\d{2} differ.*', + + // This is a normal Mercurial text change, probably from "hg diff". + '(?Pdiff -r) (?P[a-f0-9]+) (?P.+)', ); $ok = false; @@ -274,6 +278,10 @@ class ArcanistDiffParser { $line = $this->nextLine(); $this->parseChangeset($change); break; + case 'diff -r': + $this->setIsMercurial(true); + $this->parseIndexHunk($change); + break; default: $this->didFailParse("Unknown diff type."); } @@ -432,8 +440,19 @@ class ArcanistDiffParser { return $this->isGit; } + public function setIsMercurial($is_mercurial) { + $this->isMercurial = $is_mercurial; + return $this; + } + + public function getIsMercurial() { + return $this->isMercurial; + } + protected function parseIndexHunk(ArcanistDiffChange $change) { $is_git = $this->getIsGit(); + $is_mercurial = $this->getIsMercurial(); + $is_svn = (!$is_git && !$is_mercurial); $line = $this->getLine(); if ($is_git) { @@ -532,19 +551,27 @@ class ArcanistDiffParser { } $line = $this->getLine(); - $ok = preg_match('/^=+$/', $line) || - ($is_git && preg_match('/^index .*$/', $line)); - if (!$ok) { - if ($is_git) { - $this->didFailParse( - "Expected 'index af23f...a98bc' header line."); + + if ($is_svn) { + $ok = preg_match('/^=+$/', $line); + if (!$ok) { + $this->didFailParse("Expected '=======================' divider line."); } else { - $this->didFailParse( - "Expected '==========================' divider line."); + // Adding an empty file in SVN can produce an empty line here. + $line = $this->nextNonemptyLine(); + } + } else if ($is_git) { + $ok = preg_match('/^index .*$/', $line); + if (!$ok) { + // TODO: "hg diff -g" diffs ("mercurial git-style diffs") do not include + // this line, so we can't parse them if we fail on it. Maybe introduce + // a flag saying "parse this diff using relaxed git-style diff rules"? + + // $this->didFailParse("Expected 'index af23f...a98bc' header line."); + } else { + $line = $this->nextLine(); } } - // Adding an empty file in SVN can produce an empty line here. - $line = $this->nextNonemptyLine(); // If there are files with only whitespace changes and -b or -w are // supplied as command-line flags to `diff', svn and git both produce @@ -596,14 +623,23 @@ class ArcanistDiffParser { protected function parseHunkTarget() { $line = $this->getLine(); $matches = null; + + $remainder = '(?:\s*\(.*\))?'; + if ($this->getIsMercurial()) { + // Something like "Fri Aug 26 01:20:50 2005 -0700", don't bother trying + // to parse it. + $remainder = '\t.*'; + } + $ok = preg_match( - '@^[-+]{3} (?:[ab]/)?(?P.*?)(?:\s*\(.*\))?$@', + '@^[-+]{3} (?:[ab]/)?(?P.*?)'.$remainder.'$@', $line, $matches); if (!$ok) { $this->didFailParse( "Expected hunk target '+++ path/to/file.ext (revision N)'."); } + $this->nextLine(); return $matches['path']; } diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index 30df621a..1acdd39f 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/src/repository/api/base/ArcanistRepositoryAPI.php @@ -65,11 +65,21 @@ abstract class ArcanistRepositoryAPI { "any parent directory. Create an '.arcconfig' file to configure arc."); } - if (@file_exists($root.'/.svn')) { - phutil_require_module('arcanist', 'repository/api/subversion'); - return new ArcanistSubversionAPI($root); + if (Filesystem::pathExists($root.'/.svn')) { + return newv('ArcanistSubversionAPI', array($root)); } + if (Filesystem::pathExists($root.'/.hg')) { + // TODO: Stabilize and remove. + file_put_contents( + 'php://stderr', + phutil_console_format( + "**WARNING:** Mercurial support is largely imaginary right now.\n")); + + return newv('ArcanistMercurialAPI', array($root)); + } + + $git_root = self::discoverGitBaseDirectory($root); if ($git_root) { if (!Filesystem::pathsAreEquivalent($root, $git_root)) { @@ -77,16 +87,16 @@ abstract class ArcanistRepositoryAPI { "'.arcconfig' file is located at '{$root}', but working copy root ". "is '{$git_root}'. Move '.arcconfig' file to the working copy root."); } - phutil_require_module('arcanist', 'repository/api/git'); - return new ArcanistGitAPI($root); + + return newv('ArcanistGitAPI', array($root)); } throw new ArcanistUsageException( "The current working directory is not part of a working copy for a ". - "supported version control system (svn or git)."); + "supported version control system (svn, git or mercurial)."); } - protected function __construct($path) { + public function __construct($path) { $this->path = $path; } diff --git a/src/repository/api/base/__init__.php b/src/repository/api/base/__init__.php index ad7f82cd..f2fcc047 100644 --- a/src/repository/api/base/__init__.php +++ b/src/repository/api/base/__init__.php @@ -8,8 +8,10 @@ phutil_require_module('arcanist', 'exception/usage'); +phutil_require_module('phutil', 'console'); phutil_require_module('phutil', 'filesystem'); phutil_require_module('phutil', 'future/exec'); +phutil_require_module('phutil', 'utils'); phutil_require_source('ArcanistRepositoryAPI.php'); diff --git a/src/repository/api/git/ArcanistGitAPI.php b/src/repository/api/git/ArcanistGitAPI.php index c6234cb1..520aad78 100644 --- a/src/repository/api/git/ArcanistGitAPI.php +++ b/src/repository/api/git/ArcanistGitAPI.php @@ -97,7 +97,6 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI { } public function getRawDiffText($path) { - $relative_commit = $this->getRelativeCommit(); $options = $this->getDiffFullOptions(); list($stdout) = execx( "(cd %s; git diff {$options} %s -- %s)", diff --git a/src/repository/api/mercurial/ArcanistMercurialAPI.php b/src/repository/api/mercurial/ArcanistMercurialAPI.php new file mode 100644 index 00000000..16bcdffa --- /dev/null +++ b/src/repository/api/mercurial/ArcanistMercurialAPI.php @@ -0,0 +1,142 @@ +getPath(), + $this->getRelativeCommit()); + return $stdout; + } + + public function getSourceControlPath() { + return '/'; + } + + public function getBranchName() { + // TODO: I have nearly no idea how hg local branches work. + list($stdout) = execx( + '(cd %s && hg branch)', + $this->getPath()); + return $stdout; + } + + public function getRelativeCommit() { + // TODO: This is hardcoded. + return 'tip~1'; + } + + public function getBlame($path) { + list($stdout) = execx( + '(cd %s && hg blame -u -v -c --rev %s -- %s)', + $this->getPath(), + $this->getRelativeCommit(), + $path); + + $blame = array(); + foreach (explode("\n", trim($stdout)) as $line) { + if (!strlen($line)) { + continue; + } + + $matches = null; + $ok = preg_match('^/\s*([^:]+?) [a-f0-9]{12}: (.*)$/', $line, $matches); + + if (!$ok) { + throw new Exception("Unable to parse Mercurial blame line: {$line}"); + } + + $revision = $matches[2]; + $author = trim($matches[1]); + $blame[] = array($author, $revision); + } + + return $blame; + } + + public function getWorkingCopyStatus() { + + // TODO: This is critical and not yet implemented. + + return array(); + } + + private function getDiffOptions() { + $options = array( + '-g', + '-U'.$this->getDiffLinesOfContext(), + ); + return implode(' ', $options); + } + + public function getRawDiffText($path) { + $options = $this->getDiffOptions(); + + list($stdout) = execx( + '(cd %s && hg diff %C --rev %s --rev tip -- %s)', + $this->getPath(), + $options, + $this->getRelativeCommit(), + $path); + + return $stdout; + } + + public function getFullMercurialDiff() { + $options = $this->getDiffOptions(); + + list($stdout) = execx( + '(cd %s && hg diff %C --rev %s --rev tip --)', + $this->getPath(), + $options, + $this->getRelativeCommit()); + + return $stdout; + } + + public function getOriginalFileData($path) { + return $this->getFileDataAtRevision($path, $this->getRelativeCommit()); + } + + public function getCurrentFileData($path) { + return $this->getFileDataAtRevision($path, 'tip'); + } + + private function getFileDataAtRevision($path, $revision) { + list($stdout) = execx( + '(cd %s && hg cat --rev %s -- %s)', + $this->getPath(), + $path); + return $stdout; + } + +} diff --git a/src/repository/api/mercurial/__init__.php b/src/repository/api/mercurial/__init__.php new file mode 100644 index 00000000..480bf00d --- /dev/null +++ b/src/repository/api/mercurial/__init__.php @@ -0,0 +1,14 @@ +getRepositorySVNUUID(); + } else if ($repository_api instanceof ArcanistMercurialAPI) { + // TODO: Provide this information. + } else { + throw new Exception("Unsupported repository API!"); } $working_copy = $this->getWorkingCopy(); @@ -527,10 +531,18 @@ EOTEXT } protected function shouldOnlyCreateDiff() { + $repository_api = $this->getRepositoryAPI(); if ($repository_api instanceof ArcanistSubversionAPI) { return true; } + + if ($repository_api instanceof ArcanistMercurialAPI) { + // TODO: This is unlikely to be correct since it excludes using local + // branching in Mercurial. + return true; + } + return $this->getArgument('preview') || $this->getArgument('only'); } @@ -580,11 +592,19 @@ EOTEXT } } - } else { + } else if ($repository_api instanceof ArcanistGitAPI) { $this->parseGitRelativeCommit( $repository_api, $this->getArgument('paths', array())); $paths = $repository_api->getWorkingCopyStatus(); + } else if ($repository_api instanceof ArcanistMercurialAPI) { + // TODO: Unify this and the previous block. + + // TODO: Parse the relative commit. + + $paths = $repository_api->getWorkingCopyStatus(); + } else { + throw new Exception("Unknown VCS!"); } foreach ($paths as $path => $mask) { @@ -669,6 +689,9 @@ EOTEXT } $changes = $parser->parseDiff($diff); + } else if ($repository_api instanceof ArcanistMercurialAPI) { + $diff = $repository_api->getFullMercurialDiff(); + $changes = $parser->parseDiff($diff); } else { throw new Exception("Repository API is not supported."); }