diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php @@ -2,8 +2,12 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase { + private function getTestDataDirectory() { + return dirname(__FILE__).'/data/'; + } + public function testParseRender() { - $dir = dirname(__FILE__).'/data/'; + $dir = $this->getTestDataDirectory(); foreach (Filesystem::listDirectory($dir, $show_hidden = false) as $file) { if (!preg_match('/\.diff$/', $file)) { continue; @@ -22,46 +26,94 @@ } foreach (array('one', 'two') as $type) { - $parser = $this->buildChangesetParser($type, $data, $file); - $actual = $parser->render(null, null, array()); + $this->runParser($type, $data, $file, 'expect'); + $this->runParser($type, $data, $file, 'unshielded'); + $this->runParser($type, $data, $file, 'whitespace'); + } + } + } - $expect = Filesystem::readFile($dir.$file.'.'.$type.'.expect'); - $this->assertEqual($expect, (string)$actual, $file.'.'.$type); + private function runParser($type, $data, $file, $extension) { + $dir = $this->getTestDataDirectory(); + $test_file = $dir.$file.'.'.$type.'.'.$extension; + if (!Filesystem::pathExists($test_file)) { + return; + } + + $unshielded = false; + $whitespace = false; + switch ($extension) { + case 'unshielded': + $unshielded = true; + break; + case 'whitespace'; + $unshielded = true; + $whitespace = true; + break; + } + + $parsers = $this->buildChangesetParsers($type, $data, $file); + $actual = $this->renderParsers($parsers, $unshielded, $whitespace); + $expect = Filesystem::readFile($test_file); + + $this->assertEqual($expect, $actual, basename($test_file)); + } + + private function renderParsers(array $parsers, $unshield, $whitespace) { + $result = array(); + foreach ($parsers as $parser) { + if ($unshield) { + $s_range = 0; + $e_range = 0xFFFF; + } else { + $s_range = null; + $e_range = null; } + + if ($whitespace) { + $parser->setWhitespaceMode( + DifferentialChangesetParser::WHITESPACE_SHOW_ALL); + } + + $result[] = $parser->render($s_range, $e_range, array()); } + return implode(str_repeat('~', 80)."\n", $result); } - private function buildChangesetParser($type, $data, $file) { + private function buildChangesetParsers($type, $data, $file) { $parser = new ArcanistDiffParser(); $changes = $parser->parseDiff($data); $diff = DifferentialDiff::newFromRawChanges( PhabricatorUser::getOmnipotentUser(), $changes); - if (count($diff->getChangesets()) !== 1) { - throw new Exception("Expected one changeset: {$file}"); - } - $changeset = head($diff->getChangesets()); + $changesets = $diff->getChangesets(); $engine = new PhabricatorMarkupEngine(); $engine->setViewer(new PhabricatorUser()); - $cparser = new DifferentialChangesetParser(); - $cparser->setUser(new PhabricatorUser()); - $cparser->setDisableCache(true); - $cparser->setChangeset($changeset); - $cparser->setMarkupEngine($engine); - - if ($type == 'one') { - $cparser->setRenderer(new DifferentialChangesetOneUpTestRenderer()); - } else if ($type == 'two') { - $cparser->setRenderer(new DifferentialChangesetTwoUpTestRenderer()); - } else { - throw new Exception("Unknown renderer type '{$type}'!"); + $parsers = array(); + foreach ($changesets as $changeset) { + $cparser = new DifferentialChangesetParser(); + $cparser->setUser(new PhabricatorUser()); + $cparser->setDisableCache(true); + $cparser->setChangeset($changeset); + $cparser->setMarkupEngine($engine); + + if ($type == 'one') { + $cparser->setRenderer(new DifferentialChangesetOneUpTestRenderer()); + } else if ($type == 'two') { + $cparser->setRenderer(new DifferentialChangesetTwoUpTestRenderer()); + } else { + throw new Exception( + pht('Unknown renderer type "%s"!', $type)); + } + + $parsers[] = $cparser; } - return $cparser; + return $parsers; } } diff --git a/src/applications/differential/__tests__/data/fruit.diff.one.expect b/src/applications/differential/__tests__/data/fruit.diff.one.expect --- a/src/applications/differential/__tests__/data/fruit.diff.one.expect +++ b/src/applications/differential/__tests__/data/fruit.diff.one.expect @@ -1,12 +1,14 @@ CTYPE 1 1 (unforced) - +fruit - -N 1 + apple~ -N 2 + banana~ -N 3 + cherry~ -N 4 + date~ -N 5 + elderberry~ -N 6 + fig~ -N 7 + grape~ -N 8 + honeydew~ -N 9 + ~ +N 1 + apple\n~ +N 2 + banana\n~ +N 3 + cherry\n~ +N 4 + date\n~ +N 5 + elderberry\n~ +N 6 + fig\n~ +N 7 + grape\n~ +N 8 + honeydew\n~ +N 9 + \n~ +O X diff --git a/src/applications/differential/__tests__/data/fruit.diff.two.expect b/src/applications/differential/__tests__/data/fruit.diff.two.expect --- a/src/applications/differential/__tests__/data/fruit.diff.two.expect +++ b/src/applications/differential/__tests__/data/fruit.diff.two.expect @@ -1,21 +1,22 @@ CTYPE 1 1 (unforced) - +fruit - O - . ~ -N 1 + apple~ +N 1 + apple\n~ O - . ~ -N 2 + banana~ +N 2 + banana\n~ O - . ~ -N 3 + cherry~ +N 3 + cherry\n~ O - . ~ -N 4 + date~ +N 4 + date\n~ O - . ~ -N 5 + elderberry~ +N 5 + elderberry\n~ O - . ~ -N 6 + fig~ +N 6 + fig\n~ O - . ~ -N 7 + grape~ +N 7 + grape\n~ O - . ~ -N 8 + honeydew~ +N 8 + honeydew\n~ O - . ~ -N 9 + ~ +N 9 + \n~ diff --git a/src/applications/differential/__tests__/data/generated.diff b/src/applications/differential/__tests__/data/generated.diff new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/generated.diff @@ -0,0 +1,10 @@ +diff --git a/GENERATED b/GENERATED +index 5dcff7f..eff82ef 100644 +--- a/GENERATED ++++ b/GENERATED +@@ -1,4 +1,4 @@ + @generated + +-This is a generated file. ++This is a generated file, full of generated code. + diff --git a/src/applications/differential/__tests__/data/generated.diff.one.expect b/src/applications/differential/__tests__/data/generated.diff.one.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/generated.diff.one.expect @@ -0,0 +1,5 @@ +CTYPE 2 1 (unforced) +GENERATED +GENERATED +- +SHIELD (default) This file contains generated code, which does not normally need to be reviewed. diff --git a/src/applications/differential/__tests__/data/generated.diff.one.unshielded b/src/applications/differential/__tests__/data/generated.diff.one.unshielded new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/generated.diff.one.unshielded @@ -0,0 +1,6 @@ +N 1 . @generated\n~ +O 2 - \n~ +N 2 + \n~ +O 3 - This is a generated file.\n~ +N 3 + This is a generated file{(, full of generated code)}.\n~ +N 4 . \n~ diff --git a/src/applications/differential/__tests__/data/generated.diff.two.expect b/src/applications/differential/__tests__/data/generated.diff.two.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/generated.diff.two.expect @@ -0,0 +1,5 @@ +CTYPE 2 1 (unforced) +GENERATED +GENERATED +- +SHIELD (default) This file contains generated code, which does not normally need to be reviewed. diff --git a/src/applications/differential/__tests__/data/generated.diff.two.unshielded b/src/applications/differential/__tests__/data/generated.diff.two.unshielded new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/generated.diff.two.unshielded @@ -0,0 +1,8 @@ +O 1 . @generated\n~ +N 1 . @generated\n~ +O 2 - \n~ +N 2 + \n~ +O 3 - This is a generated file.\n~ +N 3 + This is a generated file{(, full of generated code)}.\n~ +O 4 . \n~ +N 4 . \n~ diff --git a/src/applications/differential/__tests__/data/move-unedited.diff b/src/applications/differential/__tests__/data/move-unedited.diff new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move-unedited.diff @@ -0,0 +1,4 @@ +diff --git a/SRC b/DST +similarity index 100% +rename from SRC +rename to DST diff --git a/src/applications/differential/__tests__/data/move-unedited.diff.one.expect b/src/applications/differential/__tests__/data/move-unedited.diff.one.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move-unedited.diff.one.expect @@ -0,0 +1,10 @@ +CTYPE 6 1 (unforced) +SRC +DST +- +SHIELD (none) The contents of this file were not changed. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +CTYPE 4 1 (forced) +- +SRC +DST diff --git a/src/applications/differential/__tests__/data/move-unedited.diff.two.expect b/src/applications/differential/__tests__/data/move-unedited.diff.two.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move-unedited.diff.two.expect @@ -0,0 +1,10 @@ +CTYPE 6 1 (unforced) +SRC +DST +- +SHIELD (none) The contents of this file were not changed. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +CTYPE 4 1 (forced) +- +SRC +DST diff --git a/src/applications/differential/__tests__/data/move.diff b/src/applications/differential/__tests__/data/move.diff new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move.diff @@ -0,0 +1,12 @@ +diff --git a/SRC b/DST +similarity index 57% +rename from SRC +rename to DST +index 5d5c76f..f7bd789 100644 +--- a/SRC ++++ b/DST +@@ -1,3 +1,3 @@ + Apples +-Bananas ++Blueberries + Cherries diff --git a/src/applications/differential/__tests__/data/move.diff.one.expect b/src/applications/differential/__tests__/data/move.diff.one.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move.diff.one.expect @@ -0,0 +1,13 @@ +CTYPE 6 1 (unforced) +SRC +DST +- +N 1 . Apples\n~ +O 2 - B{(anana)}s\n~ +N 2 + B{(lueberrie)}s\n~ +N 3 . Cherries\n~ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +CTYPE 4 1 (forced) +- +SRC +DST diff --git a/src/applications/differential/__tests__/data/move.diff.two.expect b/src/applications/differential/__tests__/data/move.diff.two.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/move.diff.two.expect @@ -0,0 +1,15 @@ +CTYPE 6 1 (unforced) +SRC +DST +- +O 1 . Apples\n~ +N 1 . Apples\n~ +O 2 - B{(anana)}s\n~ +N 2 + B{(lueberrie)}s\n~ +O 3 . Cherries\n~ +N 3 . Cherries\n~ +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +CTYPE 4 1 (forced) +- +SRC +DST diff --git a/src/applications/differential/__tests__/data/whitespace.diff b/src/applications/differential/__tests__/data/whitespace.diff new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/whitespace.diff @@ -0,0 +1,7 @@ +diff --git a/WHITESPACE b/WHITESPACE +index 4505d51..bca7deb 100644 +--- a/WHITESPACE ++++ b/WHITESPACE +@@ -1 +1 @@ +--=[-Rocket-Ship> ++ -=[-Rocket-Ship> diff --git a/src/applications/differential/__tests__/data/whitespace.diff.one.expect b/src/applications/differential/__tests__/data/whitespace.diff.one.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/whitespace.diff.one.expect @@ -0,0 +1,5 @@ +CTYPE 2 1 (unforced) +WHITESPACE +WHITESPACE +- +SHIELD (whitespace) This file was changed only by adding or removing whitespace. diff --git a/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace b/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace @@ -0,0 +1,2 @@ +O 1 - -=[-Rocket-Ship>\n~ +N 1 + {( )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/__tests__/data/whitespace.diff.two.expect b/src/applications/differential/__tests__/data/whitespace.diff.two.expect new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/whitespace.diff.two.expect @@ -0,0 +1,5 @@ +CTYPE 2 1 (unforced) +WHITESPACE +WHITESPACE +- +SHIELD (whitespace) This file was changed only by adding or removing whitespace. diff --git a/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace b/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace new file mode 100644 --- /dev/null +++ b/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace @@ -0,0 +1,2 @@ +O 1 - -=[-Rocket-Ship>\n~ +N 1 + {( )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -612,11 +612,6 @@ $moveaway = false; $changetype = $this->changeset->getChangeType(); if ($changetype == DifferentialChangeType::TYPE_MOVE_AWAY) { - // sometimes we show moved files as unchanged, sometimes deleted, - // and sometimes inconsistent with what actually happened at the - // destination of the move. Rather than make a false claim, - // omit the 'not changed' notice if this is the source of a move - $unchanged = false; $moveaway = true; } @@ -776,6 +771,16 @@ pht( 'This file contains generated code, which does not normally '. 'need to be reviewed.')); + } else if ($this->isMoveAway()) { + // We put an empty shield on these files. Normally, they do not have + // any diff content anyway. However, if they come through `arc`, they + // may have content. We don't want to show it (it's not useful) and + // we bailed out of fully processing it earlier anyway. + + // We could show a message like "this file was moved", but we show + // that as a change header anyway, so it would be redundant. Instead, + // just render an empty shield to skip rendering the diff body. + $shield = ''; } else if ($this->isUnchanged()) { $type = 'text'; if (!$rows) { @@ -791,8 +796,6 @@ $shield = $renderer->renderShield( pht('The contents of this file were not changed.'), $type); - } else if ($this->isMoveAway()) { - $shield = null; } else if ($this->isWhitespaceOnly()) { $shield = $renderer->renderShield( pht('This file was changed only by adding or removing whitespace.'), @@ -809,7 +812,7 @@ } } - if ($shield) { + if ($shield !== null) { return $renderer->renderChangesetTable($shield); } diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -96,7 +96,7 @@ public function setOldLineTypeMap(array $map) { $lines = $this->getOldLines(); foreach ($lines as $key => $data) { - $lines[$key]['type'] = $map[$data['line']]; + $lines[$key]['type'] = idx($map, $data['line']); } $this->oldLines = $lines; return $this; @@ -117,7 +117,7 @@ public function setNewLineTypeMap(array $map) { $lines = $this->getNewLines(); foreach ($lines as $key => $data) { - $lines[$key]['type'] = $map[$data['line']]; + $lines[$key]['type'] = idx($map, $data['line']); } $this->newLines = $lines; return $this; diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -7,13 +7,16 @@ $changeset = $this->getChangeset(); $old = nonempty($changeset->getOldFile(), '-'); + $current = nonempty($changeset->getFilename(), '-'); $away = nonempty(implode(', ', $changeset->getAwayPaths()), '-'); + $ctype = $changeset->getChangeType(); $ftype = $changeset->getFileType(); $force = ($force ? '(forced)' : '(unforced)'); return "CTYPE {$ctype} {$ftype} {$force}\n". "{$old}\n". + "{$current}\n". "{$away}\n"; } @@ -47,19 +50,50 @@ $out = array(); + $any_old = false; + $any_new = false; $primitives = $this->buildPrimitives($range_start, $range_len); foreach ($primitives as $p) { $type = $p['type']; switch ($type) { case 'old': case 'new': + if ($type == 'old') { + $any_old = true; + } + if ($type == 'new') { + $any_new = true; + } $num = nonempty($p['line'], '-'); $render = $p['render']; $htype = nonempty($p['htype'], '.'); // TODO: This should probably happen earlier, whenever we deal with // \r and \t normalization? - $render = rtrim($render, "\r\n"); + $render = str_replace( + array( + "\r", + "\n", + ), + array( + '\\r', + '\\n', + ), + $render); + + $render = str_replace( + array( + '', + '', + ), + array( + '{(', + ')}', + ), + $render); + + $render = html_entity_decode($render); + $t = ($type == 'old') ? 'O' : 'N'; $out[] = "{$t} {$num} {$htype} {$render}~"; @@ -70,6 +104,14 @@ } } + if (!$any_old) { + $out[] = 'O X '; + } + + if (!$any_new) { + $out[] = 'N X '; + } + $out = implode("\n", $out)."\n"; return $out; }