From 1b363a831e7cedde1475429b4361d184bfd7bf82 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 4 Apr 2018 06:42:20 -0700 Subject: [PATCH] When a revision changes more than 1,000 files, don't show the changes on the main page Summary: Depends on D19295. Ref T13110. Degrade the review UX when users try to interact with changes which are too large to receive human review. Test Plan: Reduced the "very large" limit, browsed some changes, saw various elements degrade. Maniphest Tasks: T13110 Differential Revision: https://secure.phabricator.com/D19296 --- .../DifferentialRevisionViewController.php | 110 ++++++++++++------ 1 file changed, 75 insertions(+), 35 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index b4f3799f7c..60ed08a8d2 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -3,11 +3,20 @@ final class DifferentialRevisionViewController extends DifferentialController { private $revisionID; + private $veryLargeDiff; public function shouldAllowPublic() { return true; } + public function isVeryLargeDiff() { + return $this->veryLargeDiff; + } + + public function getVeryLargeDiffLimit() { + return 1000; + } + public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); $this->revisionID = $request->getURIData('id'); @@ -73,6 +82,10 @@ final class DifferentialRevisionViewController extends DifferentialController { idx($diffs, $diff_vs), $repository); + if (count($rendering_references) > $this->getVeryLargeDiffLimit()) { + $this->veryLargeDiff = true; + } + if ($request->getExists('download')) { return $this->buildRawDiffResponse( $revision, @@ -150,7 +163,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $warning->appendChild(hsprintf( '%s %s', pht( - 'This diff is very large and affects %s files. '. + 'This diff is large and affects %s files. '. 'You may load each file individually or ', new PhutilNumber($count)), phutil_tag( @@ -265,35 +278,53 @@ final class DifferentialRevisionViewController extends DifferentialController { $timeline->setQuoteRef($revision->getMonogram()); - $changeset_view = id(new DifferentialChangesetListView()) - ->setChangesets($changesets) - ->setVisibleChangesets($visible_changesets) - ->setStandaloneURI('/differential/changeset/') - ->setRawFileURIs( - '/differential/changeset/?view=old', - '/differential/changeset/?view=new') - ->setUser($viewer) - ->setDiff($target) - ->setRenderingReferences($rendering_references) - ->setVsMap($vs_map) - ->setWhitespace($whitespace) - ->setSymbolIndexes($symbol_indexes) - ->setTitle(pht('Diff %s', $target->getID())) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); + if ($this->isVeryLargeDiff()) { + $messages = array(); + $messages[] = pht( + 'This very large diff affects more than %s files. Use the %s to '. + 'browse changes.', + new PhutilNumber($this->getVeryLargeDiffLimit()), + phutil_tag( + 'a', + array( + 'href' => '/differential/diff/'.$target->getID().'/changesets/', + ), + phutil_tag('strong', array(), pht('Changeset List')))); - $revision_id = $revision->getID(); - $inline_list_uri = "/revision/inlines/{$revision_id}/"; - $inline_list_uri = $this->getApplicationURI($inline_list_uri); - $changeset_view->setInlineListURI($inline_list_uri); + $changeset_view = id(new PHUIInfoView()) + ->setErrors($messages); + } else { + $changeset_view = id(new DifferentialChangesetListView()) + ->setChangesets($changesets) + ->setVisibleChangesets($visible_changesets) + ->setStandaloneURI('/differential/changeset/') + ->setRawFileURIs( + '/differential/changeset/?view=old', + '/differential/changeset/?view=new') + ->setUser($viewer) + ->setDiff($target) + ->setRenderingReferences($rendering_references) + ->setVsMap($vs_map) + ->setWhitespace($whitespace) + ->setSymbolIndexes($symbol_indexes) + ->setTitle(pht('Diff %s', $target->getID())) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); - if ($repository) { - $changeset_view->setRepository($repository); - } - if (!$viewer_is_anonymous) { - $changeset_view->setInlineCommentControllerURI( - '/differential/comment/inline/edit/'.$revision->getID().'/'); + $revision_id = $revision->getID(); + $inline_list_uri = "/revision/inlines/{$revision_id}/"; + $inline_list_uri = $this->getApplicationURI($inline_list_uri); + $changeset_view->setInlineListURI($inline_list_uri); + + if ($repository) { + $changeset_view->setRepository($repository); + } + + if (!$viewer_is_anonymous) { + $changeset_view->setInlineCommentControllerURI( + '/differential/comment/inline/edit/'.$revision->getID().'/'); + } } $broken_diffs = $this->loadHistoryDiffStatus($diffs); @@ -312,7 +343,7 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setLocalCommits(idx($props, 'local:commits')) ->setCommitsForLinks($commits_for_links); - if ($repository) { + if ($repository && !$this->isVeryLargeDiff()) { $other_revisions = $this->loadOtherRevisions( $changesets, $target, @@ -328,10 +359,14 @@ final class DifferentialRevisionViewController extends DifferentialController { $this->buildPackageMaps($changesets); - $toc_view = $this->buildTableOfContents( - $changesets, - $visible_changesets, - $target->loadCoverageMap($viewer)); + if ($this->isVeryLargeDiff()) { + $toc_view = null; + } else { + $toc_view = $this->buildTableOfContents( + $changesets, + $visible_changesets, + $target->loadCoverageMap($viewer)); + } // Attach changesets to each reviewer so we can show which Owners package // reviewers own no files. @@ -341,12 +376,17 @@ final class DifferentialRevisionViewController extends DifferentialController { $reviewer->attachChangesets($reviewer_changesets); } - $tab_group = id(new PHUITabGroupView()) - ->addTab( + $tab_group = id(new PHUITabGroupView()); + + if ($toc_view) { + $tab_group->addTab( id(new PHUITabView()) ->setName(pht('Files')) ->setKey('files') - ->appendChild($toc_view)) + ->appendChild($toc_view)); + } + + $tab_group ->addTab( id(new PHUITabView()) ->setName(pht('History')) @@ -479,7 +519,7 @@ final class DifferentialRevisionViewController extends DifferentialController { PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); $nav = null; - if ($filetree_on) { + if ($filetree_on && !$this->isVeryLargeDiff()) { $collapsed_key = PhabricatorFiletreeVisibleSetting::SETTINGKEY; $collapsed_value = $viewer->getUserSetting($collapsed_key); -- 2.51.2