Add support for a global status files for OWNERS

This allows for having some global comments such as timezones or
long-term unavailability.

The comments go into build/OWNERS.status (that way, they should be
available in all repos that map in build/ for the gn config files).
The local can be overwritten in codereview.settings.

The format is

email: status

Comments (starting with #) are allowed in that file, but they're ignored.

BUG=694222
R=dpranke@chromium.org

Change-Id: I49f58be87497d1ccaaa74f0a2f3d373403be44e7
Reviewed-on: https://chromium-review.googlesource.com/459542
Commit-Queue: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

git_cl.py

@@ -3244,7 +3244,8 @@ class ChangeDescription(object):
         reviewers.append(name)
     if add_owners_tbr:
       owners_db = owners.Database(change.RepositoryRoot(),
-        fopen=file, os_path=os.path)
+                                  change.GetOwnersStatusFile(),
+                                  fopen=file, os_path=os.path)
       all_reviewers = set(tbr_names + reviewers)
       missing_files = owners_db.files_not_covered_by(change.LocalPaths(),
                                                      all_reviewers)
@@ -3463,6 +3464,9 @@ def LoadCodereviewSettingsFromFile(fileobj):
   SetProperty('run-post-upload-hook', 'RUN_POST_UPLOAD_HOOK',
               unset_error_ok=True)
 
+  if 'OWNERS_STATUS_FILE' in keyvals:
+    SetProperty('owners-status-file', 'OWNERS_STATUS_FILE', unset_error_ok=True)
+
   if 'GERRIT_HOST' in keyvals:
     RunGit(['config', 'gerrit.host', keyvals['GERRIT_HOST']])
 
@@ -5512,8 +5516,9 @@ def CMDowners(parser, args):
   return owners_finder.OwnersFinder(
       [f.LocalPath() for f in
           cl.GetChange(base_branch, None).AffectedFiles()],
-      change.RepositoryRoot(), author,
-      fopen=file, os_path=os.path,
+      change.RepositoryRoot(),
+      change.GetOwnersStatusFile(),
+      author, fopen=file, os_path=os.path,
       disable_color=options.no_color).run()
 
 

owners.py

@@ -68,6 +68,11 @@ EVERYONE = '*'
 BASIC_EMAIL_REGEXP = r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$'
 
 
+# Key for global comments per email address. Should be unlikely to be a
+# pathname.
+GLOBAL_STATUS = '*'
+
+
 def _assert_is_collection(obj):
   assert not isinstance(obj, basestring)
   # Module 'collections' has no 'Iterable' member
@@ -95,14 +100,16 @@ 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, status_file, fopen, os_path):
     """Args:
       root: the path to the root of the Repository
+      status_file: the path relative to root to global status entries or None
       open: function callback to open a text file for reading
       os_path: module/object callback with fields for 'abspath', 'dirname',
           'exists', 'join', and 'relpath'
     """
     self.root = root
+    self.status_file = status_file
     self.fopen = fopen
     self.os_path = os_path
 
@@ -196,6 +203,7 @@ class Database(object):
     return dirpath
 
   def load_data_needed_for(self, files):
+    self._read_global_comments()
     for f in files:
       dirpath = self.os_path.dirname(f)
       while not self._owners_for(dirpath):
@@ -267,6 +275,44 @@ class Database(object):
       self._add_entry(dirpath, line, owners_path, lineno,
                       ' '.join(comment))
 
+  def _read_global_comments(self):
+    if not self.status_file:
+      return
+
+    owners_status_path = self.os_path.join(self.root, self.status_file)
+    if not self.os_path.exists(owners_status_path):
+      raise IOError('Could not find global status file "%s"' % 
+                    owners_status_path)
+
+    if owners_status_path in self.read_files:
+      return
+
+    self.read_files.add(owners_status_path)
+
+    lineno = 0
+    for line in self.fopen(owners_status_path):
+      lineno += 1
+      line = line.strip()
+      if line.startswith('#'):
+        continue
+      if line == '':
+        continue
+
+      m = re.match('(.+?):(.+)', line)
+      if m:
+        owner = m.group(1).strip()
+        comment = m.group(2).strip()
+        if not self.email_regexp.match(owner):
+          raise SyntaxErrorInOwnersFile(owners_status_path, lineno,
+              'invalid email address: "%s"' % owner)
+
+        self.comments.setdefault(owner, {})
+        self.comments[owner][GLOBAL_STATUS] = comment
+        continue
+
+      raise SyntaxErrorInOwnersFile(owners_status_path, lineno,
+          'cannot parse status entry: "%s"' % line.strip())
+
   def _add_entry(self, owned_paths, directive, owners_path, lineno, comment):
     if directive == 'set noparent':
       self._stop_looking.add(owned_paths)
