NOTE: Since packages do not own paths exclusively, any user can create a package on / of every repository and be allowed to force-accept every package review because their "everything" package is now a containing package for all other packages. However, they could already just remove the reviewers, so I don't think this is important. We could add options to Owners (e.g., an optional whitelist of "Stronger" packages) or something to prevent this, but I don't plan to do this. Just fire anyone abusing it.

NOTE: The definition of "package A contains package B" will be something like "package A owns a shorter prefix of a path changed by the revision than package B does", not "all paths in package B are completely contained by paths in package A". Package B is allowed to own random stuff elsewhere in the tree and still be force-accepted by owners of package A if A owns /webroot/javascript/ and B owns /webroot/javascript/project_b/. Again, just fire anyone abusing this.

These rules work totally fine for our use case. Twitter doesn't have any concerns about violating a bunch of federal regulations. We've been operating with similar rules in mind and depend on trusting our engineers to not abuse them.

From our point of view, we prefer the empirical path-based rules. We'd likely just mimic the rules, so in our / owner package in our gigantic monorepo, it would likely include thousands of paths, so I'd be concerned about potential performance issues. If it makes more sense from upstream's perspective, perhaps this could be a more explicit "opt-in" for these rules. I don't have any good ideas for names of what that boolean could be though.

As far as "opt-in", are you mostly concerned about performance?

I plan to use the rule "package A contains package B if package A owns a shorter prefix of a path changed by the revision/commit than package B does" partly because this rule is very inexpensive to evaluate.

The cost to identify owners for a revision or commit rises very little if a package owns a large number of paths. You can define a package with a million different owned paths with no significant impact on lookup performance (see T11002 for some discussion; keeping this operation fast is why we don't support wildcards in paths). Particularly, we currently do not need to evaluate each owned path against each changed path, and performance is approximately O(number of changed paths), but almost always dominated by network overhead in realistic situations (i.e., situations other than extreme edge cases like commits which change a million paths).

This rule will create a few unusual situations. For example, suppose:

• Package A owns /src/.
• Package B owns /src/android/ and /docs/android/.

Now:

• If a revision touches /src/android/fastmath.php (and zero or more other files), owners of package A will be able to force-accept on behalf of package B.
• If a revision touches /docs/android/fastmath.latex (only), owners of package A will not be able to force-accept on behalf of package B.

I think this is a reasonable rule -- and it's a rule which we can evaluate very efficiently -- but it creates the unintuitive result that "Package A" can only contain "Package B" in the context of a set of changes, and generally that "Package A" will sometimes contain "Package B" (allowing force-accept) and sometimes not if the paths "Package B" owns are not a strict subset of the paths "Package A" owns.

In practice, I imagine that most hierarchical packages like this own strict subsets, and that this rule actually makes fairly good practical sense the cases where they don't (/src/ really probably shouldn't give you control over all of /docs/ in the above example). But it's primarily attractive because it is very efficient to evaluate.

As far as "opt-in", are you mostly concerned about performance?

The "opt-in" suggestion was mainly about offering an explicit option if you think other companies are potentially uncomfortable with having these rules in their owner packages. It wouldn't be necessary for us, we'd "opt-in" for all of our owner packages.

