Implement basic one-up and test renderers

Summary:
This is a half-step toward one-up and test renderers. This is mostly a structural change, and has no user-facing impact. It splits the rendering hierarchy like this:

  - Renderer (more methods are abstract than before)
    - HTML Renderer (most HTML stuff has moved down from the base to here)
      - HTML 1-up (placeholder only -- not yet a functional implementation)
      - HTML 2-up (minimal changes, uses mostly old code)
    - Test Renderer (unit-testable renderer base, implements text versions of the HTML stuff)
      - Test 1-up (selects 1-up mode for test rendering)
      - Test 2-up (selects 2-up mode for test rendering)

Broadly, I'm trying to share as much code as possible by splitting rendering into more, smaller stages. Specifically, we do this:

  - Combine the various sorts of inputs (changes, context, inlines, etc.) into a single, relatively homogenous list of "primitives". This happens in the base class.
    - The primitive types are: old (diff left side), new (diff right side), context ("show more context"), no-context ("context not available") and inline (inline comment).
  - Possibly, apply a filtering/reordering step to the primitives to get them ready for 1-up rendering. This mostly removes information, and does a small amount of reordering. This also happens in the base class.
  - Pass the primitives to the actual renderer, to convert them into HTML, text, or whatever else. This happens in the leaf class.