@@ -281,8 +327,9 @@ class Database(object):
         self._owners_to_paths.setdefault(owner, set()).add(owned_paths)
         self._paths_to_owners.setdefault(owned_paths, set()).add(owner)
     elif self.email_regexp.match(directive) or directive == EVERYONE:
-      self.comments.setdefault(directive, {})
-      self.comments[directive][owned_paths] = comment
+      if comment:
+        self.comments.setdefault(directive, {})
+        self.comments[directive][owned_paths] = comment
       self._owners_to_paths.setdefault(directive, set()).add(owned_paths)
       self._paths_to_owners.setdefault(owned_paths, set()).add(directive)
     else:

owners_finder.py

@@ -22,7 +22,7 @@ class OwnersFinder(object):
 
   indentation = 0
 
-  def __init__(self, files, local_root, author,
+  def __init__(self, files, local_root, owners_status_file, author,
                fopen, os_path,
                email_postfix='@chromium.org',
                disable_color=False):
@@ -34,7 +34,8 @@ class OwnersFinder(object):
       self.COLOR_GREY = ''
       self.COLOR_RESET = ''
 
-    self.db = owners_module.Database(local_root, fopen, os_path)
+    self.db = owners_module.Database(local_root, owners_status_file, fopen,
+                                     os_path)
     self.db.load_data_needed_for(files)
 
     self.os_path = os_path
@@ -51,7 +52,8 @@ class OwnersFinder(object):
     if len(filtered_files) != len(files):
       files = filtered_files
       # Reload the database.
-      self.db = owners_module.Database(local_root, fopen, os_path)
+      self.db = owners_module.Database(local_root, owners_status_file, fopen,
+                                       os_path)
       self.db.load_data_needed_for(files)
 
     self.all_possible_owners = self.db.all_possible_owners(files, None)
@@ -207,8 +209,17 @@ class OwnersFinder(object):
     else:
       self.writeln(self.bold_name(owner) + ' is commented as:')
       self.indent()
+      if owners_module.GLOBAL_STATUS in self.comments[owner]:
+        self.writeln(
+            self.greyed(self.comments[owner][owners_module.GLOBAL_STATUS]) +
+            ' (global status)')
+        if len(self.comments[owner]) == 1:
+          self.unindent()
+          return
       for path in self.comments[owner]:
-        if len(self.comments[owner][path]) > 0:
+        if path == owners_module.GLOBAL_STATUS:
+          continue
+        elif len(self.comments[owner][path]) > 0:
           self.writeln(self.greyed(self.comments[owner][path]) +
                        ' (at ' + self.bold(path or '<root>') + ')')
         else:

presubmit_support.py

@@ -418,10 +418,12 @@ class InputApi(object):
     # We carry the canned checks so presubmit scripts can easily use them.
     self.canned_checks = presubmit_canned_checks
 
+
     # 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)
+                                     change.GetOwnersStatusFile(),
+                                     fopen=file, os_path=self.os_path)
     self.verbose = verbose
     self.Command = CommandData
 
@@ -916,6 +918,10 @@ class Change(object):
         x for x in self.AffectedFiles(include_deletes=False)
         if x.IsTestableFile())
 
+  def GetOwnersStatusFile(self):
+    """Returns the name of the global OWNERS status file."""
+    return None
+
 
 class GitChange(Change):
   _AFFECTED_FILES = GitAffectedFile