Performance was only a concern with explicitly defined paths (but sounds like it shouldn't be, which is great), but I failed to mention the bigger concern about complexity on our end with managing/syncing explicitly defined paths. On every commit, for every path created, we'd have to find every owner package that could potentially add these explicit paths. Perhaps I might be having overblown concerns, but I feel like this could be susceptible to sync errors.

I think this is a reasonable rule -- and it's a rule which we can evaluate very efficiently -- but it creates the unintuitive result that "Package A" can only contain "Package B" in the context of a set of changes, and generally that "Package A" will sometimes contain "Package B" (allowing force-accept) and sometimes not if the paths "Package B" owns are not a strict subset of the paths "Package A" owns.

I agree that this is a reasonable rule and that we should stick to this. I actually think the example you put up is pretty intuitive, but my perspective is probably biased because Twitter's tooling has operated with these exact rules for awhile.

How do we envision these rules applying to the following situation?

I am an owner in three packages:

• /
• /src/java
• /src/java/org/apache/

I am not a member of:

• /tests/java/org/apache/

A non-owning engineer makes a change to /src/java/org/apache/ and /tests/java/org/apache/ (I do not explicitly own anything in the /tests directory).

My expectation is that the revision would auto-append packages for the relevant paths:

• /src/java/org/apache/
• /tests/java/org/apache/

However, determining what "Accept Revision" dialog would present, is tricky. One option is:

Accept Revision: 
                 [√] As jmeador  <-- is this one necessary..?
                 [√] As "/" <--- accepting as "tests/java/org/apache" preempts this. 
                                 Maybe this isn't visible because it's not an affected path in the file list?
                 [ ] As "src/java/" <-- this should probably not be here since no intermediate paths are touched?
                 [ ] As "src/java/org/apache/"
                 [ ] (Force-Accept) As "tests/java/org/apache/" <--- accepting as "/" preempts this, if it's present

The complexity introduced with intricate path structures is something we may want to handle delicately. I can envision some users being confused as to what the implications of each action are. To avoid scope creeping this task, is it worth discussing something like this as a different feature?

Accept Revision: 
                 [√] As jmeador
                 [ ] As "src/java/org/apache/"
                 [ ] (Force-Accept) As "tests/java/org/apache/" on behalf of "/".
                     !!! You don't explicitly own this package, but you own "/". Explain Force-Accept <-- clicking this gets you more detail 

Additional complexity:
Ticking all boxes that would allow a user to land their revision might be able to display something like "The author could land the revision if you go through with this. Are you sure?" although I doubt that this would be unclear to the power users who have this permission anyway. Something tells me the thought here has probably already been approached, but decided against. (i.e. accepting changes currently does not give you this detail in the pending action preview list)

Overall, you only get checkboxes for users, packages and projects which are already reviewers, with the exception that you'll get one for yourself if you aren't a reviewer yet.

So the answer to some of that depends on, e.g., whether the / package is configured with "weak dominion" or not. If it's a weak package, it won't get added as a reviewer so you won't see a checkbox for it.

Assuming all of those packages have strong dominion, the default would be:

[√] As jmeador
[√] As "/"
[√] As "/src/java/"
[√] As "/src/java/org/apache/"
[ ] (Force-Accept) As "/tests/java/org/apache/"

None of the checkboxes impact the other checkboxes, since each one corresponds to a different entry in the "Reviewers" list. That is, to arrive at this state, the "Reviewers" list would already need to look like this:

      Reviewers: ( ) User jmeador
Group Reviewers: ( ) Package "/"
                 ( ) Package "/src/java/"
                 ( ) Package "/src/java/org/apache/"
                 ( ) Package "/tests/java/org/apache/"

In this scenario, the "user" row (if present) would be highlighted yellow to indicate you have authority, as would the first three packages.

We could add the "on behalf of" text or "click to explain" if there's confusion. I'd guess that these interactions will usually be obvious-ish from context? But this text is probably easy to add.

We could add a "this will become accepted" hint too, but I'd probably want to wait for T10574 first.

A rule that works most of the time now to avoid accidental transition-to-accepted states is "uncheck your self-review", which is possible after T12271. If you "Accept", but uncheck yourself, the accept effectively means "unblock on behalf of these packages/projects", but won't transition the revision to "Accepted" unless another individual human reviewer has already accepted it as themselves.

One other possible thing we could do to make that clearer is render these states differently:

• You are a reviewer who is named individually.
• You are a reviewer who is named individually, and blocking.
• You are not an individually named reviewer.

But I'm not sure if that's relevant in your environment or if users will actually find it confusing or not.

FWIW, All packages in our monorepo have weak dominion.

Ah. I think you'd just see this, then:

[√] As jmeador
[√] As "/src/java/org/apache/"
[ ] (Force-Accept) As "/tests/java/org/apache/"

Not sure if this is a known bug / missing feature, but FYI just in case...

Reproduction steps:

• Add myself to a weak dominion owner package owning /
• Get a review containing weak dominion owner package /devprod/submitqueue/

• Accept as myself (leave force accept unchecked)

Expected: Only myself marked as accepted.

Actual: Myself + /devprod/submitqueue/ accepted.

Thanks, that definitely looks like a bug.

No prob! If it helps:

phabricator d7426a0f19bb47dbe4d1386aca214f01ba221edb (Thu, Apr 6)
arcanist 65125d5ac6286a88b3bdfa83693fd2344df2e190 (Thu, Apr 6)
phutil db82199467d95c6a079624d2725aa67a656d5e23 (Tue, Apr 4)

Argh! I broke it while doing last-minute fixes to improve rendering, fix in a moment.

I think things should work now, let me know if you're still seeing issues. Thanks for the report!

Just tested, works great. Thanks for the quick fix!