Implement support for file: includes in OWNERS files.

owners.py

Index: owners.py
diff --git a/owners.py b/owners.py
index 30646ffa24dd203754a08abf5bba6d60f0f938cf..d830f6793579a4f966a553dfa032431955b9cca7 100644
--- a/owners.py
+++ b/owners.py
@@ -18,6 +18,7 @@ line      := directive
           | comment
 
 directive := "set noparent"
+          |  "file:" glob
           |  email_address
           |  "*"
 
@@ -45,6 +46,10 @@ If "per-file glob=set noparent" is used, then global directives are ignored
 for the glob, and only the "per-file" owners are used for files matching that
 glob.
 
+If the "file:" directive is used, the referred to OWNERS file will be parsed and
+considered when determining the valid set of OWNERS. If the filename starts with
+"//" it is considered to be an absolute path, otherwise a relative path.

[Dirk Pranke, 2015/04/15 19:42:37]

"absolute path" might be confusing. I might call this "source-absolute", or
"relative to the top of the repo, otherwise it is relative to the current file".

[Peter Beverloo, 2015/04/15 20:04:31]

Ah yes, thanks. I'll adopt your comment with s/top/root/ to match terminology in
this class. (Database.root)

[Peter Beverloo, 2015/04/16 09:54:05]

Done.

+
 Examples for all of these combinations can be found in tests/owners_unittest.py.
 """
 
@@ -52,6 +57,9 @@ import collections
 import random
 import re
 
+# Name of the file in a directory containing the OWNERS contents.
+OWNERS_FILE = 'OWNERS'
+
 
 # If this is present by itself on a line, this means that everyone can review.
 EVERYONE = '*'
@@ -118,6 +126,9 @@ class Database(object):
     # (This is implicitly true for the root directory).
     self.stop_looking = set([''])
 
+    # Set of files which have already been read.
+    self.read_files = set()
+
   def reviewers_for(self, files, author):
     """Returns a suggested set of reviewers that will cover the files.
 
@@ -189,15 +200,21 @@ class Database(object):
     for f in files:
       dirpath = self.os_path.dirname(f)
       while not dirpath in self.owners_for:
-        self._read_owners_in_dir(dirpath)
+        self._read_owners_in_dir(dirpath, OWNERS_FILE)
         if self._stop_looking(dirpath):
           break
         dirpath = self.os_path.dirname(dirpath)
 
-  def _read_owners_in_dir(self, dirpath):
-    owners_path = self.os_path.join(self.root, dirpath, 'OWNERS')
+  def _read_owners_in_dir(self, dirpath, owners_file):
+    owners_path = self.os_path.join(self.root, dirpath, owners_file)
     if not self.os_path.exists(owners_path):
       return
+
+    if owners_path in self.read_files:
+      return

[Dirk Pranke, 2015/04/15 19:42:37]

Since read_files is a set, calling add() multiple times is harmless and
presumably cheap, so having this check is unnecessary and potentially confusing.

[Peter Beverloo, 2015/04/15 20:04:31]

I may be misunderstanding you, but both the set and the check exist to avoid
infinite recursion. See the test_file_include_recursive_loop() test.

We can't safely move the check to _resolve_include() since two paths could need
to include the same file, for example (in a CL touching both directories):

//content/child/my_feature/OWNERS
  "file://content/browser/my_feature/OWNERS"
//content/renderer/my_feature/OWNERS
  "file://content/browser/my_feature/OWNERS"

[Dirk Pranke, 2015/04/15 20:52:03]

Oh, I was misreading the code, and thought the function ended after line 216.
Never mind :).

+
+    self.read_files.add(owners_path)
+
     comment = []
     in_comment = False
     lineno = 0
@@ -244,6 +261,21 @@ class Database(object):
                  line_type, owners_path, lineno, comment):
     if directive == 'set noparent':
       self.stop_looking.add(path)
