Page MenuHomePhabricator

Should Owners allow multiple packages to own the same file?
Closed, ResolvedPublic

Description

Currently, Owners allows multiple packages to own the same file. It does this in two ways:

  • Parent/Child: Package A owns /x/ and Package B owns /x/y.c.
  • Siblings: Package C owns /x/y.c explicitly, and Package D also owns /x/y.c explicitly.

Since Owners doesn't have much explicit behavior, these behaviors are sort of implicit. Some reasonable questions about these behaviors:

  1. Does "Package A" still own /x/y.c? That is, does "Package B" having a more specific claim on it invalidate the claim that "Package A" makes?
  2. Should Package C and Package D be allowed to explicitly own the same file?

The current answers are a little murky. In particular:

  • The database and UI allow C and D to be created and to exist.
  • The Diffusion browse UI currently shows all owning packages, ordered from most-specific to least-specific. In this UI, both "A" and "B" own the file, but "B" has a slightly stronger claim. Likewise, both "C" and "D" own the file, although one of them arbitrarily gets listed first in the UI.
  • The Diffusion and Differential table of contents UIs currently show only the most specific owning package.
  • The Herald and Audit rules allow all of A, B, C, and D to exist and have claims on files.

Event Timeline

epriestley claimed this task.
epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Owners.
epriestley added a subscriber: nickz.

I think the answer for "A" and "B" is almost certainly that both packages still own the file, and the more specific claim made by "B" does not invalidate the claim made by "A". In particular, "A" can add paths in "B" with 'exclude' rules if the intent is truly for "B" to exclusively own the files.

Permissions are a little flimsy right now, but this approach also simplifies this situation in the future:

  • An attacker wants to modify /x/secret.c without being detected, but /x/ is owned and has audit rules.

The attacker should not be able to weaken audit rules by creating a new package which owns just /x/secret.c. This is much easier to accomplish if the other package's claim just gets to stand. If /x/secret.c is some codegen thing that no one cares about, it can be excluded.

Ordering packages by specificity still seems useful, it just shouldn't invalidate claims made by more general packages.


Given that, the "C" and "D" use case is probably also fine. @nickz has an example like this:

  • Package C owns /ios/ and /shared/config.xml
  • Package D owns /android/ and /shared/config.xml

This is a little odd, but seems basically reasonable, and generally increases the power of packages.


The consequence of embracing these rules is that the table of contents needs to list all the packages a file belongs to, which starts getting messy, but this is mostly voluntary complexity. Now that the TOC is unified we're in a more reasonable position to tackle general UI improvements or view options, anyway.

For binding builds to packages to work I think they need to overlap. For example: Run the 'Super Sweet Selinium Spaces Tests' if src/applications/spaces OR src/infrastructure changes, but src/infrastructure is also a trigger for other packages.

Ah, thanks! That use case makes sense to me too.

Owners is mostly just super old and was largely defined by implicit behaviors, some of which were really bugs. Getting a handle on some of these use cases helps me figure out what's a bug or accident vs what's actually desirable behavior that we want to build or retain.