From 7f6fa283639f814a23e58d5da88835ebcf3da55c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 16 Sep 2016 11:59:20 -0700
Subject: [PATCH] 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
---
 .../differential/editor/DifferentialTransactionEditor.php  | 7 -------
 .../owners/query/PhabricatorOwnersPackageQuery.php         | 7 +++++++
 .../owners/storage/PhabricatorOwnersPackage.php            | 5 ++++-
 .../worker/PhabricatorRepositoryCommitOwnersWorker.php     | 5 -----
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 7eaf5ef52d..3e9fe2b99f 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -1511,13 +1511,6 @@ final class DifferentialTransactionEditor
     $packages = PhabricatorOwnersPackage::loadAffectedPackages(
       $repository,
       $this->affectedPaths);
-
-    foreach ($packages as $key => $package) {
-      if ($package->isArchived()) {
-        unset($packages[$key]);
-      }
-    }
-
     if (!$packages) {
       return array();
     }
diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
index c585f4022f..4fbd717a2a 100644
--- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
+++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php
@@ -358,6 +358,13 @@ final class PhabricatorOwnersPackageQuery
       $best_match = null;
       $include = false;
 
+      // If this package is archived, it's no longer a controlling package
+      // for the given path. In particular, it can not force active packages
+      // with weak dominion to give up control.
+      if ($package->isArchived()) {
+        continue;
+      }
+
       foreach ($package->getPaths() as $package_path) {
         if ($package_path->getRepositoryPHID() != $repository_phid) {
           // If this path is for some other repository, skip it.
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 35f8ac58b5..f2290d2866 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -211,10 +211,13 @@ final class PhabricatorOwnersPackage
         $conn,
         'SELECT pkg.id, pkg.dominion, p.excluded, p.path
           FROM %T pkg JOIN %T p ON p.packageID = pkg.id
-          WHERE p.path IN (%Ls) %Q',
+          WHERE p.path IN (%Ls) AND pkg.status IN (%Ls) %Q',
         $package->getTableName(),
         $path->getTableName(),
         $chunk,
+        array(
+          self::STATUS_ACTIVE,
+        ),
         $repository_clause);
     }
     $rows = array_mergev($rows);
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 8f94df90be..20869ce273 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -77,11 +77,6 @@ final class PhabricatorRepositoryCommitOwnersWorker
         continue;
       }
 
-      if ($package->isArchived()) {
-        // Don't trigger audits if the package is archived.
-        continue;
-      }
-
       if ($package->getAuditingEnabled()) {
         $reasons = $this->checkAuditReasons(
           $commit,