mirror of
https://we.phorge.it/source/arcanist.git
synced 2024-12-29 17:00:58 +01:00
Correct lint rendering when patching trailing whitespace in files
Summary: Ref PHI48. If a patch removes all of the lines at the end of a file, we can get some array index errors. Test Plan: Added failing test, made it pass. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D18631
This commit is contained in:
parent
df81d79d77
commit
b836557b12
5 changed files with 28 additions and 9 deletions
|
@ -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,
|
||||
'');
|
||||
}
|
||||
|
|
|
@ -118,7 +118,6 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
|
|||
// 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) {
|
||||
|
@ -135,11 +134,13 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
|
|||
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 {
|
||||
if ($old_lines[$start - 1] != $new_lines[$start - 1]) {
|
||||
$old_line = idx($old_lines, $start - 1, null);
|
||||
$new_line = idx($new_lines, $start - 1, null);
|
||||
|
||||
if ($old_line !== $new_line) {
|
||||
break;
|
||||
}
|
||||
|
||||
|
@ -161,10 +162,10 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
|
|||
// same, shrink the range. This happens when a patch just removes a
|
||||
// line.
|
||||
do {
|
||||
$old_suffix = $old_lines[$start + $old_impact - 2];
|
||||
$new_suffix = $new_lines[$start + $new_impact - 2];
|
||||
$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) {
|
||||
if ($old_suffix !== $new_suffix) {
|
||||
break;
|
||||
}
|
||||
|
||||
|
|
|
@ -113,6 +113,13 @@ EOTEXT;
|
|||
'original' => $remline_original,
|
||||
'replacement' => $remline_replacement,
|
||||
),
|
||||
|
||||
'extrawhitespace' => array(
|
||||
'line' => 2,
|
||||
'char' => 1,
|
||||
'original' => "\n",
|
||||
'replacement' => '',
|
||||
),
|
||||
);
|
||||
|
||||
$defaults = array(
|
||||
|
@ -125,6 +132,8 @@ 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;
|
||||
|
|
8
src/lint/renderer/__tests__/data/extrawhitespace.expect
Normal file
8
src/lint/renderer/__tests__/data/extrawhitespace.expect
Normal file
|
@ -0,0 +1,8 @@
|
|||
>>> Lint for path/to/example.c:
|
||||
|
||||
|
||||
Warning (WARN123) Lint Warning
|
||||
Consider this.
|
||||
|
||||
1 Adrift upon the sea.
|
||||
>>> - 2 ~
|
3
src/lint/renderer/__tests__/data/extrawhitespace.txt
Normal file
3
src/lint/renderer/__tests__/data/extrawhitespace.txt
Normal file
|
@ -0,0 +1,3 @@
|
|||
Adrift upon the sea.
|
||||
|
||||
~
|
Loading…
Reference in a new issue