@@ -927,6 +933,19 @@ class GitChange(Change):
     return subprocess.check_output(
         ['git', 'ls-files', '--', '.'], cwd=root).splitlines()
 
+  def GetOwnersStatusFile(self):
+    """Returns the name of the global OWNERS status file."""
+
+    try:
+      status_file = subprocess.check_output(
+          ['git', 'config', 'rietveld.owners-status-file'],
+          cwd=self.RepositoryRoot())
+      return status_file
+    except subprocess.CalledProcessError:
+      pass
+
+    return None
+
 
 def ListRelevantPresubmitFiles(files, root):
   """Finds all presubmit files that apply to a given set of source files.

tests/git_cl_test.py

@@ -684,6 +684,25 @@ class TestGitCl(TestCase):
     ]
     self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file))
 
+  def test_LoadCodereviewSettingsFromFile_owners_status(self):
+    codereview_file = StringIO.StringIO('OWNERS_STATUS_FILE: status')
+    self.calls = [
+      ((['git', 'config', '--unset-all', 'rietveld.server'],), ''),
+      ((['git', 'config', '--unset-all', 'rietveld.cc'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.private'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.tree-status-url'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.viewvc-url'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.bug-prefix'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.cpplint-regex'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.cpplint-ignore-regex'],),
+        CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.project'],), CERR1),
+      ((['git', 'config', '--unset-all', 'rietveld.run-post-upload-hook'],),
+        CERR1),
+      ((['git', 'config', 'rietveld.owners-status-file', 'status'],), ''),
+    ]
+    self.assertIsNone(git_cl.LoadCodereviewSettingsFromFile(codereview_file))
+
   @classmethod
   def _is_gerrit_calls(cls, gerrit=False):
     return [((['git', 'config', 'rietveld.autoupdate'],), ''),

tests/owners_finder_test.py

@@ -21,6 +21,7 @@ import owners
 ben = 'ben@example.com'
 brett = 'brett@example.com'
 darin = 'darin@example.com'
+jochen = 'jochen@example.com'
 john = 'john@example.com'
 ken = 'ken@example.com'
 peter = 'peter@example.com'
@@ -41,6 +42,7 @@ def test_repo():
   return filesystem_mock.MockFileSystem(files={
     '/DEPS': '',
     '/OWNERS': owners_file(ken, peter, tom),
+    '/build/OWNERS.status': '%s: bar' % jochen,
     '/base/vlog.h': '',
     '/chrome/OWNERS': owners_file(ben, brett),
     '/chrome/browser/OWNERS': owners_file(brett),
@@ -57,6 +59,10 @@ def test_repo():
     '/content/baz/froboz.h': '',
     '/content/baz/ugly.cc': '',
     '/content/baz/ugly.h': '',
+    '/content/common/OWNERS': owners_file(jochen),
+    '/content/common/common.cc': '',
+    '/content/foo/OWNERS': owners_file(jochen, comment='foo'),
+    '/content/foo/foo.cc': '',
     '/content/views/OWNERS': owners_file(ben, john, owners.EVERYONE,
                                          noparent=True),
     '/content/views/pie.h': '',
@@ -67,7 +73,7 @@ class OutputInterceptedOwnersFinder(owners_finder.OwnersFinder):
   def __init__(self, files, local_root,
                fopen, os_path, disable_color=False):
     super(OutputInterceptedOwnersFinder, self).__init__(
-      files, local_root, None,
+      files, local_root, os_path.join('build', 'OWNERS.status'), None,
       fopen, os_path, disable_color=disable_color)
     self.output = []
     self.indentation_stack = []
@@ -232,6 +238,17 @@ class OwnersFinderTests(_BaseTestCase):
     self.assertEqual(finder.output,
                      [darin + ' is commented as:', ['foo (at content)']])
 
+  def test_print_global_comments(self):
+    finder = self.ownersFinder(['content/common/common.cc'])
+    finder.print_comments(jochen)
+    self.assertEqual(finder.output,
+                     [jochen + ' is commented as:', ['bar (global status)']])
+
+    finder = self.ownersFinder(['content/foo/foo.cc'])
+    finder.print_comments(jochen)
+    self.assertEqual(finder.output,
+                     [jochen + ' is commented as:', ['bar (global status)',
+                                                     'foo (at content/foo)']])
 
 if __name__ == '__main__':
   unittest.main()

tests/owners_unittest.py

@@ -74,11 +74,11 @@ class _BaseTestCase(unittest.TestCase):
     self.root = '/'
     self.fopen = self.repo.open_for_reading
 
-  def db(self, root=None, fopen=None, os_path=None):
+  def db(self, root=None, fopen=None, os_path=None, status_file=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)
+    return owners.Database(root, status_file, fopen, os_path)
 
 
 class OwnersDatabaseTest(_BaseTestCase):
@@ -334,6 +334,12 @@ class OwnersDatabaseTest(_BaseTestCase):
   def test_syntax_error__invalid_relative_file(self):
     self.assert_syntax_error('file:foo/bar/baz\n')
 
+  def test_non_existant_status_file(self):
+    db = self.db(status_file='does_not_exist')
+    self.files['/foo/OWNERS'] = brett
+    self.files['/foo/DEPS'] = ''
+    self.assertRaises(IOError, db.reviewers_for, ['foo/DEPS'], None)
+
 
 class ReviewersForTest(_BaseTestCase):
   def assert_reviewers_for(self, files, potential_suggested_reviewers,

tests/presubmit_unittest.py

@@ -120,6 +120,8 @@ index fe3de7b..54ae6e1 100755
         self._root = obj.fake_root_dir
       def RepositoryRoot(self):
         return self._root
+      def GetOwnersStatusFile(self):
+        return None
 
     self.mox.StubOutWithMock(presubmit, 'random')
     self.mox.StubOutWithMock(presubmit, 'warn')
@@ -511,6 +513,7 @@ class PresubmitUnittest(PresubmitTestsBase):
         0,
         0,
         None)
+    change.GetOwnersStatusFile = lambda: None
     executer = presubmit.PresubmitExecuter(change, False, None, False)
     self.failIf(executer.ExecPresubmitScript('', fake_presubmit))
     # No error if no on-upload entry point
@@ -1065,6 +1068,7 @@ class InputApiUnittest(PresubmitTestsBase):
         0,
         0,
         None)
+    change.GetOwnersStatusFile = lambda: None
     input_api = presubmit.InputApi(
         change,
         presubmit.os.path.join(self.fake_root_dir, 'foo', 'PRESUBMIT.py'),
@@ -1193,6 +1197,7 @@ class InputApiUnittest(PresubmitTestsBase):
 
     change = presubmit.GitChange(
         'mychange', '', self.fake_root_dir, files, 0, 0, None)
+    change.GetOwnersStatusFile = lambda: None
     input_api = presubmit.InputApi(
         change,
         presubmit.os.path.join(self.fake_root_dir, 'PRESUBMIT.py'),
@@ -1213,6 +1218,7 @@ class InputApiUnittest(PresubmitTestsBase):
 
     change = presubmit.GitChange(
         'mychange', '', self.fake_root_dir, files, 0, 0, None)
+    change.GetOwnersStatusFile = lambda: None
     input_api = presubmit.InputApi(
         change, './PRESUBMIT.py', False, None, False)
     # Sample usage of overiding the default white and black lists.
@@ -1535,9 +1541,9 @@ class ChangeUnittest(PresubmitTestsBase):
     members = [
         'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTestableFiles',
         'AffectedTextFiles',
-        'AllFiles', 'DescriptionText', 'FullDescriptionText', 'LocalPaths',
-        'Name', 'RepositoryRoot', 'RightHandSideLines',
-        'SetDescriptionText', 'TAG_LINE_RE',
+        'AllFiles', 'DescriptionText', 'FullDescriptionText',
+        'GetOwnersStatusFile', 'LocalPaths', 'Name', 'RepositoryRoot',
+        'RightHandSideLines', 'SetDescriptionText', 'TAG_LINE_RE',
         'author_email', 'issue', 'patchset', 'scm', 'tags',
     ]
     # If this test fails, you should add the relevant test.