Simplify context parsing and add test coverage

Summary:
  - Remove 'missingNew', etc. It's impossible for a diff to popluate these, as far as I can tell (I can't generate such a diff, or find any which generate it).
  - Add unit tests.

Test Plan: Unit tests, viewed a diff with some missing context.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2009

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

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -10,8 +10,7 @@ final class DifferentialChangesetParser {
   protected $oldRender    = null;
 
   protected $filename     = null;
-  protected $missingOld   = array();
-  protected $missingNew   = array();
+  protected $hunkStartLines = array();
 
   protected $comments     = array();
   protected $specialAttributes = array();
@@ -41,7 +40,7 @@ final class DifferentialChangesetParser {
   private $markupEngine;
   private $highlightErrors;
 
-  const CACHE_VERSION = 8;
+  const CACHE_VERSION = 9;
   const CACHE_MAX_SIZE = 8e6;
 
   const ATTR_GENERATED  = 'attr:generated';
@@ -74,16 +73,6 @@ final class DifferentialChangesetParser {
     return $this;
   }
 
-  public function setMissingNewLineMarkerMap(array $map) {
-    $this->missingNew = $map;
-    return $this;
-  }
-
-  public function setMissingOldLineMarkerMap(array $map) {
-    $this->missingOld = $map;
-    return $this;
-  }
-
   public function setIntraLineDiffs(array $diffs) {
     $this->intra = $diffs;
     return $this;
@@ -298,8 +287,7 @@ final class DifferentialChangesetParser {
       'newRender',
       'oldRender',
       'specialAttributes',
-      'missingOld',
-      'missingNew',
+      'hunkStartLines',
       'cacheVersion',
       'cacheHost',
     );
@@ -593,8 +581,8 @@ final class DifferentialChangesetParser {
     $this->setNewLines($hunk_parser->getNewLines());
     $this->setIntraLineDiffs($hunk_parser->getIntraLineDiffs());
     $this->setVisibileLinesMask($hunk_parser->getVisibleLinesMask());
-    $this->setMissingOldLineMarkerMap($hunk_parser->getOldLineMarkerMap());
-    $this->setMissingNewLineMarkerMap($hunk_parser->getNewLineMarkerMap());
+    $this->hunkStartLines = $hunk_parser->getHunkStartLines(
+      $changeset->getHunks());
 
     $new_corpus = $hunk_parser->getNewCorpus();
     $new_corpus_block = implode('', $new_corpus);
@@ -711,8 +699,7 @@ final class DifferentialChangesetParser {
       ->setLineCount($rows)
       ->setOldRender($this->oldRender)
       ->setNewRender($this->newRender)
-      ->setMissingOldLines($this->missingOld)
-      ->setMissingNewLines($this->missingNew)
+      ->setHunkStartLines($this->hunkStartLines)
       ->setOldChangesetID($this->leftSideChangesetID)
       ->setNewChangesetID($this->rightSideChangesetID)
       ->setOldAttachesToNewFile($this->leftSideAttachesToNewFile)

src/applications/differential/parser/DifferentialHunkParser.php

@@ -7,13 +7,32 @@ final class DifferentialHunkParser {
   private $isDeleted;
   private $oldLines;
   private $newLines;
-  private $oldLineMarkerMap;
-  private $newLineMarkerMap;
   private $skipIntraLines;
   private $whitespaceMode;
   private $intraLineDiffs;
   private $visibleLinesMask;
 
+  /**
+   * Get a map of lines on which hunks start, other than line 1. This
+   * datastructure is used to determine when to render "Context not available."
+   * in diffs with multiple hunks.
+   *
+   * @return dict<int, bool>  Map of lines where hunks start, other than line 1.
+   */
+  public function getHunkStartLines(array $hunks) {
+    assert_instances_of($hunks, 'DifferentialHunk');
+
+    $map = array();
+    foreach ($hunks as $hunk) {
+      $line = $hunk->getOldOffset();
+      if ($line > 1) {
+        $map[$line] = true;
+      }
+    }
+
+    return $map;
+  }
+
   private function setVisibleLinesMask($mask) {
     $this->visibleLinesMask = $mask;
     return $this;
@@ -66,32 +85,6 @@ final class DifferentialHunkParser {
     return $this->skipIntraLines;
   }
 
-  private function setNewLineMarkerMap($new_line_marker_map) {
-    $this->newLineMarkerMap = $new_line_marker_map;
-    return $this;
-  }
-  public function getNewLineMarkerMap() {
-    if ($this->newLineMarkerMap === null) {
-      throw new Exception(
-        'You must parseHunksForLineData before accessing this data.'
-      );
-    }
-    return $this->newLineMarkerMap;
-  }
-
-  private function setOldLineMarkerMap($old_line_marker_map) {
-    $this->oldLineMarkerMap = $old_line_marker_map;
-    return $this;
-  }
-  public function getOldLineMarkerMap() {
-    if ($this->oldLineMarkerMap === null) {
-      throw new Exception(
-        'You must parseHunksForLineData before accessing this data.'
-      );
-    }
-    return $this->oldLineMarkerMap;
-  }
-
   private function setNewLines($new_lines) {
     $this->newLines = $new_lines;
     return $this;
@@ -419,9 +412,6 @@ final class DifferentialHunkParser {
 
     $old_lines = array();
     $new_lines = array();
-    $old_line_marker_map = array();
-    $new_line_marker_map = array();
-
     foreach ($hunks as $hunk) {
 
       $lines = $hunk->getChanges();
@@ -443,11 +433,6 @@ final class DifferentialHunkParser {
 
       $old_line = $hunk->getOldOffset();
       $new_line = $hunk->getNewOffset();
-      if ($old_line > 1) {
-        $old_line_marker_map[$old_line] = true;
-      } else if ($new_line > 1) {
-        $new_line_marker_map[$new_line] = true;
-      }
 
       $num_lines = count($lines);
       for ($cursor = 0; $cursor < $num_lines; $cursor++) {
@@ -484,8 +469,6 @@ final class DifferentialHunkParser {
 
     $this->setOldLines($old_lines);
     $this->setNewLines($new_lines);
-    $this->setOldLineMarkerMap($old_line_marker_map);
-    $this->setNewLineMarkerMap($new_line_marker_map);
 
     return $this;
   }

src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php

@@ -39,6 +39,19 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
     );
   }
 
+  private function createHunksFromFile($name) {
+    $data = Filesystem::readFile(dirname(__FILE__).'/data/'.$name);
+
+    $parser = new ArcanistDiffParser();
+    $changes = $parser->parseDiff($data);
+    if (count($changes) !== 1) {
+      throw new Exception("Expected 1 changeset for '{$name}'!");
+    }
+
+    $diff = DifferentialDiff::newFromRawChanges($changes);
+    return head($diff->getChangesets())->getHunks();
+  }
+
   public function testOneLineOldComment() {
     $parser = new DifferentialHunkParser();
     $hunks = $this->createSingleChange(1, 0, "-a");
@@ -68,7 +81,7 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
       0);
     $this->assertEqual("", $context);
   }
-  
+
   public function testOverlapFromStartOfHunk() {
     $parser = new DifferentialHunkParser();
     $hunks = array(
@@ -232,5 +245,30 @@ final class DifferentialHunkParserTestCase extends PhabricatorTestCase {
         "-o3\n".
         "+n2", $context);
   }
+
+  public function testMissingContext() {
+    $tests = array(
+      'missing_context.diff' => array(
+        4 => true,
+      ),
+      'missing_context_2.diff' => array(
+        5 => true,
+      ),
+      'missing_context_3.diff' => array(
+        4 => true,
+        13 => true,
+      ),
+    );
+
+    foreach ($tests as $name => $expect) {
+      $hunks = $this->createHunksFromFile($name);
+
+      $parser = new DifferentialHunkParser();
+      $actual = $parser->getHunkStartLines($hunks);
+
+      $this->assertEqual($expect, $actual, $name);
+    }
+  }
+
 }
 

src/applications/differential/parser/__tests__/data/missing_context.diff

@@ -0,0 +1,11 @@
+diff --git a/fruit b/fruit
+index a1f7255..b5fb7b8 100644
+--- a/fruit
++++ b/fruit
+@@ -4,6 +4,5 @@ cherry
+ date
+ elderberry
+ fig
+-grape
+ honeydew
+

src/applications/differential/parser/__tests__/data/missing_context_2.diff

@@ -0,0 +1,12 @@
+diff --git a/fruit b/fruit
+index a1f7255..fa84742 100644
+--- a/fruit
++++ b/fruit
+@@ -5,5 +5,7 @@ date
+ elderberry
+ fig
+ grape
++guava
++gooseberry
+ honeydew
+

src/applications/differential/parser/__tests__/data/missing_context_3.diff

@@ -0,0 +1,18 @@
+diff --git a/fruit b/fruit
+index bf66874..071dd49 100644
+--- a/fruit
++++ b/fruit
+@@ -4,7 +4,6 @@ date
+ elderberry
+ fig
+ banana
+-grape
+ honeydew
+ apple
+ cherry
+@@ -13,5 +12,4 @@ elderberry
+ fig
+ banana
+ grape
+-honeydew
+

src/applications/differential/render/DifferentialChangesetRenderer.php

@@ -6,8 +6,7 @@ abstract class DifferentialChangesetRenderer {
   private $changeset;
   private $renderingReference;
   private $renderPropertyChangeHeader;
-  private $missingOldLines;
-  private $missingNewLines;
+  private $hunkStartLines;
   private $oldLines;
   private $newLines;
   private $oldComments;
@@ -205,20 +204,13 @@ abstract class DifferentialChangesetRenderer {
     return $this->oldLines;
   }
 
-  public function setMissingNewLines(array $missing_new_lines) {
-    $this->missingNewLines = $missing_new_lines;
+  public function setHunkStartLines(array $hunk_start_lines) {
+    $this->hunkStartLines = $hunk_start_lines;
     return $this;
   }
-  protected function getMissingNewLines() {
-    return $this->missingNewLines;
-  }
 
-  public function setMissingOldLines(array $missing_old_lines) {
-    $this->missingOldLines = $missing_old_lines;
-    return $this;
-  }
-  protected function getMissingOldLines() {
-    return $this->missingOldLines;
+  protected function getHunkStartLines() {
+    return $this->hunkStartLines;
   }
 
   public function setUser(PhabricatorUser $user) {
@@ -260,9 +252,9 @@ abstract class DifferentialChangesetRenderer {
     $rows
   );
   abstract public function renderFileChange(
-    $old = null, 
-    $new = null, 
-    $id = 0, 
+    $old = null,
+    $new = null,
+    $id = 0,
     $vs = 0
   );
 

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php

@@ -44,11 +44,10 @@ final class DifferentialChangesetTwoUpRenderer
     $range_len,
     $rows) {
 
-    $missing_old = $this->getMissingOldLines();
-    $missing_new = $this->getMissingNewLines();
+    $hunk_starts = $this->getHunkStartLines();
 
     $context_not_available = null;
-    if ($missing_old || $missing_new) {
+    if ($hunk_starts) {
       $context_not_available = javelin_render_tag(
         'tr',
         array(
@@ -282,8 +281,7 @@ final class DifferentialChangesetTwoUpRenderer
       }
       $n_classes .= ' right'.$n_colspan;
 
-      if (($o_num && !empty($missing_old[$o_num])) ||
-          ($n_num && !empty($missing_new[$n_num]))) {
+      if (isset($hunk_starts[$o_num])) {
         $html[] = $context_not_available;
       }