Expand Differential test coverage to include moves, shields, and more

Summary:
See D11468 and D11465. Fixes T5163. Fixes T4105. This makes it practical to test shields, unshielding, moves, etc.

This fixes the issue in D11468, where line maps from whitespace-ignored hunks could have fewer lines than line maps from whitespace-respected hunks, causing a warning.

This encodes the behavior which D11465 changed, making it the canon behavior. Specifically, we do **not** show a shield. I think this is correct. It seems misleading to show "the contents of this file were not changed", because they were changed in both the sense that the file was completely removed, and also changed in the sense that the content itself was (or may have been) changed at the destination. Instead, we just show nothing.

Test Plan:
  - Added test coverage.
  - Ran tests.
  - Used `arc diff --raw --browse` to verify that web behavior was consistent with CLI/test behavior.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4105, T5163

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

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 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase {
       }
 
       foreach (array('one', 'two') as $type) {
-        $parser = $this->buildChangesetParser($type, $data, $file);
-        $actual = $parser->render(null, null, array());
-
-        $expect = Filesystem::readFile($dir.$file.'.'.$type.'.expect');
-        $this->assertEqual($expect, (string)$actual, $file.'.'.$type);
+        $this->runParser($type, $data, $file, 'expect');
+        $this->runParser($type, $data, $file, 'unshielded');
+        $this->runParser($type, $data, $file, 'whitespace');
       }
     }
   }
 
-  private function buildChangesetParser($type, $data, $file) {
+  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 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);
+    $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("Unknown renderer type '{$type}'!");
+      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;
   }
 
 }

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 <EMPTY>

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~

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.
+

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.

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~

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.

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~

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

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

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

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

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

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

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>

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.

src/applications/differential/__tests__/data/whitespace.diff.one.whitespace

@@ -0,0 +1,2 @@
+O 1 - -=[-Rocket-Ship>\n~
+N 1 + {(          )}-=[-Rocket-Ship>\n~

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.

src/applications/differential/__tests__/data/whitespace.diff.two.whitespace

@@ -0,0 +1,2 @@
+O 1 - -=[-Rocket-Ship>\n~
+N 1 + {(          )}-=[-Rocket-Ship>\n~

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -612,11 +612,6 @@ final class DifferentialChangesetParser {
     $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 @@ final class DifferentialChangesetParser {
           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 @@ final class DifferentialChangesetParser {
         $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 @@ final class DifferentialChangesetParser {
       }
     }
 
-    if ($shield) {
+    if ($shield !== null) {
       return $renderer->renderChangesetTable($shield);
     }
 

src/applications/differential/parser/DifferentialHunkParser.php

@@ -96,7 +96,7 @@ final class DifferentialHunkParser {
   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 @@ final class DifferentialHunkParser {
   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;

src/applications/differential/render/DifferentialChangesetTestRenderer.php

@@ -7,13 +7,16 @@ abstract class DifferentialChangesetTestRenderer
     $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 @@ abstract class DifferentialChangesetTestRenderer
 
     $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(
+              '<span class="bright">',
+              '</span>',
+            ),
+            array(
+              '{(',
+              ')}',
+            ),
+            $render);
+
+          $render = html_entity_decode($render);
+
           $t = ($type == 'old') ? 'O' : 'N';
 
           $out[] = "{$t} {$num} {$htype} {$render}~";
@@ -70,6 +104,14 @@ abstract class DifferentialChangesetTestRenderer
       }
     }
 
+    if (!$any_old) {
+      $out[] = 'O X <EMPTY>';
+    }
+
+    if (!$any_new) {
+      $out[] = 'N X <EMPTY>';
+    }
+
     $out = implode("\n", $out)."\n";
     return $out;
   }