From efb8219196abf047f14b505959e54d078e1df6d3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 9 Jan 2011 20:40:13 -0800 Subject: [PATCH] Fix some problems with the module linter that would cause various conflicting warnings raised in __init__.php to overwrite eachother in uncomfortable ways. --- src/lint/engine/base/ArcanistLintEngine.php | 6 ++- src/lint/linter/base/ArcanistLinter.php | 7 ++- .../ArcanistPhutilModuleLinter.php | 49 ++++++++++++------- src/lint/message/ArcanistLintMessage.php | 20 ++++++++ src/lint/patcher/ArcanistLintPatcher.php | 5 +- src/lint/renderer/ArcanistLintRenderer.php | 3 +- src/lint/renderer/__init__.php | 1 + src/lint/result/ArcanistLintResult.php | 25 ++++++++-- src/workflow/base/ArcanistBaseWorkflow.php | 17 ++++--- src/workflow/lint/ArcanistLintWorkflow.php | 5 +- 10 files changed, 104 insertions(+), 34 deletions(-) diff --git a/src/lint/engine/base/ArcanistLintEngine.php b/src/lint/engine/base/ArcanistLintEngine.php index d2dbd56e..5a199d4b 100644 --- a/src/lint/engine/base/ArcanistLintEngine.php +++ b/src/lint/engine/base/ArcanistLintEngine.php @@ -68,7 +68,9 @@ abstract class ArcanistLintEngine { } public function getFilePathOnDisk($path) { - return $path; + return Filesystem::resolvePath( + $path, + $this->getWorkingCopy()->getProjectRoot()); } public function setMinimumSeverity($severity) { @@ -145,12 +147,12 @@ abstract class ArcanistLintEngine { } foreach ($this->results as $path => $result) { + $result->setFilePathOnDisk($this->getFilePathOnDisk($path)); if (isset($this->fileData[$path])) { // Only set the data if any linter loaded it. The goal here is to // avoid binaries when we don't actually care about their contents, // for performance. $result->setData($this->fileData[$path]); - $result->setFilePathOnDisk($this->getFilePathOnDisk($path)); } } diff --git a/src/lint/linter/base/ArcanistLinter.php b/src/lint/linter/base/ArcanistLinter.php index 5f841247..55c7c669 100644 --- a/src/lint/linter/base/ArcanistLinter.php +++ b/src/lint/linter/base/ArcanistLinter.php @@ -156,7 +156,12 @@ abstract class ArcanistLinter { $path = $this->getActivePath(); $engine = $this->getEngine(); - list($line, $char) = $engine->getLineAndCharFromOffset($path, $offset); + if ($offset === null) { + $line = null; + $char = null; + } else { + list($line, $char) = $engine->getLineAndCharFromOffset($path, $offset); + } return $this->raiseLintAtLine( $line + 1, diff --git a/src/lint/linter/phutilmodule/ArcanistPhutilModuleLinter.php b/src/lint/linter/phutilmodule/ArcanistPhutilModuleLinter.php index bde388bb..709b0fb3 100644 --- a/src/lint/linter/phutilmodule/ArcanistPhutilModuleLinter.php +++ b/src/lint/linter/phutilmodule/ArcanistPhutilModuleLinter.php @@ -136,12 +136,18 @@ class ArcanistPhutilModuleLinter extends ArcanistLinter { $bin = dirname($arc_root).'/scripts/phutil_analyzer.php'; $futures = array(); - foreach ($modules as $key) { + foreach ($modules as $mkey => $key) { $disk_path = $this->getModulePathOnDisk($key); - $futures[$key] = new ExecFuture( - '%s %s', - $bin, - $disk_path); + if (Filesystem::pathExists($disk_path)) { + $futures[$key] = new ExecFuture( + '%s %s', + $bin, + $disk_path); + } else { + // This can occur in git when you add a module in HEAD and then remove + // it in unstaged changes in the working copy. Just ignore it. + unset($modules[$mkey]); + } } $requirements = array(); @@ -340,19 +346,26 @@ class ArcanistPhutilModuleLinter extends ArcanistLinter { $need_functions, $drop_modules); $init_path = $this->getModulePathOnDisk($key).'/__init__.php'; - $init_path = Filesystem::readablePath($init_path); - if (file_exists($init_path)) { + $try_path = Filesystem::readablePath($init_path); + if (Filesystem::pathExists($try_path)) { + $init_path = $try_path; $old_file = Filesystem::readFile($init_path); - $this->willLintPath($init_path); - $message = $this->raiseLintAtOffset( - 0, - self::LINT_INIT_REBUILD, - "This regenerated phutil '__init__.php' file is suggested to ". - "address lint problems with static dependencies in the module.", - $old_file, - $new_file); - $message->setDependentMessages($resolvable); + } else { + $old_file = ''; } + $this->willLintPath($init_path); + $message = $this->raiseLintAtOffset( + null, + self::LINT_INIT_REBUILD, + "This generated phutil '__init__.php' file is suggested to address ". + "lint problems with static dependencies in the module.", + $old_file, + $new_file); + $message->setDependentMessages($resolvable); + foreach ($resolvable as $message) { + $message->setObsolete(true); + } + $message->setGenerateFile(true); } } @@ -456,7 +469,9 @@ EOHEADER; foreach ($places as $place) { list($file, $offset) = explode(':', $place); $this->willLintPath( - Filesystem::readablePath($this->getModulePathOnDisk($key).'/'.$file)); + Filesystem::readablePath( + $this->getModulePathOnDisk($key).'/'.$file, + $this->getEngine()->getWorkingCopy()->getProjectRoot())); return $this->raiseLintAtOffset( $offset, $code, diff --git a/src/lint/message/ArcanistLintMessage.php b/src/lint/message/ArcanistLintMessage.php index fd938786..6a8e1495 100644 --- a/src/lint/message/ArcanistLintMessage.php +++ b/src/lint/message/ArcanistLintMessage.php @@ -28,7 +28,9 @@ class ArcanistLintMessage { protected $originalText; protected $replacementText; protected $appliedToDisk; + protected $generateFile; protected $dependentMessages = array(); + protected $obsolete; public static function newFromDictionary(array $dict) { $message = new ArcanistLintMessage(); @@ -142,6 +144,24 @@ class ArcanistLintMessage { return ($this->getLine() !== null); } + public function setGenerateFile($generate_file) { + $this->generateFile = $generate_file; + return $this; + } + + public function getGenerateFile() { + return $this->generateFile; + } + + public function setObsolete($obsolete) { + $this->obsolete = $obsolete; + return $this; + } + + public function getObsolete() { + return $this->obsolete; + } + public function isPatchable() { return ($this->getReplacementText() !== null); } diff --git a/src/lint/patcher/ArcanistLintPatcher.php b/src/lint/patcher/ArcanistLintPatcher.php index a2ab8825..e8289845 100644 --- a/src/lint/patcher/ArcanistLintPatcher.php +++ b/src/lint/patcher/ArcanistLintPatcher.php @@ -53,7 +53,10 @@ final class ArcanistLintPatcher { // Copy existing file to preserve permissions. 'chmod --reference' is not // supported under OSX. - execx('cp -p %s %s', $path, $lint); + if (Filesystem::pathExists($path)) { + // This path may not exist if we're generating a new file. + execx('cp -p %s %s', $path, $lint); + } Filesystem::writeFile($lint, $data); list($err) = exec_manual("mv -f %s %s", $lint, $path); diff --git a/src/lint/renderer/ArcanistLintRenderer.php b/src/lint/renderer/ArcanistLintRenderer.php index 8f74f5d2..12510377 100644 --- a/src/lint/renderer/ArcanistLintRenderer.php +++ b/src/lint/renderer/ArcanistLintRenderer.php @@ -115,7 +115,8 @@ class ArcanistLintRenderer { for (; $cursor < $line_num + $text_length; $cursor++) { $chevron = ($cursor == $line_num); - $data = $line_data[$cursor - 1]; + // We may not have any data if, e.g., the old file does not exist. + $data = idx($line_data, $cursor - 1, null); // Highlight the problem substring. $text_line = $text_lines[$cursor - $line_num]; diff --git a/src/lint/renderer/__init__.php b/src/lint/renderer/__init__.php index c89f46b8..e8872d7c 100644 --- a/src/lint/renderer/__init__.php +++ b/src/lint/renderer/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('arcanist', 'lint/severity'); phutil_require_module('phutil', 'console'); +phutil_require_module('phutil', 'utils'); phutil_require_source('ArcanistLintRenderer.php'); diff --git a/src/lint/result/ArcanistLintResult.php b/src/lint/result/ArcanistLintResult.php index 5eb25ead..d33dee42 100644 --- a/src/lint/result/ArcanistLintResult.php +++ b/src/lint/result/ArcanistLintResult.php @@ -22,6 +22,7 @@ final class ArcanistLintResult { protected $data; protected $filePathOnDisk; protected $messages = array(); + protected $effectiveMessages = array(); private $needsSort; public function setPath($path) { @@ -41,9 +42,9 @@ final class ArcanistLintResult { public function getMessages() { if ($this->needsSort) { - $this->sortMessages(); + $this->sortAndFilterMessages(); } - return $this->messages; + return $this->effectiveMessages; } public function setData($data) { @@ -73,18 +74,32 @@ final class ArcanistLintResult { return false; } - private function sortMessages() { + private function sortAndFilterMessages() { $messages = $this->messages; + + foreach ($messages as $key => $message) { + if ($message->getObsolete()) { + unset($messages[$key]); + continue; + } + if ($message->getGenerateFile()) { + $messages = array( + $key => $message, + ); + break; + } + } + $map = array(); foreach ($messages as $key => $message) { $map[$key] = ($message->getLine() * (2 << 12)) + $message->getChar(); } asort($map); $messages = array_select_keys($messages, array_keys($map)); - $this->messages = $messages; + + $this->effectiveMessages = $messages; $this->needsSort = false; - return $this; } } diff --git a/src/workflow/base/ArcanistBaseWorkflow.php b/src/workflow/base/ArcanistBaseWorkflow.php index daf6c647..54bd74f7 100644 --- a/src/workflow/base/ArcanistBaseWorkflow.php +++ b/src/workflow/base/ArcanistBaseWorkflow.php @@ -478,12 +478,17 @@ class ArcanistBaseWorkflow { } if (empty($this->changeCache[$path])) { - // TODO: This can legitimately occur under git if you make a change, - // "git commit" it, and then revert the change in the working copy and - // run "arc lint". We should probably just make a dummy, empty changeset - // in this case, at least under git. - throw new Exception( - "Trying to get change for unchanged path '{$path}'!"); + if ($repository_api instanceof ArcanistGitAPI) { + // This can legitimately occur under git if you make a change, "git + // commit" it, and then revert the change in the working copy and run + // "arc lint". + $change = new ArcanistDiffChange(); + $change->setCurrentPath($path); + return $change; + } else { + throw new Exception( + "Trying to get change for unchanged path '{$path}'!"); + } } return $this->changeCache[$path]; diff --git a/src/workflow/lint/ArcanistLintWorkflow.php b/src/workflow/lint/ArcanistLintWorkflow.php index dc7c9301..e32d3426 100644 --- a/src/workflow/lint/ArcanistLintWorkflow.php +++ b/src/workflow/lint/ArcanistLintWorkflow.php @@ -168,6 +168,9 @@ EOTEXT if ($prompt_patches) { $old_file = $result->getFilePathOnDisk(); + if (!Filesystem::pathExists($old_file)) { + $old_file = '/dev/null'; + } $new_file = new TempFile(); Filesystem::writeFile($new_file, $new); @@ -193,7 +196,7 @@ EOTEXT "Amend HEAD with lint patches?", $default_no = false); if (!$amend) { - throw new ArcanistUsageException("Resolve lint changes and rediff."); + throw new ArcanistUsageException("Resolve lint changes and relint."); } execx( '(cd %s; git commit -a --amend -C HEAD)',