From 7371277d20650160c5192417cd9aefaf132372f5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 5 Sep 2017 11:49:51 -0700
Subject: [PATCH] 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              | 14 +++++++++---
 .../__tests__/ArcanistLintMessageTestCase.php |  8 +++++++
 .../ArcanistConsoleLintRendererTestCase.php   | 22 +++++++++++++++++++
 .../renderer/__tests__/data/midline.expect    | 11 ++++++++++
 src/lint/renderer/__tests__/data/midline.txt  |  4 ++++
 5 files changed, 56 insertions(+), 3 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/midline.expect
 create mode 100644 src/lint/renderer/__tests__/data/midline.txt

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/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index 68a35420..bf7bca4b 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/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(
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;