diff --git a/scripts/arcanist.php b/scripts/arcanist.php index c90b084a..6743f394 100755 --- a/scripts/arcanist.php +++ b/scripts/arcanist.php @@ -448,7 +448,8 @@ try { } if (!$is_usage) { - fwrite(STDERR, phutil_console_format("**%s**\n", pht('Exception'))); + fwrite(STDERR, phutil_console_format( + "** %s **\n", pht('Exception'))); while ($ex) { fwrite(STDERR, $ex->getMessage()."\n"); @@ -666,16 +667,33 @@ function arcanist_load_libraries( if ($ex->getLibrary() != 'arcanist') { throw $ex; } - $arc_dir = dirname(dirname(__FILE__)); - $error = pht( - "You are trying to run one copy of Arcanist on another copy of ". - "Arcanist. This operation is not supported. To execute Arcanist ". - "operations against this working copy, run `%s` (from the current ". - "working copy) not some other copy of '%s' (you ran one from '%s').", - './bin/arc', - 'arc', - $arc_dir); - throw new ArcanistUsageException($error); + + // NOTE: If you are running `arc` against itself, we ignore the library + // conflict created by loading the local `arc` library (in the current + // working directory) and continue without loading it. + + // This means we only execute code in the `arcanist/` directory which is + // associated with the binary you are running, whereas we would normally + // execute local code. + + // This can make `arc` development slightly confusing if your setup is + // especially bizarre, but it allows `arc` to be used in automation + // workflows more easily. For some context, see PHI13. + + $executing_directory = dirname(dirname(__FILE__)); + $working_directory = dirname($location); + + fwrite( + STDERR, + tsprintf( + "** %s ** %s\n", + pht('VERY META'), + pht( + 'You are running one copy of Arcanist (at path "%s") against '. + 'another copy of Arcanist (at path "%s"). Code in the current '. + 'working directory will not be loaded or executed.', + $executing_directory, + $working_directory))); } } } diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 73b8d638..9aeff71e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -100,6 +100,7 @@ phutil_register_library_map(array( 'ArcanistConfigurationDrivenUnitTestEngine' => 'unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php', 'ArcanistConfigurationManager' => 'configuration/ArcanistConfigurationManager.php', 'ArcanistConsoleLintRenderer' => 'lint/renderer/ArcanistConsoleLintRenderer.php', + 'ArcanistConsoleLintRendererTestCase' => 'lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistConstructorParenthesesXHPASTLinterRule.php', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistConstructorParenthesesXHPASTLinterRuleTestCase.php', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistControlStatementSpacingXHPASTLinterRule.php', @@ -238,6 +239,7 @@ phutil_register_library_map(array( 'ArcanistLibraryTestCase' => '__tests__/ArcanistLibraryTestCase.php', 'ArcanistLintEngine' => 'lint/engine/ArcanistLintEngine.php', 'ArcanistLintMessage' => 'lint/ArcanistLintMessage.php', + 'ArcanistLintMessageTestCase' => 'lint/__tests__/ArcanistLintMessageTestCase.php', 'ArcanistLintPatcher' => 'lint/ArcanistLintPatcher.php', 'ArcanistLintRenderer' => 'lint/renderer/ArcanistLintRenderer.php', 'ArcanistLintResult' => 'lint/ArcanistLintResult.php', @@ -287,6 +289,7 @@ phutil_register_library_map(array( 'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php', 'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php', 'ArcanistPHPCloseTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php', + 'ArcanistPHPCloseTagXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php', 'ArcanistPHPCompatibilityXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php', 'ArcanistPHPCompatibilityXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistPHPCompatibilityXHPASTLinterRuleTestCase.php', 'ArcanistPHPEchoTagXHPASTLinterRule' => 'lint/linter/xhpast/rules/ArcanistPHPEchoTagXHPASTLinterRule.php', @@ -543,6 +546,7 @@ phutil_register_library_map(array( 'ArcanistConfigurationDrivenUnitTestEngine' => 'ArcanistUnitTestEngine', 'ArcanistConfigurationManager' => 'Phobject', 'ArcanistConsoleLintRenderer' => 'ArcanistLintRenderer', + 'ArcanistConsoleLintRendererTestCase' => 'PhutilTestCase', 'ArcanistConstructorParenthesesXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistConstructorParenthesesXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistControlStatementSpacingXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', @@ -681,6 +685,7 @@ phutil_register_library_map(array( 'ArcanistLibraryTestCase' => 'PhutilLibraryTestCase', 'ArcanistLintEngine' => 'Phobject', 'ArcanistLintMessage' => 'Phobject', + 'ArcanistLintMessageTestCase' => 'PhutilTestCase', 'ArcanistLintPatcher' => 'Phobject', 'ArcanistLintRenderer' => 'Phobject', 'ArcanistLintResult' => 'Phobject', @@ -730,6 +735,7 @@ phutil_register_library_map(array( 'ArcanistPEP8Linter' => 'ArcanistExternalLinter', 'ArcanistPEP8LinterTestCase' => 'ArcanistExternalLinterTestCase', 'ArcanistPHPCloseTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', + 'ArcanistPHPCloseTagXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistPHPCompatibilityXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', 'ArcanistPHPCompatibilityXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase', 'ArcanistPHPEchoTagXHPASTLinterRule' => 'ArcanistXHPASTLinterRule', diff --git a/src/configuration/ArcanistSettings.php b/src/configuration/ArcanistSettings.php index 2ecd1abd..6e77b917 100644 --- a/src/configuration/ArcanistSettings.php +++ b/src/configuration/ArcanistSettings.php @@ -80,13 +80,6 @@ final class ArcanistSettings extends Phobject { 'arc land'), 'example' => '"develop"', ), - 'arc.land.update.default' => array( - 'type' => 'string', - 'help' => pht( - 'The default strategy to use when arc land updates the feature '. - 'branch. Supports "rebase" and "merge" strategies.'), - 'example' => '"rebase"', - ), 'arc.lint.cache' => array( 'type' => 'bool', 'help' => pht( diff --git a/src/hgdaemon/ArcanistHgServerChannel.php b/src/hgdaemon/ArcanistHgServerChannel.php index 1850c9af..22f6b4ab 100644 --- a/src/hgdaemon/ArcanistHgServerChannel.php +++ b/src/hgdaemon/ArcanistHgServerChannel.php @@ -54,6 +54,7 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel { private $mode = self::MODE_CHANNEL; private $byteLengthOfNextChunk = 1; private $buf = ''; + private $outputChannel; /* -( Protocol Implementation )-------------------------------------------- */ @@ -137,7 +138,7 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel { // 'output', 'error', 'result' or 'debug' respectively. This is a // single byte long. Next, we'll expect a length. - $this->channel = $chunk; + $this->outputChannel = $chunk; $this->byteLengthOfNextChunk = 4; $this->mode = self::MODE_LENGTH; break; @@ -153,11 +154,11 @@ final class ArcanistHgServerChannel extends PhutilProtocolChannel { // given length. We produce a message from the channel and the data // and return it. Next, we expect another channel name. - $message = array($this->channel, $chunk); + $message = array($this->outputChannel, $chunk); $this->byteLengthOfNextChunk = 1; $this->mode = self::MODE_CHANNEL; - $this->channel = null; + $this->outputChannel = null; $messages[] = $message; break; diff --git a/src/land/ArcanistGitLandEngine.php b/src/land/ArcanistGitLandEngine.php index 12b90548..6b0a38cd 100644 --- a/src/land/ArcanistGitLandEngine.php +++ b/src/land/ArcanistGitLandEngine.php @@ -59,7 +59,7 @@ final class ArcanistGitLandEngine public function __destruct() { if ($this->restoreWhenDestroyed) { - $this->writeWARN( + $this->writeWarn( pht('INTERRUPTED!'), pht('Restoring working copy to its original state.')); diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index bd60addc..c92c5ec8 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -40,7 +40,8 @@ final class ArcanistLintMessage extends Phobject { $message->setGranularity(idx($dict, 'granularity')); $message->setOtherLocations(idx($dict, 'locations', array())); if (isset($dict['bypassChangedLineFiltering'])) { - $message->bypassChangedLineFiltering($dict['bypassChangedLineFiltering']); + $message->setBypassChangedLineFiltering( + $dict['bypassChangedLineFiltering']); } return $message; } @@ -288,4 +289,83 @@ final class ArcanistLintMessage extends Phobject { return $value; } + public function newTrimmedMessage() { + if (!$this->isPatchable()) { + return clone $this; + } + + // If the original and replacement text have a similar prefix or suffix, + // we trim it to reduce the size of the diff we show to the user. + + $replacement = $this->getReplacementText(); + $original = $this->getOriginalText(); + + $replacement_length = strlen($replacement); + $original_length = strlen($original); + + $minimum_length = min($original_length, $replacement_length); + + $prefix_length = 0; + for ($ii = 0; $ii < $minimum_length; $ii++) { + if ($original[$ii] !== $replacement[$ii]) { + break; + } + $prefix_length++; + } + + // NOTE: The two strings can't be the same because the message won't be + // "patchable" if they are, so we don't need a special check for the case + // where the entire string is a shared prefix. + + // However, if the two strings are in the form "ABC" and "ABBC", we may + // find a prefix and a suffix with a combined length greater than the + // total size of the smaller string if we don't limit the search. + $max_suffix = ($minimum_length - $prefix_length); + + $suffix_length = 0; + for ($ii = 1; $ii <= $max_suffix; $ii++) { + $original_char = $original[$original_length - $ii]; + $replacement_char = $replacement[$replacement_length - $ii]; + if ($original_char !== $replacement_char) { + break; + } + $suffix_length++; + } + + if ($suffix_length) { + $original = substr($original, 0, -$suffix_length); + $replacement = substr($replacement, 0, -$suffix_length); + } + + $line = $this->getLine(); + $char = $this->getChar(); + + if ($prefix_length) { + $prefix = substr($original, 0, $prefix_length); + + // NOTE: Prior to PHP7, `substr("a", 1)` returned false instead of + // the empty string. Cast these to force the PHP7-ish behavior we + // expect. + $original = (string)substr($original, $prefix_length); + $replacement = (string)substr($replacement, $prefix_length); + + // If we've removed a prefix, we need to push the character and line + // number for the warning forward to account for the characters we threw + // away. + for ($ii = 0; $ii < $prefix_length; $ii++) { + $char++; + if ($prefix[$ii] == "\n") { + $line++; + $char = 1; + } + } + } + + return id(clone $this) + ->setOriginalText($original) + ->setReplacementText($replacement) + ->setLine($line) + ->setChar($char); + } + } diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php new file mode 100644 index 00000000..c27acd1b --- /dev/null +++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php @@ -0,0 +1,88 @@ + array( + 'old' => 'a', + 'new' => 'b', + 'old.expect' => 'a', + 'new.expect' => 'b', + 'line' => 1, + 'char' => 1, + ), + 'prefix' => array( + 'old' => 'ever after', + 'new' => 'evermore', + 'old.expect' => ' after', + 'new.expect' => 'more', + 'line' => 1, + 'char' => 5, + ), + 'suffix' => array( + 'old' => 'arcane archaeology', + 'new' => 'mythic archaeology', + 'old.expect' => 'arcane', + 'new.expect' => 'mythic', + 'line' => 1, + 'char' => 1, + ), + 'both' => array( + 'old' => 'large red apple', + 'new' => 'large blue apple', + 'old.expect' => 'red', + 'new.expect' => 'blue', + 'line' => 1, + 'char' => 7, + ), + 'prefix-newline' => array( + 'old' => "four score\nand five years ago", + 'new' => "four score\nand seven years ago", + 'old.expect' => 'five', + 'new.expect' => 'seven', + 'line' => 2, + 'char' => 5, + ), + 'mid-newline' => array( + 'old' => 'ABA', + 'new' => 'ABBA', + 'old.expect' => '', + 'new.expect' => 'B', + 'line' => 1, + 'char' => 3, + ), + ); + + foreach ($map as $key => $test_case) { + $message = id(new ArcanistLintMessage()) + ->setOriginalText($test_case['old']) + ->setReplacementText($test_case['new']) + ->setLine(1) + ->setChar(1); + + $actual = $message->newTrimmedMessage(); + + $this->assertEqual( + $test_case['old.expect'], + $actual->getOriginalText(), + pht('Original text for "%s".', $key)); + + $this->assertEqual( + $test_case['new.expect'], + $actual->getReplacementText(), + pht('Replacement text for "%s".', $key)); + + $this->assertEqual( + $test_case['line'], + $actual->getLine(), + pht('Line for "%s".', $key)); + + $this->assertEqual( + $test_case['char'], + $actual->getChar(), + pht('Char for "%s".', $key)); + } + } +} diff --git a/src/lint/linter/ArcanistBaseXHPASTLinter.php b/src/lint/linter/ArcanistBaseXHPASTLinter.php index 72a9bd7e..f0f44827 100644 --- a/src/lint/linter/ArcanistBaseXHPASTLinter.php +++ b/src/lint/linter/ArcanistBaseXHPASTLinter.php @@ -189,7 +189,7 @@ abstract class ArcanistBaseXHPASTLinter extends ArcanistFutureLinter { * Get a path's parse exception from the responsible linter. * * @param string Path to retrieve exception for. - * @return Exeption|null Parse exception, if available. + * @return Exception|null Parse exception, if available. * @task sharing */ final protected function getXHPASTExceptionForPath($path) { diff --git a/src/lint/linter/ArcanistFlake8Linter.php b/src/lint/linter/ArcanistFlake8Linter.php index 19d57432..37fb45e1 100644 --- a/src/lint/linter/ArcanistFlake8Linter.php +++ b/src/lint/linter/ArcanistFlake8Linter.php @@ -51,12 +51,14 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { protected function parseLinterOutput($path, $err, $stdout, $stderr) { $lines = phutil_split_lines($stdout, false); + // stdin:2: W802 undefined name 'foo' # pyflakes + // stdin:3:1: E302 expected 2 blank lines, found 1 # pep8 + $regexp = + '/^(?:.*?):(?P\d+):(?:(?P\d+):)? (?P\S+) (?P.*)$/'; + $messages = array(); foreach ($lines as $line) { $matches = null; - // stdin:2: W802 undefined name 'foo' # pyflakes - // stdin:3:1: E302 expected 2 blank lines, found 1 # pep8 - $regexp = '/^(.*?):(\d+):(?:(\d+):)? (\S+) (.*)$/'; if (!preg_match($regexp, $line, $matches)) { continue; } @@ -66,14 +68,14 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter { $message = new ArcanistLintMessage(); $message->setPath($path); - $message->setLine($matches[2]); - if (!empty($matches[3])) { - $message->setChar($matches[3]); + $message->setLine($matches['line']); + if (!empty($matches['char'])) { + $message->setChar($matches['char']); } - $message->setCode($matches[4]); - $message->setName($this->getLinterName().' '.$matches[3]); - $message->setDescription($matches[5]); - $message->setSeverity($this->getLintMessageSeverity($matches[4])); + $message->setCode($matches['code']); + $message->setName($this->getLinterName().' '.$matches['code']); + $message->setDescription($matches['msg']); + $message->setSeverity($this->getLintMessageSeverity($matches['code'])); $messages[] = $message; } diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php index 2f81b039..c7c4af52 100644 --- a/src/lint/linter/ArcanistTextLinter.php +++ b/src/lint/linter/ArcanistTextLinter.php @@ -245,7 +245,7 @@ final class ArcanistTextLinter extends ArcanistLinter { $matches = null; $preg = preg_match_all( - '/ +$/m', + '/[[:blank:]]+$/m', $data, $matches, PREG_OFFSET_CAPTURE); @@ -311,9 +311,7 @@ final class ArcanistTextLinter extends ArcanistLinter { $this->raiseLintAtOffset( $offset, self::LINT_EOF_WHITESPACE, - pht( - 'This file contains trailing whitespace at the end of the file. '. - 'This is unnecessary and should be avoided when possible.'), + pht('This file contains unnecessary trailing whitespace.'), $string, ''); } diff --git a/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php b/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php index 50c01ca5..f02dc21b 100644 --- a/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php +++ b/src/lint/linter/__tests__/ArcanistClosureLinterTestCase.php @@ -3,11 +3,14 @@ final class ArcanistClosureLinterTestCase extends ArcanistExternalLinterTestCase { - public function testLinter() { + protected function getLinter() { $linter = new ArcanistClosureLinter(); $linter->setFlags(array('--additional_extensions=lint-test')); + return $linter; + } - $this->executeTestsInDirectory(dirname(__FILE__).'/gjslint/', $linter); + public function testLinter() { + $this->executeTestsInDirectory(dirname(__FILE__).'/gjslint/'); } } diff --git a/src/lint/linter/__tests__/text/trailing-whitespace.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-1.lint-test similarity index 100% rename from src/lint/linter/__tests__/text/trailing-whitespace.lint-test rename to src/lint/linter/__tests__/text/trailing-whitespace-1.lint-test diff --git a/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test new file mode 100644 index 00000000..0863c187 --- /dev/null +++ b/src/lint/linter/__tests__/text/trailing-whitespace-2.lint-test @@ -0,0 +1,15 @@ +Lorem ipsum dolor sit amet, +consectetur adipiscing elit. +Phasellus sodales nibh erat, +in hendrerit nulla dictum interdum. +~~~~~~~~~~ +error:1:28 +autofix:1:28 +autofix:2:29 +autofix:3:29 +autofix:4:36 +~~~~~~~~~~ +Lorem ipsum dolor sit amet, +consectetur adipiscing elit. +Phasellus sodales nibh erat, +in hendrerit nulla dictum interdum. diff --git a/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test b/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test new file mode 100644 index 00000000..e92f7bbd --- /dev/null +++ b/src/lint/linter/__tests__/text/trailing-whitespace-3.lint-test @@ -0,0 +1,15 @@ +Lorem ipsum dolor sit amet, +consectetur adipiscing elit. +Phasellus sodales nibh erat, +in hendrerit nulla dictum interdum. +~~~~~~~~~~ +error:1:28 +autofix:1:28 +autofix:2:29 +autofix:3:29 +autofix:4:36 +~~~~~~~~~~ +Lorem ipsum dolor sit amet, +consectetur adipiscing elit. +Phasellus sodales nibh erat, +in hendrerit nulla dictum interdum. diff --git a/src/lint/linter/__tests__/xhpast/empty.lint-test b/src/lint/linter/__tests__/xhpast/empty.lint-test index 57595708..ddbbaebd 100644 --- a/src/lint/linter/__tests__/xhpast/empty.lint-test +++ b/src/lint/linter/__tests__/xhpast/empty.lint-test @@ -1,3 +1,4 @@ - +oooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo -~~~~~~~~~~ -error:1:2 -error:2:1 diff --git a/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php b/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php index 17547476..77560d11 100644 --- a/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php +++ b/src/lint/linter/standards/phutil/ArcanistPhutilXHPASTLinterStandard.php @@ -56,12 +56,14 @@ final class ArcanistPhutilXHPASTLinterStandard } public function getLinterSeverityMap() { - $advice = ArcanistLintSeverity::SEVERITY_ADVICE; - $error = ArcanistLintSeverity::SEVERITY_ERROR; + $advice = ArcanistLintSeverity::SEVERITY_ADVICE; + $error = ArcanistLintSeverity::SEVERITY_ERROR; + $warning = ArcanistLintSeverity::SEVERITY_WARNING; return array( - ArcanistTodoCommentXHPASTLinterRule::ID => $advice, - ArcanistCommentSpacingXHPASTLinterRule::ID => $error, + ArcanistTodoCommentXHPASTLinterRule::ID => $advice, + ArcanistCommentSpacingXHPASTLinterRule::ID => $error, + ArcanistRaggedClassTreeEdgeXHPASTLinterRule::ID => $warning, ); } diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php index 95a9918f..8321a27e 100644 --- a/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistPHPCloseTagXHPASTLinterRule.php @@ -12,7 +12,7 @@ final class ArcanistPHPCloseTagXHPASTLinterRule public function process(XHPASTNode $root) { $inline_html = $root->selectDescendantsOfType('n_INLINE_HTML'); - if ($inline_html) { + if (count($inline_html) > 0) { return; } diff --git a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php index c7790eb2..d73d1fae 100644 --- a/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistPHPCompatibilityXHPASTLinterRule.php @@ -369,7 +369,7 @@ final class ArcanistPHPCompatibilityXHPASTLinterRule } } - $literals = $root->selectDescendantsOftype('n_ARRAY_LITERAL'); + $literals = $root->selectDescendantsOfType('n_ARRAY_LITERAL'); foreach ($literals as $literal) { $open_token = head($literal->getTokens())->getValue(); if ($open_token == '[') { diff --git a/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php b/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php new file mode 100644 index 00000000..b7c2067e --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/ArcanistPHPCloseTagXHPASTLinterRuleTestCase.php @@ -0,0 +1,10 @@ +executeTestsInDirectory(dirname(__FILE__).'/php-close-tag/'); + } + +} diff --git a/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test new file mode 100644 index 00000000..826beb54 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag-inline-html-good.lint-test @@ -0,0 +1,6 @@ + +stuff. + +~~~~~~~~~~ diff --git a/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test new file mode 100644 index 00000000..54fb1fc5 --- /dev/null +++ b/src/lint/linter/xhpast/rules/__tests__/php-close-tag/php-close-tag.lint-test @@ -0,0 +1,5 @@ + +~~~~~~~~~~ +error:3:1 diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 6f3476b0..ea233e3d 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -6,20 +6,30 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { private $showAutofixPatches = false; + private $testableMode; public function setShowAutofixPatches($show_autofix_patches) { $this->showAutofixPatches = $show_autofix_patches; return $this; } + public function setTestableMode($testable_mode) { + $this->testableMode = $testable_mode; + return $this; + } + + public function getTestableMode() { + return $this->testableMode; + } + public function renderLintResult(ArcanistLintResult $result) { $messages = $result->getMessages(); $path = $result->getPath(); + $data = $result->getData(); - $lines = explode("\n", $result->getData()); + $line_map = $this->newOffsetMap($data); $text = array(); - foreach ($messages as $message) { if (!$this->showAutofixPatches && $message->isAutofix()) { continue; @@ -57,7 +67,7 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { phutil_console_wrap($description, 4)); if ($message->hasFileContext()) { - $text[] = $this->renderContext($message, $lines); + $text[] = $this->renderContext($message, $data, $line_map); } } @@ -75,153 +85,183 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { protected function renderContext( ArcanistLintMessage $message, - array $line_data) { + $data, + array $line_map) { + + $context = 3; + + $message = $message->newTrimmedMessage(); + + $original = $message->getOriginalText(); + $replacement = $message->getReplacementText(); + + $line = $message->getLine(); + $char = $message->getChar(); + + $old = $data; + $old_lines = phutil_split_lines($old); + $old_impact = substr_count($original, "\n") + 1; + $start = $line; + + if ($message->isPatchable()) { + $patch_offset = $line_map[$line] + ($char - 1); + + $new = substr_replace( + $old, + $replacement, + $patch_offset, + strlen($original)); + $new_lines = phutil_split_lines($new); + + // Figure out how many "-" and "+" lines we have by counting the newlines + // for the relevant patches. This may overestimate things if we are adding + // or removing entire lines, but we'll adjust things below. + $new_impact = substr_count($replacement, "\n") + 1; + + // If this is a change on a single line, we'll try to highlight the + // changed character range to make it easier to pick out. + if ($old_impact === 1 && $new_impact === 1) { + $old_lines[$start - 1] = substr_replace( + $old_lines[$start - 1], + $this->highlightText($original), + $char - 1, + strlen($original)); + + $new_lines[$start - 1] = substr_replace( + $new_lines[$start - 1], + $this->highlightText($replacement), + $char - 1, + strlen($replacement)); + } + + // If lines at the beginning of the changed line range are actually the + // same, shrink the range. This happens when a patch just adds a line. + do { + $old_line = idx($old_lines, $start - 1, null); + $new_line = idx($new_lines, $start - 1, null); + + if ($old_line !== $new_line) { + break; + } + + $start++; + $old_impact--; + $new_impact--; + + if ($old_impact < 0 || $new_impact < 0) { + throw new Exception( + pht( + 'Modified prefix line range has become negative '. + '(old = %d, new = %d).', + $old_impact, + $new_impact)); + } + } while (true); + + // If the lines at the end of the changed line range are actually the + // same, shrink the range. This happens when a patch just removes a + // line. + do { + $old_suffix = idx($old_lines, $start + $old_impact - 2, null); + $new_suffix = idx($new_lines, $start + $new_impact - 2, null); + + if ($old_suffix !== $new_suffix) { + break; + } + + $old_impact--; + $new_impact--; + + // We can end up here if a patch removes a line which occurs after + // another identical line. + if ($old_impact <= 0 || $new_impact <= 0) { + break; + } + } while (true); + + } else { + + // If we have "original" text and it is contained on a single line, + // highlight the affected area. If we don't have any text, we'll mark + // the character with a caret (below, in rendering) instead. + if ($old_impact == 1 && strlen($original)) { + $old_lines[$start - 1] = substr_replace( + $old_lines[$start - 1], + $this->highlightText($original), + $char - 1, + strlen($original)); + } + + $old_impact = 0; + $new_impact = 0; + } - $lines_of_context = 3; $out = array(); - $num_lines = count($line_data); - // make line numbers line up with array indexes - array_unshift($line_data, ''); - - $line_num = min($message->getLine(), $num_lines); - $line_num = max(1, $line_num); - - // Print out preceding context before the impacted region. - $cursor = max(1, $line_num - $lines_of_context); - for (; $cursor < $line_num; $cursor++) { - $out[] = $this->renderLine($cursor, $line_data[$cursor]); + $head = max(1, $start - $context); + for ($ii = $head; $ii < $start; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + ); } - $text = $message->getOriginalText(); - $start = $message->getChar() - 1; - $patch = ''; - // Refine original and replacement text to eliminate start and end in common - if ($message->isPatchable()) { - $patch = $message->getReplacementText(); - $text_strlen = strlen($text); - $patch_strlen = strlen($patch); - $min_length = min($text_strlen, $patch_strlen); - - $same_at_front = 0; - for ($ii = 0; $ii < $min_length; $ii++) { - if ($text[$ii] !== $patch[$ii]) { - break; - } - $same_at_front++; - $start++; - if ($text[$ii] == "\n") { - $out[] = $this->renderLine($cursor, $line_data[$cursor]); - $cursor++; - $start = 0; - $line_num++; - } - } - // deal with shorter string ' ' longer string ' a ' - $min_length -= $same_at_front; - - // And check the end of the string - $same_at_end = 0; - for ($ii = 1; $ii <= $min_length; $ii++) { - if ($text[$text_strlen - $ii] !== $patch[$patch_strlen - $ii]) { - break; - } - $same_at_end++; - } - - $text = substr( - $text, - $same_at_front, - $text_strlen - $same_at_end - $same_at_front); - $patch = substr( - $patch, - $same_at_front, - $patch_strlen - $same_at_end - $same_at_front); - } - // Print out the impacted region itself. - $diff = $message->isPatchable() ? '-' : null; - - $text_lines = explode("\n", $text); - $text_length = count($text_lines); - - $intraline = ($text != '' || $start || !preg_match('/\n$/', $patch)); - - if ($intraline) { - for (; $cursor < $line_num + $text_length; $cursor++) { - $chevron = ($cursor == $line_num); - // We may not have any data if, e.g., the old file does not exist. - $data = idx($line_data, $cursor, null); - - // Highlight the problem substring. - $text_line = $text_lines[$cursor - $line_num]; - if (strlen($text_line)) { - $data = substr_replace( - $data, - phutil_console_format('##%s##', $text_line), - ($cursor == $line_num ? ($start > 0 ? $start : null) : 0), - strlen($text_line)); - } - - $out[] = $this->renderLine($cursor, $data, $chevron, $diff); - } + for ($ii = $start; $ii < $start + $old_impact; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + 'type' => '-', + 'chevron' => ($ii == $start), + ); } - // Print out replacement text. - if ($message->isPatchable()) { - // Strip trailing newlines, since "explode" will create an extra patch - // line for these. - if (strlen($patch) && ($patch[strlen($patch) - 1] === "\n")) { - $patch = substr($patch, 0, -1); - } - $patch_lines = explode("\n", $patch); - $patch_length = count($patch_lines); - - $patch_line = $patch_lines[0]; - - $len = isset($text_lines[0]) ? strlen($text_lines[0]) : 0; - - $patched = phutil_console_format('##%s##', $patch_line); - - if ($intraline) { - $patched = substr_replace( - $line_data[$line_num], - $patched, - $start, - $len); - } - - $out[] = $this->renderLine(null, $patched, false, '+'); - - foreach (array_slice($patch_lines, 1) as $patch_line) { - $out[] = $this->renderLine( - null, - phutil_console_format('##%s##', $patch_line), false, '+'); - } + for ($ii = $start; $ii < $start + $new_impact; $ii++) { + $out[] = array( + 'text' => $new_lines[$ii - 1], + 'type' => '+', + 'chevron' => ($ii == $start), + ); } - $end = min($num_lines, $cursor + $lines_of_context); - for (; $cursor < $end; $cursor++) { - // If there is no original text, we didn't print out a chevron or any - // highlighted text above, so print it out here. This allows messages - // which don't have any original/replacement information to still - // render with indicator chevrons. - if ($text || $message->isPatchable()) { + $cursor = $start + $old_impact; + $foot = min(count($old_lines), $cursor + $context); + for ($ii = $cursor; $ii <= $foot; $ii++) { + $out[] = array( + 'text' => $old_lines[$ii - 1], + 'number' => $ii, + 'chevron' => ($ii == $cursor), + ); + } + + $result = array(); + + $seen_chevron = false; + foreach ($out as $spec) { + if ($seen_chevron) { $chevron = false; } else { - $chevron = ($cursor == $line_num); + $chevron = !empty($spec['chevron']); + if ($chevron) { + $seen_chevron = true; + } } - $out[] = $this->renderLine($cursor, $line_data[$cursor], $chevron); - // With original text, we'll render the text highlighted above. If the - // lint message only has a line/char offset there's nothing to - // highlight, so print out a caret on the next line instead. - if ($chevron && $message->getChar()) { - $out[] = $this->renderCaret($message->getChar()); + $result[] = $this->renderLine( + idx($spec, 'number'), + $spec['text'], + $chevron, + idx($spec, 'type')); + + // If this is just a message and does not have a patch, put a little + // caret underneath the line to point out where the issue is. + if ($chevron) { + if (!$message->isPatchable() && !strlen($original)) { + $result[] = $this->renderCaret($char)."\n"; + } } } - $out[] = null; - return implode("\n", $out); + return implode('', $result); } private function renderCaret($pos) { @@ -245,4 +285,28 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { pht('No lint warnings.')); } + private function newOffsetMap($data) { + $lines = phutil_split_lines($data); + + $line_map = array(); + + $number = 1; + $offset = 0; + foreach ($lines as $line) { + $line_map[$number] = $offset; + $number++; + $offset += strlen($line); + } + + return $line_map; + } + + private function highlightText($text) { + if ($this->getTestableMode()) { + return '>'.$text.'<'; + } else { + return (string)tsprintf('##%s##', $text); + } + } + } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php new file mode 100644 index 00000000..9014288b --- /dev/null +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -0,0 +1,200 @@ + array( + 'line' => 1, + 'char' => 1, + 'original' => 'a', + 'replacement' => 'z', + ), + 'inline' => array( + 'line' => 1, + 'char' => 7, + 'original' => 'cat', + 'replacement' => 'dog', + ), + + // In this test, the original and replacement texts have a large + // amount of overlap. + 'overlap' => array( + 'line' => 1, + 'char' => 1, + 'original' => 'tantawount', + 'replacement' => 'tantamount', + ), + + 'newline' => array( + 'line' => 6, + 'char' => 1, + 'original' => "\n", + 'replacement' => '', + ), + + 'addline' => array( + 'line' => 3, + 'char' => 1, + 'original' => '', + 'replacement' => "cherry\n", + ), + + 'addlinesuffix' => array( + 'line' => 2, + 'char' => 7, + 'original' => '', + 'replacement' => "\ncherry", + ), + + 'xml' => array( + 'line' => 3, + 'char' => 6, + 'original' => '', + 'replacement' => "\n", + ), + + 'caret' => array( + 'line' => 2, + 'char' => 13, + 'name' => 'Fruit Misinformation', + 'description' => 'Arguably untrue.', + ), + + 'original' => array( + 'line' => 1, + 'char' => 4, + 'original' => 'should of', + ), + + 'midline' => array( + 'line' => 1, + 'char' => 1, + 'original' => $midline_original, + 'replacement' => $midline_replacement, + ), + + 'remline' => array( + 'line' => 1, + 'char' => 1, + 'original' => $remline_original, + 'replacement' => $remline_replacement, + ), + + 'extrawhitespace' => array( + 'line' => 2, + 'char' => 1, + 'original' => "\n", + 'replacement' => '', + ), + ); + + $defaults = array( + 'severity' => ArcanistLintSeverity::SEVERITY_WARNING, + 'name' => 'Lint Warning', + 'path' => 'path/to/example.c', + 'description' => 'Consider this.', + 'code' => 'WARN123', + ); + + foreach ($map as $key => $test_case) { + $data = $this->readTestData("{$key}.txt"); + $data = preg_replace('/~+\s*$/m', '', $data); + + $expect = $this->readTestData("{$key}.expect"); + + $test_case = $test_case + $defaults; + + $path = $test_case['path']; + $severity = $test_case['severity']; + $name = $test_case['name']; + $description = $test_case['description']; + $code = $test_case['code']; + + $line = $test_case['line']; + $char = $test_case['char']; + + $original = idx($test_case, 'original'); + $replacement = idx($test_case, 'replacement'); + + $message = id(new ArcanistLintMessage()) + ->setPath($path) + ->setSeverity($severity) + ->setName($name) + ->setDescription($description) + ->setCode($code) + ->setLine($line) + ->setChar($char) + ->setOriginalText($original) + ->setReplacementText($replacement); + + $result = id(new ArcanistLintResult()) + ->setPath($path) + ->setData($data) + ->addMessage($message); + + $renderer = id(new ArcanistConsoleLintRenderer()) + ->setTestableMode(true); + + try { + PhutilConsoleFormatter::disableANSI(true); + $actual = $renderer->renderLintResult($result); + PhutilConsoleFormatter::disableANSI(false); + } catch (Exception $ex) { + PhutilConsoleFormatter::disableANSI(false); + throw $ex; + } + + // Trim "~" off the ends of lines. This allows the "expect" file to test + // for trailing whitespace without actually containing trailing + // whitespace. + $expect = preg_replace('/~+$/m', '', $expect); + + $this->assertEqual( + $expect, + $actual, + pht( + 'Lint rendering for "%s".', + $key)); + } + } + + private function readTestData($filename) { + $path = dirname(__FILE__).'/data/'.$filename; + return Filesystem::readFile($path); + } + +} diff --git a/src/lint/renderer/__tests__/data/addline.expect b/src/lint/renderer/__tests__/data/addline.expect new file mode 100644 index 00000000..44aa945c --- /dev/null +++ b/src/lint/renderer/__tests__/data/addline.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 apple + 2 banana + >>> + cherry + 3 date + 4 eclaire + 5 fig diff --git a/src/lint/renderer/__tests__/data/addline.txt b/src/lint/renderer/__tests__/data/addline.txt new file mode 100644 index 00000000..07147513 --- /dev/null +++ b/src/lint/renderer/__tests__/data/addline.txt @@ -0,0 +1,5 @@ +apple +banana +date +eclaire +fig diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.expect b/src/lint/renderer/__tests__/data/addlinesuffix.expect new file mode 100644 index 00000000..44aa945c --- /dev/null +++ b/src/lint/renderer/__tests__/data/addlinesuffix.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 apple + 2 banana + >>> + cherry + 3 date + 4 eclaire + 5 fig diff --git a/src/lint/renderer/__tests__/data/addlinesuffix.txt b/src/lint/renderer/__tests__/data/addlinesuffix.txt new file mode 100644 index 00000000..07147513 --- /dev/null +++ b/src/lint/renderer/__tests__/data/addlinesuffix.txt @@ -0,0 +1,5 @@ +apple +banana +date +eclaire +fig diff --git a/src/lint/renderer/__tests__/data/caret.expect b/src/lint/renderer/__tests__/data/caret.expect new file mode 100644 index 00000000..1bb26faa --- /dev/null +++ b/src/lint/renderer/__tests__/data/caret.expect @@ -0,0 +1,11 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Fruit Misinformation + Arguably untrue. + + 1 Apples are round. + >>> 2 Bananas are round. + ^ + 3 Cherries are round. + 4 Dates are round. diff --git a/src/lint/renderer/__tests__/data/caret.txt b/src/lint/renderer/__tests__/data/caret.txt new file mode 100644 index 00000000..a36a81cf --- /dev/null +++ b/src/lint/renderer/__tests__/data/caret.txt @@ -0,0 +1,4 @@ +Apples are round. +Bananas are round. +Cherries are round. +Dates are round. diff --git a/src/lint/renderer/__tests__/data/extrawhitespace.expect b/src/lint/renderer/__tests__/data/extrawhitespace.expect new file mode 100644 index 00000000..f083852d --- /dev/null +++ b/src/lint/renderer/__tests__/data/extrawhitespace.expect @@ -0,0 +1,8 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 Adrift upon the sea. + >>> - 2 ~ diff --git a/src/lint/renderer/__tests__/data/extrawhitespace.txt b/src/lint/renderer/__tests__/data/extrawhitespace.txt new file mode 100644 index 00000000..ba587d4c --- /dev/null +++ b/src/lint/renderer/__tests__/data/extrawhitespace.txt @@ -0,0 +1,3 @@ +Adrift upon the sea. + +~ diff --git a/src/lint/renderer/__tests__/data/inline.expect b/src/lint/renderer/__tests__/data/inline.expect new file mode 100644 index 00000000..d1d63fda --- /dev/null +++ b/src/lint/renderer/__tests__/data/inline.expect @@ -0,0 +1,8 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 adjudi>catdog>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 import apple; + 2 import banana; + >>> + ~ + 3 import cat; + 4 import dog; diff --git a/src/lint/renderer/__tests__/data/midline.txt b/src/lint/renderer/__tests__/data/midline.txt new file mode 100644 index 00000000..8639daf5 --- /dev/null +++ b/src/lint/renderer/__tests__/data/midline.txt @@ -0,0 +1,4 @@ +import apple; +import banana; +import cat; +import dog; diff --git a/src/lint/renderer/__tests__/data/newline.expect b/src/lint/renderer/__tests__/data/newline.expect new file mode 100644 index 00000000..0533707c --- /dev/null +++ b/src/lint/renderer/__tests__/data/newline.expect @@ -0,0 +1,14 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 3 ccc + 4 ddd + 5 eee + >>> - 6 ~ + 7 fff + 8 ggg + 9 hhh + 10 iii diff --git a/src/lint/renderer/__tests__/data/newline.txt b/src/lint/renderer/__tests__/data/newline.txt new file mode 100644 index 00000000..830c73a1 --- /dev/null +++ b/src/lint/renderer/__tests__/data/newline.txt @@ -0,0 +1,11 @@ +aaa +bbb +ccc +ddd +eee + +fff +ggg +hhh +iii +jjj diff --git a/src/lint/renderer/__tests__/data/original.expect b/src/lint/renderer/__tests__/data/original.expect new file mode 100644 index 00000000..f3bec302 --- /dev/null +++ b/src/lint/renderer/__tests__/data/original.expect @@ -0,0 +1,7 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> 1 He >should of< known. diff --git a/src/lint/renderer/__tests__/data/original.txt b/src/lint/renderer/__tests__/data/original.txt new file mode 100644 index 00000000..090fe16d --- /dev/null +++ b/src/lint/renderer/__tests__/data/original.txt @@ -0,0 +1 @@ +He should of known. diff --git a/src/lint/renderer/__tests__/data/overlap.expect b/src/lint/renderer/__tests__/data/overlap.expect new file mode 100644 index 00000000..d7756f3d --- /dev/null +++ b/src/lint/renderer/__tests__/data/overlap.expect @@ -0,0 +1,8 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 tanta>wm>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 import apple; + 2 import banana; + 3 ~ + >>> - 4 ~ + 5 import cat; + 6 import dog; diff --git a/src/lint/renderer/__tests__/data/remline.txt b/src/lint/renderer/__tests__/data/remline.txt new file mode 100644 index 00000000..4e526abe --- /dev/null +++ b/src/lint/renderer/__tests__/data/remline.txt @@ -0,0 +1,6 @@ +import apple; +import banana; + + +import cat; +import dog; diff --git a/src/lint/renderer/__tests__/data/simple.expect b/src/lint/renderer/__tests__/data/simple.expect new file mode 100644 index 00000000..c363ba07 --- /dev/null +++ b/src/lint/renderer/__tests__/data/simple.expect @@ -0,0 +1,10 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 >a< + + >z< + 2 b + 3 c diff --git a/src/lint/renderer/__tests__/data/simple.txt b/src/lint/renderer/__tests__/data/simple.txt new file mode 100644 index 00000000..de980441 --- /dev/null +++ b/src/lint/renderer/__tests__/data/simple.txt @@ -0,0 +1,3 @@ +a +b +c diff --git a/src/lint/renderer/__tests__/data/xml.expect b/src/lint/renderer/__tests__/data/xml.expect new file mode 100644 index 00000000..767ea655 --- /dev/null +++ b/src/lint/renderer/__tests__/data/xml.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 1 < + 2 wow + >>> - 3 xml> + + xml + + > + 4 diff --git a/src/lint/renderer/__tests__/data/xml.txt b/src/lint/renderer/__tests__/data/xml.txt new file mode 100644 index 00000000..136a688e --- /dev/null +++ b/src/lint/renderer/__tests__/data/xml.txt @@ -0,0 +1,4 @@ +< + wow + xml> + diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index 8bb51c44..2422fdfe 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -446,6 +446,9 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // would ship up the binaries for 'arc patch' but display the textconv // output in the visual diff. '--no-textconv', + // Provide a standard view of submodule changes; the 'log' and 'diff' + // values do not parse by the diff parser. + '--submodule=short', ); return implode(' ', $options); } @@ -544,8 +547,10 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { } $uri = rtrim($stdout); - // 'origin' is what ls-remote outputs if no origin remote URI exists - if (!$uri || $uri === 'origin') { + // ls-remote echos the remote name (ie 'origin') if no remote URI is found + // TODO: In 2.7.0 (circa 2016) git introduced `git remote get-url` + // with saner error handling. + if (!$uri || $uri === $remote) { return null; } @@ -1422,7 +1427,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { * or cycle locally. * * @param string Ref to start from. - * @return list Path to an upstream. + * @return ArcanistGitUpstreamPath Path to an upstream. */ public function getPathToUpstream($start) { $cursor = $start; diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php index 8517c9a2..8e9ea7e7 100644 --- a/src/repository/api/ArcanistMercurialAPI.php +++ b/src/repository/api/ArcanistMercurialAPI.php @@ -804,7 +804,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { 'commit --amend -l %s', $tmp_file); } catch (CommandException $ex) { - if (preg_match('/nothing changed/', $ex->getStdOut())) { + if (preg_match('/nothing changed/', $ex->getStdout())) { // NOTE: Mercurial considers it an error to make a no-op amend. Although // we generally defer to the underlying VCS to dictate behavior, this // one seems a little goofy, and we use amend as part of various diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php index 4501f8ff..e8866ecb 100644 --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -282,7 +282,7 @@ abstract class ArcanistRepositoryAPI extends Phobject { * Hook for implementations to dirty working copy caches after the working * copy has been updated. * - * @return this + * @return void * @task status */ protected function didReloadWorkingCopy() { diff --git a/src/unit/engine/XUnitTestEngine.php b/src/unit/engine/XUnitTestEngine.php index 803880af..d6a20464 100644 --- a/src/unit/engine/XUnitTestEngine.php +++ b/src/unit/engine/XUnitTestEngine.php @@ -255,7 +255,7 @@ class XUnitTestEngine extends ArcanistUnitTestEngine { throw $exc; } $result->setResult(ArcanistUnitTestResult::RESULT_FAIL); - $result->setUserdata($exc->getStdout()); + $result->setUserData($exc->getStdout()); } $result->setDuration(microtime(true) - $regenerate_start); @@ -301,7 +301,7 @@ class XUnitTestEngine extends ArcanistUnitTestEngine { throw $exc; } $result->setResult(ArcanistUnitTestResult::RESULT_FAIL); - $result->setUserdata($exc->getStdout()); + $result->setUserData($exc->getStdout()); $build_failed = true; } diff --git a/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php b/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php index c26356d0..960bb46c 100644 --- a/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php +++ b/src/unit/parser/__tests__/XUnitTestResultParserTestCase.php @@ -28,7 +28,7 @@ final class XUnitTestResultParserTestCase extends PhutilTestCase { $parsed_results = id(new ArcanistXUnitTestResultParser()) ->parseTestResults(''); - $this->failTest(pht('Should throw on empty input')); + $this->assertFailure(pht('Should throw on empty input')); } catch (Exception $e) { // OK } @@ -43,7 +43,7 @@ final class XUnitTestResultParserTestCase extends PhutilTestCase { $parsed_results = id(new ArcanistXUnitTestResultParser()) ->parseTestResults($stubbed_results); - $this->failTest(pht('Should throw on non-xml input')); + $this->assertFailure(pht('Should throw on non-xml input')); } catch (Exception $e) { // OK } diff --git a/src/unit/renderer/ArcanistUnitConsoleRenderer.php b/src/unit/renderer/ArcanistUnitConsoleRenderer.php index fa646fc5..6294cdee 100644 --- a/src/unit/renderer/ArcanistUnitConsoleRenderer.php +++ b/src/unit/renderer/ArcanistUnitConsoleRenderer.php @@ -21,7 +21,8 @@ final class ArcanistUnitConsoleRenderer extends ArcanistUnitRenderer { $this->getFormattedResult($result->getResult()).$duration, $test_name); - if ($result_code != ArcanistUnitTestResult::RESULT_PASS) { + if ($result_code != ArcanistUnitTestResult::RESULT_PASS + && strlen($result->getUserData())) { $return .= $result->getUserData()."\n"; } diff --git a/src/upload/ArcanistFileDataRef.php b/src/upload/ArcanistFileDataRef.php index 318418ec..99ed03d9 100644 --- a/src/upload/ArcanistFileDataRef.php +++ b/src/upload/ArcanistFileDataRef.php @@ -222,15 +222,6 @@ final class ArcanistFileDataRef extends Phobject { $path)); } - $hash = @sha1_file($path); - if ($hash === false) { - throw new Exception( - pht( - 'Unable to upload file: failed to calculate file data hash for '. - 'path "%s".', - $path)); - } - $size = @filesize($path); if ($size === false) { throw new Exception( @@ -240,11 +231,11 @@ final class ArcanistFileDataRef extends Phobject { $path)); } - $this->hash = $hash; + $this->hash = $this->newFileHash($path); $this->size = $size; } else { $data = $this->data; - $this->hash = sha1($data); + $this->hash = $this->newDataHash($data); $this->size = strlen($data); } } @@ -354,4 +345,24 @@ final class ArcanistFileDataRef extends Phobject { return $data; } + private function newFileHash($path) { + $hash = hash_file('sha256', $path, $raw_output = false); + + if ($hash === false) { + return null; + } + + return $hash; + } + + private function newDataHash($data) { + $hash = hash('sha256', $data, $raw_output = false); + + if ($hash === false) { + return null; + } + + return $hash; + } + } diff --git a/src/workflow/ArcanistDiffWorkflow.php b/src/workflow/ArcanistDiffWorkflow.php index 7834f4b8..14a7ef8f 100644 --- a/src/workflow/ArcanistDiffWorkflow.php +++ b/src/workflow/ArcanistDiffWorkflow.php @@ -1355,7 +1355,7 @@ EOTEXT 'Unit testing raised errors, but all '. 'failing tests are unsound.')); } else { - $continue = $this->console->confirm( + $continue = phutil_console_confirm( pht( 'Unit test results included failures, but all failing tests '. 'are known to be unsound. Ignore unsound test failures?')); @@ -1962,7 +1962,13 @@ EOTEXT $faux_message[] = pht('CC: %s', $this->getArgument('cc')); } + // See T12069. After T10312, the first line of a message is always parsed + // as a title. Add a placeholder so "Reviewers" and "CC" are never the + // first line. + $placeholder_title = pht(''); + if ($faux_message) { + array_unshift($faux_message, $placeholder_title); $faux_message = implode("\n\n", $faux_message); $local = array( '(Flags) ' => array( @@ -2034,6 +2040,10 @@ EOTEXT continue; } + if ($title === $placeholder_title) { + continue; + } + if (!isset($result['title'])) { // We don't have a title yet, so use this one. $result['title'] = $title; diff --git a/src/workflow/ArcanistDownloadWorkflow.php b/src/workflow/ArcanistDownloadWorkflow.php index f3eb466b..543b715e 100644 --- a/src/workflow/ArcanistDownloadWorkflow.php +++ b/src/workflow/ArcanistDownloadWorkflow.php @@ -79,6 +79,172 @@ EOTEXT public function run() { $conduit = $this->getConduit(); + $id = $this->id; + $display_name = 'F'.$id; + $is_show = $this->show; + $save_as = $this->saveAs; + $path = null; + + try { + $file = $conduit->callMethodSynchronous( + 'file.search', + array( + 'constraints' => array( + 'ids' => array($id), + ), + )); + + $data = $file['data']; + if (!$data) { + throw new ArcanistUsageException( + pht( + 'File "%s" is not a valid file, or not visible.', + $display_name)); + } + + $file = head($data); + $data_uri = idxv($file, array('fields', 'dataURI')); + + if ($data_uri === null) { + throw new ArcanistUsageException( + pht( + 'File "%s" can not be downloaded.', + $display_name)); + } + + if ($is_show) { + // Skip all the file path stuff if we're just going to echo the + // file contents. + } else { + if ($save_as !== null) { + $path = Filesystem::resolvePath($save_as); + + $try_unique = false; + } else { + $path = idxv($file, array('fields', 'name'), $display_name); + $path = basename($path); + $path = Filesystem::resolvePath($path); + + $try_unique = true; + } + + if ($try_unique) { + $path = Filesystem::writeUniqueFile($path, ''); + } else { + if (Filesystem::pathExists($path)) { + throw new ArcanistUsageException( + pht( + 'File "%s" already exists.', + $save_as)); + } + + Filesystem::writeFile($path, ''); + } + + $display_path = Filesystem::readablePath($path); + } + + $size = idxv($file, array('fields', 'size'), 0); + + if ($is_show) { + $file_handle = null; + } else { + $file_handle = fopen($path, 'ab+'); + if ($file_handle === false) { + throw new Exception( + pht( + 'Failed to open file "%s" for writing.', + $path)); + } + + $this->writeInfo( + pht('DATA'), + pht( + 'Downloading "%s" (%s byte(s))...', + $display_name, + new PhutilNumber($size))); + } + + $future = new HTTPSFuture($data_uri); + + // For small files, don't bother drawing a progress bar. + $minimum_bar_bytes = (1024 * 1024 * 4); + + if ($is_show || ($size < $minimum_bar_bytes)) { + $bar = null; + } else { + $bar = id(new PhutilConsoleProgressBar()) + ->setTotal($size); + } + + // TODO: We should stream responses to disk, but cURL gives us the raw + // HTTP response data and BaseHTTPFuture can not currently parse it in + // a stream-oriented way. Until this is resolved, buffer the file data + // in memory and write it to disk in one shot. + + list($status, $data) = $future->resolve(); + if ($status->getStatusCode() !== 200) { + throw new Exception( + pht( + 'Got HTTP %d status response, expected HTTP 200.', + $status->getStatusCode())); + } + + if (strlen($data)) { + if ($is_show) { + echo $data; + } else { + $ok = fwrite($file_handle, $data); + if ($ok === false) { + throw new Exception( + pht( + 'Failed to write file data to "%s".', + $path)); + } + } + } + + if ($bar) { + $bar->update(strlen($data)); + } + + if ($bar) { + $bar->done(); + } + + if ($file_handle) { + $ok = fclose($file_handle); + if ($ok === false) { + throw new Exception( + pht( + 'Failed to close file handle for "%s".', + $path)); + } + } + + if (!$is_show) { + $this->writeOkay( + pht('DONE'), + pht( + 'Saved "%s" as "%s".', + $display_name, + $display_path)); + } + + return 0; + } catch (Exception $ex) { + + // If we created an empty file, clean it up. + if (!$is_show) { + if ($path !== null) { + Filesystem::remove($path); + } + } + + // If we fail for any reason, fall back to the older mechanism using + // "file.info" and "file.download". + } + $this->writeStatusMessage(pht('Getting file information...')."\n"); $info = $conduit->callMethodSynchronous( 'file.info', diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 884f3a54..5f13683a 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -17,7 +17,6 @@ final class ArcanistLandWorkflow extends ArcanistWorkflow { private $remote; private $useSquash; private $keepBranch; - private $shouldUpdateWithRebase; private $branchType; private $ontoType; private $preview; @@ -197,43 +196,8 @@ EOTEXT 'conflicts' => array( 'keep-branch' => true, ), - ), - 'update-with-rebase' => array( - 'help' => pht( - "When updating the feature branch, use rebase instead of merge. ". - "This might make things work better in some cases. Set ". - "%s to '%s' to make this the default.", - 'arc.land.update.default', - 'rebase'), - 'conflicts' => array( - 'merge' => pht( - 'The %s strategy does not update the feature branch.', - '--merge'), - 'update-with-merge' => pht( - 'Cannot be used with %s.', - '--update-with-merge'), - ), 'supports' => array( - 'git', - ), - ), - 'update-with-merge' => array( - 'help' => pht( - "When updating the feature branch, use merge instead of rebase. ". - "This is the default behavior. Setting %s to '%s' can also be ". - "used to make this the default.", - 'arc.land.update.default', - 'merge'), - 'conflicts' => array( - 'merge' => pht( - 'The %s strategy does not update the feature branch.', - '--merge'), - 'update-with-rebase' => pht( - 'Cannot be used with %s.', - '--update-with-rebase'), - ), - 'supports' => array( - 'git', + 'hg', ), ), 'revision' => array( @@ -261,22 +225,6 @@ EOTEXT if ($engine) { $this->readEngineArguments(); - - $obsolete = array( - 'delete-remote', - 'update-with-merge', - 'update-with-rebase', - ); - - foreach ($obsolete as $flag) { - if ($this->getArgument($flag)) { - throw new ArcanistUsageException( - pht( - 'Flag "%s" is no longer supported under Git.', - '--'.$flag)); - } - } - $this->requireCleanWorkingCopy(); $should_hold = $this->getArgument('hold'); @@ -514,16 +462,6 @@ EOTEXT $this->branch = head($branch); $this->keepBranch = $this->getArgument('keep-branch'); - $update_strategy = $this->getConfigFromAnySource( - 'arc.land.update.default', - 'merge'); - $this->shouldUpdateWithRebase = $update_strategy == 'rebase'; - if ($this->getArgument('update-with-rebase')) { - $this->shouldUpdateWithRebase = true; - } else if ($this->getArgument('update-with-merge')) { - $this->shouldUpdateWithRebase = false; - } - $this->preview = $this->getArgument('preview'); if (!$this->branchType) { @@ -888,7 +826,7 @@ EOTEXT } } catch (CommandException $ex) { $err = $ex->getError(); - $stdout = $ex->getStdOut(); + $stdout = $ex->getStdout(); // Copied from: PhabricatorRepositoryPullLocalDaemon.php // NOTE: Between versions 2.1 and 2.1.1, Mercurial changed the @@ -953,42 +891,7 @@ EOTEXT $repository_api = $this->getRepositoryAPI(); chdir($repository_api->getPath()); - if ($this->isGit) { - if ($this->shouldUpdateWithRebase) { - echo phutil_console_format(pht( - 'Rebasing **%s** onto **%s**', - $this->branch, - $this->onto)."\n"); - $err = phutil_passthru('git rebase %s', $this->onto); - if ($err) { - throw new ArcanistUsageException(pht( - "'%s' failed. You can abort with '%s', or resolve conflicts ". - "and use '%s' to continue forward. After resolving the rebase, ". - "run '%s' again.", - sprintf('git rebase %s', $this->onto), - 'git rebase --abort', - 'git rebase --continue', - 'arc land')); - } - } else { - echo phutil_console_format(pht( - 'Merging **%s** into **%s**', - $this->branch, - $this->onto)."\n"); - $err = phutil_passthru( - 'git merge --no-stat %s -m %s', - $this->onto, - pht("Automatic merge by '%s'", 'arc land')); - if ($err) { - throw new ArcanistUsageException(pht( - "'%s' failed. To continue: resolve the conflicts, commit ". - "the changes, then run '%s' again. To abort: run '%s'.", - sprintf('git merge %s', $this->onto), - 'arc land', - 'git merge --abort')); - } - } - } else if ($this->isHg) { + if ($this->isHg) { $onto_tip = $repository_api->getCanonicalRevisionName($this->onto); $common_ancestor = $repository_api->getCanonicalRevisionName( hgsprintf('ancestor(%s, %s)', $this->onto, $this->branch)); @@ -1369,31 +1272,7 @@ EOTEXT } if ($this->getArgument('delete-remote')) { - if ($this->isGit) { - list($err, $ref) = $repository_api->execManualLocal( - 'rev-parse --verify %s/%s', - $this->remote, - $this->branch); - - if ($err) { - echo pht( - 'No remote feature %s to clean up.', - $this->branchType); - echo "\n"; - } else { - - // NOTE: In Git, you delete a remote branch by pushing it with a - // colon in front of its name: - // - // git push : - - echo pht('Cleaning up remote feature %s...', $this->branchType), "\n"; - $repository_api->execxLocal( - 'push %s :%s', - $this->remote, - $this->branch); - } - } else if ($this->isHg) { + if ($this->isHg) { // named branches were closed as part of the earlier commit // so only worry about bookmarks if ($repository_api->isBookmark($this->branch)) { @@ -1538,7 +1417,7 @@ EOTEXT pht('Harbormaster URI'), $buildable['uri']); - if (!$console->confirm($prompt)) { + if (!phutil_console_confirm($prompt)) { throw new ArcanistUserAbortException(); } } diff --git a/src/workflow/ArcanistLintWorkflow.php b/src/workflow/ArcanistLintWorkflow.php index bb53cde7..de215aad 100644 --- a/src/workflow/ArcanistLintWorkflow.php +++ b/src/workflow/ArcanistLintWorkflow.php @@ -227,7 +227,7 @@ EOTEXT } if ($everything) { - $paths = iterator_to_array($this->getRepositoryApi()->getAllFiles()); + $paths = iterator_to_array($this->getRepositoryAPI()->getAllFiles()); $this->shouldLintAll = true; } else { $paths = $this->selectPathsForWorkflow($paths, $rev); @@ -518,7 +518,7 @@ EOTEXT $prompt = pht( 'Apply this patch to %s?', phutil_console_format('__%s__', $result->getPath())); - if (!$console->confirm($prompt, $default = true)) { + if (!phutil_console_confirm($prompt, $default_no = false)) { continue; } } @@ -546,7 +546,7 @@ EOTEXT pht('Automatically amending HEAD with lint patches.')); $amend = true; } else { - $amend = $console->confirm(pht('Amend HEAD with lint patches?')); + $amend = phutil_console_confirm(pht('Amend HEAD with lint patches?')); } if ($amend) { diff --git a/src/workflow/ArcanistLintersWorkflow.php b/src/workflow/ArcanistLintersWorkflow.php index 9932f3b9..600d1019 100644 --- a/src/workflow/ArcanistLintersWorkflow.php +++ b/src/workflow/ArcanistLintersWorkflow.php @@ -36,7 +36,7 @@ EOTEXT 'param' => 'search', 'repeat' => true, 'help' => pht( - 'Search for linters. Search is case-insensitive, and is performed'. + 'Search for linters. Search is case-insensitive, and is performed '. 'against name and description of each linter.'), ), '*' => 'exact', diff --git a/src/workflow/ArcanistPatchWorkflow.php b/src/workflow/ArcanistPatchWorkflow.php index e7c6760d..227b020f 100644 --- a/src/workflow/ArcanistPatchWorkflow.php +++ b/src/workflow/ArcanistPatchWorkflow.php @@ -430,6 +430,18 @@ EOTEXT $repository_api = $this->getRepositoryAPI(); $has_base_revision = $repository_api->hasLocalCommit( $bundle->getBaseRevision()); + if (!$has_base_revision) { + if ($repository_api instanceof ArcanistGitAPI) { + echo phutil_console_format( + "** %s ** %s\n", + pht('INFO'), + pht('Base commit is not in local repository; trying to fetch.')); + $repository_api->execManualLocal('fetch --quiet --all'); + $has_base_revision = $repository_api->hasLocalCommit( + $bundle->getBaseRevision()); + } + } + if ($this->canBranch() && ($this->shouldBranch() || ($this->shouldCommit() && $has_base_revision))) { @@ -710,7 +722,7 @@ EOTEXT } // in case there were any submodule changes involved - $repository_api->execpassthru('submodule update --init --recursive'); + $repository_api->execPassthru('submodule update --init --recursive'); if ($this->shouldCommit()) { if ($bundle->getFullAuthor()) { @@ -763,7 +775,7 @@ EOTEXT echo phutil_console_format( "\n** %s **\n", pht('Patch Failed!')); - $stderr = $ex->getStdErr(); + $stderr = $ex->getStderr(); if (preg_match('/case-folding collision/', $stderr)) { echo phutil_console_wrap( phutil_console_format( diff --git a/src/workflow/ArcanistSetConfigWorkflow.php b/src/workflow/ArcanistSetConfigWorkflow.php index 9d9b099d..75b4334f 100644 --- a/src/workflow/ArcanistSetConfigWorkflow.php +++ b/src/workflow/ArcanistSetConfigWorkflow.php @@ -69,6 +69,19 @@ EOTEXT $settings = new ArcanistSettings(); + $console = PhutilConsole::getConsole(); + + if (!$settings->getHelp($key)) { + $warning = tsprintf( + "**%s:** %s\n", + pht('Warning'), + pht( + 'The configuration key "%s" is not recognized by arc. It may '. + 'be misspelled or out of date.', + $key)); + $console->writeErr('%s', $warning); + } + $old = null; if (array_key_exists($key, $config)) { $old = $config[$key]; @@ -85,17 +98,18 @@ EOTEXT $old = $settings->formatConfigValueForDisplay($key, $old); if ($old === null) { - echo pht( - "Deleted key '%s' from %s config.\n", + $message = pht( + 'Deleted key "%s" from %s config.', $key, $which); } else { - echo pht( - "Deleted key '%s' from %s config (was %s).\n", + $message = pht( + 'Deleted key "%s" from %s config (was %s).', $key, $which, $old); } + $console->writeOut('%s', tsprintf("%s\n", $message)); } else { $val = $settings->willWriteValue($key, $val); @@ -110,19 +124,20 @@ EOTEXT $old = $settings->formatConfigValueForDisplay($key, $old); if ($old === null) { - echo pht( - "Set key '%s' = %s in %s config.\n", + $message = pht( + 'Set key "%s" = %s in %s config.', $key, $val, $which); } else { - echo pht( - "Set key '%s' = %s in %s config (was %s).\n", + $message = pht( + 'Set key "%s" = %s in %s config (was %s).', $key, $val, $which, $old); } + $console->writeOut('%s', tsprintf("%s\n", $message)); } return 0; diff --git a/src/workflow/ArcanistStartWorkflow.php b/src/workflow/ArcanistStartWorkflow.php index b940265e..b39cea5a 100644 --- a/src/workflow/ArcanistStartWorkflow.php +++ b/src/workflow/ArcanistStartWorkflow.php @@ -76,7 +76,7 @@ EOTEXT "%s: %s\n\n", pht('Started'), implode(', ', ipull($phid_query, 'fullName'))); - $this->printCurrentTracking(true); + $this->printCurrentTracking(); } } diff --git a/src/workflow/ArcanistStopWorkflow.php b/src/workflow/ArcanistStopWorkflow.php index 1e9f2518..2af72edb 100644 --- a/src/workflow/ArcanistStopWorkflow.php +++ b/src/workflow/ArcanistStopWorkflow.php @@ -105,7 +105,7 @@ EOTEXT "%s %s\n\n", pht('Stopped:'), implode(', ', ipull($phid_query, 'fullName'))); - $this->printCurrentTracking(true); + $this->printCurrentTracking(); } } diff --git a/src/workflow/ArcanistTasksWorkflow.php b/src/workflow/ArcanistTasksWorkflow.php index 95cc7de4..3b9a0789 100644 --- a/src/workflow/ArcanistTasksWorkflow.php +++ b/src/workflow/ArcanistTasksWorkflow.php @@ -81,7 +81,7 @@ EOTEXT $unassigned = $this->getArgument('unassigned'); if ($owner) { - $owner_phid = $this->findOwnerPhid($owner); + $owner_phid = $this->findOwnerPHID($owner); } else if ($unassigned) { $owner_phid = null; } else { diff --git a/src/workflow/ArcanistUnitWorkflow.php b/src/workflow/ArcanistUnitWorkflow.php index a1aadd3b..00ef8a59 100644 --- a/src/workflow/ArcanistUnitWorkflow.php +++ b/src/workflow/ArcanistUnitWorkflow.php @@ -140,7 +140,7 @@ EOTEXT } if ($everything) { - $paths = iterator_to_array($this->getRepositoryApi()->getAllFiles()); + $paths = iterator_to_array($this->getRepositoryAPI()->getAllFiles()); } else { $paths = $this->selectPathsForWorkflow($paths, $rev); } diff --git a/src/workflow/ArcanistVersionWorkflow.php b/src/workflow/ArcanistVersionWorkflow.php index 807c5732..9c470fc6 100644 --- a/src/workflow/ArcanistVersionWorkflow.php +++ b/src/workflow/ArcanistVersionWorkflow.php @@ -51,8 +51,14 @@ EOTEXT pht('%s is not a git working copy.', $lib)); } - list($stdout) = $repository->execxLocal('log -1 --format=%s', '%H %ct'); - list($commit, $timestamp) = explode(' ', $stdout); + // NOTE: Carefully execute these commands in a way that works on Windows + // until T8298 is properly fixed. See PHI52. + + list($commit) = $repository->execxLocal('log -1 --format=%%H'); + $commit = trim($commit); + + list($timestamp) = $repository->execxLocal('log -1 --format=%%ct'); + $timestamp = trim($timestamp); $console->writeOut( "%s %s (%s)\n", diff --git a/src/workflow/ArcanistWorkflow.php b/src/workflow/ArcanistWorkflow.php index 0acea54b..d7afc47e 100644 --- a/src/workflow/ArcanistWorkflow.php +++ b/src/workflow/ArcanistWorkflow.php @@ -338,7 +338,7 @@ abstract class ArcanistWorkflow extends Phobject { $this->conduitAuthenticated = true; - return; + return $this; } catch (Exception $ex) { $conduit->setConduitToken(null); throw $ex; @@ -1011,9 +1011,7 @@ abstract class ArcanistWorkflow extends Phobject { } $should_commit = true; } else { - $permit_autostash = $this->getConfigFromAnySource( - 'arc.autostash', - false); + $permit_autostash = $this->getConfigFromAnySource('arc.autostash'); if ($permit_autostash && $api->canStashChanges()) { echo pht( 'Stashing uncommitted changes. (You can restore them with `%s`).', @@ -1199,6 +1197,14 @@ abstract class ArcanistWorkflow extends Phobject { $future = $conduit->callMethod('differential.querydiffs', $params); $diff = head($future->resolve()); + if ($diff == null) { + throw new Exception( + phutil_console_wrap( + pht("The diff or revision you specified is either invalid or you ". + "don't have permission to view it.")) + ); + } + $changes = array(); foreach ($diff['changes'] as $changedict) { $changes[] = ArcanistDiffChange::newFromDictionary($changedict);