The primitive implementation is not yet complete (it doesn't attach as much information to the primitives as it should -- stuff like coverage and copies), but covers the basics.

The existing HTMLTwoUp renderer does not use the primitive path; instead, it still goes down the old path.

Test Plan: Ran unit tests, looked at a bunch of diffs.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2009

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

src/__phutil_library_map__.php

@@ -225,11 +225,16 @@ phutil_register_library_map(array(
     'DifferentialChangeset' => 'applications/differential/storage/DifferentialChangeset.php',
     'DifferentialChangesetDetailView' => 'applications/differential/view/DifferentialChangesetDetailView.php',
     'DifferentialChangesetFileTreeSideNavBuilder' => 'applications/differential/view/DifferentialChangesetFileTreeSideNavBuilder.php',
+    'DifferentialChangesetHTMLRenderer' => 'applications/differential/render/DifferentialChangesetHTMLRenderer.php',
     'DifferentialChangesetListView' => 'applications/differential/view/DifferentialChangesetListView.php',
+    'DifferentialChangesetOneUpRenderer' => 'applications/differential/render/DifferentialChangesetOneUpRenderer.php',
+    'DifferentialChangesetOneUpTestRenderer' => 'applications/differential/render/DifferentialChangesetOneUpTestRenderer.php',
     'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php',
     'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php',
     'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php',
+    'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php',
     'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php',
+    'DifferentialChangesetTwoUpTestRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php',
     'DifferentialChangesetViewController' => 'applications/differential/controller/DifferentialChangesetViewController.php',
     'DifferentialComment' => 'applications/differential/storage/DifferentialComment.php',
     'DifferentialCommentEditor' => 'applications/differential/editor/DifferentialCommentEditor.php',
@@ -281,6 +286,7 @@ phutil_register_library_map(array(
     'DifferentialMailPhase' => 'applications/differential/constants/DifferentialMailPhase.php',
     'DifferentialManiphestTasksFieldSpecification' => 'applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php',
     'DifferentialNewDiffMail' => 'applications/differential/mail/DifferentialNewDiffMail.php',
+    'DifferentialParseRenderTestCase' => 'applications/differential/__tests__/DifferentialParseRenderTestCase.php',
     'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/DifferentialPathFieldSpecification.php',
     'DifferentialPrimaryPaneView' => 'applications/differential/view/DifferentialPrimaryPaneView.php',
     'DifferentialReplyHandler' => 'applications/differential/DifferentialReplyHandler.php',
@@ -1624,9 +1630,14 @@ phutil_register_library_map(array(
     'DifferentialCCsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialChangeset' => 'DifferentialDAO',
     'DifferentialChangesetDetailView' => 'AphrontView',
+    'DifferentialChangesetHTMLRenderer' => 'DifferentialChangesetRenderer',
     'DifferentialChangesetListView' => 'AphrontView',
-    'DifferentialChangesetParserTestCase' => 'ArcanistPhutilTestCase',
+    'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer',
-    'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetRenderer',
+    'DifferentialChangesetOneUpTestRenderer' => 'DifferentialChangesetTestRenderer',
+    'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase',
+    'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer',
+    'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer',
+    'DifferentialChangesetTwoUpTestRenderer' => 'DifferentialChangesetTestRenderer',
     'DifferentialChangesetViewController' => 'DifferentialController',
     'DifferentialComment' =>
     array(
@@ -1680,6 +1691,7 @@ phutil_register_library_map(array(
     'DifferentialLocalCommitsView' => 'AphrontView',
     'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail',
+    'DifferentialParseRenderTestCase' => 'PhabricatorTestCase',
     'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialPrimaryPaneView' => 'AphrontView',
     'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler',

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php

@@ -0,0 +1,63 @@
+<?php
+
+final class DifferentialParseRenderTestCase extends PhabricatorTestCase {
+
+  public function testParseRender() {
+    $dir = dirname(__FILE__).'/data/';
+    foreach (Filesystem::listDirectory($dir, $show_hidden = false) as $file) {
+      if (!preg_match('/\.diff$/', $file)) {
+        continue;
+      }
+      $data = Filesystem::readFile($dir.$file);
+
+      $opt_file = $dir.$file.'.options';
+      if (Filesystem::pathExists($opt_file)) {
+        $options = Filesystem::readFile($opt_file);
+        $options = json_decode($options, true);
+        if (!is_array($options)) {
+          throw new Exception("Invalid options file: {$opt_file}.");
+        }
+      } else {
+        $options = array();
+      }
+
+      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, $actual, $file.'.'.$type);
+      }
+    }
+  }
+
+  private function buildChangesetParser($type, $data, $file) {
+    $parser = new ArcanistDiffParser();
+    $changes = $parser->parseDiff($data);
+
+    $diff = DifferentialDiff::newFromRawChanges($changes);
+    if (count($diff->getChangesets()) !== 1) {
+      throw new Exception("Expected one changeset: {$file}");
+    }
+
+    $changeset = head($diff->getChangesets());
+
+    $engine = new PhabricatorMarkupEngine();
+
+    $cparser = new DifferentialChangesetParser();
+    $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}'!");
+    }
+
+    return $cparser;
+  }
+
+}

src/applications/differential/__tests__/data/fruit.diff

@@ -0,0 +1,15 @@
+diff --git a/fruit b/fruit
+new file mode 100644
+index 0000000..a1f7255
+--- /dev/null
++++ b/fruit
+@@ -0,0 +1,9 @@
++apple
++banana
++cherry
++date
++elderberry
++fig
++grape
++honeydew
++

src/applications/differential/__tests__/data/fruit.diff.one.expect

@@ -0,0 +1,12 @@
+CTYPE 1 1 (unforced)
+-
+-
+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 + ~

src/applications/differential/__tests__/data/fruit.diff.two.expect

@@ -0,0 +1,21 @@
+CTYPE 1 1 (unforced)
+-
+-
+O - . ~
+N 1 + apple~
+O - . ~
+N 2 + banana~
+O - . ~
+N 3 + cherry~
+O - . ~
+N 4 + date~
+O - . ~
+N 5 + elderberry~
+O - . ~
+N 6 + fig~
+O - . ~
+N 7 + grape~
+O - . ~
+N 8 + honeydew~
+O - . ~
+N 9 + ~

src/applications/differential/parser/DifferentialChangesetParser.php

@@ -39,6 +39,29 @@ final class DifferentialChangesetParser {
   private $coverage;
   private $markupEngine;
   private $highlightErrors;
+  private $disableCache;
+  private $renderer;
+
+  public function setRenderer($renderer) {
+    $this->renderer = $renderer;
+    return $this;
+  }
+
+  public function getRenderer() {
+    if (!$this->renderer) {
+      return new DifferentialChangesetTwoUpRenderer();
+    }
+    return $this->renderer;
+  }
+
+  public function setDisableCache($disable_cache) {
+    $this->disableCache = $disable_cache;
+    return $this;
+  }
+
+  public function getDisableCache() {
+    return $this->disableCache;
+  }
 
   const CACHE_VERSION = 10;
   const CACHE_MAX_SIZE = 8e6;
@@ -434,6 +457,10 @@ final class DifferentialChangesetParser {
     }
 
     $skip_cache = ($whitespace_mode != self::WHITESPACE_IGNORE_ALL);
+    if ($this->disableCache) {
+      $skip_cache = true;
+    }
+
     $this->whitespaceMode = $whitespace_mode;
 
     $changeset = $this->changeset;
@@ -664,7 +691,7 @@ final class DifferentialChangesetParser {
       count($this->old),
       count($this->new));
 
-    $renderer = id(new DifferentialChangesetTwoUpRenderer())
+    $renderer = $this->getRenderer()
       ->setChangeset($this->changeset)
       ->setRenderPropertyChangeHeader($render_pch)
       ->setLineCount($rows)

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

@@ -1,6 +1,6 @@
 <?php
 
-final class DifferentialChangesetParserTestCase extends ArcanistPhutilTestCase {
+final class DifferentialChangesetParserTestCase extends PhabricatorTestCase {
 
   public function testDiffChangesets() {
     $hunk = new DifferentialHunk();

src/applications/differential/render/DifferentialChangesetHTMLRenderer.php

@@ -0,0 +1,362 @@
+<?php
+
+abstract class DifferentialChangesetHTMLRenderer
+  extends DifferentialChangesetRenderer {
+
+  protected function renderChangeTypeHeader($force) {
+    $changeset = $this->getChangeset();
+
+    $change = $changeset->getChangeType();
+    $file = $changeset->getFileType();
+
+    $message = null;
+    if ($change == DifferentialChangeType::TYPE_CHANGE &&
+        $file   == DifferentialChangeType::FILE_TEXT) {
+      if ($force) {
+        // We have to force something to render because there were no changes
+        // of other kinds.
+        $message = pht('This file was not modified.');
+      } else {
+        // Default case of changes to a text file, no metadata.
+        return null;
+      }
+    } else {
+      switch ($change) {
+
+        case DifferentialChangeType::TYPE_ADD:
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was <strong>added</strong>.');
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was <strong>added</strong>.');
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was <strong>added</strong>.');
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was <strong>added</strong>.');
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was <strong>added</strong>.');
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was <strong>added</strong>.');
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_DELETE:
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was <strong>deleted</strong>.');
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was <strong>deleted</strong>.');
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was <strong>deleted</strong>.');
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was <strong>deleted</strong>.');
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was <strong>deleted</strong>.');
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was <strong>deleted</strong>.');
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_MOVE_HERE:
+          $from =
+            "<strong>".
+              phutil_escape_html($changeset->getOldFile()).
+            "</strong>";
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was moved from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was moved from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was moved from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was moved from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was moved from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was moved from %s.', $from);
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_COPY_HERE:
+          $from =
+            "<strong>".
+              phutil_escape_html($changeset->getOldFile()).
+            "</strong>";
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was copied from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was copied from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was copied from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was copied from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was copied from %s.', $from);
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was copied from %s.', $from);
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_MOVE_AWAY:
+          $paths =
+            "<strong>".
+              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
+            "</strong>";
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was moved to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was moved to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was moved to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was moved to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was moved to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was moved to %s.', $paths);
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_COPY_AWAY:
+          $paths =
+            "<strong>".
+              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
+            "</strong>";
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This file was copied to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This image was copied to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This directory was copied to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This binary file was copied to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This symlink was copied to %s.', $paths);
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This submodule was copied to %s.', $paths);
+              break;
+          }
+          break;
+
+        case DifferentialChangeType::TYPE_MULTICOPY:
+          $paths =
+            "<strong>".
+              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
+            "</strong>";
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht(
+                'This file was deleted after being copied to %s.',
+                $paths);
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht(
+                'This image was deleted after being copied to %s.',
+                $paths);
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht(
+                'This directory was deleted after being copied to %s.',
+                $paths);
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht(
+                'This binary file was deleted after being copied to %s.',
+                $paths);
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht(
+                'This symlink was deleted after being copied to %s.',
+                $paths);
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht(
+                'This submodule was deleted after being copied to %s.',
+                $paths);
+              break;
+          }
+          break;
+
+        default:
+          switch ($file) {
+            case DifferentialChangeType::FILE_TEXT:
+              $message = pht('This is a file.');
+              break;
+            case DifferentialChangeType::FILE_IMAGE:
+              $message = pht('This is an image.');
+              break;
+            case DifferentialChangeType::FILE_DIRECTORY:
+              $message = pht('This is a directory.');
+              break;
+            case DifferentialChangeType::FILE_BINARY:
+              $message = pht('This is a binary file.');
+              break;
+            case DifferentialChangeType::FILE_SYMLINK:
+              $message = pht('This is a symlink.');
+              break;
+            case DifferentialChangeType::FILE_SUBMODULE:
+              $message = pht('This is a submodule.');
+              break;
+          }
+          break;
+      }
+    }
+
+    return
+      '<div class="differential-meta-notice">'.
+        $message.
+      '</div>';
+  }
+
+  protected function renderPropertyChangeHeader() {
+    $changeset = $this->getChangeset();
+
+    $old = $changeset->getOldProperties();
+    $new = $changeset->getNewProperties();
+
+    $keys = array_keys($old + $new);
+    sort($keys);
+
+    $rows = array();
+    foreach ($keys as $key) {
+      $oval = idx($old, $key);
+      $nval = idx($new, $key);
+      if ($oval !== $nval) {
+        if ($oval === null) {
+          $oval = '<em>null</em>';
+        } else {
+          $oval = nl2br(phutil_escape_html($oval));
+        }
+
+        if ($nval === null) {
+          $nval = '<em>null</em>';
+        } else {
+          $nval = nl2br(phutil_escape_html($nval));
+        }
+
+        $rows[] =
+          '<tr>'.
+            '<th>'.phutil_escape_html($key).'</th>'.
+            '<td class="oval">'.$oval.'</td>'.
+            '<td class="nval">'.$nval.'</td>'.
+          '</tr>';
+      }
+    }
+
+    return
+      '<table class="differential-property-table">'.
+        '<tr class="property-table-header">'.
+          '<th>Property Changes</th>'.
+          '<td class="oval">Old Value</td>'.
+          '<td class="nval">New Value</td>'.
+        '</tr>'.
+        implode('', $rows).
+      '</table>';
+  }
+
+  protected function renderShield($message, $force = 'default') {
+    $end = count($this->getOldLines());
+    $reference = $this->getRenderingReference();
+
+    if ($force !== 'text' &&
+        $force !== 'whitespace' &&
+        $force !== 'none' &&
+        $force !== 'default') {
+      throw new Exception("Invalid 'force' parameter '{$force}'!");
+    }
+
+    $range = "0-{$end}";
+    if ($force == 'text') {
+      // If we're forcing text, force the whole file to be rendered.
+      $range = "{$range}/0-{$end}";
+    }
+
+    $meta = array(
+      'ref'   => $reference,
+      'range' => $range,
+    );
+
+    if ($force == 'whitespace') {
+      $meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL;
+    }
+
+    $more = null;
+    if ($force !== 'none') {
+      $more = ' '.javelin_render_tag(
+        'a',
+        array(
+          'mustcapture' => true,
+          'sigil'       => 'show-more',
+          'class'       => 'complete',
+          'href'        => '#',
+          'meta'        => $meta,
+        ),
+        'Show File Contents');
+    }
+
+    return javelin_render_tag(
+      'tr',
+      array(
+        'sigil' => 'context-target',
+      ),
+      '<td class="differential-shield" colspan="6">'.
+        phutil_escape_html($message).
+        $more.
+      '</td>');
+  }
+
+  protected function wrapChangeInTable($content) {
+    if (!$content) {
+      return null;
+    }
+
+    return javelin_render_tag(
+      'table',
+      array(
+        'class' => 'differential-diff remarkup-code PhabricatorMonospaced',
+        'sigil' => 'differential-diff',
+      ),
+      $content);
+  }
+
+
+}

src/applications/differential/render/DifferentialChangesetOneUpRenderer.php

@@ -0,0 +1,28 @@
+<?php
+
+final class DifferentialChangesetOneUpRenderer
+  extends DifferentialChangesetHTMLRenderer {
+
+  public function isOneUpRenderer() {
+    return true;
+  }
+
+  public function renderChangesetTable($contents) {
+    throw new Exception("Not implemented!");
+  }
+
+  public function renderTextChange(
+    $range_start,
+    $range_len,
+    $rows) {
+    throw new Exception("Not implemented!");
+  }
+
+  public function renderFileChange($old_file = null,
+                                   $new_file = null,
+                                   $id = 0,
+                                   $vs = 0) {
+    throw new Exception("Not implemented!");
+  }
+
+}

src/applications/differential/render/DifferentialChangesetOneUpTestRenderer.php

@@ -0,0 +1,10 @@
+<?php
+
+final class DifferentialChangesetOneUpTestRenderer
+  extends DifferentialChangesetTestRenderer {
+
+  public function isOneUpRenderer() {
+    return true;
+  }
+
+}

src/applications/differential/render/DifferentialChangesetRenderer.php

@@ -245,7 +245,26 @@ abstract class DifferentialChangesetRenderer {
     return $this->renderPropertyChangeHeader;
   }
 
-  abstract public function renderChangesetTable($contents);
+  final public function renderChangesetTable($content) {
+    $props = null;
+    if ($this->shouldRenderPropertyChangeHeader()) {
+      $props = $this->renderPropertyChangeHeader();
+    }
+
+    $force = (!$content && !$props);
+    $notice = $this->renderChangeTypeHeader($force);
+
+    $result = $notice.$props.$content;
+
+    // TODO: Let the user customize their tab width / display style.
+    // TODO: We should possibly post-process "\r" as well.
+    // TODO: Both these steps should happen earlier.
+    $result = str_replace("\t", '  ', $result);
+
+    return $result;
+  }
+
+  abstract public function isOneUpRenderer();
   abstract public function renderTextChange(
     $range_start,
     $range_len,
@@ -258,6 +277,12 @@ abstract class DifferentialChangesetRenderer {
     $vs = 0
   );
 
+  abstract protected function renderChangeTypeHeader($force);
+
+  protected function didRenderChangesetTableContents($contents) {
+    return $contents;
+  }
+
   /**
    * Render a "shield" over the diff, with a message like "This file is
    * generated and does not need to be reviewed." or "This file was completely
@@ -282,348 +307,9 @@ abstract class DifferentialChangesetRenderer {
    * @param   string|null   Force mode, see above.
    * @return  string        Shield markup.
    */
-  public function renderShield($message, $force = 'default') {
+  abstract protected function renderShield($message, $force = 'default');
 
-    $end = $this->getLineCount();
+  abstract protected function renderPropertyChangeHeader();
-    $reference = $this->getRenderingReference();
-
-    if ($force !== 'text' &&
-        $force !== 'whitespace' &&
-        $force !== 'none' &&
-        $force !== 'default') {
-      throw new Exception("Invalid 'force' parameter '{$force}'!");
-    }
-
-    $range = "0-{$end}";
-    if ($force == 'text') {
-      // If we're forcing text, force the whole file to be rendered.
-      $range = "{$range}/0-{$end}";
-    }
-
-    $meta = array(
-      'ref'   => $reference,
-      'range' => $range,
-    );
-
-    if ($force == 'whitespace') {
-      $meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL;
-    }
-
-    $more = null;
-    if ($force !== 'none') {
-      $more = ' '.javelin_render_tag(
-        'a',
-        array(
-          'mustcapture' => true,
-          'sigil'       => 'show-more',
-          'class'       => 'complete',
-          'href'        => '#',
-          'meta'        => $meta,
-        ),
-        'Show File Contents');
-    }
-
-    return javelin_render_tag(
-      'tr',
-      array(
-        'sigil' => 'context-target',
-      ),
-      '<td class="differential-shield" colspan="6">'.
-        phutil_escape_html($message).
-        $more.
-      '</td>');
-  }
-
-
-  protected function renderPropertyChangeHeader($changeset) {
-    if (!$this->shouldRenderPropertyChangeHeader()) {
-      return null;
-    }
-
-    $old = $changeset->getOldProperties();
-    $new = $changeset->getNewProperties();
-
-    $keys = array_keys($old + $new);
-    sort($keys);
-
-    $rows = array();
-    foreach ($keys as $key) {
-      $oval = idx($old, $key);
-      $nval = idx($new, $key);
-      if ($oval !== $nval) {
-        if ($oval === null) {
-          $oval = '<em>null</em>';
-        } else {
-          $oval = nl2br(phutil_escape_html($oval));
-        }
-
-        if ($nval === null) {
-          $nval = '<em>null</em>';
-        } else {
-          $nval = nl2br(phutil_escape_html($nval));
-        }
-
-        $rows[] =
-          '<tr>'.
-            '<th>'.phutil_escape_html($key).'</th>'.
-            '<td class="oval">'.$oval.'</td>'.
-            '<td class="nval">'.$nval.'</td>'.
-          '</tr>';
-      }
-    }
-
-    return
-      '<table class="differential-property-table">'.
-        '<tr class="property-table-header">'.
-          '<th>Property Changes</th>'.
-          '<td class="oval">Old Value</td>'.
-          '<td class="nval">New Value</td>'.
-        '</tr>'.
-        implode('', $rows).
-      '</table>';
-  }
-
-  protected function renderChangeTypeHeader($changeset, $force) {
-    $change = $changeset->getChangeType();
-    $file = $changeset->getFileType();
-
-    $message = null;
-    if ($change == DifferentialChangeType::TYPE_CHANGE &&
-        $file   == DifferentialChangeType::FILE_TEXT) {
-      if ($force) {
-        // We have to force something to render because there were no changes
-        // of other kinds.
-        $message = pht('This file was not modified.');
-      } else {
-        // Default case of changes to a text file, no metadata.
-        return null;
-      }
-    } else {
-      switch ($change) {
-
-        case DifferentialChangeType::TYPE_ADD:
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was <strong>added</strong>.');
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was <strong>added</strong>.');
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was <strong>added</strong>.');
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was <strong>added</strong>.');
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was <strong>added</strong>.');
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was <strong>added</strong>.');
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_DELETE:
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was <strong>deleted</strong>.');
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was <strong>deleted</strong>.');
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was <strong>deleted</strong>.');
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was <strong>deleted</strong>.');
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was <strong>deleted</strong>.');
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was <strong>deleted</strong>.');
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_MOVE_HERE:
-          $from =
-            "<strong>".
-              phutil_escape_html($changeset->getOldFile()).
-            "</strong>";
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was moved from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was moved from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was moved from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was moved from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was moved from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was moved from %s.', $from);
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_COPY_HERE:
-          $from =
-            "<strong>".
-              phutil_escape_html($changeset->getOldFile()).
-            "</strong>";
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was copied from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was copied from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was copied from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was copied from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was copied from %s.', $from);
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was copied from %s.', $from);
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_MOVE_AWAY:
-          $paths =
-            "<strong>".
-              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
-            "</strong>";
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was moved to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was moved to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was moved to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was moved to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was moved to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was moved to %s.', $paths);
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_COPY_AWAY:
-          $paths =
-            "<strong>".
-              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
-            "</strong>";
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This file was copied to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This image was copied to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This directory was copied to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This binary file was copied to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This symlink was copied to %s.', $paths);
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This submodule was copied to %s.', $paths);
-              break;
-          }
-          break;
-
-        case DifferentialChangeType::TYPE_MULTICOPY:
-          $paths =
-            "<strong>".
-              phutil_escape_html(implode(', ', $changeset->getAwayPaths())).
-            "</strong>";
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht(
-                'This file was deleted after being copied to %s.',
-                $paths);
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht(
-                'This image was deleted after being copied to %s.',
-                $paths);
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht(
-                'This directory was deleted after being copied to %s.',
-                $paths);
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht(
-                'This binary file was deleted after being copied to %s.',
-                $paths);
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht(
-                'This symlink was deleted after being copied to %s.',
-                $paths);
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht(
-                'This submodule was deleted after being copied to %s.',
-                $paths);
-              break;
-          }
-          break;
-
-        default:
-          switch ($file) {
-            case DifferentialChangeType::FILE_TEXT:
-              $message = pht('This is a file.');
-              break;
-            case DifferentialChangeType::FILE_IMAGE:
-              $message = pht('This is an image.');
-              break;
-            case DifferentialChangeType::FILE_DIRECTORY:
-              $message = pht('This is a directory.');
-              break;
-            case DifferentialChangeType::FILE_BINARY:
-              $message = pht('This is a binary file.');
-              break;
-            case DifferentialChangeType::FILE_SYMLINK:
-              $message = pht('This is a symlink.');
-              break;
-            case DifferentialChangeType::FILE_SUBMODULE:
-              $message = pht('This is a submodule.');
-              break;
-          }
-          break;
-      }
-    }
-
-    return
-      '<div class="differential-meta-notice">'.
-        $message.
-      '</div>';
-  }
 
   protected function renderInlineComment(
     PhabricatorInlineCommentInterface $comment,
@@ -645,4 +331,174 @@ abstract class DifferentialChangesetRenderer {
       ->render();
   }
 
+  protected function buildPrimitives($range_start, $range_len) {
+    $primitives = array();
+
+    $hunk_starts = $this->getHunkStartLines();
+
+    $mask = $this->getMask();
+    $gaps = $this->getGaps();
+
+    $old = $this->getOldLines();
+    $new = $this->getNewLines();
+    $old_render = $this->getOldRender();
+    $new_render = $this->getNewRender();
+    $old_comments = $this->getOldComments();
+    $new_comments = $this->getNewComments();
+
+    $size = count($old);
+    for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) {
+      if (empty($mask[$ii])) {
+        list($top, $len) = array_pop($gaps);
+        $primitives[] = array(
+          'type' => 'context',
+          'top' => $top,
+          'len' => $len,
+        );
+
+        $ii += ($len - 1);
+        continue;
+      }
+
+      $ospec = array(
+        'type' => 'old',
+        'htype' => null,
+        'cursor' => $ii,
+        'line' => null,
+        'render' => null,
+      );
+
+      $nspec = array(
+        'type' => 'new',
+        'htype' => null,
+        'cursor' => $ii,
+        'line' => null,
+        'render' => null,
+        'copy' => null,
+        'coverage' => null,
+      );
+
+      if (isset($old[$ii])) {
+        $ospec['line'] = $old[$ii]['line'];
+        $ospec['htype'] = $old[$ii]['type'];
+        if (isset($old_render[$ii])) {
+          $ospec['render'] = $old_render[$ii];
+        }
+      }
+
+      if (isset($new[$ii])) {
+        $nspec['line'] = $new[$ii]['line'];
+        $nspec['htype'] = $new[$ii]['type'];
+        if (isset($new_render[$ii])) {
+          $nspec['render'] = $new_render[$ii];
+        }
+      }
+
+      if (isset($hunk_starts[$ospec['line']])) {
+        $primitives[] = array(
+          'type' => 'no-context',
+        );
+      }
+
+      $primitives[] = $ospec;
+      $primitives[] = $nspec;
+
+      if ($ospec['line'] !== null && isset($old_comments[$ospec['line']])) {
+        foreach ($old_comments[$ospec['line']] as $comment) {
+          $primitives[] = array(
+            'type' => 'inline',
+            'comment' => $comment,
+            'right' => false,
+          );
+        }
+      }
+
+      if ($nspec['line'] !== null && isset($new_comments[$nspec['line']])) {
+        foreach ($new_comments[$nspec['line']] as $comment) {
+          $primitives[] = array(
+            'type' => 'inline',
+            'comment' => 'comment',
+            'right' => true,
+          );
+        }
+      }
+
+      if ($hunk_starts && ($ii == $size - 1)) {
+        $primitives[] = array(
+          'type' => 'no-context',
+        );
+      }
+    }
+
+    if ($this->isOneUpRenderer()) {
+      $primitives = $this->processPrimitivesForOneUp($primitives);
+    }
+
+    return $primitives;
+  }
+
+  private function processPrimitivesForOneUp(array $primitives) {
+    // Primitives come out of buildPrimitives() in two-up format, because it
+    // is the most general, flexible format. To put them into one-up format,
+    // we need to filter and reorder them. In particular:
+    //
+    //   - We discard unchanged lines in the old file; in one-up format, we
+    //     render them only once.
+    //   - We group contiguous blocks of old-modified and new-modified lines, so
+    //     they render in "block of old, block of new" order instead of
+    //     alternating old and new lines.
+
+    $out = array();
+
+    $old_buf = array();
+    $new_buf = array();
+    foreach ($primitives as $primitive) {
+      $type = $primitive['type'];
+      if ($type == 'old') {
+        if (!$primitive['htype']) {
+          // This is a line which appears in both the old file and the new
+          // file, or the spacer corresponding to a line added in the new file.
+          // Ignore it when rendering a one-up diff.
+          continue;
+        }
+        if ($new_buf) {
+          $out[] = $new_buf;
+          $new_buf = array();
+        }
+        $old_buf[] = $primitive;
+      } else if ($type == 'new') {
+        if ($primitive['line'] === null) {
+          // This is an empty spacer corresponding to a line removed from the
+          // old file. Ignore it when rendering a one-up diff.
+          continue;
+        }
+        if ($old_buf) {
+          $out[] = $old_buf;
+          $old_buf = array();
+        }
+        $new_buf[] = $primitive;
+      } else if ($type == 'context' || $type == 'no-context') {
+        $out[] = $old_buf;
+        $out[] = $new_buf;
+        $old_buf = array();
+        $new_buf = array();
+        $out[] = array($primitive);
+      } else if ($type == 'inline') {
+        if ($primitive['right']) {
+          $new_buf[] = $primitive;
+        } else {
+          $old_buf[] = $primitive;
+        }
+      } else {
+        throw new Exception("Unknown primitive type '{$primitive}'!");
+      }
+    }
+
+    $out[] = $old_buf;
+    $out[] = $new_buf;
+    $out = array_mergev($out);
+
+    return $out;
+  }
+
 }

src/applications/differential/render/DifferentialChangesetTestRenderer.php

@@ -0,0 +1,86 @@
+<?php
+
+abstract class DifferentialChangesetTestRenderer
+  extends DifferentialChangesetRenderer {
+
+  protected function renderChangeTypeHeader($force) {
+    $changeset = $this->getChangeset();
+
+    $old = nonempty($changeset->getOldFile(), '-');
+    $away = nonempty(implode(', ', $changeset->getAwayPaths()), '-');
+    $ctype = $changeset->getChangeType();
+    $ftype = $changeset->getFileType();
+    $force = ($force ? '(forced)' : '(unforced)');
+
+    return "CTYPE {$ctype} {$ftype} {$force}\n".
+           "{$old}\n".
+           "{$away}\n";
+  }
+
+  protected function renderShield($message, $force = 'default') {
+    return "SHIELD ({$force}) {$message}\n";
+  }
+
+  protected function renderPropertyChangeHeader() {
+    $changeset = $this->getChangeset();
+    $old = $changeset->getOldProperties();
+    $new = $changeset->getNewProperties();
+
+    if (!$old && !$new) {
+      return null;
+    }
+
+    $props = '';
+    foreach ($old as $key => $value) {
+      $props .= "P - {$key} {$value}~\n";
+    }
+    foreach ($new as $key => $value) {
+      $props .= "P + {$key} {$value}~\n";
+    }
+
+    return "PROPERTIES\n".$props;
+  }
+
+  public function renderTextChange(
+    $range_start,
+    $range_len,
+    $rows) {
+
+    $out = array();
+
+    $primitives = $this->buildPrimitives($range_start, $range_len);
+    foreach ($primitives as $p) {
+      $type = $p['type'];
+      switch ($type) {
+        case 'old':
+        case 'new':
+          $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");
+          $t = ($type == 'old') ? 'O' : 'N';
+
+          $out[] = "{$t} {$num} {$htype} {$render}~";
+          break;
+        default:
+          $out[] = $type;
+          break;
+      }
+    }
+
+    $out = implode("\n", $out)."\n";
+    return $out;
+  }
+
+
+  public function renderFileChange($old_file = null,
+                                   $new_file = null,
+                                   $id = 0,
+                                   $vs = 0) {
+    throw new Exception("Not implemented!");
+  }
+
+}

src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php

@@ -1,42 +1,10 @@
 <?php
 
 final class DifferentialChangesetTwoUpRenderer
-  extends DifferentialChangesetRenderer {
+  extends DifferentialChangesetHTMLRenderer {
 
-  public function renderChangesetTable($contents) {
+  public function isOneUpRenderer() {
-    $changeset = $this->getChangeset();
+    return false;
-    $props = $this->renderPropertyChangeHeader($changeset);
-    $table = null;
-    if ($contents) {
-      $table = javelin_render_tag(
-        'table',
-        array(
-          'class' => 'differential-diff remarkup-code PhabricatorMonospaced',
-          'sigil' => 'differential-diff',
-        ),
-        $contents);
-    }
-
-    if (!$table && !$props) {
-      $notice = $this->renderChangeTypeHeader($changeset, true);
-    } else {
-      $notice = $this->renderChangeTypeHeader($changeset, false);
-    }
-
-    $result = implode(
-      "\n",
-      array(
-        $notice,
-        $props,
-        $table,
-      ));
-
-    // TODO: Let the user customize their tab width / display style.
-    $result = str_replace("\t", '  ', $result);
-
-    // TODO: We should possibly post-process "\r" as well.
-
-    return $result;
   }
 
   public function renderTextChange(
@@ -360,7 +328,7 @@ final class DifferentialChangesetTwoUpRenderer
       }
     }
 
-    return implode('', $html);
+    return $this->wrapChangeInTable(implode('', $html));
   }
 
   public function renderFileChange($old_file = null,
@@ -450,7 +418,7 @@ final class DifferentialChangesetTwoUpRenderer
       implode('', $html_old).
       implode('', $html_new));
 
-    return $output;
+    return $this->wrapChangeInTable($output);
   }
 
 }

src/applications/differential/render/DifferentialChangesetTwoUpTestRenderer.php

@@ -0,0 +1,10 @@
+<?php
+
+final class DifferentialChangesetTwoUpTestRenderer
+  extends DifferentialChangesetTestRenderer {
+
+  public function isOneUpRenderer() {
+    return false;
+  }
+
+}