implement per-file OWNERS support

R=maruel@chromium.org
BUG=119394


Review URL: https://chromiumcodereview.appspot.com/11114005

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@162529 0039d316-1c4b-4281-b951-d872f2087c98

owners.py

@@ -2,7 +2,51 @@
 # 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."""
+"""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 "=" directive
+          | comment
+
+directive := "set noparent"
+          |  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.
+
+Examples for all of these combinations can be found in tests/owners_unittest.py.
+"""
 
 import collections
 import re
@@ -43,16 +87,19 @@ class Database(object):
   of changed files, and see if a list of changed files is covered by a
   list of reviewers."""
 
-  def __init__(self, root, fopen, os_path):
+  def __init__(self, root, fopen, os_path, glob):
     """Args:
       root: the path to the root of the Repository
       open: function callback to open a text file for reading
       os_path: module/object callback with fields for 'abspath', 'dirname',
           'exists', and 'join'
+      glob: function callback to list entries in a directory match a glob
+          (i.e., glob.glob)
     """
     self.root = root
     self.fopen = fopen
     self.os_path = os_path
+    self.glob = glob
 
     # Pick a default email regexp to use; callers can override as desired.
     self.email_regexp = re.compile(BASIC_EMAIL_REGEXP)
@@ -75,6 +122,7 @@ class Database(object):
     self._load_data_needed_for(files)
     return self._covering_set_of_owners_for(files)
 
+  # TODO(dpranke): rename to objects_not_covered_by
   def directories_not_covered_by(self, files, reviewers):
     """Returns the set of directories that are not owned by a reviewer.
 
@@ -90,12 +138,20 @@ class Database(object):
     self._check_reviewers(reviewers)
     self._load_data_needed_for(files)
 
-    dirs = set([self.os_path.dirname(f) for f in files])
-    covered_dirs = self._dirs_covered_by(reviewers)
-    uncovered_dirs = [self._enclosing_dir_with_owners(d) for d in dirs
-                      if not self._is_dir_covered_by(d, covered_dirs)]
+    objs = set()
+    for f in files:
+      if f in self.owners_for:
+        objs.add(f)
+      else:
+        objs.add(self.os_path.dirname(f))
+
+    covered_objs = self._objs_covered_by(reviewers)
+    uncovered_objs = [self._enclosing_obj_with_owners(o) for o in objs
+                      if not self._is_obj_covered_by(o, covered_objs)]
 
-    return set(uncovered_dirs)
+    return set(uncovered_objs)
+
+  objects_not_covered_by = directories_not_covered_by
 
   def _check_paths(self, files):
     def _is_under(f, pfx):
@@ -107,20 +163,27 @@ class Database(object):
     _assert_is_collection(reviewers)
     assert all(self.email_regexp.match(r) for r in reviewers)
 
+  # TODO(dpranke): Rename to _objs_covered_by and update_callers
   def _dirs_covered_by(self, reviewers):
     dirs = self.owned_by[EVERYONE]
     for r in reviewers:
       dirs = dirs | self.owned_by.get(r, set())
     return dirs
 
+  _objs_covered_by = _dirs_covered_by
+
   def _stop_looking(self, dirname):
     return dirname in self.stop_looking
 
+  # TODO(dpranke): Rename to _is_dir_covered_by and update callers.
   def _is_dir_covered_by(self, dirname, covered_dirs):
     while not dirname in covered_dirs and not self._stop_looking(dirname):
       dirname = self.os_path.dirname(dirname)
     return dirname in covered_dirs
 
+  _is_obj_covered_by = _is_dir_covered_by
+
+  # TODO(dpranke): Rename to _enclosing_obj_with_owners and update callers.
   def _enclosing_dir_with_owners(self, directory):
     """Returns the innermost enclosing directory that has an OWNERS file."""
     dirpath = directory
@@ -130,6 +193,8 @@ class Database(object):
       dirpath = self.os_path.dirname(dirpath)
     return dirpath
 
+  _enclosing_obj_with_owners = _enclosing_dir_with_owners
+
   def _load_data_needed_for(self, files):
     for f in files:
       dirpath = self.os_path.dirname(f)
