Page MenuHomePhabricator
Create Task
Maniphest T11650

Owners does not assign as reviewer if an Owners package with the same path has been archived
Closed, ResolvedPublic
Actions

Description

Steps to reproduce:

  1. Create a new directory with a file in it.
  2. Commit the new directory and file.
  3. Create a new Owners package with the path set to the newly created directory and dominion = weak and auto review = review changes (blocking).
  4. Create another new Owners package with the exact same settings.
  5. Archive the second Owners package you created.
  6. Create a revision that touches the file you created in the newly created directory.

Expected: The first Owners package you created is added as a blocking reviewer.
Actual: The first Owners package you created is NOT added as a blocking reviewer.

Note: I saw some other weirdness with order of creation but this was the only one I could reliably reproduce.

Also, what is the expected behavior if two Owners packages point to the same path? I believe it currently only assigns one of them as a reviewer in the revision. Seems like they should both be assigned as reviewers to the revision though. If that is in fact the case I can create another bug.

Thanks!

Revisions and Commits

rP Phabricator
D16564rP7f6fa283639f When loading packages affected by a change to a particular path, ignore…

Related Objects

Event Timeline

bizrad6 created this task.Sep 16 2016, 5:22 PM
Herald added a subscriber: eadler. · View Herald TranscriptSep 16 2016, 5:22 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Sep 16 2016, 6:27 PM
epriestley added a subscriber: epriestley.Sep 16 2016, 6:52 PM

Is it possible that this is slightly narrower: "package dominion rules incorrectly consider archived packages"? That seems to fit with the observations, but maybe I'm overlooking something.

bizrad6 added a comment.Sep 16 2016, 6:55 PM
In T11650#194907, @epriestley wrote:

Is it possible that this is slightly narrower: "package dominion rules incorrectly consider archived packages"? That seems to fit with the observations, but maybe I'm overlooking something.

Yeah that seems correct.

epriestley added a comment.Sep 16 2016, 7:02 PM

Also, what is the expected behavior if two Owners packages point to the same path?

The expectation is that they're both assigned, yes (provided they're both configured for review, the revision author isn't an owner of either, isn't a member of any owning group, neither has weak dominion that cedes ownership to the other one, neither is archived, neither "exclude" the path, etc).

epriestley added a revision: D16564: When loading packages affected by a change to a particular path, ignore archived packages.Sep 16 2016, 7:09 PM
epriestley added a comment.Sep 16 2016, 7:10 PM

D16564 might fix this but I need to actually test it.

epriestley added a comment.Sep 16 2016, 9:00 PM

The "multiple packages" issue may have been a symptom of the dominion/archive problem, here's a case I hit while testing that made it look like there was a second issue:

  • Package A on / (strong dominion).
  • Package B on /x/ (weak dominion).
  • Package C on /x/y (archived).

The code incorrectly gave C dominion over B, selecting "A" and "C" as the packages to add as reviewers. Then it filtered out "C" because it was archived, and ultimately added only "A". This looks like it ignored B, but it's actually just a symptom of dominion rules not being archive-aware.

Otherwise, I couldn't immediately reproduce any issues with multiple package owners after applying D16564:

Screen Shot 2016-09-16 at 1.59.40 PM.png (61×474 px, 11 KB)

If you're still seeing issues after upgrading to D16564, let me know if you can narrow it down a bit? Expected behavior is definitely that all packages have their reviewer behavior triggered, not just the first/strongest/one-arbitrarily-selected one.

epriestley added a commit: rP7f6fa283639f: When loading packages affected by a change to a particular path, ignore….Sep 16 2016, 9:03 PM
epriestley added a comment.Sep 16 2016, 9:07 PM

I think the first part of this should be fixed in HEAD now, thanks for the report!

bizrad6 added a comment.Sep 16 2016, 9:37 PM

Awesome! Thanks for the quick fix @epriestley!

epriestley closed this task as Resolved.Sep 27 2016, 4:38 PM
epriestley claimed this task.

I'm going to presume we got this, but reopen or yell if you're still seeing issues on the new code.

bizrad6 added a comment.Sep 27 2016, 5:47 PM

Yes, thanks!

Phabricator · Built by Phacility