From 90546042144f6793796ec9d1eb93e48e71bc2258 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 1 Nov 2017 12:21:53 -0700
Subject: [PATCH] Fix some lint rendering issues when lines prior to other
 identical lines are removed

Summary: See PHI191. This is a rehash of an earlier fix, but we didn't have a test case for this half yet.

Test Plan:
  - Added a failing test, made it pass.
  - Added a linter like the one in PHI191, ran it, got a valid lint result instead of an exception.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18759
---
 .../renderer/ArcanistConsoleLintRenderer.php  | 41 +++++++++----------
 .../ArcanistConsoleLintRendererTestCase.php   | 14 +++++++
 .../renderer/__tests__/data/rmmulti.expect    | 10 +++++
 src/lint/renderer/__tests__/data/rmmulti.txt  |  4 ++
 .../renderer/__tests__/data/rmmulti2.expect   | 10 +++++
 src/lint/renderer/__tests__/data/rmmulti2.txt |  4 ++
 6 files changed, 62 insertions(+), 21 deletions(-)
 create mode 100644 src/lint/renderer/__tests__/data/rmmulti.expect
 create mode 100644 src/lint/renderer/__tests__/data/rmmulti.txt
 create mode 100644 src/lint/renderer/__tests__/data/rmmulti2.expect
 create mode 100644 src/lint/renderer/__tests__/data/rmmulti2.txt

diff --git a/src/lint/renderer/ArcanistConsoleLintRenderer.php b/src/lint/renderer/ArcanistConsoleLintRenderer.php
index a4dfffa9..4862f47f 100644
--- a/src/lint/renderer/ArcanistConsoleLintRenderer.php
+++ b/src/lint/renderer/ArcanistConsoleLintRenderer.php
@@ -148,36 +148,35 @@ final class ArcanistConsoleLintRenderer extends ArcanistLintRenderer {
         $old_impact--;
         $new_impact--;
 
-        if ($old_impact < 0 || $new_impact < 0) {
-          throw new Exception(
-            pht(
-              'Modified prefix 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 before
+        // another identical line.
+        if ($old_impact <= 0 || $new_impact <= 0) {
+          break;
         }
       } while (true);
 
       // If the lines at the end of the changed line range are actually the
       // same, shrink the range. This happens when a patch just removes a
       // line.
-      do {
-        $old_suffix = idx($old_lines, $start + $old_impact - 2, null);
-        $new_suffix = idx($new_lines, $start + $new_impact - 2, null);
+      if ($old_impact > 0 && $new_impact > 0) {
+        do {
+          $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) {
-          break;
-        }
+          if ($old_suffix !== $new_suffix) {
+            break;
+          }
 
-        $old_impact--;
-        $new_impact--;
+          $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);
+          // 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);
+      }
 
     } else {
 
diff --git a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
index f8928229..f65e40c9 100644
--- a/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
+++ b/src/lint/renderer/__tests__/ArcanistConsoleLintRendererTestCase.php
@@ -142,6 +142,20 @@ EOTEXT;
         'replacement' => "\nX\nY\n",
       ),
 
+      'rmmulti' => array(
+        'line' => 2,
+        'char' => 1,
+        'original' => "\n",
+        'replacement' => '',
+      ),
+
+      'rmmulti2' => array(
+        'line' => 1,
+        'char' => 2,
+        'original' => "\n",
+        'replacement' => '',
+      ),
+
     );
 
     $defaults = array(
diff --git a/src/lint/renderer/__tests__/data/rmmulti.expect b/src/lint/renderer/__tests__/data/rmmulti.expect
new file mode 100644
index 00000000..1e4518cf
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/rmmulti.expect
@@ -0,0 +1,10 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 A
+               2 ~
+    >>> -      3 ~
+               4 B
diff --git a/src/lint/renderer/__tests__/data/rmmulti.txt b/src/lint/renderer/__tests__/data/rmmulti.txt
new file mode 100644
index 00000000..82cb3214
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/rmmulti.txt
@@ -0,0 +1,4 @@
+A
+
+
+B
diff --git a/src/lint/renderer/__tests__/data/rmmulti2.expect b/src/lint/renderer/__tests__/data/rmmulti2.expect
new file mode 100644
index 00000000..cd49440f
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/rmmulti2.expect
@@ -0,0 +1,10 @@
+>>> Lint for path/to/example.c:
+
+
+   Warning  (WARN123) Lint Warning
+    Consider this.
+
+               1 A
+    >>> -      2 ~
+               3 ~
+               4 B
diff --git a/src/lint/renderer/__tests__/data/rmmulti2.txt b/src/lint/renderer/__tests__/data/rmmulti2.txt
new file mode 100644
index 00000000..82cb3214
--- /dev/null
+++ b/src/lint/renderer/__tests__/data/rmmulti2.txt
@@ -0,0 +1,4 @@
+A
+
+
+B