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.

When loading packages affected by a change to a particular path, ignore archived packages

Summary:
Ref T11650. Currently, we load packages and then discard the archived ones.

However, this gets "dominion" rules (where a more-general package gives up ownership if a more-specific package exists) wrong if the more-specific package is archived: we incorrectly give up ownership.

Instead, just ignore these packages completely when loading affected packages. This is slightly simpler.

(There are technically two pieces of code we have to do this for, which should be a single piece of code but which haven't yet been unified.)

Test Plan:
- Created packages:
- Package A, on "/" (strong dominion, autoreview).
- Package B, on "/x/" (weak dominion, autoreview).
- Package C, on "/x/y" (archived, autoreview).
- Create a revision affecting "/x/y".
- Saw correct path ownership in table of contents ("B", strongest package only).
- Saw correct autoreview behavior (A + B).
- (Prior to patch, in `master`, reproduced the problem behaviors described in T11650, with bad dominion rules and failure to autoreview B.)

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11650

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

epriestley 7f6fa283 5cffe9e5

+11 -13
-7
src/applications/differential/editor/DifferentialTransactionEditor.php
··· 1511 1511 $packages = PhabricatorOwnersPackage::loadAffectedPackages( 1512 1512 $repository, 1513 1513 $this->affectedPaths); 1514 - 1515 - foreach ($packages as $key => $package) { 1516 - if ($package->isArchived()) { 1517 - unset($packages[$key]); 1518 - } 1519 - } 1520 - 1521 1514 if (!$packages) { 1522 1515 return array(); 1523 1516 }
+7
src/applications/owners/query/PhabricatorOwnersPackageQuery.php
··· 358 358 $best_match = null; 359 359 $include = false; 360 360 361 + // If this package is archived, it's no longer a controlling package 362 + // for the given path. In particular, it can not force active packages 363 + // with weak dominion to give up control. 364 + if ($package->isArchived()) { 365 + continue; 366 + } 367 + 361 368 foreach ($package->getPaths() as $package_path) { 362 369 if ($package_path->getRepositoryPHID() != $repository_phid) { 363 370 // If this path is for some other repository, skip it.
+4 -1
src/applications/owners/storage/PhabricatorOwnersPackage.php
··· 211 211 $conn, 212 212 'SELECT pkg.id, pkg.dominion, p.excluded, p.path 213 213 FROM %T pkg JOIN %T p ON p.packageID = pkg.id 214 - WHERE p.path IN (%Ls) %Q', 214 + WHERE p.path IN (%Ls) AND pkg.status IN (%Ls) %Q', 215 215 $package->getTableName(), 216 216 $path->getTableName(), 217 217 $chunk, 218 + array( 219 + self::STATUS_ACTIVE, 220 + ), 218 221 $repository_clause); 219 222 } 220 223 $rows = array_mergev($rows);
-5
src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
··· 77 77 continue; 78 78 } 79 79 80 - if ($package->isArchived()) { 81 - // Don't trigger audits if the package is archived. 82 - continue; 83 - } 84 - 85 80 if ($package->getAuditingEnabled()) { 86 81 $reasons = $this->checkAuditReasons( 87 82 $commit,