From 76b54ce0a9af112bac2fb0498d9d44532b46f2d5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 4 Oct 2017 08:22:10 -0700 Subject: [PATCH 1/3] Fix parsing of Git branches with common and useful name "0" Summary: See . Oh, PHP! Test Plan: Created a branch named "0", ran `arc diff`. Before: fatal. After: this beautiful revision. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18678 --- src/repository/api/ArcanistGitAPI.php | 8 ++++++-- src/workflow/ArcanistLandWorkflow.php | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index a316093e..d1406096 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -497,6 +497,10 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { return null; } + if (!strlen($branch)) { + return null; + } + return $branch; } @@ -509,7 +513,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // Verify this, and strip it. $ref = rtrim($stdout); $branch = $this->getBranchNameFromRef($ref); - if (!$branch) { + if ($branch === null) { throw new Exception( pht('Failed to parse %s output!', 'git symbolic-ref')); } @@ -1015,7 +1019,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { list($ref, $hash, $epoch, $tree, $desc, $text) = $fields; $branch = $this->getBranchNameFromRef($ref); - if ($branch) { + if ($branch !== null) { $result[] = array( 'current' => ($branch === $current), 'name' => $branch, diff --git a/src/workflow/ArcanistLandWorkflow.php b/src/workflow/ArcanistLandWorkflow.php index 5f13683a..b3c2b181 100644 --- a/src/workflow/ArcanistLandWorkflow.php +++ b/src/workflow/ArcanistLandWorkflow.php @@ -444,7 +444,7 @@ EOTEXT $branch = $this->getArgument('branch'); if (empty($branch)) { $branch = $this->getBranchOrBookmark(); - if ($branch) { + if ($branch !== null) { $this->branchType = $this->getBranchType($branch); // TODO: This message is misleading when landing a detached head or From 617c2e46d6a5307ce23abb3d4ac3859068ebc2f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Oct 2017 14:20:38 -0700 Subject: [PATCH 2/3] Make "line" and "char" strictly optional in ArcanistLintMessage Summary: See PHI136. These are already optional on the server side in `HarbormasterBuildLintMessage`, and effectively mean "file-level issue", which is a bit niche but not unreasonable. Test Plan: Checked that `HarbormasterBuildLintMessage` doesn't care if these keys exist, created this revision. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18711 --- src/lint/ArcanistLintMessage.php | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index c92c5ec8..cc9e58fb 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -25,8 +25,15 @@ final class ArcanistLintMessage extends Phobject { $message = new ArcanistLintMessage(); $message->setPath($dict['path']); - $message->setLine($dict['line']); - $message->setChar($dict['char']); + + if (isset($dict['line'])) { + $message->setLine($dict['line']); + } + + if (isset($dict['char'])) { + $message->setChar($dict['char']); + } + $message->setCode($dict['code']); $message->setSeverity($dict['severity']); $message->setName($dict['name']); From 0989343a4e0c27357a72b9adcce338e7ec497965 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 20 Oct 2017 09:20:58 -0700 Subject: [PATCH 3/3] Correct some lint console renderer issues with files missing trailing newlines Summary: See PHI162. This corrects a couple more bugs: - If the old file didn't end in a newline, we could end up printing two lines next to each other in the output. - If the patch targeted "Line 6, character 1" instead of "line 5, character 3" in a file "1\n2\n3\n4\n5\n", we would fail to figure out what that meant when computing an offset because the last line has 0 characters on it. Test Plan: Added failing unit tests, made them pass. Also tested with some fake linters similar to the ones described in PHI162. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18716 --- .../renderer/ArcanistConsoleLintRenderer.php | 24 ++++++++++- .../ArcanistConsoleLintRendererTestCase.php | 43 +++++++++++++++---- .../__tests__/data/eofmultilinechar.expect | 12 ++++++ .../__tests__/data/eofmultilinechar.txt | 5 +++ .../__tests__/data/eofmultilineline.expect | 12 ++++++ .../__tests__/data/eofmultilineline.txt | 5 +++ .../renderer/__tests__/data/eofnewline.expect | 9 ++++ .../renderer/__tests__/data/eofnewline.txt | 1 + .../__tests__/data/extrawhitespace.txt | 2 +- 9 files changed, 102 insertions(+), 11 deletions(-) create mode 100644 src/lint/renderer/__tests__/data/eofmultilinechar.expect create mode 100644 src/lint/renderer/__tests__/data/eofmultilinechar.txt create mode 100644 src/lint/renderer/__tests__/data/eofmultilineline.expect create mode 100644 src/lint/renderer/__tests__/data/eofmultilineline.txt create mode 100644 src/lint/renderer/__tests__/data/eofnewline.expect create mode 100644 src/lint/renderer/__tests__/data/eofnewline.txt diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index ea233e3d..a4dfffa9 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -216,8 +216,11 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { } for ($ii = $start; $ii < $start + $new_impact; $ii++) { + // If the patch was at the end of the file and ends with a newline, we + // won't have an actual entry in the array for the last line, even though + // we want to show it in the diff. $out[] = array( - 'text' => $new_lines[$ii - 1], + 'text' => idx($new_lines, $ii - 1, ''), 'type' => '+', 'chevron' => ($ii == $start), ); @@ -246,9 +249,17 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { } } + // If the line doesn't actually end in a newline, add one so the layout + // doesn't mess up. This can happen when the last line of the old file + // didn't have a newline at the end. + $text = $spec['text']; + if (!preg_match('/\n\z/', $spec['text'])) { + $text .= "\n"; + } + $result[] = $this->renderLine( idx($spec, 'number'), - $spec['text'], + $text, $chevron, idx($spec, 'type')); @@ -298,6 +309,15 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $offset += strlen($line); } + // If the last line ends in a newline, add a virtual offset for the final + // line with no characters on it. This allows lint messages to target the + // last line of the file at character 1. + if ($lines) { + if (preg_match('/\n\z/', $line)) { + $line_map[$number] = $offset; + } + } + return $line_map; } diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 9014288b..f8928229 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -120,6 +120,28 @@ EOTEXT; 'original' => "\n", 'replacement' => '', ), + + 'eofnewline' => array( + 'line' => 1, + 'char' => 7, + 'original' => '', + 'replacement' => "\n", + ), + + 'eofmultilinechar' => array( + 'line' => 5, + 'char' => 3, + 'original' => '', + 'replacement' => "\nX\nY\n", + ), + + 'eofmultilineline' => array( + 'line' => 6, + 'char' => 1, + 'original' => '', + 'replacement' => "\nX\nY\n", + ), + ); $defaults = array( @@ -132,8 +154,6 @@ EOTEXT; 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; @@ -178,11 +198,6 @@ EOTEXT; 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, @@ -194,7 +209,19 @@ EOTEXT; private function readTestData($filename) { $path = dirname(__FILE__).'/data/'.$filename; - return Filesystem::readFile($path); + $data = Filesystem::readFile($path); + + // If we find "~~~" at the end of the file, get rid of it and any whitespace + // afterwards. This allows specifying data files with trailing empty + // lines. + $data = preg_replace('/~~~\s*\z/', '', $data); + + // Trim "~" off the ends of lines. This allows the "expect" file to test + // for trailing whitespace without actually containing trailing + // whitespace. + $data = preg_replace('/~$/m', '', $data); + + return $data; } } diff --git a/src/lint/renderer/__tests__/data/eofmultilinechar.expect b/src/lint/renderer/__tests__/data/eofmultilinechar.expect new file mode 100644 index 00000000..5ec8fdbe --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofmultilinechar.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 3 3 + 4 4 + 5 5 + >>> + ~ + + X + + Y diff --git a/src/lint/renderer/__tests__/data/eofmultilinechar.txt b/src/lint/renderer/__tests__/data/eofmultilinechar.txt new file mode 100644 index 00000000..8a1218a1 --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofmultilinechar.txt @@ -0,0 +1,5 @@ +1 +2 +3 +4 +5 diff --git a/src/lint/renderer/__tests__/data/eofmultilineline.expect b/src/lint/renderer/__tests__/data/eofmultilineline.expect new file mode 100644 index 00000000..5ec8fdbe --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofmultilineline.expect @@ -0,0 +1,12 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + 3 3 + 4 4 + 5 5 + >>> + ~ + + X + + Y diff --git a/src/lint/renderer/__tests__/data/eofmultilineline.txt b/src/lint/renderer/__tests__/data/eofmultilineline.txt new file mode 100644 index 00000000..8a1218a1 --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofmultilineline.txt @@ -0,0 +1,5 @@ +1 +2 +3 +4 +5 diff --git a/src/lint/renderer/__tests__/data/eofnewline.expect b/src/lint/renderer/__tests__/data/eofnewline.expect new file mode 100644 index 00000000..5310bdb9 --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofnewline.expect @@ -0,0 +1,9 @@ +>>> Lint for path/to/example.c: + + + Warning (WARN123) Lint Warning + Consider this. + + >>> - 1 abcdef + + abcdef + + ~ diff --git a/src/lint/renderer/__tests__/data/eofnewline.txt b/src/lint/renderer/__tests__/data/eofnewline.txt new file mode 100644 index 00000000..9836135e --- /dev/null +++ b/src/lint/renderer/__tests__/data/eofnewline.txt @@ -0,0 +1 @@ +abcdef~~~ diff --git a/src/lint/renderer/__tests__/data/extrawhitespace.txt b/src/lint/renderer/__tests__/data/extrawhitespace.txt index ba587d4c..00ba812f 100644 --- a/src/lint/renderer/__tests__/data/extrawhitespace.txt +++ b/src/lint/renderer/__tests__/data/extrawhitespace.txt @@ -1,3 +1,3 @@ Adrift upon the sea. -~ +~~~