From b3c684284a0bf083f1fba9497b0b9a53f5a47135 Mon Sep 17 00:00:00 2001
From: Nick Alexander <nalexander@mozilla.com>
Date: Thu, 13 Feb 2014 09:09:08 -0800
Subject: [PATCH] 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           | 24 +++++++++++++++---
 python/mozbuild/mozpack/test/test_copier.py | 28 +++++++++++++++++++++
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/python/mozbuild/mozpack/copier.py b/python/mozbuild/mozpack/copier.py
index e53c3668c0e..d047972ecbc 100644
--- a/python/mozbuild/mozpack/copier.py
+++ b/python/mozbuild/mozpack/copier.py
@@ -13,6 +13,7 @@ from mozpack.files import (
 import mozpack.path
 import errno
 from collections import (
+    Counter,
     OrderedDict,
 )
 
@@ -31,6 +32,19 @@ class FileRegistry(object):
 
     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):
         '''
@@ -39,14 +53,17 @@ class FileRegistry(object):
         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):
         '''
@@ -76,6 +93,7 @@ class FileRegistry(object):
                                 "not matching anything previously added"))
         for i in items:
             del self._files[i]
+            self._required_directories.subtract(self._partial_paths(i))
 
     def paths(self):
         '''
diff --git a/python/mozbuild/mozpack/test/test_copier.py b/python/mozbuild/mozpack/test/test_copier.py
index 13f74e04307..afa757d506a 100644
--- a/python/mozbuild/mozpack/test/test_copier.py
+++ b/python/mozbuild/mozpack/test/test_copier.py
@@ -88,6 +88,34 @@ class TestFileRegistry(MatchTestTemplate, unittest.TestCase):
         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):