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

Authored by epriestley on Sep 16 2016, 7:09 PM.

Details

Reviewers

chad

Maniphest Tasks

T11650: Owners does not assign as reviewer if an Owners package with the same path has been archived

Commits

rP7f6fa283639f: When loading packages affected by a change to a particular path, ignore…

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.)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 39865.Sep 16 2016, 7:09 PM

epriestley retitled this revision from to When loading packages affected by a change to a particular path, ignore archived packages.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T11650: Owners does not assign as reviewer if an Owners package with the same path has been archived.

epriestley mentioned this in T11650: Owners does not assign as reviewer if an Owners package with the same path has been archived.Sep 16 2016, 7:10 PM

chad accepted this revision.Sep 16 2016, 7:25 PM

chad edited edge metadata.

This revision is now accepted and ready to land.Sep 16 2016, 7:25 PM

Version with testing.

epriestley edited the test plan for this revision. (Show Details)Sep 16 2016, 9:02 PM

Closed by commit rP7f6fa283639f: When loading packages affected by a change to a particular path, ignore… (authored by epriestley, committed by epriestley). · Explain WhySep 16 2016, 9:03 PM

This revision was automatically updated to reflect the committed changes.

epriestley added inline comments.Sep 16 2016, 9:03 PM

src/applications/owners/query/PhabricatorOwnersPackageQuery.php
361–366	This isn't technically necessary because callers already specify active-packages-only. I think it's reasonable to bake the rule "archived packages can never control paths" into this API, but it's something that might eventually merit adjustment, particularly if we're eventually able to get all of the code running through this logic.