@@ -153,16 +218,35 @@ class Database(object):
       if line == 'set noparent':
         self.stop_looking.add(dirpath)
         continue
+
+      m = re.match("per-file (.+)=(.+)", line)
+      if m:
+        glob_string = m.group(1)
+        directive = m.group(2)
+        full_glob_string = self.os_path.join(self.root, dirpath, glob_string)
+        baselines = self.glob(full_glob_string)
+        for baseline in (self.os_path.relpath(self.root, b) for b in baselines):
+          self._add_entry(baseline, directive, "per-file line",
+                          owners_path, lineno)
+        continue
+
       if line.startswith('set '):
         raise SyntaxErrorInOwnersFile(owners_path, lineno,
             'unknown option: "%s"' % line[4:].strip())
-      if self.email_regexp.match(line) or line == EVERYONE:
-        self.owned_by.setdefault(line, set()).add(dirpath)
-        self.owners_for.setdefault(dirpath, set()).add(line)
-        continue
+
+      self._add_entry(dirpath, line, "line", owners_path, lineno)
+
+  def _add_entry(self, path, directive, line_type, owners_path, lineno):
+    if directive == "set noparent":
+      self.stop_looking.add(path)
+    elif self.email_regexp.match(directive) or directive == EVERYONE:
+      self.owned_by.setdefault(directive, set()).add(path)
+      self.owners_for.setdefault(path, set()).add(directive)
+    else:
       raise SyntaxErrorInOwnersFile(owners_path, lineno,
-            ('line is not a comment, a "set" directive, '
-             'or an email address: "%s"' % line))
+          ('%s is not a "set" directive, "*", '
+           'or an email address: "%s"' % (line_type, directive)))
+
 
   def _covering_set_of_owners_for(self, files):
     # Get the set of directories from the files.

presubmit_support.py

@@ -238,6 +238,7 @@ class InputApi(object):
     self.basename = os.path.basename
     self.cPickle = cPickle
     self.cStringIO = cStringIO
+    self.glob = glob.glob
     self.json = json
     self.logging = logging.getLogger('PRESUBMIT')
     self.os_listdir = os.listdir
@@ -269,7 +270,7 @@ class InputApi(object):
     # TODO(dpranke): figure out a list of all approved owners for a repo
     # in order to be able to handle wildcard OWNERS files?
     self.owners_db = owners.Database(change.RepositoryRoot(),
-        fopen=file, os_path=self.os_path)
+        fopen=file, os_path=self.os_path, glob=self.glob)
     self.verbose = verbose
 
   def PresubmitLocalPath(self):

testing_support/filesystem_mock.py

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import errno
+import fnmatch
 import os
 import re
 import StringIO
@@ -65,6 +66,9 @@ class MockFileSystem(object):
     # it works.
     return re.sub(re.escape(os.path.sep), self.sep, os.path.join(*comps))
 
+  def glob(self, path):
+    return fnmatch.filter(self.files.keys(), path)
+
   def open_for_reading(self, path):
     return StringIO.StringIO(self.read_binary_file(path))
 
@@ -73,3 +77,9 @@ class MockFileSystem(object):
     if self.files[path] is None:
       _RaiseNotFound(path)
     return self.files[path]
+
+  @staticmethod
+  def relpath(base, path):
+    if path.startswith(base):
+      return path[len(base):]
+    return path

tests/owners_unittest.py

@@ -21,6 +21,8 @@ darin = 'darin@example.com'
 john = 'john@example.com'
 ken = 'ken@example.com'
 peter = 'peter@example.com'
+tom = 'tom@example.com'
+
 
 def owners_file(*email_addresses, **kwargs):
   s = ''
@@ -28,6 +30,7 @@ def owners_file(*email_addresses, **kwargs):
     s += '# %s\n' % kwargs.get('comment')
   if kwargs.get('noparent'):
     s += 'set noparent\n'
+  s += '\n'.join(kwargs.get('lines', [])) + '\n'
   return s + '\n'.join(email_addresses) + '\n'
 
 
