Commit c0ae2f62 authored by epriestley's avatar epriestley
Browse files

Show change diffs in Phriction

Summary:
This is really rough and needs work (particularly, there's some diff code I
really need to refactor since I sort-of-copy-pasted it) but basically
functional.

Show text changes between diffs and allow users to revert to earlier versions.

Differential's line-oriented diff style isn't ideal for large blocks of text but
I'm betting this is probably good enough in most cases. We can see how bad it is
in practice and then fix it if needbe.

I added a bunch of support for "description" but didn't add the feature in this
diff, I'll either follow up or task it out since it should be a pretty
straightforward change.

Test Plan: Looked at history for several Phriction documents, clicked "previous
change" / "next change", clicked revert buttons.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 687
parent 6f388f97
......@@ -1162,7 +1162,7 @@ celerity_register_resource_map(array(
),
'phriction-document-css' =>
array(
'uri' => '/res/a6d15e09/rsrc/css/application/phriction/phriction-document-css.css',
'uri' => '/res/a0eb9ea1/rsrc/css/application/phriction/phriction-document-css.css',
'type' => 'css',
'requires' =>
array(
......
......@@ -321,6 +321,7 @@ phutil_register_library_map(array(
'PhabricatorDaemonReference' => 'infrastructure/daemon/control/reference',
'PhabricatorDaemonTimelineConsoleController' => 'applications/daemon/controller/timeline',
'PhabricatorDaemonTimelineEventController' => 'applications/daemon/controller/timelineevent',
'PhabricatorDifferenceEngine' => 'infrastructure/diff/engine',
'PhabricatorDirectoryCategory' => 'applications/directory/storage/category',
'PhabricatorDirectoryCategoryDeleteController' => 'applications/directory/controller/categorydelete',
'PhabricatorDirectoryCategoryEditController' => 'applications/directory/controller/categoryedit',
......@@ -590,6 +591,7 @@ phutil_register_library_map(array(
'PhrictionContent' => 'applications/phriction/storage/content',
'PhrictionController' => 'applications/phriction/controller/base',
'PhrictionDAO' => 'applications/phriction/storage/base',
'PhrictionDiffController' => 'applications/phriction/controller/diff',
'PhrictionDocument' => 'applications/phriction/storage/document',
'PhrictionDocumentController' => 'applications/phriction/controller/document',
'PhrictionDocumentPreviewController' => 'applications/phriction/controller/documentpreview',
......@@ -1096,6 +1098,7 @@ phutil_register_library_map(array(
'PhrictionContent' => 'PhrictionDAO',
'PhrictionController' => 'PhabricatorController',
'PhrictionDAO' => 'PhabricatorLiskDAO',
'PhrictionDiffController' => 'PhrictionController',
'PhrictionDocument' => 'PhrictionDAO',
'PhrictionDocumentController' => 'PhrictionController',
'PhrictionDocumentPreviewController' => 'PhrictionController',
......
......@@ -357,6 +357,7 @@ class AphrontDefaultApplicationConfiguration
'edit/(?:(?P<id>\d+)/)?$' => 'PhrictionEditController',
'preview/$' => 'PhrictionDocumentPreviewController',
'diff/(?P<id>\d+)/$' => 'PhrictionDiffController',
),
);
}
......
......@@ -110,50 +110,21 @@ class DifferentialChangesetViewController extends DifferentialController {
$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);
$engine = new PhabricatorDifferenceEngine();
$synthetic = $engine->generateChangesetFromFileContent(
$left_data,
$right_data);
$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());
}
$choice->attachHunks($synthetic->getHunks());
$changeset = $choice;
$changeset->setID(null);
}
$range_s = null;
$range_e = null;
$mask = array();
$range = $request->getStr('range');
if ($range) {
$match = null;
if (preg_match('@^(\d+)-(\d+)(?:/(\d+)-(\d+))?$@', $range, $match)) {
$range_s = (int)$match[1];
$range_e = (int)$match[2];
if (count($match) > 3) {
$start = (int)$match[3];
$len = (int)$match[4];
for ($ii = $start; $ii < $start + $len; $ii++) {
$mask[$ii] = true;
}
}
}
}
$spec = $request->getStr('range');
list($range_s, $range_e, $mask) =
DifferentialChangesetParser::parseRangeSpecification($spec);
$parser = new DifferentialChangesetParser();
$parser->setChangeset($changeset);
......
......@@ -6,8 +6,6 @@
phutil_require_module('arcanist', 'parser/diff');
phutil_require_module('phabricator', 'aphront/response/400');
phutil_require_module('phabricator', 'aphront/response/404');
phutil_require_module('phabricator', 'aphront/response/ajax');
......@@ -15,17 +13,14 @@ phutil_require_module('phabricator', 'aphront/response/file');
phutil_require_module('phabricator', 'applications/differential/controller/base');
phutil_require_module('phabricator', 'applications/differential/parser/changeset');
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/differential/view/primarypane');
phutil_require_module('phabricator', 'applications/markup/engine');
phutil_require_module('phabricator', 'applications/phid/handle/data');
phutil_require_module('phabricator', 'infrastructure/diff/engine');
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', 'markup');
phutil_require_module('phutil', 'utils');
......
......@@ -489,10 +489,20 @@ class DifferentialChangesetParser {
}
}
$old_corpus = ipull($this->old, 'text');
// NOTE: Micro-optimize a couple of ipull()s here since it gives us a
// 10% performance improvement for certain types of large diffs like
// Phriction changes.
$old_corpus = array();
foreach ($this->old as $o) {
$old_corpus[] = $o['text'];
}
$old_corpus_block = implode("\n", $old_corpus);
$new_corpus = ipull($this->new, 'text');
$new_corpus = array();
foreach ($this->new as $n) {
$new_corpus[] = $n['text'];
}
$new_corpus_block = implode("\n", $new_corpus);
$old_future = $this->getHighlightFuture($old_corpus_block);
......@@ -796,33 +806,11 @@ class DifferentialChangesetParser {
// especially for python) or often produce a much larger set of
// differences than necessary.
$old_tmp = new TempFile();
$new_tmp = new TempFile();
Filesystem::writeFile($old_tmp, $old_file);
Filesystem::writeFile($new_tmp, $new_file);
list($err, $diff) = exec_manual(
'diff -bw -U65535 %s %s ',
$old_tmp,
$new_tmp);
if (!strlen($diff)) {
// If there's no diff text, that means the files are identical
// except for whitespace changes. Build a synthetic, changeless
// diff. TODO: this is incredibly hacky.
$entire_file = explode("\n", $changeset->makeOldFile());
foreach ($entire_file as $k => $line) {
$entire_file[$k] = ' '.$line;
}
$len = count($entire_file);
$entire_file = implode("\n", $entire_file);
$diff = <<<EOSYNTHETIC
--- ignored 9999-99-99
+++ ignored 9999-99-99
@@ -{$len},{$len} +{$len},{$len} @@
{$entire_file}
EOSYNTHETIC;
}
$engine = new PhabricatorDifferenceEngine();
$engine->setIgnoreWhitespace(true);
$no_whitespace_changeset = $engine->generateChangesetFromFileContent(
$old_file,
$new_file);
// subparser takes over the current non-whitespace-ignoring changeset
$subparser = new DifferentialChangesetParser();
......@@ -832,21 +820,17 @@ EOSYNTHETIC;
$subparser->parseHunk($hunk);
}
// We need to call process() so that the subparser's values for
// things like.
// metadata (like 'unchanged') is correct.
$subparser->process();
$this->subparser = $subparser;
// this parser takes new changeset; will use subparser's text later
$changes = id(new ArcanistDiffParser())->parseDiff($diff);
$diff = DifferentialDiff::newFromRawChanges($changes);
// While we aren't updating $this->changeset (since it has a bunch
// of metadata we need to preserve, so that headers like "this file
// was moved" render correctly), we're overwriting the local
// $changeset so that the block below will choose the synthetic
// hunks we've built instead of the original hunks.
$changeset = head($diff->getChangesets());
$changeset = $no_whitespace_changeset;
}
// This either uses the real hunks, or synthetic hunks we built above.
......@@ -1597,4 +1581,42 @@ EOSYNTHETIC;
return $ret;
}
/**
* Parse the 'range' specification that this class and the client-side JS
* emit to indicate that a user clicked "Show more..." on a diff. Generally,
* use is something like this:
*
* $spec = $request->getStr('range');
* $parsed = DifferentialChangesetParser::parseRangeSpecification($spec);
* list($start, $end, $mask) = $parsed;
* $parser->render($start, $end, $mask);
*
* @param string Range specification, indicating the range of the diff that
* should be rendered.
* @return tuple List of <start, end, mask> suitable for passing to
* @{method:render}.
*/
public static function parseRangeSpecification($spec) {
$range_s = null;
$range_e = null;
$mask = array();
if ($spec) {
$match = null;
if (preg_match('@^(\d+)-(\d+)(?:/(\d+)-(\d+))?$@', $spec, $match)) {
$range_s = (int)$match[1];
$range_e = (int)$match[2];
if (count($match) > 3) {
$start = (int)$match[3];
$len = (int)$match[4];
for ($ii = $start; $ii < $start + $len; $ii++) {
$mask[$ii] = true;
}
}
}
}
return array($range_s, $range_e, $mask);
}
}
......@@ -7,22 +7,18 @@
phutil_require_module('arcanist', 'difference');
phutil_require_module('arcanist', 'parser/diff');
phutil_require_module('phabricator', 'applications/differential/constants/changetype');
phutil_require_module('phabricator', 'applications/differential/storage/changeset');
phutil_require_module('phabricator', 'applications/differential/storage/diff');
phutil_require_module('phabricator', 'applications/differential/view/inlinecomment');
phutil_require_module('phabricator', 'applications/files/uri');
phutil_require_module('phabricator', 'applications/markup/syntax');
phutil_require_module('phabricator', 'infrastructure/diff/engine');
phutil_require_module('phabricator', 'infrastructure/javelin/markup');
phutil_require_module('phabricator', 'storage/queryfx');
phutil_require_module('phutil', 'error');
phutil_require_module('phutil', 'filesystem');
phutil_require_module('phutil', 'filesystem/tempfile');
phutil_require_module('phutil', 'future');
phutil_require_module('phutil', 'future/exec');
phutil_require_module('phutil', 'markup');
phutil_require_module('phutil', 'utils');
......
......@@ -47,27 +47,9 @@ class DiffusionDiffController extends DiffusionController {
$parser->setWhitespaceMode(
DifferentialChangesetParser::WHITESPACE_SHOW_ALL);
$range_s = null;
$range_e = null;
$mask = array();
// TODO: This duplicates a block in DifferentialChangesetViewController.
$range = $request->getStr('range');
if ($range) {
$match = null;
if (preg_match('@^(\d+)-(\d+)(?:/(\d+)-(\d+))?$@', $range, $match)) {
$range_s = (int)$match[1];
$range_e = (int)$match[2];
if (count($match) > 3) {
$start = (int)$match[3];
$len = (int)$match[4];
for ($ii = $start; $ii < $start + $len; $ii++) {
$mask[$ii] = true;
}
}
}
}
$spec = $request->getStr('range');
list($range_s, $range_e, $mask) =
DifferentialChangesetParser::parseRangeSpecification($spec);
$output = $parser->render($range_s, $range_e, $mask);
return id(new AphrontAjaxResponse())
......
<?php
/*
* Copyright 2011 Facebook, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
class PhrictionDiffController
extends PhrictionController {
private $id;
public function willProcessRequest(array $data) {
$this->id = $data['id'];
}
public function processRequest() {
$request = $this->getRequest();
$user = $request->getUser();
$document = id(new PhrictionDocument())->load($this->id);
if (!$document) {
return new Aphront404Response();
}
$current = id(new PhrictionContent())->load($document->getContentID());
$l = $request->getInt('l');
$r = $request->getInt('r');
$ref = $request->getStr('ref');
if ($ref) {
list($l, $r) = explode(',', $ref);
}
$content = id(new PhrictionContent())->loadAllWhere(
'documentID = %d AND version IN (%Ld)',
$document->getID(),
array($l, $r));
$content = mpull($content, null, 'getVersion');
$content_l = idx($content, $l, null);
$content_r = idx($content, $r, null);
if (!$content_l || !$content_r) {
return new Aphront404Response();
}
$text_l = $content_l->getContent();
$text_r = $content_r->getContent();
$text_l = wordwrap($text_l, 80);
$text_r = wordwrap($text_r, 80);
$engine = new PhabricatorDifferenceEngine();
$changeset = $engine->generateChangesetFromFileContent($text_l, $text_r);
$changeset->setOldProperties(
array(
'Title' => $content_l->getTitle(),
));
$changeset->setNewProperties(
array(
'Title' => $content_r->getTitle(),
));
$whitespace_mode = DifferentialChangesetParser::WHITESPACE_SHOW_ALL;
$parser = new DifferentialChangesetParser();
$parser->setChangeset($changeset);
$parser->setRenderingReference("{$l},{$r}");
$parser->setWhitespaceMode($whitespace_mode);
$spec = $request->getStr('range');
list($range_s, $range_e, $mask) =
DifferentialChangesetParser::parseRangeSpecification($spec);
$output = $parser->render($range_s, $range_e, $mask);
if ($request->isAjax()) {
return id(new AphrontAjaxResponse())->setContent($output);
}
require_celerity_resource('differential-changeset-view-css');
require_celerity_resource('syntax-highlighting-css');
require_celerity_resource('phriction-document-css');
Javelin::initBehavior('differential-show-more', array(
'uri' => '/phriction/diff/'.$document->getID().'/',
'whitespace' => $whitespace_mode,
));
$slug = $document->getSlug();
$revert_l = $this->renderRevertButton($content_l, $current);
$revert_r = $this->renderRevertButton($content_r, $current);
$crumbs = new AphrontCrumbsView();
$crumbs->setCrumbs(
array(
'Phriction',
phutil_render_tag(
'a',
array(
'href' => PhrictionDocument::getSlugURI($slug),
),
phutil_escape_html($current->getTitle())),
phutil_render_tag(
'a',
array(
'href' => '/phriction/history/'.$document->getSlug().'/',
),
'History'),
phutil_escape_html("Changes Between Version {$l} and Version {$r}"),
));
$comparison_table = $this->renderComparisonTable(
array(
$content_r,
$content_l,
));
$navigation_table = null;
if ($l + 1 == $r) {
$nav_l = ($l > 1);
$nav_r = ($r != $current->getVersion());
$uri = $request->getRequestURI();
if ($nav_l) {
$link_l = phutil_render_tag(
'a',
array(
'href' => $uri->alter('l', $l - 1)->alter('r', $r - 1),
),
"\xC2\xAB Previous Change");
} else {
$link_l = 'Original Change';
}
$link_r = null;
if ($nav_r) {
$link_r = phutil_render_tag(
'a',
array(
'href' => $uri->alter('l', $l + 1)->alter('r', $r + 1),
),
"Next Change \xC2\xBB");
} else {
$link_r = 'Most Recent Change';
}
$navigation_table =
'<table class="phriction-history-nav-table">
<tr>
<td class="nav-prev">'.$link_l.'</td>
<td class="nav-next">'.$link_r.'</td>
</tr>
</table>';
}
$output =
'<div class="phriction-document-history-diff">'.
$comparison_table->render().
'<br />'.
'<br />'.
$navigation_table.
'<table class="phriction-revert-table">'.
'<tr><td>'.$revert_l.'</td><td>'.$revert_r.'</td>'.
'</table>'.
$output.
'</div>';
return $this->buildStandardPageResponse(
array(
$crumbs,
$output,
),
array(
'title' => 'Document History',
'history' => PhrictionDocument::getSlugURI($slug, 'history'),
'document' => PhrictionDocument::getSlugURI($slug),
));
}
private function renderRevertButton(
PhrictionContent $content,
PhrictionContent $current) {
$document_id = $content->getDocumentID();
$version = $content->getVersion();
if ($content->getID() == $current->getID()) {
return phutil_render_tag(
'a',
array(
'href' => '/phriction/edit/'.$document_id.'/',
'class' => 'button',
),
'Edit Current Version');
}
return phutil_render_tag(
'a',
array(
'href' => '/phriction/edit/'.$document_id.'/?revert='.$version,
'class' => 'button',
),
'Revert to Version '.phutil_escape_html($version).'...');
}
private function renderComparisonTable(array $content) {
$user = $this->getRequest()->getUser();
$phids = mpull($content, 'getAuthorPHID');
$handles = id(new PhabricatorObjectHandleData($phids))->loadHandles();
$rows = array();
foreach ($content as $c) {
$rows[] = array(
phabricator_date($c->getDateCreated(), $user),
phabricator_time($c->getDateCreated(), $user),
phutil_escape_html('Version '.$c->getVersion()),
$handles[$c->getAuthorPHID()]->renderLink(),
'',
);
}
$table = new AphrontTableView($rows);
$table->setHeaders(
array(
'Date',
'Time',
'Version',
'Author',
'Description',
));
$table->setColumnClasses(
array(
'',
'right',
'pri',
'',
'wide',
));
return $table;
}
}