diff --git a/src/lint/ArcanistLintMessage.php b/src/lint/ArcanistLintMessage.php index eb575f58..7be1d52d 100644 --- a/src/lint/ArcanistLintMessage.php +++ b/src/lint/ArcanistLintMessage.php @@ -302,8 +302,13 @@ final class ArcanistLintMessage extends Phobject { // "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 <= $minimum_length; $ii++) { + for ($ii = 1; $ii <= $max_suffix; $ii++) { $original_char = $original[$original_length - $ii]; $replacement_char = $replacement[$replacement_length - $ii]; if ($original_char !== $replacement_char) { @@ -323,8 +328,11 @@ final class ArcanistLintMessage extends Phobject { if ($prefix_length) { $prefix = substr($original, 0, $prefix_length); - $original = substr($original, $prefix_length); - $replacement = substr($replacement, $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 diff --git a/src/lint/__tests__/ArcanistLintMessageTestCase.php b/src/lint/__tests__/ArcanistLintMessageTestCase.php index ae02b2b6..c27acd1b 100644 --- a/src/lint/__tests__/ArcanistLintMessageTestCase.php +++ b/src/lint/__tests__/ArcanistLintMessageTestCase.php @@ -45,6 +45,14 @@ final class ArcanistLintMessageTestCase '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) { diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php index 8880fbe8..12ef4e69 100644 --- a/src/lint/renderer/ArcanistConsoleLintRenderer.php +++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php @@ -171,13 +171,10 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer { $old_impact--; $new_impact--; - if ($old_impact < 0 || $new_impact < 0) { - throw new Exception( - pht( - 'Modified suffix line range has become negative '. - '(old = %d, new = %d).', - $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); diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php index 68a35420..f36b30e7 100644 --- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php +++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php @@ -4,6 +4,38 @@ final class ArcanistConsoleLintRendererTestCase extends PhutilTestCase { public function testRendering() { + $midline_original = << array( 'line' => 1, @@ -67,6 +99,20 @@ final class ArcanistConsoleLintRendererTestCase '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, + ), ); $defaults = array( diff --git a/src/lint/renderer/__tests__/data/midline.expect b/src/lint/renderer/__tests__/data/midline.expect new file mode 100644 index 00000000..d5f513ea --- /dev/null +++ b/src/lint/renderer/__tests__/data/midline.expect @@ -0,0 +1,11 @@ +>>> 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/remline.expect b/src/lint/renderer/__tests__/data/remline.expect new file mode 100644 index 00000000..a4d65c95 --- /dev/null +++ b/src/lint/renderer/__tests__/data/remline.expect @@ -0,0 +1,12 @@ +>>> 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/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",