From b3a9ee3d846640f4cc447ca97bc305daea7464b2 Mon Sep 17 00:00:00 2001 From: Jason Ge Date: Wed, 13 Jul 2011 16:33:18 -0700 Subject: [PATCH 01/19] Setting default minimumSeverity to zero from null Summary: In hphp's version of idx, it uses debug_rlog() which is not available in phabricator's code. It will be executed if null is passed as the second value to idx. Test Plan: ran arc lint which still report error message, and php ./scripts/arcanist.php unit src/lint/linter/xhpast/__test__ doesn't throw. Reviewed By: epriestley Reviewers: tuomaspelkonen, epriestley CC: aran, jungejason, epriestley Differential Revision: 666 --- src/lint/engine/base/ArcanistLintEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lint/engine/base/ArcanistLintEngine.php b/src/lint/engine/base/ArcanistLintEngine.php index 84ff3357..6f98259b 100644 --- a/src/lint/engine/base/ArcanistLintEngine.php +++ b/src/lint/engine/base/ArcanistLintEngine.php @@ -65,7 +65,7 @@ abstract class ArcanistLintEngine { protected $charToLine = array(); protected $lineToFirstChar = array(); private $results = array(); - private $minimumSeverity = null; + private $minimumSeverity = ArcanistLintSeverity::SEVERITY_DISABLED; private $changedLines = array(); private $commitHookMode = false; From eb98ab553e86854bb892465d826516a5246b6ba4 Mon Sep 17 00:00:00 2001 From: James Ide Date: Sat, 23 Jul 2011 16:48:21 -0700 Subject: [PATCH 02/19] Munge XHP class names when generating phutil dependency maps Summary: When phutil_analyzer builds the dependency graph I convert all class names w.r.t XHP's internal naming scheme. It actually wouldn't be a terrible idea to do this munging when phutil loads the symbols. I guess it doesn't really matter at the moment since only Arcanist generates phutil maps and only libphutil reads them, but it'd be nice to do this munging in as few places as possible in the future. The XHP grammar does not allow elements to be interfaces (https://github.com/facebook/xhp/blob/dafff2cc1844d1138c3388509c9560b87167a3bb/xhp/parser.y#L1688) so I've only applied this to classes. Test Plan: Created a sample project: ./__phutil_library_init__.php hopping/__init__.php /hophophop.php where that last file contains bunny goes hop hop hop

