diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -76,6 +76,16 @@ $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 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -7,11 +7,13 @@ 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 @@ 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 @@ $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 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -120,9 +120,10 @@ $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 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -827,16 +827,18 @@ 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 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -147,9 +147,9 @@ 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 @@ 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();