diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 1dac4c137d..576e0acb2b 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -109,28 +109,7 @@ final class DifferentialChangesetViewController extends DifferentialController { } 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 index 63a6c606c5..686979bca0 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1694,27 +1694,10 @@ final class DifferentialChangesetParser extends Phobject { $changeset = $this->changeset; $viewer = $this->getViewer(); - // TODO: This should probably be made non-optional in the future. - if (!$viewer) { - return null; - } + list($old_file, $new_file) = $this->loadFileObjectsForChangeset(); - $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(); - - $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 @@ final class DifferentialChangesetParser extends Phobject { } } - $old_engines = null; if ($old_ref) { $old_engines = PhabricatorDocumentEngine::getEnginesForRef( @@ -1813,44 +1795,12 @@ final class DifferentialChangesetParser extends Phobject { $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 @@ final class DifferentialChangesetParser extends Phobject { ->setViewer($viewer) ->withPHIDs($file_phids) ->execute(); + $files = mpull($files, null, 'getPHID'); - foreach ($files as $file) { - if ($file->getPHID() == $old_phid) { - $old_file = $file; - } else if ($file->getPHID() == $new_phid) { - $new_file = $file; + 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); + } + + 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 index 6b1ed8fa27..a81a04cefb 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -647,16 +647,26 @@ abstract class DifferentialChangesetRenderer extends Phobject { $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. - // Likewise when removing files. - if ($changeset->getChangeType() == DifferentialChangeType::TYPE_DELETE && - $old == array('unix:filemode' => '100644')) { - unset($old['unix:filemode']); + $defaults = array( + 'unix:filemode' => '100644', + ); + + 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 index 682dffdd88..9f5ad0d998 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -28,6 +28,11 @@ final class DifferentialChangeset 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 @@ final class DifferentialChangeset 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 @@ final class DifferentialChangeset 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 )----------------------------------------- */