From 5e2af4b9b5cd87539609f1d5d00cb591904548e7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 May 2018 13:06:19 -0700 Subject: [PATCH] Prepare to support an "Ignore generated files" flag in Owners Summary: Depends on D19425. Ref T13130. See PHI251. Now that changesets have a durable "generated" attribute, we can let owners packages check it when we're computing which packages are affected by a revision. There's no way to actualy configure a package to have this behavior yet. Test Plan: - Created a revision affecting a generated file and a non-generated file. - When I faked `mustMatchUngeneratedPaths()` to `return true;`, saw the non-generated file get no packages owning it. - Normally: lots of packages owning it). - Created a revision affecting only generated files. - When I faked things, saw no Owners actions trigger. - Normally: some packages added reviewers or subscribers. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19426 --- .../controller/DifferentialController.php | 10 +++ .../editor/DifferentialTransactionEditor.php | 26 ++++-- .../HeraldDifferentialRevisionAdapter.php | 5 +- .../differential/storage/DifferentialDiff.php | 18 ++-- .../storage/PhabricatorOwnersPackage.php | 82 ++++++++++++++++++- 5 files changed, 121 insertions(+), 20 deletions(-) diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php index 1860a07745..8fe5b5caca 100644 --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -76,6 +76,16 @@ abstract class DifferentialController extends PhabricatorController { $repository_phid, $changeset_path); + // If this particular changeset is generated code and the package does + // not match generated code, remove it from the list. + if ($changeset->isGeneratedChangeset()) { + foreach ($packages as $key => $package) { + if ($package->getMustMatchUngeneratedPaths()) { + unset($packages[$key]); + } + } + } + $this->pathPackageMap[$changeset_path] = $packages; foreach ($packages as $package) { $this->packageChangesetMap[$package->getPHID()][] = $changeset; diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 82740459b0..3271248ce8 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -7,11 +7,13 @@ final class DifferentialTransactionEditor private $isCloseByCommit; private $repositoryPHIDOverride = false; private $didExpandInlineState = false; - private $affectedPaths; private $firstBroadcast = false; private $wasBroadcasting; private $isDraftDemotion; + private $ownersDiff; + private $ownersChangesets; + public function getEditorApplicationClass() { return 'PhabricatorDifferentialApplication'; } @@ -968,13 +970,20 @@ final class DifferentialTransactionEditor return array(); } - if (!$this->affectedPaths) { + $diff = $this->ownersDiff; + $changesets = $this->ownersChangesets; + + $this->ownersDiff = null; + $this->ownersChangesets = null; + + if (!$changesets) { return array(); } - $packages = PhabricatorOwnersPackage::loadAffectedPackages( + $packages = PhabricatorOwnersPackage::loadAffectedPackagesForChangesets( $repository, - $this->affectedPaths); + $diff, + $changesets); if (!$packages) { return array(); } @@ -1255,9 +1264,12 @@ final class DifferentialTransactionEditor $paths[] = $path_prefix.'/'.$changeset->getFilename(); } - // Save the affected paths; we'll use them later to query Owners. This - // uses the un-expanded paths. - $this->affectedPaths = $paths; + // If this change affected paths, save the changesets so we can apply + // Owners rules to them later. + if ($paths) { + $this->ownersDiff = $diff; + $this->ownersChangesets = $changesets; + } // Mark this as also touching all parent paths, so you can see all pending // changes to any file within a directory. diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index 9f7f8fe3f2..858a42dbc9 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -120,9 +120,10 @@ final class HeraldDifferentialRevisionAdapter $repository = $this->loadRepository(); if ($repository) { - $packages = PhabricatorOwnersPackage::loadAffectedPackages( + $packages = PhabricatorOwnersPackage::loadAffectedPackagesForChangesets( $repository, - $this->loadAffectedPaths()); + $this->getDiff(), + $this->loadChangesets()); $this->affectedPackages = $packages; } } diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index cd8364c8aa..f121a1cb67 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -827,16 +827,18 @@ final class DifferentialDiff DifferentialChangeset $changeset) { $is_generated_trusted = self::isTrustedGeneratedCode($changeset); - - $changeset->setTrustedChangesetAttribute( - DifferentialChangeset::ATTRIBUTE_GENERATED, - $is_generated_trusted); + if ($is_generated_trusted) { + $changeset->setTrustedChangesetAttribute( + DifferentialChangeset::ATTRIBUTE_GENERATED, + $is_generated_trusted); + } $is_generated_untrusted = self::isUntrustedGeneratedCode($changeset); - - $changeset->setUntrustedChangesetAttribute( - DifferentialChangeset::ATTRIBUTE_GENERATED, - $is_generated_untrusted); + if ($is_generated_untrusted) { + $changeset->setUntrustedChangesetAttribute( + DifferentialChangeset::ATTRIBUTE_GENERATED, + $is_generated_untrusted); + } } private static function isTrustedGeneratedCode( diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index c76864702c..5e856190ec 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -147,9 +147,9 @@ final class PhabricatorOwnersPackage return ($this->getStatus() == self::STATUS_ARCHIVED); } - public function setName($name) { - $this->name = $name; - return $this; + public function getMustMatchUngeneratedPaths() { + // TODO: For now, there's no way to actually configure this. + return false; } public function loadOwners() { @@ -181,6 +181,82 @@ final class PhabricatorOwnersPackage return self::loadPackagesForPaths($repository, $paths); } + public static function loadAffectedPackagesForChangesets( + PhabricatorRepository $repository, + DifferentialDiff $diff, + array $changesets) { + assert_instances_of($changesets, 'DifferentialChangeset'); + + $paths_all = array(); + $paths_ungenerated = array(); + + foreach ($changesets as $changeset) { + $path = $changeset->getAbsoluteRepositoryPath($repository, $diff); + + $paths_all[] = $path; + + if (!$changeset->isGeneratedChangeset()) { + $paths_ungenerated[] = $path; + } + } + + if (!$paths_all) { + return array(); + } + + $packages_all = self::loadAffectedPackages( + $repository, + $paths_all); + + // If there are no generated changesets, we can't possibly need to throw + // away any packages for matching only generated paths. Just return the + // full set of packages. + if ($paths_ungenerated === $paths_all) { + return $packages_all; + } + + $must_match_ungenerated = array(); + foreach ($packages_all as $package) { + if ($package->getMustMatchUngeneratedPaths()) { + $must_match_ungenerated[] = $package; + } + } + + // If no affected packages have the "Ignore Generated Paths" flag set, we + // can't possibly need to throw any away. + if (!$must_match_ungenerated) { + return $packages_all; + } + + if ($paths_ungenerated) { + $packages_ungenerated = self::loadAffectedPackages( + $repository, + $paths_ungenerated); + } else { + $packages_ungenerated = array(); + } + + // We have some generated paths, and some packages that ignore generated + // paths. Take all the packages which: + // + // - ignore generated paths; and + // - didn't match any ungenerated paths + // + // ...and remove them from the list. + + $must_match_ungenerated = mpull($must_match_ungenerated, null, 'getID'); + $packages_ungenerated = mpull($packages_ungenerated, null, 'getID'); + $packages_all = mpull($packages_all, null, 'getID'); + + foreach ($must_match_ungenerated as $package_id => $package) { + if (!isset($packages_ungenerated[$package_id])) { + unset($packages_all[$package_id]); + } + } + + return $packages_all; + } + public static function loadOwningPackages($repository, $path) { if (empty($path)) { return array();