config: refactor file management am: 1baec120bf

Original change: https://android-review.googlesource.com/c/platform/tools/repohooks/+/1409412

Change-Id: Id7ed769c4436ae3d316ad046d1c181fd316f60b3

3 files changed, 133 insertions, 71 deletions

diff --git a/pre-upload.py b/pre-upload.py
index 57f699b..ffefd09 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -212,7 +212,7 @@ def _get_project_config():
         # Load the config for this git repo.
         '.',
     )
-    return rh.config.PreUploadConfig(paths=paths, global_paths=global_paths)
+    return rh.config.PreUploadSettings(paths=paths, global_paths=global_paths)
 
 
 def _attempt_fixes(fixup_func_list, commit_list):
diff --git a/rh/config.py b/rh/config.py
index c3d983d..6240559 100644
--- a/rh/config.py
+++ b/rh/config.py
@@ -98,11 +98,9 @@ class RawConfigParser(configparser.RawConfigParser):
                         self.add_section(section)
                     self.set(section, option, value)
 
-class PreUploadConfig(object):
-    """Config file used for per-project `repo upload` hooks."""
 
-    FILENAME = 'PREUPLOAD.cfg'
-    GLOBAL_FILENAME = 'GLOBAL-PREUPLOAD.cfg'
+class PreUploadConfig(object):
+    """A single (abstract) config used for `repo upload` hooks."""
 
     CUSTOM_HOOKS_SECTION = 'Hook Scripts'
     BUILTIN_HOOKS_SECTION = 'Builtin Hooks'
@@ -120,34 +118,17 @@ class PreUploadConfig(object):
     OPTION_IGNORE_MERGED_COMMITS = 'ignore_merged_commits'
     VALID_OPTIONS = {OPTION_IGNORE_MERGED_COMMITS}
 
-    def __init__(self, paths=('',), global_paths=()):
+    def __init__(self, config=None, source=None):
         """Initialize.
 
-        All the config files found will be merged together in order.
-
         Args:
-          paths: The directories to look for config files.
-          global_paths: The directories to look for global config files.
+          config: A configparse.ConfigParser instance.
+          source: Where this config came from.
         """
-        config = RawConfigParser()
-
-        def _search(paths, filename):
-            for path in paths:
-                path = os.path.join(path, filename)
-                if os.path.exists(path):
-                    self.paths.append(path)
-                    try:
-                        config.read(path)
-                    except configparser.ParsingError as e:
-                        raise ValidationError('%s: %s' % (path, e))
-
-        self.paths = []
-        _search(global_paths, self.GLOBAL_FILENAME)
-        _search(paths, self.FILENAME)
-
-        self.config = config
-
-        self._validate()
+        self.config = config if config else RawConfigParser()
+        self.source = source
+        if config:
+            self._validate()
 
     @property
     def custom_hooks(self):
@@ -198,6 +179,10 @@ class PreUploadConfig(object):
                             self.OPTION_IGNORE_MERGED_COMMITS, None),
             False)
 
+    def update(self, preupload_config):
+        """Merge settings from |preupload_config| into ourself."""
+        self.config.read_dict(preupload_config.config)
+
     def _validate(self):
         """Run consistency checks on the config settings."""
         config = self.config
@@ -206,13 +191,13 @@ class PreUploadConfig(object):
         bad_sections = set(config.sections()) - self.VALID_SECTIONS
         if bad_sections:
             raise ValidationError('%s: unknown sections: %s' %
-                                  (self.paths, bad_sections))
+                                  (self.source, bad_sections))
 
         # Reject blank custom hooks.
         for hook in self.custom_hooks:
             if not config.get(self.CUSTOM_HOOKS_SECTION, hook):
                 raise ValidationError('%s: custom hook "%s" cannot be blank' %
-                                      (self.paths, hook))
+                                      (self.source, hook))
 
         # Reject unknown builtin hooks.
         valid_builtin_hooks = set(rh.hooks.BUILTIN_HOOKS.keys())
@@ -221,7 +206,7 @@ class PreUploadConfig(object):
             bad_hooks = hooks - valid_builtin_hooks
             if bad_hooks:
                 raise ValidationError('%s: unknown builtin hooks: %s' %
-                                      (self.paths, bad_hooks))
+                                      (self.source, bad_hooks))
         elif config.has_section(self.BUILTIN_HOOKS_OPTIONS_SECTION):
             raise ValidationError('Builtin hook options specified, but missing '
                                   'builtin hook settings')