+    elif directive.startswith('file:'):
+      dirpath, owners_file = self._resolve_include(directive[5:], owners_path)
+      if not dirpath:
+        raise SyntaxErrorInOwnersFile(owners_path, lineno,
+            ('%s does not refer to an existing file.' % directive[5:]))
+
+      self._read_owners_in_dir(dirpath, owners_file)
+      for key in self.owned_by:
+        if not dirpath in self.owned_by[key]:
+          continue
+        self.owned_by[key].add(path)
+
+      if dirpath in self.owners_for:
+        self.owners_for.setdefault(path, set()).update(self.owners_for[dirpath])
+
     elif self.email_regexp.match(directive) or directive == EVERYONE:
       self.comments.setdefault(directive, {})
       self.comments[directive][path] = comment
@@ -251,9 +283,25 @@ class Database(object):
       self.owners_for.setdefault(path, set()).add(directive)
     else:
       raise SyntaxErrorInOwnersFile(owners_path, lineno,
-          ('%s is not a "set" directive, "*", '
+          ('%s is not a "set" directive, include, "*", '
            'or an email address: "%s"' % (line_type, directive)))
 
+  def _resolve_include(self, include_path, base_path):
+    if include_path.startswith('//'):
+      dirpath = self.os_path.dirname(include_path[2:])
+    else:
+      assert base_path.startswith(self.root)
+      base_path = self.os_path.dirname(base_path[len(self.root):])
+      dirpath = self.os_path.join(base_path, self.os_path.dirname(include_path))
+
+    owners_file = self.os_path.basename(include_path)
+
+    owners_path = self.os_path.join(self.root, dirpath, owners_file)
+    if not self.os_path.exists(owners_path):
+      return None, None
+
+    return dirpath, owners_path

[Dirk Pranke, 2015/04/15 19:42:37]

see comment above; it's not clear to me that returning both dirpath and
owners_path is a great idea, and it's not clear what the relation between the
two values is.

[Peter Beverloo, 2015/04/15 20:04:31]

I named them to be consistent with the arguments of _read_owners_in_dir(), which
is how we use them in _add_entry().

The reason for splitting this up is to be consistent with google3 syntax: when
using includes, the name of an OWNERS file doesn't have to be "OWNERS" anymore.
The following is perfectly valid:

  "file://content/owners/my_feature"

This is covered by the test_file_include_different_filename() test.

_read_owners_in_dir() has a number of different uses of |dirpath|, which is
scoped to the root of the repository. I don't know enough about depot
tools/OWNERS files to judge whether it'd be appropriate to update all these
uses.

(Whether this is a pattern to adopt by a project is a higher level question, I
guess.)

[Dirk Pranke, 2015/04/15 20:52:03]

I'm not fully sure I understood your comments, but let me try two different
responses.

1) I agree that we will probably want to support owners files that are not named
'OWNERS',
so I don't think we're disagreeing or confused about this point.

2) I suggest that we rename _read_owners_in_dir(dirpath) to just
_read_owners(path), and 
stop passing in a dirpath (rather, join dirpath and OWNERS in
load_data_needed_for() and
other places as needed), and change change _resolve_include() to use 'path' and
'start' as
parameters and just return owners_path rather than both owners_path and dirpath.

Using the new parameter names would be consistent with os.path.relpath(), which
is
almost what you're emulating, and I think returning both dirpath and owners_file
is somewhat confusing, because it's not clear what the relationship between them
is.

I'm assuming the returned dirpath is equivalent to dirname(owners_path), but I'm
not
sure that that's true, because the way you're manipulating paths makes it a bit
hard
to follow. If that's not true, then we need comments or something to make it
clearer
how the two values are related.

Does that make more sense?

[Peter Beverloo, 2015/04/15 20:57:09]

Yes, it does. Thanks! I'll apply the changes tomorrow.

[Peter Beverloo, 2015/04/16 09:54:06]

Done.

+
   def _covering_set_of_owners_for(self, files, author):
     dirs_remaining = set(self._enclosing_dir_with_owners(f) for f in files)
     all_possible_owners = self.all_possible_owners(dirs_remaining, author)