recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Stop computing ownership for changed paths for Very Large revisions

Summary:
Depends on D19416. Ref T13110. Ref T13130. See PHI598. When rendering a "Very Large" revision (affecting more than 1,000 files) we currently compute the package/changeset ownership map normally.

This is basically a big list of which packages own which of the files affected by the change. We use it to:

# Show which packages own each file in the table of contents.
# Show an "(Owns No Changed Paths)" hint in the reviewers list to help catch out-of-date packages that are no longer relevant.

However, this is expensive to build. We don't render the table of contents at all, so (1) is pointless. The value of (2) is very small on these types of changes, and certainly not worth spending many many seconds computing ownership.

Instead, just skip building out these relationships for very large changes.

Test Plan: Viewed a very large change with package owners; verified it no longer built package map data and rendered the package owners with no "(Owns No Changed Paths)" hints.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13130, T13110

Differential Revision: https://secure.phabricator.com/D19418

epriestley ee32c186 24305cad

+47 -20
+47 -20
src/applications/differential/controller/DifferentialRevisionViewController.php
··· 1 1 <?php 2 2 3 - final class DifferentialRevisionViewController extends DifferentialController { 3 + final class DifferentialRevisionViewController 4 + extends DifferentialController { 4 5 5 6 private $revisionID; 6 - private $veryLargeDiff; 7 + private $changesetCount; 7 8 8 9 public function shouldAllowPublic() { 9 10 return true; 11 + } 12 + 13 + public function isLargeDiff() { 14 + return ($this->getChangesetCount() > $this->getLargeDiffLimit()); 10 15 } 11 16 12 17 public function isVeryLargeDiff() { 13 - return $this->veryLargeDiff; 18 + return ($this->getChangesetCount() > $this->getVeryLargeDiffLimit()); 19 + } 20 + 21 + public function getLargeDiffLimit() { 22 + return 100; 14 23 } 15 24 16 25 public function getVeryLargeDiffLimit() { 17 26 return 1000; 18 27 } 19 28 29 + public function getChangesetCount() { 30 + if ($this->changesetCount === null) { 31 + throw new PhutilInvalidStateException('setChangesetCount'); 32 + } 33 + return $this->changesetCount; 34 + } 35 + 36 + public function setChangesetCount($count) { 37 + $this->changesetCount = $count; 38 + return $this; 39 + } 40 + 20 41 public function handleRequest(AphrontRequest $request) { 21 42 $viewer = $this->getViewer(); 22 43 $this->revisionID = $request->getURIData('id'); ··· 82 103 idx($diffs, $diff_vs), 83 104 $repository); 84 105 85 - if (count($rendering_references) > $this->getVeryLargeDiffLimit()) { 86 - $this->veryLargeDiff = true; 87 - } 106 + $this->setChangesetCount(count($rendering_references)); 88 107 89 108 if ($request->getExists('download')) { 90 109 return $this->buildRawDiffResponse( ··· 153 172 154 173 $request_uri = $request->getRequestURI(); 155 174 156 - // Revisions with more than 100 files are "large". 157 - // Revisions with more than 1000 files are "very large". 158 - $limit = 100; 159 175 $large = $request->getStr('large'); 160 176 161 177 $large_warning = 178 + ($this->isLargeDiff()) && 162 179 (!$this->isVeryLargeDiff()) && 163 - (count($changesets) > $limit) && 164 180 (!$large); 165 181 166 182 if ($large_warning) { 167 - $count = count($changesets); 183 + $count = $this->getChangesetCount(); 184 + 168 185 $warning = new PHUIInfoView(); 169 186 $warning->setTitle(pht('Large Diff')); 170 187 $warning->setSeverity(PHUIInfoView::SEVERITY_WARNING); ··· 365 382 $other_view = $this->renderOtherRevisions($other_revisions); 366 383 } 367 384 368 - $this->buildPackageMaps($changesets); 369 - 370 385 if ($this->isVeryLargeDiff()) { 371 386 $toc_view = null; 387 + 388 + // When rendering a "very large" diff, we skip computation of owners 389 + // that own no files because it is significantly expensive and not very 390 + // valuable. 391 + foreach ($revision->getReviewers() as $reviewer) { 392 + // Give each reviewer a dummy nonempty value so the UI does not render 393 + // the "(Owns No Changed Paths)" note. If that behavior becomes more 394 + // sophisticated in the future, this behavior might also need to. 395 + $reviewer->attachChangesets($changesets); 396 + } 372 397 } else { 398 + $this->buildPackageMaps($changesets); 399 + 373 400 $toc_view = $this->buildTableOfContents( 374 401 $changesets, 375 402 $visible_changesets, 376 403 $target->loadCoverageMap($viewer)); 377 - } 378 404 379 - // Attach changesets to each reviewer so we can show which Owners package 380 - // reviewers own no files. 381 - foreach ($revision->getReviewers() as $reviewer) { 382 - $reviewer_phid = $reviewer->getReviewerPHID(); 383 - $reviewer_changesets = $this->getPackageChangesets($reviewer_phid); 384 - $reviewer->attachChangesets($reviewer_changesets); 405 + // Attach changesets to each reviewer so we can show which Owners package 406 + // reviewers own no files. 407 + foreach ($revision->getReviewers() as $reviewer) { 408 + $reviewer_phid = $reviewer->getReviewerPHID(); 409 + $reviewer_changesets = $this->getPackageChangesets($reviewer_phid); 410 + $reviewer->attachChangesets($reviewer_changesets); 411 + } 385 412 } 386 413 387 414 $tab_group = id(new PHUITabGroupView());