From ccf28a81121ea21d72b342cfb8ab2eee4e56bf86 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 30 Jan 2020 08:14:10 -0800
Subject: [PATCH] Fix an issue where the last line of block-based diffs could
 be incorrectly hidden

Summary:
Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.

For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.

Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.

Maniphest Tasks: T13468

Differential Revision: https://secure.phabricator.com/D20959
---
 .../parser/DifferentialHunkParser.php         | 64 +++++++++++++++++++
 .../diff/PhabricatorDocumentEngineBlocks.php  | 11 +---
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php
index 6cdadf69e7..924789ca83 100644
--- a/src/applications/differential/parser/DifferentialHunkParser.php
+++ b/src/applications/differential/parser/DifferentialHunkParser.php
@@ -354,6 +354,69 @@ final class DifferentialHunkParser extends Phobject {
     return $this;
   }
 
+  public function generateVisibleBlocksMask($lines_context) {
+
+    // See T13468. This is similar to "generateVisibleLinesMask()", but
+    // attempts to work around a series of bugs which cancel each other
+    // out but make a mess of the intermediate steps.
+
+    $old = $this->getOldLines();
+    $new = $this->getNewLines();
+
+    $length = max(count($old), count($new));
+
+    $visible_lines = array();
+    for ($ii = 0; $ii < $length; $ii++) {
+      $old_visible = (isset($old[$ii]) && $old[$ii]['type']);
+      $new_visible = (isset($new[$ii]) && $new[$ii]['type']);
+
+      $visible_lines[$ii] = ($old_visible || $new_visible);
+    }
+
+    $mask = array();
+    $reveal_cursor = -1;
+    for ($ii = 0; $ii < $length; $ii++) {
+
+      // If this line isn't visible, it isn't going to reveal anything.
+      if (!$visible_lines[$ii]) {
+
+        // If it hasn't been revealed by a nearby line, mark it as masked.
+        if (empty($mask[$ii])) {
+          $mask[$ii] = false;
+        }
+
+        continue;
+      }
+
+      // If this line is visible, reveal all the lines nearby.
+
+      // First, compute the minimum and maximum offsets we want to reveal.
+      $min_reveal = max($ii - $lines_context, 0);
+      $max_reveal = min($ii + $lines_context, $length - 1);
+
+      // Naively, we'd do more work than necessary when revealing context for
+      // several adjacent visible lines: we would mark all the overlapping
+      // lines as revealed several times.
+
+      // To avoid duplicating work, keep track of the largest line we've
+      // revealed to. Since we reveal context by marking every consecutive
+      // line, we don't need to touch any line above it.
+      $min_reveal = max($min_reveal, $reveal_cursor);
+
+      // Reveal the remaining unrevealed lines.
+      for ($jj = $min_reveal; $jj <= $max_reveal; $jj++) {
+        $mask[$jj] = true;
+      }
+
+      // Move the cursor to the next line which may still need to be revealed.
+      $reveal_cursor = $max_reveal + 1;
+    }
+
+    $this->setVisibleLinesMask($mask);
+
+    return $mask;
+  }
+
   public function generateVisibleLinesMask($lines_context) {
     $old = $this->getOldLines();
     $new = $this->getNewLines();
@@ -361,6 +424,7 @@ final class DifferentialHunkParser extends Phobject {
     $visible = false;
     $last = 0;
     $mask = array();
+
     for ($cursor = -$lines_context; $cursor < $max_length; $cursor++) {
       $offset = $cursor + $lines_context;
       if ((isset($old[$offset]) && $old[$offset]['type']) ||
diff --git a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
index 47ddf194ee..2847b53c0d 100644
--- a/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
+++ b/src/applications/files/diff/PhabricatorDocumentEngineBlocks.php
@@ -59,7 +59,7 @@ final class PhabricatorDocumentEngineBlocks
       ->parseHunksForLineData($changeset->getHunks())
       ->reparseHunksForSpecialAttributes();
 
-    $hunk_parser->generateVisibleLinesMask(2);
+    $hunk_parser->generateVisibleBlocksMask(2);
     $mask = $hunk_parser->getVisibleLinesMask();
 
     $old_lines = $hunk_parser->getOldLines();
@@ -72,14 +72,7 @@ final class PhabricatorDocumentEngineBlocks
       $old_line = idx($old_lines, $ii);
       $new_line = idx($new_lines, $ii);
 
-      $is_visible = !empty($mask[$ii + 1]);
-
-      // TODO: There's currently a bug where one-line files get incorrectly
-      // masked. This causes images to completely fail to render. Just ignore
-      // the mask if it came back empty.
-      if (!$mask) {
-        $is_visible = true;
-      }
+      $is_visible = !empty($mask[$ii]);
 
       if ($old_line) {
         $old_hash = rtrim($old_line['text'], "\n");