@@ -231,7 +216,7 @@ class PreUploadConfig(object):
             bad_hooks = hooks - valid_builtin_hooks
             if bad_hooks:
                 raise ValidationError('%s: unknown builtin hook options: %s' %
-                                      (self.paths, bad_hooks))
+                                      (self.source, bad_hooks))
 
         # Verify hooks are valid shell strings.
         for hook in self.custom_hooks:
@@ -239,7 +224,7 @@ class PreUploadConfig(object):
                 self.custom_hook(hook)
             except ValueError as e:
                 raise ValidationError('%s: hook "%s" command line is invalid: '
-                                      '%s' % (self.paths, hook, e))
+                                      '%s' % (self.source, hook, e))
 
         # Verify hook options are valid shell strings.
         for hook in self.builtin_hooks:
@@ -247,7 +232,7 @@ class PreUploadConfig(object):
                 self.builtin_hook_option(hook)
             except ValueError as e:
                 raise ValidationError('%s: hook options "%s" are invalid: %s' %
-                                      (self.paths, hook, e))
+                                      (self.source, hook, e))
 
         # Reject unknown tools.
         valid_tools = set(rh.hooks.TOOL_PATHS.keys())
@@ -256,7 +241,7 @@ class PreUploadConfig(object):
             bad_tools = tools - valid_tools
             if bad_tools:
                 raise ValidationError('%s: unknown tools: %s' %
-                                      (self.paths, bad_tools))
+                                      (self.source, bad_tools))
 
         # Reject unknown options.
         if config.has_section(self.OPTIONS_SECTION):
@@ -264,4 +249,60 @@ class PreUploadConfig(object):
             bad_options = options - self.VALID_OPTIONS
             if bad_options:
                 raise ValidationError('%s: unknown options: %s' %
-                                      (self.paths, bad_options))
+                                      (self.source, bad_options))
+
+
+class PreUploadFile(PreUploadConfig):
+    """A single config (file) used for `repo upload` hooks."""
+
+    def __init__(self, path):
+        """Initialize.
+
+        Args:
+          path: The config file to load.
+        """
+        super(PreUploadFile, self).__init__(source=path)
+
+        try:
+            self.config.read(path)
+        except configparser.ParsingError as e:
+            raise ValidationError('%s: %s' % (path, e))
+
+        self._validate()
+
+
+class PreUploadSettings(PreUploadConfig):
+    """Settings for `repo upload` hooks.
+
+    This encompasses multiple config files and provides the final (merged)
+    settings for a particular project.
+    """
+
+    FILENAME = 'PREUPLOAD.cfg'
+    GLOBAL_FILENAME = 'GLOBAL-PREUPLOAD.cfg'
+
+    def __init__(self, paths=('',), global_paths=()):
+        """Initialize.
+
+        All the config files found will be merged together in order.
+
+        Args:
+          paths: The directories to look for config files.
+          global_paths: The directories to look for global config files.
+        """
+        super(PreUploadSettings, self).__init__()
+
+        def _search(paths, filename):
+            for path in paths:
+                path = os.path.join(path, filename)
+                if os.path.exists(path):
+                    self.paths.append(path)
+                    self.update(PreUploadFile(path))
+
+        self.paths = []
+        _search(global_paths, self.GLOBAL_FILENAME)
+        _search(paths, self.FILENAME)
+
+        # We validated configs in isolation, now do one final pass altogether.
+        self.source = '{%s}' % '|'.join(self.paths)
+        self._validate()
diff --git a/rh/config_unittest.py b/rh/config_unittest.py
index d51afdc..08128e8 100755
--- a/rh/config_unittest.py
+++ b/rh/config_unittest.py
@@ -39,37 +39,35 @@ import rh.config
 class PreUploadConfigTests(unittest.TestCase):
     """Tests for the PreUploadConfig class."""
 
+    def testMissing(self):
+        """Instantiating a non-existent config file should be fine."""
+        rh.config.PreUploadConfig()
+
+
+class PreUploadFileTests(unittest.TestCase):
+    """Tests for the PreUploadFile class."""
+
     def setUp(self):
         self.tempdir = tempfile.mkdtemp()
 
     def tearDown(self):
         shutil.rmtree(self.tempdir)
 
-    def _write_config(self, data, filename=None):
+    def _write_config(self, data):
         """Helper to write out a config file for testing."""
