Page MenuHomePhabricator
Files F15425770

D21180.id50439.diff
No OneTemporary
Actions

D21180.id50439.diff
View Options

diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php
--- a/src/applications/differential/controller/DifferentialChangesetViewController.php
+++ b/src/applications/differential/controller/DifferentialChangesetViewController.php
@@ -109,28 +109,7 @@
}
if ($left) {
- $left_data = $left->makeNewFile();
- $left_properties = $left->getNewProperties();
- if ($right) {
- $right_data = $right->makeNewFile();
- $right_properties = $right->getNewProperties();
- } else {
- $right_data = $left->makeOldFile();
- $right_properties = $left->getOldProperties();
- }
-
- $engine = new PhabricatorDifferenceEngine();
- $synthetic = $engine->generateChangesetFromFileContent(
- $left_data,
- $right_data);
-
- $choice = clone nonempty($left, $right);
- $choice->attachHunks($synthetic->getHunks());
-
- $choice->setOldProperties($left_properties);
- $choice->setNewProperties($right_properties);
-
- $changeset = $choice;
+ $changeset = $left->newComparisonChangeset($right);
}
if ($left_new || $right_new) {
diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php
--- a/src/applications/differential/parser/DifferentialChangesetParser.php
+++ b/src/applications/differential/parser/DifferentialChangesetParser.php
@@ -1694,27 +1694,10 @@
$changeset = $this->changeset;
$viewer = $this->getViewer();
- // TODO: This should probably be made non-optional in the future.
- if (!$viewer) {
- return null;
- }
-
- $old_file = null;
- $new_file = null;
-
- switch ($changeset->getFileType()) {
- case DifferentialChangeType::FILE_IMAGE:
- case DifferentialChangeType::FILE_BINARY:
- list($old_file, $new_file) = $this->loadFileObjectsForChangeset();
- break;
- }
-
- $type_delete = DifferentialChangeType::TYPE_DELETE;
- $type_add = DifferentialChangeType::TYPE_ADD;
- $change_type = $changeset->getChangeType();
+ list($old_file, $new_file) = $this->loadFileObjectsForChangeset();
- $no_old = ($change_type == $type_add);
- $no_new = ($change_type == $type_delete);
+ $no_old = !$changeset->hasOldState();
+ $no_new = !$changeset->hasNewState();
if ($no_old) {
$old_ref = null;
@@ -1742,7 +1725,6 @@
}
}
-
$old_engines = null;
if ($old_ref) {
$old_engines = PhabricatorDocumentEngine::getEnginesForRef(
@@ -1813,44 +1795,12 @@
$changeset = $this->changeset;
$viewer = $this->getViewer();
+ $old_phid = $changeset->getOldFileObjectPHID();
+ $new_phid = $changeset->getNewFileObjectPHID();
+
$old_file = null;
$new_file = null;
- // TODO: Improve the architectural issue as discussed in D955
- // https://secure.phabricator.com/D955
- $reference = $this->getRenderingReference();
- $parts = explode('/', $reference);
- if (count($parts) == 2) {
- list($id, $vs) = $parts;
- } else {
- $id = $parts[0];
- $vs = 0;
- }
- $id = (int)$id;
- $vs = (int)$vs;
-
- if (!$vs) {
- $metadata = $this->changeset->getMetadata();
- $old_phid = idx($metadata, 'old:binary-phid');
- $new_phid = idx($metadata, 'new:binary-phid');
- } else {
- $vs_changeset = id(new DifferentialChangeset())->load($vs);
- $old_phid = null;
- $new_phid = null;
-
- // TODO: This is spooky, see D6851
- if ($vs_changeset) {
- $vs_metadata = $vs_changeset->getMetadata();
- $old_phid = idx($vs_metadata, 'new:binary-phid');
- }
-
- $changeset = id(new DifferentialChangeset())->load($id);
- if ($changeset) {
- $metadata = $changeset->getMetadata();
- $new_phid = idx($metadata, 'new:binary-phid');
- }
- }
-
if ($old_phid || $new_phid) {
$file_phids = array();
if ($old_phid) {
@@ -1864,13 +1814,28 @@
->setViewer($viewer)
->withPHIDs($file_phids)
->execute();
+ $files = mpull($files, null, 'getPHID');
+
+ if ($old_phid) {
+ $old_file = idx($files, $old_phid);
+ if (!$old_file) {
+ throw new Exception(
+ pht(
+ 'Failed to load file data for changeset ("%s").',
+ $old_phid));
+ }
+ $changeset->attachOldFileObject($old_file);
+ }
- foreach ($files as $file) {
- if ($file->getPHID() == $old_phid) {
- $old_file = $file;
- } else if ($file->getPHID() == $new_phid) {
- $new_file = $file;
+ if ($new_phid) {
+ $new_file = idx($files, $new_phid);
+ if (!$new_file) {
+ throw new Exception(
+ pht(
+ 'Failed to load file data for changeset ("%s").',
+ $new_phid));
}
+ $changeset->attachNewFileObject($new_file);
}
}
diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php
--- a/src/applications/differential/render/DifferentialChangesetRenderer.php
+++ b/src/applications/differential/render/DifferentialChangesetRenderer.php
@@ -647,16 +647,26 @@
$old = $changeset->getOldProperties();
$new = $changeset->getNewProperties();
- // When adding files, don't show the uninteresting 644 filemode change.
- if ($changeset->getChangeType() == DifferentialChangeType::TYPE_ADD &&
- $new == array('unix:filemode' => '100644')) {
- unset($new['unix:filemode']);
- }
+ // If a property has been changed, but is not present on one side of the
+ // change and has an uninteresting default value on the other, remove it.
+ // This most commonly happens when a change adds or removes a file: the
+ // side of the change with the file has a "100644" filemode in Git.
+
+ $defaults = array(
+ 'unix:filemode' => '100644',
+ );
- // Likewise when removing files.
- if ($changeset->getChangeType() == DifferentialChangeType::TYPE_DELETE &&
- $old == array('unix:filemode' => '100644')) {
- unset($old['unix:filemode']);
+ foreach ($defaults as $default_key => $default_value) {
+ $old_value = idx($old, $default_key, $default_value);
+ $new_value = idx($new, $default_key, $default_value);
+
+ $old_default = ($old_value === $default_value);
+ $new_default = ($new_value === $default_value);
+
+ if ($old_default && $new_default) {
+ unset($old[$default_key]);
+ unset($new[$default_key]);
+ }
}
$metadata = $changeset->getMetadata();
diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php
--- a/src/applications/differential/storage/DifferentialChangeset.php
+++ b/src/applications/differential/storage/DifferentialChangeset.php
@@ -28,6 +28,11 @@
private $newFileObject = self::ATTACHABLE;
private $oldFileObject = self::ATTACHABLE;
+ private $hasOldState;
+ private $hasNewState;
+ private $oldStateMetadata;
+ private $newStateMetadata;
+
const TABLE_CACHE = 'differential_changeset_parse_cache';
const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted';
@@ -134,6 +139,34 @@
return $this->changesetPackages;
}
+ public function setHasOldState($has_old_state) {
+ $this->hasOldState = $has_old_state;
+ return $this;
+ }
+
+ public function setHasNewState($has_new_state) {
+ $this->hasNewState = $has_new_state;
+ return $this;
+ }
+
+ public function hasOldState() {
+ if ($this->hasOldState !== null) {
+ return $this->hasOldState;
+ }
+
+ $change_type = $this->getChangeType();
+ return !DifferentialChangeType::isCreateChangeType($change_type);
+ }
+
+ public function hasNewState() {
+ if ($this->hasNewState !== null) {
+ return $this->hasNewState;
+ }
+
+ $change_type = $this->getChangeType();
+ return !DifferentialChangeType::isDeleteChangeType($change_type);
+ }
+
public function save() {
$this->openTransaction();
$ret = parent::save();
@@ -490,6 +523,110 @@
return $this->assertAttached($this->oldFileObject);
}
+ public function newComparisonChangeset(
+ DifferentialChangeset $against = null) {
+
+ $left = $this;
+ $right = $against;
+
+ $left_data = $left->makeNewFile();
+ $left_properties = $left->getNewProperties();
+ $left_metadata = $left->getNewStateMetadata();
+ $left_state = $left->hasNewState();
+ $shared_metadata = $left->getMetadata();
+ if ($right) {
+ $right_data = $right->makeNewFile();
+ $right_properties = $right->getNewProperties();
+ $right_metadata = $right->getNewStateMetadata();
+ $right_state = $right->hasNewState();
+ $shared_metadata = $right->getMetadata();
+ } else {
+ $right_data = $left->makeOldFile();
+ $right_properties = $left->getOldProperties();
+ $right_metadata = $left->getOldStateMetadata();
+ $right_state = $left->hasOldState();
+ }
+
+ $engine = new PhabricatorDifferenceEngine();
+
+ $synthetic = $engine->generateChangesetFromFileContent(
+ $left_data,
+ $right_data);
+
+ $comparison = id(new self())
+ ->makeEphemeral(true)
+ ->attachDiff($left->getDiff())
+ ->setOldFile($left->getFilename())
+ ->setFilename($right->getFilename());
+
+ // TODO: Change type?
+ // TODO: File type?
+ // TODO: Away paths?
+ // TODO: View state key?
+
+ $comparison->attachHunks($synthetic->getHunks());
+
+ $comparison->setOldProperties($left_properties);
+ $comparison->setNewProperties($right_properties);
+
+ $comparison
+ ->setOldStateMetadata($left_metadata)
+ ->setNewStateMetadata($right_metadata)
+ ->setHasOldState($left_state)
+ ->setHasNewState($right_state);
+
+ // NOTE: Some metadata is not stored statefully, like the "generated"
+ // flag. For now, use the rightmost "new state" metadata to fill in these
+ // values.
+
+ $metadata = $comparison->getMetadata();
+ $metadata = $metadata + $shared_metadata;
+ $comparison->setMetadata($metadata);
+
+ return $comparison;
+ }
+
+ public function getNewStateMetadata() {
+ return $this->getMetadataWithPrefix('new:');
+ }
+
+ public function setNewStateMetadata(array $metadata) {
+ return $this->setMetadataWithPrefix($metadata, 'new:');
+ }
+
+ public function getOldStateMetadata() {
+ return $this->getMetadataWithPrefix('old:');
+ }
+
+ public function setOldStateMetadata(array $metadata) {
+ return $this->setMetadataWithPrefix($metadata, 'old:');
+ }
+
+ private function getMetadataWithPrefix($prefix) {
+ $length = strlen($prefix);
+
+ $result = array();
+ foreach ($this->getMetadata() as $key => $value) {
+ if (strncmp($key, $prefix, $length)) {
+ continue;
+ }
+
+ $key = substr($key, $length);
+ $result[$key] = $value;
+ }
+
+ return $result;
+ }
+
+ private function setMetadataWithPrefix(array $metadata, $prefix) {
+ foreach ($metadata as $key => $value) {
+ $key = $prefix.$key;
+ $this->metadata[$key] = $value;
+ }
+
+ return $this;
+ }
+
/* -( PhabricatorPolicyInterface )----------------------------------------- */

File Metadata

Mime Type
text/plain
Expires
Mon, Mar 24, 5:58 AM (1 d, 10 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7708681
Default Alt Text
D21180.id50439.diff (11 KB)

Event Timeline

Phabricator · Built by Phacility