From 6a6910835874f813cf267399a1ccb4c12f6d7f4b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 10 Jan 2013 16:06:39 -0800 Subject: [PATCH] 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 --- .../parser/DifferentialChangesetParser.php | 25 ++------ .../parser/DifferentialHunkParser.php | 59 +++++++------------ .../DifferentialHunkParserTestCase.php | 40 ++++++++++++- .../__tests__/data/missing_context.diff | 11 ++++ .../__tests__/data/missing_context_2.diff | 12 ++++ .../__tests__/data/missing_context_3.diff | 18 ++++++ .../render/DifferentialChangesetRenderer.php | 24 +++----- .../DifferentialChangesetTwoUpRenderer.php | 8 +-- 8 files changed, 118 insertions(+), 79 deletions(-) create mode 100644 src/applications/differential/parser/__tests__/data/missing_context.diff create mode 100644 src/applications/differential/parser/__tests__/data/missing_context_2.diff create mode 100644 src/applications/differential/parser/__tests__/data/missing_context_3.diff diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index ab17a30677..1728767227 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/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) diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index d62ee5c705..0214d0092c 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/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 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; } diff --git a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php b/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php index 2ff4e5d527..cfec9d9c5e 100644 --- a/src/applications/differential/parser/__tests__/DifferentialHunkParserTestCase.php +++ b/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); + } + } + } diff --git a/src/applications/differential/parser/__tests__/data/missing_context.diff b/src/applications/differential/parser/__tests__/data/missing_context.diff new file mode 100644 index 0000000000..e5f2e19ad6 --- /dev/null +++ b/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 + diff --git a/src/applications/differential/parser/__tests__/data/missing_context_2.diff b/src/applications/differential/parser/__tests__/data/missing_context_2.diff new file mode 100644 index 0000000000..4e683981ad --- /dev/null +++ b/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 + diff --git a/src/applications/differential/parser/__tests__/data/missing_context_3.diff b/src/applications/differential/parser/__tests__/data/missing_context_3.diff new file mode 100644 index 0000000000..d8802ab89e --- /dev/null +++ b/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 + diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 8a4ee8e24e..9d7671bbc7 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/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 ); diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 5f25391421..e49cec8bef 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/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; }