mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-22 20:51:10 +01:00
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
This commit is contained in:
parent
af295341c8
commit
5e2af4b9b5
5 changed files with 121 additions and 20 deletions
|
@ -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;
|
||||
|
|
|
@ -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.
|
||||
|
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
@ -827,17 +827,19 @@ final class DifferentialDiff
|
|||
DifferentialChangeset $changeset) {
|
||||
|
||||
$is_generated_trusted = self::isTrustedGeneratedCode($changeset);
|
||||
|
||||
if ($is_generated_trusted) {
|
||||
$changeset->setTrustedChangesetAttribute(
|
||||
DifferentialChangeset::ATTRIBUTE_GENERATED,
|
||||
$is_generated_trusted);
|
||||
}
|
||||
|
||||
$is_generated_untrusted = self::isUntrustedGeneratedCode($changeset);
|
||||
|
||||
if ($is_generated_untrusted) {
|
||||
$changeset->setUntrustedChangesetAttribute(
|
||||
DifferentialChangeset::ATTRIBUTE_GENERATED,
|
||||
$is_generated_untrusted);
|
||||
}
|
||||
}
|
||||
|
||||
private static function isTrustedGeneratedCode(
|
||||
DifferentialChangeset $changeset) {
|
||||
|
|
|
@ -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();
|
||||
|
|
Loading…
Reference in a new issue