From 6cf017d68003def9dcdd02be58384b89e61435d5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 19 May 2020 08:57:42 -0700
Subject: [PATCH] Fix an unusual issue with intradiff highlighting of files
 with uncommon end-of-file modifications

Summary:
Fixes T13539. See that task for discussion and a reproduction case.

This algorithm currently counts "\ No newline at end of file" lines as though they were normal source lines. This can cause offset issues in the rare case that a diff contains two of these lines (for each side of the file) and has changes between them (because the last line of the file was modified between the diffs).

Instead, don't count "\" as a display line.

Test Plan:
  - See T13539 and PHI1740.
  - Before: got fatals on the "wild" diff and the synthetic simplified version.
  - After: clean intradiff rendering in both cases.

Maniphest Tasks: T13539

Differential Revision: https://secure.phabricator.com/D21267
---
 .../parser/DifferentialHunkParser.php         | 38 +++++++++++++++----
 .../DifferentialChangesetParserTestCase.php   | 14 +++----
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php
index 52f5bb90dc..2902022184 100644
--- a/src/applications/differential/parser/DifferentialHunkParser.php
+++ b/src/applications/differential/parser/DifferentialHunkParser.php
@@ -582,8 +582,13 @@ final class DifferentialHunkParser extends Phobject {
       $changes = $hunk->getSplitLines();
       foreach ($changes as $line) {
         $diff_type = $line[0]; // Change type in diff of diffs.
+        $is_same = ($diff_type === ' ');
+        $is_add = ($diff_type === '+');
+        $is_rem = ($diff_type === '-');
+
         $orig_type = $line[1]; // Change type in the original diff.
-        if ($diff_type == ' ') {
+
+        if ($is_same) {
           // Use the same key for lines that are next to each other.
           if ($olds_cursor > $news_cursor) {
             $key = $olds_cursor + 1;
@@ -594,17 +599,32 @@ final class DifferentialHunkParser extends Phobject {
           $news[$key] = null;
           $olds_cursor = $key;
           $news_cursor = $key;
-        } else if ($diff_type == '-') {
+        } else if ($is_rem) {
           $olds[] = array($n_old, $orig_type);
           $olds_cursor++;
-        } else if ($diff_type == '+') {
+        } else if ($is_add) {
           $news[] = array($n_new, $orig_type);
           $news_cursor++;
+        } else {
+          throw new Exception(
+            pht(
+              'Found unknown intradiff source line, expected a line '.
+              'beginning with "+", "-", or " " (space): %s.',
+              $line));
         }
-        if (($diff_type == '-' || $diff_type == ' ') && $orig_type != '-') {
+
+        // See T13539. Don't increment the line count if this line was removed,
+        // or if the line is a "No newline at end of file" marker.
+        $not_a_line = ($orig_type === '-' || $orig_type === '\\');
+        if ($not_a_line) {
+          continue;
+        }
+
+        if ($is_same || $is_rem) {
           $n_old++;
         }
-        if (($diff_type == '+' || $diff_type == ' ') && $orig_type != '-') {
+
+        if ($is_same || $is_add) {
           $n_new++;
         }
       }
@@ -623,14 +643,18 @@ final class DifferentialHunkParser extends Phobject {
         list($n, $type) = $olds[$i];
         if ($type == '+' ||
             ($type == ' ' && isset($news[$i]) && $news[$i][1] != ' ')) {
-          $highlight_old[] = $offsets_old[$n];
+          if (isset($offsets_old[$n])) {
+            $highlight_old[] = $offsets_old[$n];
+          }
         }
       }
       if (isset($news[$i])) {
         list($n, $type) = $news[$i];
         if ($type == '+' ||
             ($type == ' ' && isset($olds[$i]) && $olds[$i][1] != ' ')) {
-          $highlight_new[] = $offsets_new[$n];
+          if (isset($offsets_new[$n])) {
+            $highlight_new[] = $offsets_new[$n];
+          }
         }
       }
     }
diff --git a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
index d8d10169b5..f84f166bf6 100644
--- a/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
+++ b/src/applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php
@@ -4,19 +4,19 @@ final class DifferentialChangesetParserTestCase extends PhabricatorTestCase {
 
   public function testDiffChangesets() {
     $hunk = new DifferentialHunk();
-    $hunk->setChanges("+a\n b\n-c");
+    $hunk->setChanges("+a\n b\n-c\n");
     $hunk->setNewOffset(1);
     $hunk->setNewLen(2);
     $left = new DifferentialChangeset();
     $left->attachHunks(array($hunk));
 
     $tests = array(
-      "+a\n b\n-c" => array(array(), array()),
-      "+a\n x\n-c" => array(array(), array()),
-      "+aa\n b\n-c" => array(array(1), array(11)),
-      " b\n-c" => array(array(1), array()),
-      "+a\n b\n c" => array(array(), array(13)),
-      "+a\n x\n c" => array(array(), array(13)),
+      "+a\n b\n-c\n" => array(array(), array()),
+      "+a\n x\n-c\n" => array(array(), array()),
+      "+aa\n b\n-c\n" => array(array(1), array(11)),
+      " b\n-c\n" => array(array(1), array()),
+      "+a\n b\n c\n" => array(array(), array(13)),
+      "+a\n x\n c\n" => array(array(), array(13)),
     );
 
     foreach ($tests as $changes => $expected) {