Bug 971272 - Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps
authorNick Alexander <nalexander@mozilla.com>
Thu, 13 Feb 2014 09:09:08 -0800
changeset 168599 a808e19f51c1dd7c93ffa4bd455783837ba8bae9
parent 168598 fa1585a40c130a1b37d4871d22f2cf31c545a579
child 168600 a1870df76f9960adbfd3878876d9a7b7710c024e
push id26210
push userkwierso@gmail.com
push dateFri, 14 Feb 2014 00:52:58 +0000
treeherdermozilla-central@0f18070eb5d6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs971272
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 971272 - Part 1: Guard against adding {foo, foo/bar} to FileRegistry. r=gps This already raised if the order was [foo, foo/bar]. But it didn't prevent adding [foo/bar, foo]. The only sub-classes of FileRegistry are FileCopier and Jarrer. FileCopier.copy threw in the previously unhandled case: the order of creation is the same as the order of addition, so that foo is created after foo/bar. A zip file index can contain both foo and foo/bar. I don't think we should rely on this property in our use of Jarrer, but if we already do, I guess we need to move these guards into FileCopier. Let's hope that's not the case! (For the record: On my Mac OS X system, unzipping such a zip file prompts the user for what to do, depending on the order of the entries in the zip index.)
python/mozbuild/mozpack/copier.py
python/mozbuild/mozpack/test/test_copier.py
--- a/python/mozbuild/mozpack/copier.py
+++ b/python/mozbuild/mozpack/copier.py
@@ -8,16 +8,17 @@ import stat
 from mozpack.errors import errors
 from mozpack.files import (
     BaseFile,
     Dest,
 )
 import mozpack.path
 import errno
 from collections import (
+    Counter,
     OrderedDict,
 )
 
 
 class FileRegistry(object):
     '''
     Generic container to keep track of a set of BaseFile instances. It
     preserves the order under which the files are added, but doesn't keep
@@ -26,32 +27,48 @@ class FileRegistry(object):
     unspecified (virtual) root directory.
 
         registry = FileRegistry()
         registry.add('foo/bar', file_instance)
     '''
 
     def __init__(self):
         self._files = OrderedDict()
+        self._required_directories = Counter()
+
+    def _partial_paths(self, path):
+        '''
+        Turn "foo/bar/baz/zot" into ["foo/bar/baz", "foo/bar", "foo"].
+        '''
+        partial_paths = []
+        partial_path = path
+        while partial_path:
+            partial_path = mozpack.path.dirname(partial_path)
+            if partial_path:
+                partial_paths.append(partial_path)
+        return partial_paths
 
     def add(self, path, content):
         '''
         Add a BaseFile instance to the container, under the given path.
         '''
         assert isinstance(content, BaseFile)
         if path in self._files:
             return errors.error("%s already added" % path)
+        if self._required_directories[path] > 0:
+            return errors.error("Can't add %s: it is a required directory" %
+                                path)
         # Check whether any parent of the given path is already stored
-        partial_path = path
-        while partial_path:
-            partial_path = mozpack.path.dirname(partial_path)
+        partial_paths = self._partial_paths(path)
+        for partial_path in partial_paths:
             if partial_path in self._files:
                 return errors.error("Can't add %s: %s is a file" %
                                     (path, partial_path))
         self._files[path] = content
+        self._required_directories.update(partial_paths)
 
     def match(self, pattern):
         '''
         Return the list of paths, stored in the container, matching the
         given pattern. See the mozpack.path.match documentation for a
         description of the handled patterns.
         '''
         if '*' in pattern:
@@ -71,16 +88,17 @@ class FileRegistry(object):
         patterns.
         '''
         items = self.match(pattern)
         if not items:
             return errors.error("Can't remove %s: %s" % (pattern,
                                 "not matching anything previously added"))
         for i in items:
             del self._files[i]
+            self._required_directories.subtract(self._partial_paths(i))
 
     def paths(self):
         '''
         Return all paths stored in the container, in the order they were added.
         '''
         return self._files.keys()
 
     def __len__(self):
--- a/python/mozbuild/mozpack/test/test_copier.py
+++ b/python/mozbuild/mozpack/test/test_copier.py
@@ -83,16 +83,44 @@ class TestFileRegistry(MatchTestTemplate
         self.assertEqual(self.registry.paths(), ['bar', 'foo/qux'])
 
         self.assertEqual([f for f, c in self.registry], ['bar', 'foo/qux'])
         self.assertEqual(len(self.registry), 2)
 
         self.add('foo/.foo')
         self.assertTrue(self.registry.contains('foo/.foo'))
 
+    def test_registry_paths(self):
+        self.registry = FileRegistry()
+
+        # Can't add a file if it requires a directory in place of a
+        # file we also require.
+        self.registry.add('foo', GeneratedFile('foo'))
+        self.assertRaises(ErrorMessage, self.registry.add, 'foo/bar',
+                          GeneratedFile('foobar'))
+
+        # Can't add a file if we already have a directory there.
+        self.registry.add('bar/baz', GeneratedFile('barbaz'))
+        self.assertRaises(ErrorMessage, self.registry.add, 'bar',
+                          GeneratedFile('bar'))
+
+        # Bump the count of things that require bar/ to 2.
+        self.registry.add('bar/zot', GeneratedFile('barzot'))
+        self.assertRaises(ErrorMessage, self.registry.add, 'bar',
+                          GeneratedFile('bar'))
+
+        # Drop the count of things that require bar/ to 1.
+        self.registry.remove('bar/baz')
+        self.assertRaises(ErrorMessage, self.registry.add, 'bar',
+                          GeneratedFile('bar'))
+
+        # Drop the count of things that require bar/ to 0.
+        self.registry.remove('bar/zot')
+        self.registry.add('bar/zot', GeneratedFile('barzot'))
+
 
 class TestFileCopier(TestWithTmpDir):
     def all_dirs(self, base):
         all_dirs = set()
         for root, dirs, files in os.walk(base):
             if not dirs:
                 all_dirs.add(mozpack.path.relpath(root, base))
         return all_dirs