diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 6b13e893d5..cd71c62a1e 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -5,6 +5,7 @@ final class DifferentialRevisionViewController private $revisionID; private $changesetCount; + private $hiddenChangesets; public function shouldAllowPublic() { return true; @@ -169,6 +170,7 @@ final class DifferentialRevisionViewController } $handles = $this->loadViewerHandles($object_phids); + $warnings = array(); $request_uri = $request->getRequestURI(); @@ -203,11 +205,19 @@ final class DifferentialRevisionViewController pht('Expand All Files'))), ); - $warning = id(new PHUIInfoView()) + $warnings[] = id(new PHUIInfoView()) ->setTitle(pht('Large Diff')) ->setSeverity(PHUIInfoView::SEVERITY_WARNING) ->appendChild($message); + $folded_changesets = $changesets; + } else { + $folded_changesets = array(); + } + + // Don't hide or fold changesets which have inline comments. + $hidden_changesets = $this->hiddenChangesets; + if ($hidden_changesets || $folded_changesets) { $old = array_select_keys($changesets, $old_ids); $new = array_select_keys($changesets, $new_ids); @@ -222,16 +232,47 @@ final class DifferentialRevisionViewController $new, $revision); - $visible_changesets = array(); foreach ($inlines as $inline) { $changeset_id = $inline->getChangesetID(); - if (isset($changesets[$changeset_id])) { - $visible_changesets[$changeset_id] = $changesets[$changeset_id]; + if (!isset($changesets[$changeset_id])) { + continue; } + + unset($hidden_changesets[$changeset_id]); + unset($folded_changesets[$changeset_id]); } - } else { - $warning = null; - $visible_changesets = $changesets; + } + + // If we would hide only one changeset, don't hide anything. The notice + // we'd render about it is about the same size as the changeset. + if (count($hidden_changesets) < 2) { + $hidden_changesets = array(); + } + + // Update the set of hidden changesets, since we may have just un-hidden + // some of them. + if ($hidden_changesets) { + $warnings[] = id(new PHUIInfoView()) + ->setTitle(pht('Showing Only Differences')) + ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) + ->appendChild( + pht( + 'This revision modifies %s more files that are hidden because '. + 'they were not modified between selected diffs and they have no '. + 'inline comments.', + phutil_count($hidden_changesets))); + } + + // Compute the unfolded changesets. By default, everything is unfolded. + $unfolded_changesets = $changesets; + foreach ($folded_changesets as $changeset_id => $changeset) { + unset($unfolded_changesets[$changeset_id]); + } + + // Throw away any hidden changesets. + foreach ($hidden_changesets as $changeset_id => $changeset) { + unset($changesets[$changeset_id]); + unset($unfolded_changesets[$changeset_id]); } $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision'); @@ -267,7 +308,7 @@ final class DifferentialRevisionViewController if ($repository) { $symbol_indexes = $this->buildSymbolIndexes( $repository, - $visible_changesets); + $unfolded_changesets); } else { $symbol_indexes = array(); } @@ -328,7 +369,7 @@ final class DifferentialRevisionViewController } else { $changeset_view = id(new DifferentialChangesetListView()) ->setChangesets($changesets) - ->setVisibleChangesets($visible_changesets) + ->setVisibleChangesets($unfolded_changesets) ->setStandaloneURI('/differential/changeset/') ->setRawFileURIs( '/differential/changeset/?view=old', @@ -405,7 +446,7 @@ final class DifferentialRevisionViewController $toc_view = $this->buildTableOfContents( $changesets, - $visible_changesets, + $unfolded_changesets, $target->loadCoverageMap($viewer)); // Attach changesets to each reviewer so we can show which Owners package @@ -520,7 +561,7 @@ final class DifferentialRevisionViewController $footer[] = array( $anchor, - $warning, + $warnings, $tab_view, $changeset_view, ); @@ -807,6 +848,7 @@ final class DifferentialRevisionViewController DifferentialDiff $target, DifferentialDiff $diff_vs = null, PhabricatorRepository $repository = null) { + $viewer = $this->getViewer(); $load_diffs = array($target); if ($diff_vs) { @@ -814,7 +856,7 @@ final class DifferentialRevisionViewController } $raw_changesets = id(new DifferentialChangesetQuery()) - ->setViewer($this->getRequest()->getUser()) + ->setViewer($viewer) ->withDiffs($load_diffs) ->execute(); $changeset_groups = mgroup($raw_changesets, 'getDiffID'); @@ -822,17 +864,19 @@ final class DifferentialRevisionViewController $changesets = idx($changeset_groups, $target->getID(), array()); $changesets = mpull($changesets, null, 'getID'); - $refs = array(); - $vs_map = array(); + $refs = array(); + $vs_map = array(); $vs_changesets = array(); + $must_compare = array(); if ($diff_vs) { - $vs_id = $diff_vs->getID(); + $vs_id = $diff_vs->getID(); $vs_changesets_path_map = array(); foreach (idx($changeset_groups, $vs_id, array()) as $changeset) { $path = $changeset->getAbsoluteRepositoryPath($repository, $diff_vs); $vs_changesets_path_map[$path] = $changeset; $vs_changesets[$changeset->getID()] = $changeset; } + foreach ($changesets as $key => $changeset) { $path = $changeset->getAbsoluteRepositoryPath($repository, $target); if (isset($vs_changesets_path_map[$path])) { @@ -841,15 +885,20 @@ final class DifferentialRevisionViewController $refs[$changeset->getID()] = $changeset->getID().'/'.$vs_changesets_path_map[$path]->getID(); unset($vs_changesets_path_map[$path]); + + $must_compare[] = $changeset->getID(); + } else { $refs[$changeset->getID()] = $changeset->getID(); } } + foreach ($vs_changesets_path_map as $path => $changeset) { $changesets[$changeset->getID()] = $changeset; - $vs_map[$changeset->getID()] = -1; - $refs[$changeset->getID()] = $changeset->getID().'/-1'; + $vs_map[$changeset->getID()] = -1; + $refs[$changeset->getID()] = $changeset->getID().'/-1'; } + } else { foreach ($changesets as $changeset) { $refs[$changeset->getID()] = $changeset->getID(); @@ -858,13 +907,25 @@ final class DifferentialRevisionViewController $changesets = msort($changesets, 'getSortKey'); + // See T13137. When displaying the diff between two updates, hide any + // changesets which haven't actually changed. + $this->hiddenChangesets = array(); + foreach ($must_compare as $changeset_id) { + $changeset = $changesets[$changeset_id]; + $vs_changeset = $vs_changesets[$vs_map[$changeset_id]]; + + if ($changeset->hasSameEffectAs($vs_changeset)) { + $this->hiddenChangesets[$changeset_id] = $changesets[$changeset_id]; + } + } + return array($changesets, $vs_map, $vs_changesets, $refs); } private function buildSymbolIndexes( PhabricatorRepository $repository, - array $visible_changesets) { - assert_instances_of($visible_changesets, 'DifferentialChangeset'); + array $unfolded_changesets) { + assert_instances_of($unfolded_changesets, 'DifferentialChangeset'); $engine = PhabricatorSyntaxHighlighter::newEngine(); @@ -889,7 +950,7 @@ final class DifferentialRevisionViewController $sources); $indexed_langs = array_fill_keys($langs, true); - foreach ($visible_changesets as $key => $changeset) { + foreach ($unfolded_changesets as $key => $changeset) { $lang = $engine->getLanguageFromFilename($changeset->getFilename()); if (empty($indexed_langs) || isset($indexed_langs[$lang])) { $symbol_indexes[$key] = array( diff --git a/src/applications/differential/engine/DifferentialChangesetEngine.php b/src/applications/differential/engine/DifferentialChangesetEngine.php index ccb4381e43..e8a55a1b0e 100644 --- a/src/applications/differential/engine/DifferentialChangesetEngine.php +++ b/src/applications/differential/engine/DifferentialChangesetEngine.php @@ -7,6 +7,7 @@ final class DifferentialChangesetEngine extends Phobject { foreach ($changesets as $changeset) { $this->detectGeneratedCode($changeset); + $this->computeHashes($changeset); } $this->detectCopiedCode($changesets); @@ -59,6 +60,30 @@ final class DifferentialChangesetEngine extends Phobject { } +/* -( Content Hashes )----------------------------------------------------- */ + + + private function computeHashes(DifferentialChangeset $changeset) { + + $effect_key = DifferentialChangeset::METADATA_EFFECT_HASH; + + $effect_hash = $this->newEffectHash($changeset); + if ($effect_hash !== null) { + $changeset->setChangesetMetadata($effect_key, $effect_hash); + } + } + + private function newEffectHash(DifferentialChangeset $changeset) { + + if ($changeset->getHunks()) { + $new_data = $changeset->makeNewFile(); + return PhabricatorHash::digestForIndex($new_data); + } + + return null; + } + + /* -( Copied Code )-------------------------------------------------------- */ diff --git a/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php b/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php index 29b116fc3c..068771284b 100644 --- a/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php +++ b/src/applications/differential/management/PhabricatorDifferentialRebuildChangesetsWorkflow.php @@ -82,6 +82,10 @@ final class PhabricatorDifferentialRebuildChangesetsWorkflow id(new DifferentialChangesetEngine()) ->rebuildChangesets($changesets); + foreach ($changesets as $changeset) { + $changeset->save(); + } + echo tsprintf( "%s\n", pht('Done.')); diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 27d3ad4d64..00af84bc4e 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -26,6 +26,7 @@ final class DifferentialChangeset const METADATA_TRUSTED_ATTRIBUTES = 'attributes.trusted'; const METADATA_UNTRUSTED_ATTRIBUTES = 'attributes.untrusted'; + const METADATA_EFFECT_HASH = 'hash.effect'; const ATTRIBUTE_GENERATED = 'generated'; @@ -143,6 +144,48 @@ final class DifferentialChangeset return $ret; } + /** + * Test if this changeset and some other changeset put the affected file in + * the same state. + * + * @param DifferentialChangeset Changeset to compare against. + * @return bool True if the two changesets have the same effect. + */ + public function hasSameEffectAs(DifferentialChangeset $other) { + if ($this->getFilename() !== $other->getFilename()) { + return false; + } + + $hash_key = self::METADATA_EFFECT_HASH; + + $u_hash = $this->getChangesetMetadata($hash_key); + if ($u_hash === null) { + return false; + } + + $v_hash = $other->getChangesetMetadata($hash_key); + if ($v_hash === null) { + return false; + } + + if ($u_hash !== $v_hash) { + return false; + } + + // Make sure the final states for the file properties (like the "+x" + // executable bit) match one another. + $u_props = $this->getNewProperties(); + $v_props = $other->getNewProperties(); + ksort($u_props); + ksort($v_props); + + if ($u_props !== $v_props) { + return false; + } + + return true; + } + public function getSortKey() { $sort_key = $this->getFilename(); // Sort files with ".h" in them first, so headers (.h, .hpp) come before