Rough cut of diff-of-diffs.

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

@@ -22,15 +22,91 @@ class DifferentialChangesetViewController extends DifferentialController {
   public function processRequest() {
     $request = $this->getRequest();
 
-    $id = $request->getStr('id');
     $author_phid = $request->getUser()->getPHID();
 
+    $id = $request->getStr('id');
+    $vs = $request->getInt('vs');
+
+
     $changeset = id(new DifferentialChangeset())->load($id);
     if (!$changeset) {
       return new Aphront404Response();
     }
 
-    $changeset->attachHunks($changeset->loadHunks());
+    if ($vs && ($vs != -1)) {
+      $vs_changeset = id(new DifferentialChangeset())->load($vs);
+      if (!$vs_changeset) {
+        return new Aphront404Response();
+      }
+    }
+
+    if (!$vs) {
+      $right = $changeset;
+      $left  = null;
+
+      $right_source = $right->getID();
+      $right_new = true;
+      $left_source = $right->getID();
+      $left_new = false;
+    } else if ($vs == -1) {
+      $right = null;
+      $left = $changeset;
+
+      $right_source = $left->getID();
+      $right_new = false;
+      $left_source = $left->getID();
+      $left_new = true;
+    } else {
+      $right = $changeset;
+      $left = $vs_changeset;
+
+      $right_source = $right->getID();
+      $right_new = true;
+      $left_source = $left->getID();
+      $left_new = true;
+    }
+
+    if ($left) {
+      $left->attachHunks($left->loadHunks());
+    }
+
+    if ($right) {
+      $right->attachHunks($right->loadHunks());
+    }
+
+    if ($left) {
+
+      $left_data = $left->makeNewFile();
+      if ($right) {
+        $right_data = $right->makeNewFile();
+      } else {
+        $right_data = $left->makeOldFile();
+      }
+
+      $left_tmp = new TempFile();
+      $right_tmp = new TempFile();
+      Filesystem::writeFile($left_tmp, $left_data);
+      Filesystem::writeFile($right_tmp, $right_data);
+      list($err, $stdout) = exec_manual(
+        '/usr/bin/diff -U65535 %s %s',
+        $left_tmp,
+        $right_tmp);
+
+      $choice = nonempty($left, $right);
+      if ($stdout) {
+        $parser = new ArcanistDiffParser();
+        $changes = $parser->parseDiff($stdout);
+        $diff = DifferentialDiff::newFromRawChanges($changes);
+        $changesets = $diff->getChangesets();
+        $first = reset($changesets);
+        $choice->attachHunks($first->getHunks());
+      } else {
+        $choice->attachHunks(array());
+      }
+
+      $changeset = $choice;
+      $changeset->setID(null);
+    }
 
     $range_s = null;
     $range_e = null;
@@ -54,6 +130,8 @@ class DifferentialChangesetViewController extends DifferentialController {
 
     $parser = new DifferentialChangesetParser();
     $parser->setChangeset($changeset);
+    $parser->setRightSideCommentMapping($right_source, $right_new);
+    $parser->setLeftSideCommentMapping($left_source, $left_new);
 
     $phids = array();
     $inlines = $this->loadInlineComments($id, $author_phid);

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

@@ -6,17 +6,23 @@
 
 
 
+phutil_require_module('arcanist', 'parser/diff');
+
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'aphront/response/ajax');
 phutil_require_module('phabricator', 'applications/differential/controller/base');
 phutil_require_module('phabricator', 'applications/differential/parser/changeset');
 phutil_require_module('phabricator', 'applications/differential/parser/markup');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
+phutil_require_module('phabricator', 'applications/differential/storage/diff');
 phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
 phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
 phutil_require_module('phabricator', 'applications/phid/handle/data');
 phutil_require_module('phabricator', 'infrastructure/javelin/api');
 
+phutil_require_module('phutil', 'filesystem');
+phutil_require_module('phutil', 'filesystem/tempfile');
+phutil_require_module('phutil', 'future/exec');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/controller/diffview/DifferentialDiffViewController.php

@@ -40,6 +40,35 @@ class DifferentialDiffViewController extends DifferentialController {
       'When you are satisfied, either <strong>create a new revision</strong> '.
       'or <strong>update an existing revision</strong>.');
 
+    // TODO: implmenent optgroup support in AphrontFormSelectControl?
+    $select = array();
+    $select[] = '<optgroup label="Create New Revision">';
+    $select[] = '<option value="">Create a new Revision...</option>';
+    $select[] = '</optgroup>';
+
+    $revision_data = new DifferentialRevisionListData(
+      DifferentialRevisionListData::QUERY_OPEN_OWNED,
+      array($request->getUser()->getPHID()));
+    $revisions = $revision_data->loadRevisions();
+
+    if ($revisions) {
+      $select[] = '<optgroup label="Update Existing Revision">';
+      foreach ($revisions as $revision) {
+        $select[] = phutil_render_tag(
+          'option',
+          array(
+            'value' => $revision->getID(),
+          ),
+          phutil_escape_html($revision->getTitle()));
+      }
+      $select[] = '</optgroup>';
+    }
+
+    $select =
+      '<select name="revisionID">'.
+        implode("\n", $select).
+      '</select>';
+
     $action_form = new AphrontFormView();
     $action_form
       ->setUser($request->getUser())
@@ -47,13 +76,9 @@ class DifferentialDiffViewController extends DifferentialController {
       ->addHiddenInput('diffID', $diff->getID())
       ->addHiddenInput('viaDiffView', 1)
       ->appendChild(
-        id(new AphrontFormSelectControl())
+        id(new AphrontFormMarkupControl())
           ->setLabel('Attach To')
-          ->setName('revisionID')
+          ->setValue($select))
-          ->setValue('')
-          ->setOptions(array(
-            '' => "Create a new Revision...",
-          )))
       ->appendChild(
         id(new AphrontFormSubmitControl())
           ->setValue('Continue'));

src/applications/differential/controller/diffview/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'aphront/response/404');
 phutil_require_module('phabricator', 'applications/differential/controller/base');
+phutil_require_module('phabricator', 'applications/differential/data/revisionlist');
 phutil_require_module('phabricator', 'applications/differential/storage/diff');
 phutil_require_module('phabricator', 'applications/differential/view/changesetlistview');
 phutil_require_module('phabricator', 'applications/differential/view/difftableofcontents');
@@ -15,6 +16,7 @@ phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/submit');
 phutil_require_module('phabricator', 'view/layout/panel');
 
+phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/controller/revisionedit/DifferentialRevisionEditController.php

@@ -26,6 +26,12 @@ class DifferentialRevisionEditController extends DifferentialController {
 
   public function processRequest() {
 
+    $request = $this->getRequest();
+
+    if (!$this->id) {
+      $this->id = $request->getInt('revisionID');
+    }
+
     if ($this->id) {
       $revision = id(new DifferentialRevision())->load($this->id);
       if (!$revision) {
@@ -35,7 +41,6 @@ class DifferentialRevisionEditController extends DifferentialController {
       $revision = new DifferentialRevision();
     }
 
-    $request = $this->getRequest();
     $diff_id = $request->getInt('diffID');
     if ($diff_id) {
       $diff = id(new DifferentialDiff())->load($diff_id);

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

@@ -37,10 +37,16 @@ class DifferentialRevisionViewController extends DifferentialController {
 
     $diffs = $revision->loadDiffs();
 
-    $diff_vs = null;
+    $diff_vs = $request->getInt('vs');
     $target = end($diffs);
 
-    $changesets = $target->loadChangesets();
+    $diffs = mpull($diffs, null, 'getID');
+    if (empty($diffs[$diff_vs])) {
+      $diff_vs = null;
+    }
+
+    list($changesets, $vs_map) =
+      $this->loadChangesetsAndVsMap($diffs, $diff_vs, $target);
 
     $comments = $revision->loadComments();
     $comments = array_merge(
@@ -89,6 +95,7 @@ class DifferentialRevisionViewController extends DifferentialController {
     $changeset_view->setChangesets($changesets);
     $changeset_view->setEditable(true);
     $changeset_view->setRevision($revision);
+    $changeset_view->setVsMap($vs_map);
 
     $comment_form = new DifferentialAddCommentView();
     $comment_form->setRevision($revision);
@@ -315,6 +322,44 @@ class DifferentialRevisionViewController extends DifferentialController {
     return $inline_comments;
   }
 
+  private function loadChangesetsAndVsMap(array $diffs, $diff_vs, $target) {
+    $load_ids = array();
+    if ($diff_vs) {
+      $load_ids[] = $diff_vs;
+    }
+    $load_ids[] = $target->getID();
+
+    $raw_changesets = id(new DifferentialChangeset())
+      ->loadAllWhere(
+        'diffID IN (%Ld)',
+        $load_ids);
+    $changeset_groups = mgroup($raw_changesets, 'getDiffID');
+
+    $changesets = idx($changeset_groups, $target->getID(), array());
+    $changesets = mpull($changesets, null, 'getID');
+
+    $vs_map = array();
+    if ($diff_vs) {
+      $vs_changesets = idx($changeset_groups, $diff_vs, array());
+      $vs_changesets = mpull($vs_changesets, null, 'getFilename');
+      foreach ($changesets as $key => $changeset) {
+        $file = $changeset->getFilename();
+        if (isset($vs_changesets[$file])) {
+          $vs_map[$changeset->getID()] = $vs_changesets[$file]->getID();
+          unset($vs_changesets[$file]);
+        }
+      }
+      foreach ($vs_changesets as $changeset) {
+        $changesets[$changeset->getID()] = $changeset;
+        $vs_map[$changeset->getID()] = -1;
+      }
+    }
+
+    $changesets = msort($changesets, 'getSortKey');
+
+    return array($changesets, $vs_map);
+  }
+
 }
 /*
 
@@ -417,16 +462,6 @@ class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
-    $reviewer_links = array();
-    foreach ($revision->getReviewers() as $reviewer) {
-      $reviewer_links[] = <tools:handle handle={$handles[$reviewer]}
-                                          link={true} />;
-    }
-    if ($reviewer_links) {
-      $fields['Reviewers'] = array_implode(', ', $reviewer_links);
-    } else {
-      $fields['Reviewers'] = <em>None</em>;
-    }
 
     $ccs = $revision->getCCFBIDs();
     if ($ccs) {
@@ -777,68 +812,8 @@ class DifferentialRevisionViewController extends DifferentialController {
       $changesets = id(new DifferentialChangeset())->loadAllFromArray($objects);
     }
 
-    $against_warn = null;
-    $against_map = array();
-    $visible_changesets = array();
-    if ($old) {
-      $old_diff = $diffs[$old];
-      $new_diff = $diff;
-      $old_path = $old_diff->getSourcePath();
-      $new_path = $new_diff->getSourcePath();
-
-      $old_prefix = null;
-      $new_prefix = null;
-      if ((strlen($old_path) < strlen($new_path)) &&
-          (!strncmp($old_path, $new_path, strlen($old_path)))) {
-        $old_prefix = substr($new_path, strlen($old_path));
-      }
-      if ((strlen($new_path) < strlen($old_path)) &&
-          (!strncmp($old_path, $new_path, strlen($new_path)))) {
-        $new_prefix = substr($old_path, strlen($new_path));
-      }
-
-      $old_changesets = id(new DifferentialChangeset())
-        ->loadAllFromArray($raw_objects[$old]);
-      $old_changesets = array_pull($old_changesets, null, 'getFilename');
-      if ($new_prefix) {
-        $rekeyed_map = array();
-        foreach ($old_changesets as $key => $value) {
-          $rekeyed_map[$new_prefix.$key] = $value;
-        }
-        $old_changesets = $rekeyed_map;
-      }
-
-      foreach ($changesets as $key => $changeset) {
-        $file = $old_prefix.$changeset->getFilename();
-        if (isset($old_changesets[$file])) {
-          $checksum = $changeset->getChecksum();
-          if ($checksum !== null &&
-              $checksum == $old_changesets[$file]->getChecksum()) {
-            unset($changesets[$key]);
-            unset($old_changesets[$file]);
-          } else {
-            $against_map[$changeset->getID()] = $old_changesets[$file]->getID();
-            unset($old_changesets[$file]);
-          }
-        }
-      }
-
-      foreach ($old_changesets as $changeset) {
-        $changesets[$changeset->getID()] = $changeset;
-        $against_map[$changeset->getID()] = -1;
-      }
-
-      $against_warn =
-        <tools:notice title="NOTE - Diff of Diffs">
-          You are viewing a synthetic diff between two previous diffs in this
-          revision. You can not add new inline comments (for now).
-        </tools:notice>;
-    } else {
-      $visible_changesets = array_pull($changesets, 'getID');
-    }
 
     $changesets = array_psort($changesets, 'getSortKey');
-    $all_changesets = $changesets;
 
     $warning = null;
     $limit = 100;

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

@@ -58,6 +58,13 @@ class DifferentialChangesetParser {
   const WHITESPACE_IGNORE_TRAILING  = 'ignore-trailing';
   const WHITESPACE_IGNORE_ALL       = 'ignore-all';
 
+  public function setRightSideCommentMapping($id, $is_new) {
+
+  }
+
+  public function setLeftSideCommentMapping($id, $is_new) {
+
+  }
 
   public function setChangeset($changeset) {
     $this->changeset = $changeset;

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

@@ -21,6 +21,7 @@ class DifferentialChangesetListView extends AphrontView {
   private $changesets = array();
   private $editable;
   private $revision;
+  private $vsMap;
 
   public function setChangesets($changesets) {
     $this->changesets = $changesets;
@@ -37,48 +38,39 @@ class DifferentialChangesetListView extends AphrontView {
     return $this;
   }
 
+  public function setVsMap(array $vs_map) {
+    $this->vsMap = $vs_map;
+    return $this;
+  }
+
   public function render() {
     require_celerity_resource('differential-changeset-view-css');
 
-    $against = array(); // TODO
+    $vs_map = $this->vsMap;
-    $edit    = false;
-
     $changesets = $this->changesets;
-    foreach ($changesets as $key => $changeset) {
-      if (empty($against[$changeset->getID()])) {
-        $type = $changeset->getChangeType();
-        if ($type == DifferentialChangeType::TYPE_MOVE_AWAY ||
-            $type == DifferentialChangeType::TYPE_MULTICOPY) {
-          unset($changesets[$key]);
-        }
-      }
-    }
 
     $output = array();
     $mapping = array();
     foreach ($changesets as $key => $changeset) {
       $file = $changeset->getFilename();
       $class = 'differential-changeset';
-      if (!$edit) {
+      if (!$this->editable) {
         $class .= ' differential-changeset-noneditable';
       }
       $id = $changeset->getID();
       if ($id) {
-        $against_id = idx($against, $id);
+        $vs_id = idx($vs_map, $id);
       } else {
-        $against_id = null;
+        $vs_id = null;
       }
 
-/*
+      $detail_uri = new PhutilURI('/differential/changeset/');
-      TODO
+      $detail_uri->setQueryParams(
-      $detail_uri = URI($render_uri)
+        array(
-        ->addQueryData(array(
+          'id'          => $id,
-          'changeset'   => $id,
+          'vs'          => $vs_id,
-          'against'     => $against_id,
+          'whitespace'  => 'TODO',
-          'whitespace'  => $whitespace,
         ));
-*/
-      $detail_uri = '/differential/changeset/?id='.$changeset->getID();
 
       $detail_button = phutil_render_tag(
         'a',
@@ -104,7 +96,9 @@ class DifferentialChangesetListView extends AphrontView {
           '<div class="differential-loading">Loading...</div>'));
       $output[] = $detail->render();
 
-      $mapping[$uniq_id] = array($changeset->getID());
+      $mapping[$uniq_id] = array(
+        $changeset->getID(),
+        $vs_id);
     }
 
     $whitespace = null;

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

@@ -6,13 +6,13 @@
 
 
 
-phutil_require_module('phabricator', 'applications/differential/constants/changetype');
 phutil_require_module('phabricator', 'applications/differential/view/changesetdetailview');
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
 phutil_require_module('phabricator', 'infrastructure/javelin/api');
 phutil_require_module('phabricator', 'view/base');
 
 phutil_require_module('phutil', 'markup');
+phutil_require_module('phutil', 'parser/uri');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/view/revisionupdatehistory/DifferentialRevisionUpdateHistoryView.php

@@ -69,18 +69,38 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
 
     $idx = 0;
     $rows = array();
+    $disable = false;
+    $radios = array();
     foreach ($data as $row) {
 
       $name = phutil_escape_html($row['name']);
       $id   = phutil_escape_html($row['id']);
 
-      $radios = array();
       $lint = '*';
       $unit = '*';
 
       $old_class = null;
       $new_class = null;
 
+      if ($id) {
+        $new_checked = ($this->selectedDiffID == $id);
+        $new = javelin_render_tag(
+          'input',
+          array(
+            'type' => 'radio',
+            'name' => 'id',
+            'value' => $id,
+            'checked' => $new_checked ? 'checked' : null,
+            'sigil' => 'differential-new-radio',
+          ));
+        if ($new_checked) {
+          $new_class = " revhistory-new-now";
+          $disable = true;
+        }
+      } else {
+        $new = null;
+      }
+
       if ($max_id != $id) {
         $uniq = celerity_generate_unique_node_id();
         $old_checked = ($this->selectedVersusDiffID == $id);
@@ -89,9 +109,10 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
           array(
             'type' => 'radio',
             'name' => 'vs',
-            'value' => '0',
+            'value' => $id,
             'id' => $uniq,
             'checked' => $old_checked ? 'checked' : null,
+            'disabled' => $disable ? 'disabled' : null,
           ));
         $radios[] = $uniq;
         if ($old_checked) {
@@ -101,30 +122,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
         $old = null;
       }
 
-      if ($id) {
-        $new_checked = ($this->selectedDiffID == $id);
-        $new = phutil_render_tag(
-          'input',
-          array(
-            'type' => 'radio',
-            'name' => 'id',
-            'value' => $id,
-            'checked' => $new_checked ? 'checked' : null,
-          ));
-        if ($new_checked) {
-          $new_class = " revhistory-new-now";
-        }
-      } else {
-        $new = null;
-      }
-
-      Javelin::initBehavior(
-        'differential-diff-radios',
-        array(
-          'radios' => $radios,
-        ));
-
-
       $desc = 'TODO';
       $age = '-';
 
@@ -147,6 +144,12 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView {
         '</tr>';
     }
 
+      Javelin::initBehavior(
+        'differential-diff-radios',
+        array(
+          'radios' => $radios,
+        ));
+
     $select = '<select><option>Ignore All</option></select>';
 
     return

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

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'infrastructure/celerity/api');
 phutil_require_module('phabricator', 'infrastructure/javelin/api');
+phutil_require_module('phabricator', 'infrastructure/javelin/markup');
 phutil_require_module('phabricator', 'view/base');
 
 phutil_require_module('phutil', 'markup');

webroot/rsrc/css/core/core.css

@@ -65,6 +65,12 @@ select, input, button, textarea, button {
   font: 99% 'lucida grande', tahoma, verdana, arial, clean, sans-serif;
 }
 
+select {
+  /* NOTE: In Safari, specifying a border color makes the browser respect
+     "font-size" settings. */
+  border: 1px solid #999999;
+}
+
 table {
   font-size: inherit;
   font: 100%;

webroot/rsrc/js/application/differential/behavior-diff-radios.js

@@ -6,7 +6,7 @@
 JX.behavior('differential-diff-radios', function(config) {
   JX.Stratcom.listen(
     'click',
-    'new-radio',
+    'differential-new-radio',
     function(e) {
       var target = e.getTarget();
       var adjust;
@@ -14,7 +14,7 @@ JX.behavior('differential-diff-radios', function(config) {
       var reset = false;
       for (var ii = 0; ii < config.radios.length; ii++) {
         node = JX.$(config.radios[ii]);
-        if (node.value >= target.value) {
+        if (parseInt(node.value, 10) >= parseInt(target.value, 10)) {
           if (node.checked) {
             node.checked = false;
             reset = true;

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

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