; } } Ran phutil_mapper.php and generated __phutil_library_map__.php: array( 'xhp_bunny__hop_hop_hop' => 'hopping', ), 'function' => array( ), 'requires_class' => array( 'xhp_bunny__hop_hop_hop' => 'xhp_x__element', ), 'requires_interface' => array( ), )); Reviewed By: epriestley Reviewers: epriestley Commenters: aran CC: aran, ide, epriestley Differential Revision: 715 --- src/parser/phutilmodule/PhutilModuleRequirements.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/parser/phutilmodule/PhutilModuleRequirements.php b/src/parser/phutilmodule/PhutilModuleRequirements.php index f6bbcbb7..779b0179 100644 --- a/src/parser/phutilmodule/PhutilModuleRequirements.php +++ b/src/parser/phutilmodule/PhutilModuleRequirements.php @@ -65,6 +65,7 @@ class PhutilModuleRequirements { } public function addClassDeclaration(XHPASTNode $where, $name) { + $name = self::mungeXHPClassName($name); return $this->addDeclaration('class', $where, $name); } @@ -98,8 +99,10 @@ class PhutilModuleRequirements { } public function addClassDependency($child, XHPASTNode $where, $name) { + $name = self::mungeXHPClassName($name); if ($child !== null) { if (empty($this->builtins['class'][$name])) { + $child = self::mungeXHPClassName($child); $this->chain['class'][$child] = $name; } } @@ -173,4 +176,13 @@ class PhutilModuleRequirements { 'messages' => $this->messages, ); } + + private static function mungeXHPClassName($name) { + if (strlen($name) && $name[0] == ':') { + // XHP's semantic actions munge element names without a preceding colon. + $name = substr($name, 1); + return 'xhp_'.str_replace(array(':', '-'), array('__', '_'), $name); + } + return $name; + } } From 1bb2409d473b4b3892a00c9e6a155811a267b9dd Mon Sep 17 00:00:00 2001 From: Jason Ge Date: Mon, 25 Jul 2011 16:24:23 -0700 Subject: [PATCH 03/19] Remove the size limit on arc side Summary: remove the size check. The conduit call will fail later if the size is too large, and we will get better error message. Test Plan: run arc diff with files smaller and larger than the size limit. Reviewed By: epriestley Reviewers: epriestley CC: aran, epriestley Differential Revision: 729 --- src/workflow/diff/ArcanistDiffWorkflow.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index f5ddd206..bf365d9f 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/src/workflow/diff/ArcanistDiffWorkflow.php @@ -819,12 +819,6 @@ EOTEXT $mime_type = trim($mime_type); $result['mime'] = $mime_type; - // TODO: Make this configurable. - $bin_limit = 1024 * 1024; // 1 MB limit - if (strlen($data) > $bin_limit) { - return $result; - } - $bytes = strlen($data); echo "Uploading {$desc} '{$name}' ({$mime_type}, {$bytes} bytes)...\n"; From 3b4f9dbd9ad0716b3a9fd996dbf909d2386c7ed1 Mon Sep 17 00:00:00 2001 From: Chris Johnson Date: Tue, 26 Jul 2011 11:58:33 -0700 Subject: [PATCH 04/19] Marked ArcanistDifferentialRevisionRef::newFromDictionary as a static method to statisfy E_STRICT standard --- src/differential/revision/ArcanistDifferentialRevisionRef.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/differential/revision/ArcanistDifferentialRevisionRef.php b/src/differential/revision/ArcanistDifferentialRevisionRef.php index 8e6c19ff..5a7668b4 100644 --- a/src/differential/revision/ArcanistDifferentialRevisionRef.php +++ b/src/differential/revision/ArcanistDifferentialRevisionRef.php @@ -28,7 +28,7 @@ class ArcanistDifferentialRevisionRef { protected $statusName; protected $sourcePath; - public function newFromDictionary(array $dictionary) { + public static function newFromDictionary(array $dictionary) { $ref = new ArcanistDifferentialRevisionRef(); $ref->id = $dictionary['id']; $ref->name = $dictionary['name']; From bd9769ba92df63d0429f74ec7fb8b00a4989f28b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Jul 2011 10:05:07 -0700 Subject: [PATCH 05/19] Add 'arc upload' and 'arc download' Summary: Mechanisms for interacting with Files via Arcanist. Test Plan: - Ran 'arc upload x', 'arc upload x y z' - Ran 'arc download' with --as and --show. Reviewed By: codeblock Reviewers: codeblock, jungejason, tuomaspelkonen, aran CC: aran, codeblock, epriestley Differential Revision: 742 --- src/__phutil_library_map__.php | 4 + .../download/ArcanistDownloadWorkflow.php | 126 ++++++++++++++++++ src/workflow/download/__init__.php | 17 +++ .../upload/ArcanistUploadWorkflow.php | 94 +++++++++++++ src/workflow/upload/__init__.php | 16 +++ 5 files changed, 257 insertions(+) create mode 100644 src/workflow/download/ArcanistDownloadWorkflow.php create mode 100644 src/workflow/download/__init__.php create mode 100644 src/workflow/upload/ArcanistUploadWorkflow.php create mode 100644 src/workflow/upload/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d2696b29..d20303df 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -31,6 +31,7 @@ phutil_register_library_map(array( 'ArcanistDifferentialCommitMessage' => 'differential/commitmessage', 'ArcanistDifferentialCommitMessageParserException' => 'differential/commitmessage', 'ArcanistDifferentialRevisionRef' => 'differential/revision', + 'ArcanistDownloadWorkflow' => 'workflow/download', 'ArcanistExportWorkflow' => 'workflow/export', 'ArcanistFilenameLinter' => 'lint/linter/filename', 'ArcanistGeneratedLinter' => 'lint/linter/generated', @@ -73,6 +74,7 @@ phutil_register_library_map(array( 'ArcanistTextLinterTestCase' => 'lint/linter/text/__tests__', 'ArcanistUnitTestResult' => 'unit/result', 'ArcanistUnitWorkflow' => 'workflow/unit', + 'ArcanistUploadWorkflow' => 'workflow/upload', 'ArcanistUsageException' => 'exception/usage', 'ArcanistUserAbortException' => 'exception/usage/userabort', 'ArcanistWorkingCopyIdentity' => 'workingcopyidentity', @@ -100,6 +102,7 @@ phutil_register_library_map(array( 'ArcanistCoverWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistDiffParserTestCase' => 'ArcanistPhutilTestCase', 'ArcanistDiffWorkflow' => 'ArcanistBaseWorkflow', + 'ArcanistDownloadWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistExportWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistFilenameLinter' => 'ArcanistLinter', 'ArcanistGeneratedLinter' => 'ArcanistLinter', @@ -129,6 +132,7 @@ phutil_register_library_map(array( 'ArcanistTextLinter' => 'ArcanistLinter', 'ArcanistTextLinterTestCase' => 'ArcanistLinterTestCase', 'ArcanistUnitWorkflow' => 'ArcanistBaseWorkflow', + 'ArcanistUploadWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistUserAbortException' => 'ArcanistUsageException', 'ArcanistXHPASTLinter' => 'ArcanistLinter', 'ArcanistXHPASTLinterTestCase' => 'ArcanistLinterTestCase', diff --git a/src/workflow/download/ArcanistDownloadWorkflow.php b/src/workflow/download/ArcanistDownloadWorkflow.php new file mode 100644 index 00000000..51f6f195 --- /dev/null +++ b/src/workflow/download/ArcanistDownloadWorkflow.php @@ -0,0 +1,126 @@ + array( + 'conflicts' => array( + 'as' => 'Use --show to direct the file to stdout, or --as to direct '. + 'it to a named location.', + ), + 'help' => 'Write file to stdout instead of to disk.', + ), + 'as' => array( + 'param' => 'name', + 'help' => 'Save the file with a specific name rather than the default.', + ), + '*' => 'argv', + ); + } + + protected function didParseArguments() { + $argv = $this->getArgument('argv'); + if (!$argv) { + throw new ArcanistUsageException("Specify a file to download."); + } + if (count($argv) > 1) { + throw new ArcanistUsageException("Specify exactly one file to download."); + } + + $file = reset($argv); + if (!preg_match('/^F?\d+/', $file)) { + throw new ArcanistUsageException("Specify file by ID, e.g. F123."); + } + + $this->id = (int)ltrim($file, 'F'); + $this->saveAs = $this->getArgument('as'); + $this->show = $this->getArgument('show'); + } + + public function requiresAuthentication() { + return true; + } + + public function run() { + + $conduit = $this->getConduit(); + + $this->writeStatus("Getting file information...\n"); + $info = $conduit->callMethodSynchronous( + 'file.info', + array( + 'id' => $this->id, + )); + + $bytes = number_format($info['byteSize']); + $desc = '('.$bytes.' bytes)'; + if ($info['name']) { + $desc = "'".$info['name']."' ".$desc; + } + + $this->writeStatus("Downloading file {$desc}...\n"); + $data = $conduit->callMethodSynchronous( + 'file.download', + array( + 'phid' => $info['phid'], + )); + + $data = base64_decode($data); + + if ($this->show) { + echo $data; + } else { + $path = Filesystem::writeUniqueFile( + nonempty($this->saveAs, $info['name'], 'file'), + $data); + $this->writeStatus("Saved file as '{$path}'.\n"); + } + + return 0; + } + + private function writeStatus($msg) { + // Use stderr instead of stdout since we may echo file contents to + // stdout with --show. + file_put_contents('php://stderr', $msg); + } + +} diff --git a/src/workflow/download/__init__.php b/src/workflow/download/__init__.php new file mode 100644 index 00000000..f0051125 --- /dev/null +++ b/src/workflow/download/__init__.php @@ -0,0 +1,17 @@ + 'paths', + ); + } + + protected function didParseArguments() { + if (!$this->getArgument('paths')) { + throw new ArcanistUsageException("Specify one or more files to upload."); + } + + $this->paths = $this->getArgument('paths'); + } + + public function requiresAuthentication() { + return true; + } + + private function getPaths() { + return $this->paths; + } + + public function run() { + + $conduit = $this->getConduit(); + + foreach ($this->paths as $path) { + $name = basename($path); + echo "Uploading '{$name}'...\n"; + try { + $data = Filesystem::readFile($path); + } catch (FilesystemException $ex) { + echo "Unable to upload file: ".$ex->getMessage()."\n"; + continue; + } + + $phid = $conduit->callMethodSynchronous( + 'file.upload', + array( + 'data_base64' => base64_encode($data), + 'name' => $name, + )); + $info = $conduit->callMethodSynchronous( + 'file.info', + array( + 'phid' => $phid, + )); + + echo " {$name}: ".$info['uri']."\n\n"; + } + + echo "Done.\n"; + + return 0; + } + +} diff --git a/src/workflow/upload/__init__.php b/src/workflow/upload/__init__.php new file mode 100644 index 00000000..d7c813bb --- /dev/null +++ b/src/workflow/upload/__init__.php @@ -0,0 +1,16 @@ + Date: Fri, 29 Jul 2011 15:22:37 -0700 Subject: [PATCH 06/19] Specify date format in git blame Summary: right now 'arc blame' uses the user's blame.date setting, but it will only work when the format is iso. So it will fail for user who has customized it. Test Plan: run arc cover with 'blame.date' set to relative and verified that 'arc cover' still works Reviewed By: epriestley Reviewers: epriestley, codeblock CC: hwang, aran, epriestley Differential Revision: 745 --- src/repository/api/git/ArcanistGitAPI.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/repository/api/git/ArcanistGitAPI.php b/src/repository/api/git/ArcanistGitAPI.php index 13570a46..c6234cb1 100644 --- a/src/repository/api/git/ArcanistGitAPI.php +++ b/src/repository/api/git/ArcanistGitAPI.php @@ -317,7 +317,7 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI { public function getBlame($path) { // TODO: 'git blame' supports --porcelain and we should probably use it. list($stdout) = execx( - '(cd %s; git blame -w -C %s -- %s)', + '(cd %s; git blame --date=iso -w -C %s -- %s)', $this->getPath(), $this->getRelativeCommit(), $path); From 0e4f7774fb25880c909aa345070882535d4370cb Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 29 Jul 2011 18:55:01 -0700 Subject: [PATCH 07/19] Add an "arc paste" workflow Summary: Read and write in the same workflow! Dogs and cats living together! Test Plan: - Performed a bunch of paste reads and writes and they looked ok? Reviewed By: aran Reviewers: codeblock, jungejason, tuomaspelkonen, aran CC: aran Differential Revision: 748 --- src/__phutil_library_map__.php | 2 + src/workflow/paste/ArcanistPasteWorkflow.php | 159 +++++++++++++++++++ src/workflow/paste/__init__.php | 15 ++ 3 files changed, 176 insertions(+) create mode 100644 src/workflow/paste/ArcanistPasteWorkflow.php create mode 100644 src/workflow/paste/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d20303df..2d7d4f02 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -60,6 +60,7 @@ phutil_register_library_map(array( 'ArcanistNoLintLinter' => 'lint/linter/nolint', 'ArcanistNoLintTestCaseMisnamed' => 'lint/linter/nolint/__tests__', 'ArcanistPEP8Linter' => 'lint/linter/pep8', + 'ArcanistPasteWorkflow' => 'workflow/paste', 'ArcanistPatchWorkflow' => 'workflow/patch', 'ArcanistPhutilModuleLinter' => 'lint/linter/phutilmodule', 'ArcanistPhutilTestCase' => 'unit/engine/phutil/testcase', @@ -122,6 +123,7 @@ phutil_register_library_map(array( 'ArcanistNoLintLinter' => 'ArcanistLinter', 'ArcanistNoLintTestCaseMisnamed' => 'ArcanistLinterTestCase', 'ArcanistPEP8Linter' => 'ArcanistLinter', + 'ArcanistPasteWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistPatchWorkflow' => 'ArcanistBaseWorkflow', 'ArcanistPhutilModuleLinter' => 'ArcanistLinter', 'ArcanistPyFlakesLinter' => 'ArcanistLinter', diff --git a/src/workflow/paste/ArcanistPasteWorkflow.php b/src/workflow/paste/ArcanistPasteWorkflow.php new file mode 100644 index 00000000..96803656 --- /dev/null +++ b/src/workflow/paste/ArcanistPasteWorkflow.php @@ -0,0 +1,159 @@ + array( + 'param' => 'title', + 'help' => 'Title for the paste.', + ), + 'lang' => array( + 'param' => 'language', + 'help' => 'Language for syntax highlighting.', + ), + 'json' => array( + 'help' => 'Output in JSON format.', + ), + '*' => 'argv', + ); + } + + public function requiresAuthentication() { + return true; + } + + protected function didParseArguments() { + $this->language = $this->getArgument('lang'); + $this->title = $this->getArgument('title'); + $this->json = $this->getArgument('json'); + + $argv = $this->getArgument('argv'); + if (count($argv) > 1) { + throw new ArcanistUsageException("Specify only one paste to retrieve."); + } else if (count($argv) == 1) { + $id = $argv[0]; + if (!preg_match('/^P?\d+/', $id)) { + throw new ArcanistUsageException("Specify a paste ID, like P123."); + } + $this->id = (int)ltrim($id, 'P'); + + if ($this->language || $this->title) { + throw new ArcanistUsageException( + "Use options --lang and --title only when creating pastes."); + } + } + } + + private function getTitle() { + return $this->title; + } + + private function getLanguage() { + return $this->language; + } + + private function getJSON() { + return $this->json; + } + + public function run() { + + if ($this->id) { + return $this->getPaste(); + } else { + return $this->createPaste(); + } + } + + private function getPaste() { + $conduit = $this->getConduit(); + + $info = $conduit->callMethodSynchronous( + 'paste.info', + array( + 'paste_id' => $this->id, + )); + + if ($this->getJSON()) { + echo json_encode($info)."\n"; + } else { + echo $info['content']; + if (!preg_match('/\\n$/', $info['content'])) { + // If there's no newline, add one, since it looks stupid otherwise. If + // you want byte-for-byte equivalence you can use --json. + echo "\n"; + } + } + + return 0; + } + + private function createPaste() { + $conduit = $this->getConduit(); + + // Avoid confusion when people type "arc paste" with nothing else. + file_put_contents('php://stderr', "Reading paste from stdin...\n"); + + $info = $conduit->callMethodSynchronous( + 'paste.create', + array( + 'content' => file_get_contents('php://stdin'), + 'title' => $this->getTitle(), + 'language' => $this->getLanguage(), + )); + + if ($this->getArgument('json')) { + echo json_encode($info)."\n"; + } else { + echo $info['objectName'].': '.$info['uri']."\n"; + } + + return 0; + } + +} diff --git a/src/workflow/paste/__init__.php b/src/workflow/paste/__init__.php new file mode 100644 index 00000000..3ef0f7e3 --- /dev/null +++ b/src/workflow/paste/__init__.php @@ -0,0 +1,15 @@ + Date: Fri, 29 Jul 2011 20:17:49 -0700 Subject: [PATCH 08/19] Provide "arc upload --json" Summary: - Provide a "--json" flag for "arc upload" - Unify some of the stderr stuff across upload/download/paste. Test Plan: - Ran "arc upload" and "arc upload --json", piped stderr away with 2>/dev/null to verify only JSON got emitted to stdout - Ran "arc paste" Reviewed By: codeblock Reviewers: codeblock, jungejason, tuomaspelkonen, aran CC: aran, codeblock Differential Revision: 749 --- src/workflow/base/ArcanistBaseWorkflow.php | 11 +++++++ .../download/ArcanistDownloadWorkflow.php | 12 ++----- src/workflow/paste/ArcanistPasteWorkflow.php | 2 +- .../upload/ArcanistUploadWorkflow.php | 31 ++++++++++++++++--- 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index 7d0de227..59de92e2 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -919,4 +919,15 @@ class ArcanistBaseWorkflow { return $user_config; } + /** + * Write a message to stderr so that '--json' flags or stdout which is meant + * to be piped somewhere aren't disrupted. + * + * @param string Message to write to stderr. + * @return void + */ + protected function writeStatusMessage($msg) { + file_put_contents('php://stderr', $msg); + } + } diff --git a/src/workflow/download/ArcanistDownloadWorkflow.php b/src/workflow/download/ArcanistDownloadWorkflow.php index 51f6f195..6c40f762 100644 --- a/src/workflow/download/ArcanistDownloadWorkflow.php +++ b/src/workflow/download/ArcanistDownloadWorkflow.php @@ -83,7 +83,7 @@ EOTEXT $conduit = $this->getConduit(); - $this->writeStatus("Getting file information...\n"); + $this->writeStatusMessage("Getting file information...\n"); $info = $conduit->callMethodSynchronous( 'file.info', array( @@ -96,7 +96,7 @@ EOTEXT $desc = "'".$info['name']."' ".$desc; } - $this->writeStatus("Downloading file {$desc}...\n"); + $this->writeStatusMessage("Downloading file {$desc}...\n"); $data = $conduit->callMethodSynchronous( 'file.download', array( @@ -111,16 +111,10 @@ EOTEXT $path = Filesystem::writeUniqueFile( nonempty($this->saveAs, $info['name'], 'file'), $data); - $this->writeStatus("Saved file as '{$path}'.\n"); + $this->writeStatusMessage("Saved file as '{$path}'.\n"); } return 0; } - private function writeStatus($msg) { - // Use stderr instead of stdout since we may echo file contents to - // stdout with --show. - file_put_contents('php://stderr', $msg); - } - } diff --git a/src/workflow/paste/ArcanistPasteWorkflow.php b/src/workflow/paste/ArcanistPasteWorkflow.php index 96803656..798ab3d8 100644 --- a/src/workflow/paste/ArcanistPasteWorkflow.php +++ b/src/workflow/paste/ArcanistPasteWorkflow.php @@ -137,7 +137,7 @@ EOTEXT $conduit = $this->getConduit(); // Avoid confusion when people type "arc paste" with nothing else. - file_put_contents('php://stderr', "Reading paste from stdin...\n"); + $this->writeStatusMessage("Reading paste from stdin...\n"); $info = $conduit->callMethodSynchronous( 'paste.create', diff --git a/src/workflow/upload/ArcanistUploadWorkflow.php b/src/workflow/upload/ArcanistUploadWorkflow.php index e501c14b..0bbbe1a1 100644 --- a/src/workflow/upload/ArcanistUploadWorkflow.php +++ b/src/workflow/upload/ArcanistUploadWorkflow.php @@ -24,10 +24,11 @@ final class ArcanistUploadWorkflow extends ArcanistBaseWorkflow { private $paths; + private $json; public function getCommandHelp() { return phutil_console_format(<< array( + 'help' => 'Output upload information in JSON format.', + ), '*' => 'paths', ); } @@ -47,6 +51,7 @@ EOTEXT } $this->paths = $this->getArgument('paths'); + $this->json = $this->getArgument('json'); } public function requiresAuthentication() { @@ -57,17 +62,25 @@ EOTEXT return $this->paths; } + private function getJSON() { + return $this->json; + } + public function run() { $conduit = $this->getConduit(); + $results = array(); + foreach ($this->paths as $path) { $name = basename($path); - echo "Uploading '{$name}'...\n"; + $this->writeStatusMessage("Uploading '{$name}'...\n"); try { $data = Filesystem::readFile($path); } catch (FilesystemException $ex) { - echo "Unable to upload file: ".$ex->getMessage()."\n"; + $this->writeStatusMessage( + "Unable to upload file: ".$ex->getMessage()."\n"); + $results[$path] = null; continue; } @@ -83,10 +96,18 @@ EOTEXT 'phid' => $phid, )); - echo " {$name}: ".$info['uri']."\n\n"; + $results[$path] = $info; + + if (!$this->getJSON()) { + echo " {$name}: ".$info['uri']."\n\n"; + } } - echo "Done.\n"; + if ($this->getJSON()) { + echo json_encode($results)."\n"; + } else { + $this->writeStatusMessage("Done.\n"); + } return 0; } From 17c82ff03cb9a33ad0165970d2cc8be337e82d13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Aug 2011 13:18:53 -0700 Subject: [PATCH 09/19] Add an XHPAST lint rule for brace formatting Summary: See D753. Let's just have lint fix this. Depends on D754 (it fails to detect all the blocks without that patch, but doesn't do anything bad). Test Plan: Ran unit tests. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: hunterbridges, aran, epriestley, jungejason Differential Revision: 755 --- .../linter/xhpast/ArcanistXHPASTLinter.php | 24 ++++++++ .../data/creative-brace-use.lint-test | 56 +++++++++++++++++++ 2 files changed, 80 insertions(+) create mode 100644 src/lint/linter/xhpast/__tests__/data/creative-brace-use.lint-test diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index 404e13e7..cf616f4c 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php @@ -155,6 +155,30 @@ class ArcanistXHPASTLinter extends ArcanistLinter { $this->lintPlusOperatorOnStrings($root); $this->lintDuplicateKeysInArray($root); $this->lintReusedIterators($root); + $this->lintBraceFormatting($root); + } + + private function lintBraceFormatting($root) { + + foreach ($root->selectDescendantsOfType('n_STATEMENT_LIST') as $list) { + $tokens = $list->getTokens(); + if (!$tokens || head($tokens)->getValue() != '{') { + continue; + } + list($before, $after) = $list->getSurroundingNonsemanticTokens(); + if (count($before) == 1) { + $before = reset($before); + if ($before->getValue() != ' ') { + $this->raiseLintAtToken( + $before, + self::LINT_FORMATTING_CONVENTIONS, + 'Put opening braces on the same line as control statements and '. + 'declarations, with a single space before them.', + ' '); + } + } + } + } private function lintTautologicalExpressions($root) { diff --git a/src/lint/linter/xhpast/__tests__/data/creative-brace-use.lint-test b/src/lint/linter/xhpast/__tests__/data/creative-brace-use.lint-test new file mode 100644 index 00000000..dc4f8bc4 --- /dev/null +++ b/src/lint/linter/xhpast/__tests__/data/creative-brace-use.lint-test @@ -0,0 +1,56 @@ + Date: Fri, 29 Jul 2011 09:12:24 -0700 Subject: [PATCH 10/19] Fix an obscure dependency issue in Arcanist + libphutil Summary: The module analyzer reads "phutil_require_module" in the source of a module as a dependency, and tries to regenerate __init__.php if symbols from that module aren't actually used. This creates patches which don't actually resolve the problem, since changing __init__.php won't change the dependency. Instead, trust that anyone using phutil_require_module in the source of a module knows what they're doing and don't mark it as a dependency. We currently have an issue with this in phabricator's Setup process since I load some other libraries' modules just to test if they can be loaded @lesha, this might be the issue you reported a while ago. Test Plan: Ran "arc lint" on a module which pulls in another module explicitly in the source, didn't get a no-op lint error. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran, lesha CC: aran, epriestley, jungejason Differential Revision: 770 --- scripts/phutil_analyzer.php | 15 +++++++++------ src/lint/message/ArcanistLintMessage.php | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/scripts/phutil_analyzer.php b/scripts/phutil_analyzer.php index 2fdc5134..91b253dc 100755 --- a/scripts/phutil_analyzer.php +++ b/scripts/phutil_analyzer.php @@ -122,7 +122,7 @@ foreach (Futures($futures) as $file => $future) { } $requirements->addSourceDependency($name, $value); } else if ($call_name == 'phutil_require_module') { - analyze_phutil_require_module($call, $requirements); + analyze_phutil_require_module($call, $requirements, true); } } } else { @@ -154,7 +154,7 @@ foreach (Futures($futures) as $file => $future) { $call_name = $name->getConcreteString(); if ($call_name == 'phutil_require_module') { - analyze_phutil_require_module($call, $requirements); + analyze_phutil_require_module($call, $requirements, false); } else if ($call_name == 'call_user_func' || $call_name == 'call_user_func_array') { $params = $call->getChildByIndex(1)->getChildren(); @@ -327,7 +327,8 @@ echo json_encode($requirements->toDictionary()); */ function analyze_phutil_require_module( XHPASTNode $call, - PhutilModuleRequirements $requirements) { + PhutilModuleRequirements $requirements, + $create_dependency) { $name = $call->getChildByIndex(0); $params = $call->getChildByIndex(1)->getChildren(); @@ -363,7 +364,9 @@ function analyze_phutil_require_module( return; } - $requirements->addModuleDependency( - $name, - $library_value.':'.$module_value); + if ($create_dependency) { + $requirements->addModuleDependency( + $name, + $library_value.':'.$module_value); + } } diff --git a/src/lint/message/ArcanistLintMessage.php b/src/lint/message/ArcanistLintMessage.php index 5320a202..dcfa2418 100644 --- a/src/lint/message/ArcanistLintMessage.php +++ b/src/lint/message/ArcanistLintMessage.php @@ -168,7 +168,8 @@ class ArcanistLintMessage { } public function isPatchable() { - return ($this->getReplacementText() !== null); + return ($this->getReplacementText() !== null) && + ($this->getReplacementText() !== $this->getOriginalText()); } public function didApplyPatch() { From 344fbb8d358c9a4f496c10ffed8d0c41337df5f8 Mon Sep 17 00:00:00 2001 From: Edward Speyer Date: Tue, 2 Aug 2011 18:12:36 +0100 Subject: [PATCH 11/19] Fix documentation: stop __init__ rendering in bold Summary: Requires https://secure.phabricator.com/rPHU0acf708b9b0fdbf59e4399f14dd8295b6a96972c from libphutil, which allows a backslash prefix to control escape sequences such as bold, underline, and invert. Test Plan: "arc help liberate" Reviewed By: epriestley Reviewers: epriestley CC: aran, epriestley Differential Revision: 767 --- src/workflow/liberate/ArcanistLiberateWorkflow.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workflow/liberate/ArcanistLiberateWorkflow.php b/src/workflow/liberate/ArcanistLiberateWorkflow.php index 645cee4c..68289392 100644 --- a/src/workflow/liberate/ArcanistLiberateWorkflow.php +++ b/src/workflow/liberate/ArcanistLiberateWorkflow.php @@ -34,7 +34,7 @@ class ArcanistLiberateWorkflow extends ArcanistBaseWorkflow { **liberate** [__path__] Supports: libphutil Create or update a libphutil library, generating required metadata - files like __init__.php. + files like \__init__.php. EOTEXT ); } From 40b445b3874493981768c04d3774e4f1ad253490 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 7 Aug 2011 14:54:32 -0700 Subject: [PATCH 12/19] Fix a variable usage Summary: This stopped being available in scope when I refactored this recentlyish. Test Plan: Got error, saw useful message. Reviewed By: jungejason Reviewers: mgummelt, jungejason, tuomaspelkonen, aran CC: aran, epriestley, jungejason Differential Revision: 787 --- src/workflow/base/ArcanistBaseWorkflow.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index 59de92e2..4eb876e6 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -222,6 +222,7 @@ class ArcanistBaseWorkflow { } catch (ConduitClientException $ex) { if ($ex->getErrorCode() == 'ERR-NO-CERTIFICATE' || $ex->getErrorCode() == 'ERR-INVALID-USER') { + $conduit_uri = $this->conduitURI; $message = "\n". phutil_console_format( From 268de6428c4a2483a3590090064276dd556dd5a6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Aug 2011 09:00:29 -0700 Subject: [PATCH 13/19] 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."); } From 58c09ab36da6f9028a69350916b4c3bd9384c6dc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 9 Aug 2011 18:54:10 -0700 Subject: [PATCH 14/19] Improve Arcanist Mercurial support Summary: - Build the manifest of file changes so unit and lint workflows work. - Default to creating a diff between the parent of the first outgoing change and the tip. Test Plan: - Ran "arc diff" in a dirty mercurial repo, got warned about untracked/uncommitted changes. - Ran "arc diff" in a clean mercurial repo, got a diff of everything I'd done locally. Reviewed By: aran Reviewers: Makinde, aran, jungejason, tuomaspelkonen CC: aran, epriestley Differential Revision: 796 --- .../api/mercurial/ArcanistMercurialAPI.php | 163 +++++++++++++++++- src/repository/api/mercurial/__init__.php | 4 + src/workflow/base/ArcanistBaseWorkflow.php | 4 + 3 files changed, 167 insertions(+), 4 deletions(-) diff --git a/src/repository/api/mercurial/ArcanistMercurialAPI.php b/src/repository/api/mercurial/ArcanistMercurialAPI.php index 16bcdffa..0571cced 100644 --- a/src/repository/api/mercurial/ArcanistMercurialAPI.php +++ b/src/repository/api/mercurial/ArcanistMercurialAPI.php @@ -25,6 +25,7 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { private $status; private $base; + private $relativeCommit; public function getSourceControlSystemName() { return 'hg'; @@ -50,9 +51,35 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $stdout; } + public function setRelativeCommit($commit) { + list($err) = exec_manual( + '(cd %s && hg id -ir %s)', + $this->getPath(), + $commit); + + if ($err) { + throw new ArcanistUsageException( + "Commit '{$commit}' is not a valid Mercurial commit identifier."); + } + + $this->relativeCommit = $commit; + return $this; + } + public function getRelativeCommit() { - // TODO: This is hardcoded. - return 'tip~1'; + if (empty($this->relativeCommit)) { + list($stdout) = execx( + '(cd %s && hg outgoing --limit 1)', + $this->getPath()); + $logs = $this->parseMercurialLog($stdout); + if (!count($logs)) { + throw new ArcanistUsageException("You have no outgoing changes!"); + } + $oldest_log = head($logs); + + $this->relativeCommit = $oldest_log['rev'].'~1'; + } + return $this->relativeCommit; } public function getBlame($path) { @@ -85,9 +112,52 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { public function getWorkingCopyStatus() { - // TODO: This is critical and not yet implemented. + // 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. - return array(); + $diff = $this->getFullMercurialDiff(); + $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) = execx( + '(cd %s && hg status)', + $this->getPath()); + + $working_status = $this->parseMercurialStatus($stdout); + foreach ($working_status as $path => $status) { + $status |= self::FLAG_UNCOMMITTED; + if (!empty($status_map[$path])) { + $status_map[$path] |= $status; + } else { + $status_map[$path] = $status; + } + } + + return $status_map; } private function getDiffOptions() { @@ -139,4 +209,89 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $stdout; } + private function parseMercurialStatus($status) { + $result = array(); + + $status = trim($status); + if (!strlen($status)) { + return $result; + } + + $lines = explode("\n", $status); + foreach ($lines as $line) { + $flags = 0; + list($code, $path) = explode(' ', $line, 2); + switch ($code) { + case 'A': + $flags |= self::FLAG_ADDED; + break; + case 'R': + $flags |= self::FLAG_REMOVED; + break; + case 'M': + $flags |= self::FLAG_MODIFIED; + break; + case 'C': + // This is "clean" and included only for completeness, these files + // have not been changed. + break; + case '!': + $flags |= self::FLAG_MISSING; + break; + case '?': + $flags |= self::FLAG_UNTRACKED; + break; + case 'I': + // This is "ignored" and included only for completeness. + break; + default: + throw new Exception("Unknown Mercurial status '{$code}'."); + } + + $result[$path] = $flags; + } + + return $result; + } + + private function parseMercurialLog($log) { + $result = array(); + + $chunks = explode("\n\n", trim($log)); + foreach ($chunks as $chunk) { + $commit = array(); + $lines = explode("\n", $chunk); + foreach ($lines as $line) { + if (preg_match('/^(comparing with|searching for changes)/', $line)) { + // These are sent to stdout when you run "hg outgoing" although the + // format is otherwise identical to "hg log". + continue; + } + list($name, $value) = explode(':', $line, 2); + $value = trim($value); + switch ($name) { + case 'user': + $commit['user'] = $value; + break; + case 'date': + $commit['date'] = strtotime($value); + break; + case 'summary': + $commit['summary'] = $value; + break; + case 'changeset': + list($local, $rev) = explode(':', $value, 2); + $commit['local'] = $local; + $commit['rev'] = $rev; + break; + default: + throw new Exception("Unknown Mercurial log field '{$name}'!"); + } + } + $result[] = $commit; + } + + return $result; + } + } diff --git a/src/repository/api/mercurial/__init__.php b/src/repository/api/mercurial/__init__.php index 480bf00d..f4ae19bc 100644 --- a/src/repository/api/mercurial/__init__.php +++ b/src/repository/api/mercurial/__init__.php @@ -6,9 +6,13 @@ +phutil_require_module('arcanist', 'exception/usage'); +phutil_require_module('arcanist', 'parser/diff'); +phutil_require_module('arcanist', 'parser/diff/changetype'); phutil_require_module('arcanist', 'repository/api/base'); phutil_require_module('phutil', 'future/exec'); +phutil_require_module('phutil', 'utils'); phutil_require_source('ArcanistMercurialAPI.php'); diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index 4eb876e6..0d622ac4 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -598,6 +598,10 @@ class ArcanistBaseWorkflow { echo phutil_console_wrap( "Since you don't have 'svn:ignore' rules for these files, you may ". "have forgotten to 'svn add' them."); + } else if ($api instanceof ArcanistMercurialAPI) { + echo phutil_console_wrap( + "Since you don't have '.hgignore' rules for these files, you ". + "may have forgotten to 'hg add' them to your commit."); } $prompt = "Do you want to continue without adding these files?"; From 0eab28fe1bf1ccedabc97540d7573916ae9f5ae8 Mon Sep 17 00:00:00 2001 From: Felipe Oliveira Carvalho Date: Wed, 10 Aug 2011 11:50:10 -0300 Subject: [PATCH 15/19] Fix phutil_analyzer bug on interface dependency creation Summary: Use $interface_name instead of $class_name when dealing with interfaces. Problem happens when you extend an interface: interface SubIface extends SuperIface { ... Notice: Undefined variable: class_name in ... Fatal error: Call to a member function getConcreteString() on a non-object in ... Test Plan: phutil_mapper.php works! --- scripts/phutil_analyzer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/phutil_analyzer.php b/scripts/phutil_analyzer.php index 91b253dc..c11b75d4 100755 --- a/scripts/phutil_analyzer.php +++ b/scripts/phutil_analyzer.php @@ -303,7 +303,7 @@ foreach (Futures($futures) as $file => $future) { $extends = $interface->getChildByIndex(2); foreach ($extends->selectDescendantsOfType('n_CLASS_NAME') as $parent) { $requirements->addInterfaceDependency( - $class_name->getConcreteString(), + $interface_name->getConcreteString(), $parent, $parent->getConcreteString()); } From 7a72b0b4f9b96b005b4a87cec870d33af3b18f09 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 10 Aug 2011 11:57:07 -0700 Subject: [PATCH 16/19] Be slightly less dumb about detecting "binary" files Summary: A better definition of "binary" is "not utf-8", instead of "has some characters not in this arbitrary regexp". Principally, this makes files with windows newlines not autodetect as binary. This might fix some of the issues in T365. Test Plan: @egillth applied this patch and verified that Diffusion now shows file content instead of detecting everything as binary in his repo full of Windows newlines. Reviewed By: jungejason Reviewers: egillth, tuomaspelkonen, jungejason, aran CC: aran, jungejason Differential Revision: 799 --- src/parser/diff/ArcanistDiffParser.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parser/diff/ArcanistDiffParser.php b/src/parser/diff/ArcanistDiffParser.php index f7fece8f..28a7dd83 100644 --- a/src/parser/diff/ArcanistDiffParser.php +++ b/src/parser/diff/ArcanistDiffParser.php @@ -751,7 +751,7 @@ class ArcanistDiffParser { $is_binary = false; if ($this->detectBinaryFiles) { - $is_binary = preg_match('/([^\x09\x0A\x20-\x7E]+)/', $corpus); + $is_binary = !phutil_is_utf8($corpus); } if ($is_binary) { From 4c05dc3cd61dd99b09ec3328ae7e2ee52d2f212b Mon Sep 17 00:00:00 2001 From: Nicholas Harper Date: Mon, 15 Aug 2011 12:25:05 -0700 Subject: [PATCH 17/19] Change patch generation for moving a file Summary: The unix utility `patch` will not move a file (unless it is in or out of /dev/null). If a diff copies or moves a file and also makes changes to the file, the generated patch needs to list the new filename as the path to both the original and new files so the changes get written to the new file. (When arc applies this patch, it copies or moves the original as needed before running `patch`.) (This only matters for svn repos, since arc uses git commands for git repos instead of using `patch`.) Test Plan: Created an arc bundle of a diff that copied a file to a new location and made changes in both locations, and then ran arc patch with this bundle (in an svn repo) to see that it correctly patches. Reviewed By: epriestley Reviewers: epriestley, jungejason, tuomaspelkonen CC: aran, epriestley, nh Differential Revision: 813 --- src/parser/bundle/ArcanistBundle.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/parser/bundle/ArcanistBundle.php b/src/parser/bundle/ArcanistBundle.php index a591ca54..10320ec3 100644 --- a/src/parser/bundle/ArcanistBundle.php +++ b/src/parser/bundle/ArcanistBundle.php @@ -150,6 +150,17 @@ class ArcanistBundle { $cur_path = '/dev/null'; } + // When the diff is used by `patch`, `patch` ignores what is listed as the + // current path and just makes changes to the file at the old path (unless + // the current path is '/dev/null'. + // If the old path and the current path aren't the same (and neither is + // /dev/null), this indicates the file was moved or copied. By listing + // both paths as the new file, `patch` will apply the diff to the new + // file. + if ($cur_path !== '/dev/null' && $old_path !== '/dev/null') { + $old_path = $cur_path; + } + $result[] = '--- '.$old_path; $result[] = '+++ '.$cur_path; From aa138a80d29313c528728e2d22c73a3ac5eea1fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Aug 2011 18:48:55 -0700 Subject: [PATCH 18/19] Attach local commit information to DVCS revisions Summary: When a revision is created, attach relevant information about the local commits which it came from if applicable. This supports T473, for DCVSes and DCVS workflows with immutable history where we can't just amend commit messages. It will also allow us to enrich the web interface. Test Plan: Will verify this info shows up for this very diff. Reviewers: fratrik, aran, jungejason, tuomaspelkonen Reviewed By: fratrik CC: aran, epriestley, fratrik Differential Revision: 857 --- .../api/base/ArcanistRepositoryAPI.php | 1 + src/repository/api/git/ArcanistGitAPI.php | 29 +++++++++++++++++++ .../api/mercurial/ArcanistMercurialAPI.php | 20 +++++++++++++ .../api/subversion/ArcanistSubversionAPI.php | 4 +++ src/workflow/diff/ArcanistDiffWorkflow.php | 11 +++++++ 5 files changed, 65 insertions(+) diff --git a/src/repository/api/base/ArcanistRepositoryAPI.php b/src/repository/api/base/ArcanistRepositoryAPI.php index 1acdd39f..1a987707 100644 --- a/src/repository/api/base/ArcanistRepositoryAPI.php +++ b/src/repository/api/base/ArcanistRepositoryAPI.php @@ -157,5 +157,6 @@ abstract class ArcanistRepositoryAPI { abstract public function getRawDiffText($path); abstract public function getOriginalFileData($path); abstract public function getCurrentFileData($path); + abstract public function getLocalCommitInformation(); } diff --git a/src/repository/api/git/ArcanistGitAPI.php b/src/repository/api/git/ArcanistGitAPI.php index 520aad78..17181d76 100644 --- a/src/repository/api/git/ArcanistGitAPI.php +++ b/src/repository/api/git/ArcanistGitAPI.php @@ -46,6 +46,35 @@ class ArcanistGitAPI extends ArcanistRepositoryAPI { return $this; } + public function getLocalCommitInformation() { + list($info) = execx( + '(cd %s && git log %s..%s --format=%s --)', + $this->getPath(), + $this->getRelativeCommit(), + 'HEAD', + '%H%x00%T%x00%P%x00%at%x00%an%x00%s'); + + $commits = array(); + + $info = trim($info); + $info = explode("\n", $info); + foreach ($info as $line) { + list($commit, $tree, $parents, $time, $author, $title) + = explode("\0", $line, 6); + + $commits[] = array( + 'commit' => $commit, + 'tree' => $tree, + 'parents' => array_filter(explode(' ', $parents)), + 'time' => $time, + 'author' => $author, + 'summary' => $title, + ); + } + + return $commits; + } + public function getRelativeCommit() { if ($this->relativeCommit === null) { list($err) = exec_manual( diff --git a/src/repository/api/mercurial/ArcanistMercurialAPI.php b/src/repository/api/mercurial/ArcanistMercurialAPI.php index 0571cced..d9de1a1b 100644 --- a/src/repository/api/mercurial/ArcanistMercurialAPI.php +++ b/src/repository/api/mercurial/ArcanistMercurialAPI.php @@ -82,6 +82,16 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { return $this->relativeCommit; } + public function getLocalCommitInformation() { + list($info) = execx( + '(cd %s && hg log --rev %s..%s --)', + $this->getPath(), + $this->getRelativeCommit(), + 'tip'); + return $this->parseMercurialLog($info); + } + + public function getBlame($path) { list($stdout) = execx( '(cd %s && hg blame -u -v -c --rev %s -- %s)', @@ -284,6 +294,16 @@ class ArcanistMercurialAPI extends ArcanistRepositoryAPI { $commit['local'] = $local; $commit['rev'] = $rev; break; + case 'parent': + if (empty($commit['parents'])) { + $commit['parents'] = array(); + } + list($local, $rev) = explode(':', $value, 2); + $commit['parents'][] = array( + 'local' => $local, + 'rev' => $rev, + ); + break; default: throw new Exception("Unknown Mercurial log field '{$name}'!"); } diff --git a/src/repository/api/subversion/ArcanistSubversionAPI.php b/src/repository/api/subversion/ArcanistSubversionAPI.php index 9a6941e2..4616ed35 100644 --- a/src/repository/api/subversion/ArcanistSubversionAPI.php +++ b/src/repository/api/subversion/ArcanistSubversionAPI.php @@ -478,4 +478,8 @@ EODIFF; return $info['Repository UUID']; } + public function getLocalCommitInformation() { + return null; + } + } diff --git a/src/workflow/diff/ArcanistDiffWorkflow.php b/src/workflow/diff/ArcanistDiffWorkflow.php index 0a8c4cde..8fa93390 100644 --- a/src/workflow/diff/ArcanistDiffWorkflow.php +++ b/src/workflow/diff/ArcanistDiffWorkflow.php @@ -339,6 +339,17 @@ EOTEXT )); } + $local_info = $repository_api->getLocalCommitInformation(); + if ($local_info) { + $conduit->callMethodSynchronous( + 'differential.setdiffproperty', + array( + 'diff_id' => $diff_info['diffid'], + 'name' => 'local:commits', + 'data' => json_encode($local_info), + )); + } + if ($this->unresolvedTests) { $data = array(); foreach ($this->unresolvedTests as $test) { From 0c11b5c70c60c9cdd7ad031d33c50420279b3679 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Aug 2011 13:07:01 -0700 Subject: [PATCH 19/19] Allow XHPAST lint name rules to be overridden through configuration Summary: See T326. Allow lint rules to be selectively overridden, e.g. for Conduit methods. Since FB has a long history of suggesting crazy patches for this stuff I think we're safer just adding a hook class than trying to do some kind of regexp magic. Test Plan: Wrote a hook for Phabricator and linted some Conduit files without issues. Ran unit tests. Reviewers: nh, jungejason, tuomaspelkonen, aran Reviewed By: nh CC: aran, nh Differential Revision: 874 --- src/__phutil_library_map__.php | 1 + .../linter/xhpast/ArcanistXHPASTLinter.php | 160 ++++++++++++------ .../ArcanistXHPASTLintNamingHook.php | 60 +++++++ .../linter/xhpast/naminghook/__init__.php | 10 ++ 4 files changed, 176 insertions(+), 55 deletions(-) create mode 100644 src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php create mode 100644 src/lint/linter/xhpast/naminghook/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b5e28bac..7d9ac707 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -80,6 +80,7 @@ phutil_register_library_map(array( 'ArcanistUsageException' => 'exception/usage', 'ArcanistUserAbortException' => 'exception/usage/userabort', 'ArcanistWorkingCopyIdentity' => 'workingcopyidentity', + 'ArcanistXHPASTLintNamingHook' => 'lint/linter/xhpast/naminghook', 'ArcanistXHPASTLinter' => 'lint/linter/xhpast', 'ArcanistXHPASTLinterTestCase' => 'lint/linter/xhpast/__tests__', 'BranchInfo' => 'branch', diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index cf616f4c..09af5d15 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php @@ -665,27 +665,37 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } protected function lintNamingConventions($root) { + + // We're going to build up a list of tuples + // and then try to instantiate a hook class which has the opportunity to + // override us. + $names = array(); + $classes = $root->selectDescendantsOfType('n_CLASS_DECLARATION'); foreach ($classes as $class) { $name_token = $class->getChildByIndex(1); $name_string = $name_token->getConcreteString(); $is_xhp = ($name_string[0] == ':'); if ($is_xhp) { - if (!$this->isLowerCaseWithXHP($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: xhp elements should be named using '. - 'lower case.'); - } + $names[] = array( + 'xhp-class', + $name_string, + $name_token, + $this->isLowerCaseWithXHP($name_string) + ? null + : 'Follow naming conventions: XHP elements should be named using '. + 'lower case.', + ); } else { - if (!$this->isUpperCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: classes should be named using '. - 'UpperCamelCase.'); - } + $names[] = array( + 'class', + $name_string, + $name_token, + $this->isUpperCamelCase($name_string) + ? null + : 'Follow naming conventions: classes should be named using '. + 'UpperCamelCase.', + ); } } @@ -693,13 +703,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($ifaces as $iface) { $name_token = $iface->getChildByIndex(1); $name_string = $name_token->getConcreteString(); - if (!$this->isUpperCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: interfaces should be named using '. - 'UpperCamelCase.'); - } + $names[] = array( + 'interface', + $name_string, + $name_token, + $this->isUpperCamelCase($name_string) + ? null + : 'Follow naming conventions: interfaces should be named using '. + 'UpperCamelCase.', + ); } @@ -711,13 +723,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { continue; } $name_string = $name_token->getConcreteString(); - if (!$this->isLowercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: functions should be named using '. - 'lowercase_with_underscores.'); - } + $names[] = array( + 'function', + $name_string, + $name_token, + $this->isLowercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: functions should be named using '. + 'lowercase_with_underscores.', + ); } @@ -725,13 +739,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($methods as $method) { $name_token = $method->getChildByIndex(2); $name_string = $name_token->getConcreteString(); - if (!$this->isLowerCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: methods should be named using '. - 'lowerCamelCase.'); - } + $names[] = array( + 'method', + $name_string, + $name_token, + $this->isLowerCamelCase($name_string) + ? null + : 'Follow naming conventions: methods should be named using '. + 'lowerCamelCase.', + ); } @@ -740,13 +756,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($param_list->getChildren() as $param) { $name_token = $param->getChildByIndex(1); $name_string = $name_token->getConcreteString(); - if (!$this->isLowercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: parameters should be named using '. - 'lowercase_with_underscores.'); - } + $names[] = array( + 'parameter', + $name_string, + $name_token, + $this->isLowercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: parameters should be named using '. + 'lowercase_with_underscores.', + ); } } @@ -757,13 +775,15 @@ class ArcanistXHPASTLinter extends ArcanistLinter { foreach ($constant_list->getChildren() as $constant) { $name_token = $constant->getChildByIndex(0); $name_string = $name_token->getConcreteString(); - if (!$this->isUppercaseWithUnderscores($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: class constants should be named using '. - 'UPPERCASE_WITH_UNDERSCORES.'); - } + $names[] = array( + 'constant', + $name_string, + $name_token, + $this->isUppercaseWithUnderscores($name_string) + ? null + : 'Follow naming conventions: class constants should be named '. + 'using UPPERCASE_WITH_UNDERSCORES.', + ); } } @@ -775,15 +795,45 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } $name_token = $prop->getChildByIndex(0); $name_string = $name_token->getConcreteString(); - if (!$this->isLowerCamelCase($name_string)) { - $this->raiseLintAtNode( - $name_token, - self::LINT_NAMING_CONVENTIONS, - 'Follow naming conventions: class properties should be named '. - 'using lowerCamelCase.'); + $names[] = array( + 'member', + $name_string, + $name_token, + $this->isLowerCamelCase($name_string) + ? null + : 'Follow naming conventions: class properties should be named '. + 'using lowerCamelCase.', + ); + } + } + + $engine = $this->getEngine(); + $working_copy = $engine->getWorkingCopy(); + + if ($working_copy) { + // If a naming hook is configured, give it a chance to override the + // default results for all the symbol names. + $hook_class = $working_copy->getConfig('lint.xhpast.naminghook'); + if ($hook_class) { + $hook_obj = newv($hook_class, array()); + foreach ($names as $k => $name_attrs) { + list($type, $name, $token, $default) = $name_attrs; + $result = $hook_obj->lintSymbolName($type, $name, $default); + $names[$k][3] = $result; } } } + + // Raise anything we're left with. + foreach ($names as $k => $name_attrs) { + list($type, $name, $token, $result) = $name_attrs; + if ($result) { + $this->raiseLintAtNode( + $token, + self::LINT_NAMING_CONVENTIONS, + $result); + } + } } protected function isUpperCamelCase($str) { diff --git a/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php new file mode 100644 index 00000000..e526f555 --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/ArcanistXHPASTLintNamingHook.php @@ -0,0 +1,60 @@ + + } + + /** + * Callback invoked for each symbol, which can override the default + * determination of name validity or accept it by returning $default. The + * symbol types are: xhp-class, class, interface, function, method, parameter, + * constant, and member. + * + * For example, if you want to ban all symbols with "quack" in them and + * otherwise accept all the defaults, except allow any naming convention for + * methods with "duck" in them, you might implement the method like this: + * + * if (preg_match('/quack/i', $name)) { + * return 'Symbol names containing "quack" are forbidden.'; + * } + * if ($type == 'method' && preg_match('/duck/i', $name)) { + * return null; // Always accept. + * } + * return $default; + * + * @param string The symbol type. + * @param string The symbol name. + * @param string|null The default result from the main rule engine. + * @return string|null Null to accept the name, or a message to reject it + * with. You should return the default value if you don't + * want to specifically provide an override. + * @task override + */ + abstract public function lintSymbolName($type, $name, $default); + +} diff --git a/src/lint/linter/xhpast/naminghook/__init__.php b/src/lint/linter/xhpast/naminghook/__init__.php new file mode 100644 index 00000000..da02b025 --- /dev/null +++ b/src/lint/linter/xhpast/naminghook/__init__.php @@ -0,0 +1,10 @@ +