Fix a prefix/suffix counting issue in Arcanist lint rendering

Summary: Ref T9846. See PHI48. For replacing text in the form "ABC" with "ABBC", the trimmer had a bug.

Test Plan: Added failing tests, fixed 'em.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9846

Differential Revision: https://secure.phabricator.com/D18538

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);
+      // NOTE: Prior to PHP7, `substr("a", 1)` returned false instead of
-      $replacement = substr($replacement, $prefix_length);
+      // 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

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) {

src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php

@@ -4,6 +4,21 @@ final class ArcanistConsoleLintRendererTestCase
   extends PhutilTestCase {
 
   public function testRendering() {
+    $midline_original = <<<EOTEXT
+import apple;
+import banana;
+import cat;
+import dog;
+EOTEXT;
+
+    $midline_replacement = <<<EOTEXT
+import apple;
+import banana;
+
+import cat;
+import dog;
+EOTEXT;
+
     $map = array(
       'simple' => array(
         'line' => 1,
@@ -67,6 +82,13 @@ final class ArcanistConsoleLintRendererTestCase
         'char' => 4,
         'original' => 'should of',
       ),
+
+      'midline' => array(
+        'line' => 1,
+        'char' => 1,
+        'original' => $midline_original,
+        'replacement' => $midline_replacement,
+      ),
     );
 
     $defaults = array(

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;

src/lint/renderer/__tests__/data/midline.txt

@@ -0,0 +1,4 @@
+import apple;
+import banana;
+import cat;
+import dog;