-        if filename is None:
-            filename = rh.config.PreUploadConfig.FILENAME
-        path = os.path.join(self.tempdir, filename)
+        path = os.path.join(self.tempdir, 'temp.cfg')
         with open(path, 'w') as fp:
             fp.write(data)
-
-    def _write_global_config(self, data):
-        """Helper to write out a global config file for testing."""
-        self._write_config(
-            data, filename=rh.config.PreUploadConfig.GLOBAL_FILENAME)
-
-    def testMissing(self):
-        """Instantiating a non-existent config file should be fine."""
-        rh.config.PreUploadConfig()
+        return path
 
     def testEmpty(self):
         """Instantiating an empty config file should be fine."""
-        self._write_config('')
-        rh.config.PreUploadConfig(paths=(self.tempdir,))
+        path = self._write_config('')
+        rh.config.PreUploadFile(path)
 
     def testValid(self):
         """Verify a fully valid file works."""
-        self._write_config("""# This be a comment me matey.
+        path = self._write_config("""# This be a comment me matey.
 [Hook Scripts]
 name = script --with "some args"
 
@@ -82,39 +80,62 @@ cpplint = --some 'more args'
 [Options]
 ignore_merged_commits = true
 """)
-        rh.config.PreUploadConfig(paths=(self.tempdir,))
+        rh.config.PreUploadFile(path)
 
     def testUnknownSection(self):
         """Reject unknown sections."""
-        self._write_config('[BOOGA]')
-        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig,
-                          paths=(self.tempdir,))
+        path = self._write_config('[BOOGA]')
+        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile,
+                          path)
 
     def testUnknownBuiltin(self):
         """Reject unknown builtin hooks."""
-        self._write_config('[Builtin Hooks]\nbooga = borg!')
-        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig,
-                          paths=(self.tempdir,))
+        path = self._write_config('[Builtin Hooks]\nbooga = borg!')
+        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile,
+                          path)
 
     def testEmptyCustomHook(self):
         """Reject empty custom hooks."""
-        self._write_config('[Hook Scripts]\nbooga = \t \n')
-        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig,
-                          paths=(self.tempdir,))
+        path = self._write_config('[Hook Scripts]\nbooga = \t \n')
+        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile,
+                          path)
 
     def testInvalidIni(self):
         """Reject invalid ini files."""
-        self._write_config('[Hook Scripts]\n =')
-        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig,
-                          paths=(self.tempdir,))
+        path = self._write_config('[Hook Scripts]\n =')
+        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile,
+                          path)
 
     def testInvalidString(self):
         """Catch invalid string quoting."""
-        self._write_config("""[Hook Scripts]
+        path = self._write_config("""[Hook Scripts]
 name = script --'bad-quotes
 """)
-        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadConfig,
-                          paths=(self.tempdir,))
+        self.assertRaises(rh.config.ValidationError, rh.config.PreUploadFile,
+                          path)
+
+
+class PreUploadSettingsTests(unittest.TestCase):
+    """Tests for the PreUploadSettings class."""
+
+    def setUp(self):
+        self.tempdir = tempfile.mkdtemp()
+
+    def tearDown(self):
+        shutil.rmtree(self.tempdir)
+
+    def _write_config(self, data, filename=None):
+        """Helper to write out a config file for testing."""
+        if filename is None:
+            filename = rh.config.PreUploadSettings.FILENAME
+        path = os.path.join(self.tempdir, filename)
+        with open(path, 'w') as fp:
+            fp.write(data)
+
+    def _write_global_config(self, data):
+        """Helper to write out a global config file for testing."""
+        self._write_config(
+            data, filename=rh.config.PreUploadSettings.GLOBAL_FILENAME)
 
     def testGlobalConfigs(self):
         """Verify global configs stack properly."""
@@ -125,8 +146,8 @@ commit_msg_test_field = false""")
         self._write_config("""[Builtin Hooks]
 commit_msg_bug_field = false
 commit_msg_test_field = true""")
-        config = rh.config.PreUploadConfig(paths=(self.tempdir,),
-                                           global_paths=(self.tempdir,))
+        config = rh.config.PreUploadSettings(paths=(self.tempdir,),
+                                             global_paths=(self.tempdir,))
         self.assertEqual(config.builtin_hooks,
                          ['commit_msg_changeid_field', 'commit_msg_test_field'])