Implement support for file: includes in OWNERS files.

owners.py

 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """A database of OWNERS files.
 
 OWNERS files indicate who is allowed to approve changes in a specific directory
 (or who is allowed to make changes without needing approval of another OWNER).
 Note that all changes must still be reviewed by someone familiar with the code,
 so you may need approval from both an OWNER and a reviewer in many cases.
 
 The syntax of the OWNERS file is, roughly:
 
 lines     := (\s* line? \s* "\n")*
 
 line      := directive
           | "per-file" \s+ glob \s* "=" \s* directive
           | comment
 
 directive := "set noparent"
+          |  "file:" glob
           |  email_address
           |  "*"
 
 glob      := [a-zA-Z0-9_-*?]+
 
 comment   := "#" [^"\n"]*
 
 Email addresses must follow the foo@bar.com short form (exact syntax given
 in BASIC_EMAIL_REGEXP, below). Filename globs follow the simple unix
 shell conventions, and relative and absolute paths are not allowed (i.e.,
 globs only refer to the files in the current directory).
 
 If a user's email is one of the email_addresses in the file, the user is
 considered an "OWNER" for all files in the directory.
 
 If the "per-file" directive is used, the line only applies to files in that
 directory that match the filename glob specified.
 
 If the "set noparent" directive used, then only entries in this OWNERS file
 apply to files in this directory; if the "set noparent" directive is not
 used, then entries in OWNERS files in enclosing (upper) directories also
 apply (up until a "set noparent is encountered").
 
 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.
 """
 
 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 = '*'
 
 
 # Recognizes 'X@Y' email addresses. Very simplistic.
 BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$'
 
 
 def _assert_is_collection(obj):
@@ skipping 46 matching lines @@
     # Mapping of paths to authorized owners.
     self.owners_for = {}
 
     # Mapping reviewers to the preceding comment per file in the OWNERS files.
     self.comments = {}
 
     # Set of paths that stop us from looking above them for owners.
     # (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.
 
     files is a sequence of paths relative to (and under) self.root.
     If author is nonempty, we ensure it is not included in the set returned
     in order avoid suggesting the author as a reviewer for their own changes."""
     self._check_paths(files)
     self.load_data_needed_for(files)
     suggested_owners = self._covering_set_of_owners_for(files, author)
     if EVERYONE in suggested_owners:
@@ skipping 51 matching lines @@
     while not dirpath in self.owners_for:
       if self._stop_looking(dirpath):
         break
       dirpath = self.os_path.dirname(dirpath)
     return dirpath
 
   def load_data_needed_for(self, files):
     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
     for line in self.fopen(owners_path):
       lineno += 1
       line = line.strip()
       if line.startswith('#'):
         if not in_comment:
           comment = []
         comment.append(line[1:].strip())
@@ skipping 26 matching lines @@
         raise SyntaxErrorInOwnersFile(owners_path, lineno,
             'unknown option: "%s"' % line[4:].strip())
 
       self._add_entry(dirpath, line, 'line', owners_path, lineno,
                       ' '.join(comment))
 
   def _add_entry(self, path, directive,
                  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
       self.owned_by.setdefault(directive, set()).add(path)
       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)
     suggested_owners = set()
     while dirs_remaining:
       owner = self.lowest_cost_owner(all_possible_owners, dirs_remaining)
       suggested_owners.add(owner)
       dirs_to_remove = set(el[0] for el in all_possible_owners[owner])
       dirs_remaining -= dirs_to_remove
     return suggested_owners
@@ skipping 44 matching lines @@
   @staticmethod
   def lowest_cost_owner(all_possible_owners, dirs):
     total_costs_by_owner = Database.total_costs_by_owner(all_possible_owners,
                                                          dirs)
     # Return the lowest cost owner. In the case of a tie, pick one randomly.
     lowest_cost = min(total_costs_by_owner.itervalues())
     lowest_cost_owners = filter(
         lambda owner: total_costs_by_owner[owner] == lowest_cost,
         total_costs_by_owner)
     return random.Random().choice(lowest_cost_owners)