@@ -47,6 +50,8 @@ def test_repo():
     '/content/bar/foo.cc': '',
     '/content/baz/OWNERS': owners_file(brett),
     '/content/baz/froboz.h': '',
+    '/content/baz/ugly.cc': '',
+    '/content/baz/ugly.h': '',
     '/content/views/OWNERS': owners_file(ben, john, owners.EVERYONE,
                                          noparent=True),
     '/content/views/pie.h': '',
@@ -59,12 +64,14 @@ class OwnersDatabaseTest(unittest.TestCase):
     self.files = self.repo.files
     self.root = '/'
     self.fopen = self.repo.open_for_reading
+    self.glob = self.repo.glob
 
-  def db(self, root=None, fopen=None, os_path=None):
+  def db(self, root=None, fopen=None, os_path=None, glob=None):
     root = root or self.root
     fopen = fopen or self.fopen
     os_path = os_path or self.repo
-    return owners.Database(root, fopen, os_path)
+    glob = glob or self.glob
+    return owners.Database(root, fopen, os_path, glob)
 
   def test_constructor(self):
     self.assertNotEquals(self.db(), None)
@@ -131,6 +138,42 @@ class OwnersDatabaseTest(unittest.TestCase):
         [ken],
         ['content', 'content/baz'])
 
+  def test_per_file(self):
+    # brett isn't allowed to approve ugly.cc
+    self.files['/content/baz/OWNERS'] = owners_file(brett,
+        lines=['per-file ugly.*=tom@example.com'])
+    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+                                    [brett],
+                                    [])
+
+    # tom is allowed to approve ugly.cc, but not froboz.h
+    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+                                    [tom],
+                                    [])
+    self.assert_dirs_not_covered_by(['content/baz/froboz.h'],
+                                    [tom],
+                                    ['content/baz'])
+
+  def test_per_file__set_noparent(self):
+    self.files['/content/baz/OWNERS'] = owners_file(brett,
+        lines=['per-file ugly.*=tom@example.com',
+               'per-file ugly.*=set noparent'])
+
+    # brett isn't allowed to approve ugly.cc
+    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+                                    [brett],
+                                    ['content/baz/ugly.cc'])
+
+    # tom is allowed to approve ugly.cc, but not froboz.h
+    self.assert_dirs_not_covered_by(['content/baz/ugly.cc'],
+                                    [tom],
+                                    [])
+
+    self.assert_dirs_not_covered_by(['content/baz/froboz.h'],
+                                    [tom],
+                                    ['content/baz'])
+
+
   def assert_reviewers_for(self, files, expected_reviewers):
     db = self.db()
     self.assertEquals(db.reviewers_for(set(files)), set(expected_reviewers))

tests/presubmit_unittest.py

@@ -748,7 +748,7 @@ def CheckChangeOnCommit(input_api, output_api):
     expected_result1 = ['1', '2']
     expected_result2 = ['a', 'b', 'c']
     script = self.presubmit_tryslave_project % (
-        repr('foo'), repr(expected_result1), repr(expected_result2)) 
+        repr('foo'), repr(expected_result1), repr(expected_result2))
     self.assertEqual(
         expected_result1, executer.ExecPresubmitScript(script, '', 'foo',
                                                        change))
@@ -868,8 +868,8 @@ class InputApiUnittest(PresubmitTestsBase):
       'LocalToDepotPath',
       'PresubmitLocalPath', 'ReadFile', 'RightHandSideLines', 'ServerPaths',
       'basename', 'cPickle', 'cStringIO', 'canned_checks', 'change', 'environ',
-      'host_url', 'is_committing', 'json', 'logging', 'marshal', 'os_listdir',
-      'os_walk',
+      'glob', 'host_url', 'is_committing', 'json', 'logging', 'marshal',
+      'os_listdir', 'os_walk',
       'os_path', 'owners_db', 'pickle', 'platform', 'python_executable', 're',
       'rietveld', 'subprocess', 'tbr', 'tempfile', 'time', 'traceback',
       'unittest', 'urllib2', 'version', 'verbose',