Move "Rendering References" to the DifferentialChangesetParser level

Summary:
Separates changeset IDs from rendering. Now each changeset has a "rendering
reference" which is basically a description of what the ajax endpoint should
render. For Differential, it's in the form "id/vs". For Diffusion,
"branch/path;commit".

I believe this fixes pretty much all of the bugs related to "show more" breaking
in various obscure ways, although I never got a great repro for T153.

Test Plan:
Clicked "show more" in diffusion change and commit views and differential diff,
diff-of-diff, standalone-diff, standalone-diff-of-diff views. Verified refs and
'whitespace' were always sent correctly.

Made inline comments on diffs and diffs-of-diffs. Used "Reply".

Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 274

src/applications/differential/controller/changesetview/DifferentialChangesetViewController.php

@@ -24,9 +24,17 @@ class DifferentialChangesetViewController extends DifferentialController {
 
     $author_phid = $request->getUser()->getPHID();
 
-    $id = $request->getStr('id');
+    $rendering_reference = $request->getStr('ref');
-    $vs = $request->getInt('vs');
+    $parts = explode('/', $rendering_reference);
+    if (count($parts) == 2) {
+      list($id, $vs) = $parts;
+    } else {
+      $id = $parts[0];
+      $vs = 0;
+    }
 
+    $id = (int)$id;
+    $vs = (int)$vs;
 
     $changeset = id(new DifferentialChangeset())->load($id);
     if (!$changeset) {
@@ -149,6 +157,7 @@ class DifferentialChangesetViewController extends DifferentialController {
 
     $parser = new DifferentialChangesetParser();
     $parser->setChangeset($changeset);
+    $parser->setRenderingReference($rendering_reference);
     $parser->setRenderCacheKey($render_cache_key);
     $parser->setRightSideCommentMapping($right_source, $right_new);
     $parser->setLeftSideCommentMapping($left_source, $left_new);

src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php

@@ -58,7 +58,7 @@ class DifferentialRevisionViewController extends DifferentialController {
       $diff_vs = null;
     }
 
-    list($changesets, $vs_map) =
+    list($changesets, $vs_map, $rendering_references) =
       $this->loadChangesetsAndVsMap($diffs, $diff_vs, $target);
 
     $comments = $revision->loadComments();
@@ -181,7 +181,7 @@ class DifferentialRevisionViewController extends DifferentialController {
     $changeset_view->setEditable(true);
     $changeset_view->setStandaloneViews(true);
     $changeset_view->setRevision($revision);
-    $changeset_view->setVsMap($vs_map);
+    $changeset_view->setRenderingReferences($rendering_references);
     $changeset_view->setWhitespace($whitespace);
 
     $diff_history = new DifferentialRevisionUpdateHistoryView();
@@ -604,6 +604,11 @@ class DifferentialRevisionViewController extends DifferentialController {
     $changesets = idx($changeset_groups, $target->getID(), array());
     $changesets = mpull($changesets, null, 'getID');
 
+    $refs = array();
+    foreach ($changesets as $changeset) {
+      $refs[$changeset->getID()] = $changeset->getID();
+    }
+
     $vs_map = array();
     if ($diff_vs) {
       $vs_changesets = idx($changeset_groups, $diff_vs, array());
@@ -612,18 +617,23 @@ class DifferentialRevisionViewController extends DifferentialController {
         $file = $changeset->getFilename();
         if (isset($vs_changesets[$file])) {
           $vs_map[$changeset->getID()] = $vs_changesets[$file]->getID();
+          $refs[$changeset->getID()] =
+            $changeset->getID().'/'.$vs_changesets[$file]->getID();
           unset($vs_changesets[$file]);
+        } else {
+          $refs[$changeset->getID()] = $changeset->getID();
         }
       }
       foreach ($vs_changesets as $changeset) {
         $changesets[$changeset->getID()] = $changeset;
         $vs_map[$changeset->getID()] = -1;
+        $refs[$changeset->getID()] = $changeset->getID().'/-1';
       }
     }
 
     $changesets = msort($changesets, 'getSortKey');
 
-    return array($changesets, $vs_map);
+    return array($changesets, $vs_map, $refs);
   }
 
   private function updateViewTime($user_phid, $revision_phid) {

src/applications/differential/parser/changeset/DifferentialChangesetParser.php

@@ -28,7 +28,6 @@ class DifferentialChangesetParser {
 
   protected $filename     = null;
   protected $filetype     = null;
-  protected $changesetID  = null;
   protected $missingOld   = array();
   protected $missingNew   = array();
 
@@ -52,6 +51,8 @@ class DifferentialChangesetParser {
   private $rightSideChangesetID;
   private $rightSideAttachesToNewFile;
 
+  private $renderingReference;
+
   const CACHE_VERSION = 4;
 
   const ATTR_GENERATED  = 'attr:generated';
@@ -123,9 +124,7 @@ class DifferentialChangesetParser {
 
   public function setChangeset($changeset) {
     $this->changeset = $changeset;
-
     $this->setFilename($changeset->getFilename());
-    $this->setChangesetID($changeset->getID());
 
     return $this;
   }
@@ -135,8 +134,8 @@ class DifferentialChangesetParser {
     return $this;
   }
 
-  public function setChangesetID($changeset_id) {
+  public function setRenderingReference($ref) {
-    $this->changesetID = $changeset_id;
+    $this->renderingReference = $ref;
     return $this;
   }
 
@@ -144,10 +143,6 @@ class DifferentialChangesetParser {
     return $this->changeset;
   }
 
-  public function getChangesetID() {
-    return $this->changesetID;
-  }
-
   public function setFilename($filename) {
     $this->filename = $filename;
     if (strpos($filename, '.', 1) !== false) {
@@ -968,7 +963,7 @@ EOSYNTHETIC;
 
     if ($more) {
       $end = $this->getLength();
-      $reference = $this->getChangeset()->getRenderingReference();
+      $reference = $this->renderingReference;
       $more =
         ' '.
         javelin_render_tag(
@@ -979,7 +974,7 @@ EOSYNTHETIC;
             'class'       => 'complete',
             'href'        => '#',
             'meta'        => array(
-              'id'    => $reference,
+              'ref'         => $reference,
               'range'       => "0-{$end}",
             ),
           ),
@@ -1073,7 +1068,7 @@ EOSYNTHETIC;
 
     $gaps = array_reverse($gaps);
 
-    $reference = $this->getChangeset()->getRenderingReference();
+    $reference = $this->renderingReference;
 
     $left_id = $this->leftSideChangesetID;
     $right_id = $this->rightSideChangesetID;
@@ -1112,7 +1107,7 @@ EOSYNTHETIC;
               'mustcapture' => true,
               'sigil'       => 'show-more',
               'meta'        => array(
-                'id'    => $reference,
+                'ref'    => $reference,
                 'range' => "{$top}-{$len}/{$top}-20",
               ),
             ),
@@ -1126,7 +1121,7 @@ EOSYNTHETIC;
             'mustcapture' => true,
             'sigil'       => 'show-more',
             'meta'        => array(
-              'id'    => $reference,
+              'ref'    => $reference,
               'range' => "{$top}-{$len}/{$top}-{$len}",
             ),
           ),
@@ -1140,7 +1135,7 @@ EOSYNTHETIC;
               'mustcapture' => true,
               'sigil'       => 'show-more',
               'meta'        => array(
-                'id'    => $reference,
+                'ref'    => $reference,
                 'range' => "{$top}-{$len}/{$end}-20",
               ),
             ),

src/applications/differential/storage/changeset/DifferentialChangeset.php

@@ -32,7 +32,6 @@ class DifferentialChangeset extends DifferentialDAO {
 
   private $unsavedHunks = array();
   private $hunks;
-  private $renderingReference;
 
   protected function getConfiguration() {
     return array(
@@ -76,18 +75,6 @@ class DifferentialChangeset extends DifferentialDAO {
     return $name;
   }
 
-  public function setRenderingReference($rendering_reference) {
-    $this->renderingReference = $rendering_reference;
-    return $this;
-  }
-
-  public function getRenderingReference() {
-    if ($this->renderingReference) {
-      return $this->renderingReference;
-    }
-    return $this->getID();
-  }
-
   public function addUnsavedHunk(DifferentialHunk $hunk) {
     if ($this->hunks === null) {
       $this->hunks = array();

src/applications/differential/view/changesetlistview/DifferentialChangesetListView.php

@@ -22,7 +22,6 @@ class DifferentialChangesetListView extends AphrontView {
   private $editable;
   private $revision;
   private $renderURI = '/differential/changeset/';
-  private $vsMap = array();
   private $whitespace;
   private $standaloneViews;
 
@@ -46,8 +45,8 @@ class DifferentialChangesetListView extends AphrontView {
     return $this;
   }
 
-  public function setVsMap(array $vs_map) {
+  public function setRenderingReferences(array $references) {
-    $this->vsMap = $vs_map;
+    $this->references = $references;
     return $this;
   }
 
@@ -64,7 +63,6 @@ class DifferentialChangesetListView extends AphrontView {
   public function render() {
     require_celerity_resource('differential-changeset-view-css');
 
-    $vs_map = $this->vsMap;
     $changesets = $this->changesets;
 
     $output = array();
@@ -75,22 +73,15 @@ class DifferentialChangesetListView extends AphrontView {
       if (!$this->editable) {
         $class .= ' differential-changeset-noneditable';
       }
-      $id = $changeset->getID();
-      if ($id) {
-        $vs_id = idx($vs_map, $id);
-      } else {
-        $vs_id = null;
-      }
 
-      $ref = $changeset->getRenderingReference();
+      $ref = $this->references[$key];
 
       $detail_button = null;
       if ($this->standaloneViews) {
         $detail_uri = new PhutilURI($this->renderURI);
         $detail_uri->setQueryParams(
           array(
-            'id'          => $ref,
+            'ref'         => $ref,
-            'vs'          => $vs_id,
             'whitespace'  => $this->whitespace,
           ));
 
@@ -118,9 +109,7 @@ class DifferentialChangesetListView extends AphrontView {
           '<div class="differential-loading">Loading...</div>'));
       $output[] = $detail->render();
 
-      $mapping[$uniq_id] = array(
+      $mapping[$uniq_id] = $ref;
-        $ref,
-        $vs_id);
     }
 
     Javelin::initBehavior('differential-populate', array(
@@ -131,6 +120,7 @@ class DifferentialChangesetListView extends AphrontView {
 
     Javelin::initBehavior('differential-show-more', array(
       'uri' => $this->renderURI,
+      'whitespace' => $this->whitespace,
     ));
 
     Javelin::initBehavior('differential-comment-jump', array());

src/applications/differential/view/changesetlistview/__init__.php

@@ -13,7 +13,6 @@ phutil_require_module('phabricator', 'view/base');
 
 phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'parser/uri');
-phutil_require_module('phutil', 'utils');
 
 
 phutil_require_source('DifferentialChangesetListView.php');

src/applications/diffusion/controller/change/DiffusionChangeController.php

@@ -32,9 +32,18 @@ class DiffusionChangeController extends DiffusionController {
     }
 
     $changeset_view = new DifferentialChangesetListView();
-    $changeset_view->setChangesets(array($changeset));
+    $changeset_view->setChangesets(
+      array(
+        0 => $changeset,
+      ));
+    $changeset_view->setRenderingReferences(
+      array(
+        0 => $diff_query->getRenderingReference(),
+      ));
     $changeset_view->setRenderURI(
       '/diffusion/'.$drequest->getRepository()->getCallsign().'/diff/');
+    $changeset_view->setWhitespace(
+      DifferentialChangesetParser::WHITESPACE_SHOW_ALL);
 
     $content[] = $this->buildCrumbs(
       array(

src/applications/diffusion/controller/change/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'aphront/response/404');
+phutil_require_module('phabricator', 'applications/differential/parser/changeset');
 phutil_require_module('phabricator', 'applications/differential/view/changesetlistview');
 phutil_require_module('phabricator', 'applications/diffusion/controller/base');
 phutil_require_module('phabricator', 'applications/diffusion/query/diff/base');

src/applications/diffusion/controller/commit/DiffusionCommitController.php

@@ -146,6 +146,7 @@ class DiffusionCommitController extends DiffusionController {
             throw new Exception("Unknown VCS.");
         }
 
+        $references = array();
         foreach ($changesets as $key => $changeset) {
           $file_type = $changeset->getFileType();
           if ($file_type == DifferentialChangeType::FILE_DIRECTORY) {
@@ -160,11 +161,12 @@ class DiffusionCommitController extends DiffusionController {
           $filename = $changeset->getFilename();
           $commit = $drequest->getCommit();
           $reference = "{$branch}{$filename};{$commit}";
-          $changeset->setRenderingReference($reference);
+          $references[$key] = $reference;
         }
 
         $change_list = new DifferentialChangesetListView();
         $change_list->setChangesets($changesets);
+        $change_list->setRenderingReferences($references);
         $change_list->setRenderURI('/diffusion/'.$callsign.'/diff/');
 
         // TODO: This is pretty awkward, unify the CSS between Diffusion and

src/applications/diffusion/controller/diff/DiffusionDiffController.php

@@ -20,8 +20,8 @@ class DiffusionDiffController extends DiffusionController {
 
   public function willProcessRequest(array $data) {
     $request = $this->getRequest();
-    if ($request->getStr('id')) {
+    if ($request->getStr('ref')) {
-      $parts = explode(';', $request->getStr('id'));
+      $parts = explode(';', $request->getStr('ref'));
       $data['path'] = idx($parts, 0);
       $data['commit'] = idx($parts, 1);
     }
@@ -43,6 +43,7 @@ class DiffusionDiffController extends DiffusionController {
 
     $parser = new DifferentialChangesetParser();
     $parser->setChangeset($changeset);
+    $parser->setRenderingReference($diff_query->getRenderingReference());
     $parser->setWhitespaceMode(
       DifferentialChangesetParser::WHITESPACE_SHOW_ALL);
 

src/applications/diffusion/controller/lastmodified/DiffusionLastModifiedController.php

@@ -27,7 +27,7 @@ class DiffusionLastModifiedController extends DiffusionController {
     list($commit, $commit_data) = $modified_query->loadLastModification();
 
     $phids = array();
-    if ($commit_data->getCommitDetail('authorPHID')) {
+    if ($commit_data && $commit_data->getCommitDetail('authorPHID')) {
       $phids = array($commit_data->getCommitDetail('authorPHID'));
     }
 

src/applications/diffusion/query/diff/base/DiffusionDiffQuery.php

@@ -19,6 +19,7 @@
 abstract class DiffusionDiffQuery {
 
   private $request;
+  protected $renderingReference;
 
   final private function __construct() {
     // <private>
@@ -52,6 +53,10 @@ abstract class DiffusionDiffQuery {
     return $this->request;
   }
 
+  final public function getRenderingReference() {
+    return $this->renderingReference;
+  }
+
   final public function loadChangeset() {
     return $this->executeQuery();
   }

src/applications/diffusion/query/diff/git/DiffusionGitDiffQuery.php

@@ -59,13 +59,11 @@ final class DiffusionGitDiffQuery extends DiffusionDiffQuery {
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 
-    $id =
+    $this->renderingReference =
       $drequest->getBranchURIComponent($drequest->getBranch()).
       $drequest->getPath().';'.
       $drequest->getCommit();
 
-    $changeset->setID($id);
-
     return $changeset;
   }
 

src/applications/diffusion/query/diff/svn/DiffusionSvnDiffQuery.php

@@ -125,8 +125,7 @@ final class DiffusionSvnDiffQuery extends DiffusionDiffQuery {
     $changesets = $diff->getChangesets();
     $changeset = reset($changesets);
 
-    $reference = $drequest->getPath().';'.$drequest->getCommit();
+    $this->renderingReference = $drequest->getPath().';'.$drequest->getCommit();
-    $changeset->setRenderingReference($reference);
 
     return $changeset;
   }

webroot/rsrc/js/application/differential/behavior-populate.js

@@ -15,8 +15,7 @@ JX.behavior('differential-populate', function(config) {
   for (var k in config.registry) {
     new JX.Request(config.uri, JX.bind(null, onresponse, k))
       .setData({
-        id: config.registry[k][0],
+        ref : config.registry[k],
-        vs: config.registry[k][1],
         whitespace: config.whitespace
       })
       .send();