Note that the brokenness predates dtu's changes.

Three ways to fix this, I'm not sure which is our intent:

Option A: per_file perms shouldn't propagate to subdirs. Changing this this would break the *.mm permissions currently in use by chrome/OWNERS, and maybe some of the *.mojom usages.

Option B: per_file perms in an outer dir are allowed to propagate past a directory-level 'set noparent', as above -- in which case the bug is that alice ought to be able to LGTM the /base/ one-file changes in the above example. This could conceivably make sense if we want to use a top level, per-file perms to require security review of mojom files.

Option C: 'set noparent' always makes it as if rules in outer OWNERS files don't exist. From the code, which skips loading outer OWNERS files this seems to be the originally intended behavior.

I'm prepping a fix for this, assuming option C. Let me know if that's wrong. 

I should add that the case of 'per-file *.mojom=set noparent' is fairly mindboggling when it is applied to subdirectories. In this case, is the 'noparent' relative to the mojom file, or relative to the OWNERS file with the rule? If the latter then it seems fairly toothless; if the former then it's especially bizzarre semantically.

Sorry for the delay in responding to this; I was out on vacation and then busy catching back up on things.

OWNERS as currently implemented is not meant to have per-file rules apply to subdirectories. That is an often-requested feature, but it requires new syntax. So, if the code was working correctly, we would break the *.mm rules.

The syntax we should have which would indicate whether something should apply to subdirs should be to say `per-file .../*.ext=alice@chromium.org`, i.e. the triple-dot ellipsis indicates "this dir and all subdirs".

Option C is correct; if that's not how things work today, that's a bug. 

Yes, `set noparent` on per-file rules is very confusing, but I think it can be implemented in a consistent way in conjunction with the ellipses.

Thanks for the clarification.

My understanding is that perfile rules like io_thread* don't propagate to subdirs (though it would apply to all files under possible subdirs io_thread/ and io_thread_test/), but perfiles rules based on the extension, like *.mm, or *_mac.cc, do effectively propagate.

The ... syntax looks like a nice idea.

To implement transitive perfile rules consistent with noparent (i.e. to get the above test to pass), we need to store additional information in the _paths_to_owners struct: in particular, we need to know the PATH of the owners file from which the glob rule (or whatever) emanated, and we need to make sure there's not stop_looking entry between the authorizing OWNERS file and the file being checked.

Feel free to steal this. I might be able to come back to it in a couple weeks, but all I've done so far is write the above unittest.

> My understanding is that perfile rules like io_thread* don't propagate 
> to subdirs (though it would apply to all files under possible subdirs
> io_thread/ and io_thread_test/), but perfiles rules based on the 
> extension, like *.mm, or *_mac.cc, do effectively propagate.

If there is a difference, that's not intentional and probably just an obvious bug resulting from trying to convert a glob to a regex.

> The ... syntax looks like a nice idea.

I didn't invent it; this is how this is done in google3 (internally) and comes from the way Perforce refers to things in subdirs :).

Yes, implementing transitive rules requires a non-trivial revamp of the implementation, which is why I've never gotten around to doing it. I think it is quite doable, and the implementation as a whole just isn't that big or complicated, but probably takes at least multiple hours if not a couple days.

If this is really a Pri-1, find an owner and update the priority.

This is the result of a bulk edit that moved high priority available bugs to a lower priority in an attempt to be more honest with bug filers.

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This is still worth digging into.

I'd like to start working on this.

To make sure I understand:
- 'per-file <glob>=<something>' means <something> applies to files in the <glob>, but *only* for the current directory.
- 'per-file .../<glob>=<something>' means <something> applies also to files in subdirectories.

It's not clear to me what 'per-file <glob>=set noparent' or 'per-file .../<glob>=set noparent' mean. 

Where possible, we should match what google3 does (as documented in go/OWNERS). 

I think I'd expect things to work like the following:

% ls -F
OWNERS
foo
bar
baz/
quux
% ls -F baz
quux
% cat OWNERS
alice
per-file bar=set noparent
per-file bar=bob
per-file quux=bob
per-file .../quux=set noparent
per-file .../quux=charlie
%

alice@ can approve foo, but not bar or quux. bob@ can approve bar and quux, but not foo or baz/quux. charlie@ can approve quux and bar/quux, but not foo or bar.

There's probably weirder corner cases that I haven't thought of yet :). If google3 does something different than the above, (a) I'd like to know, and (b) we should probably do what they do.

 Issue 235756  has been merged into this issue.

Related to bug 869232 (which is about per-file rules needing to be recursive in all situations).