protect against circular patch chains

direct/src/p3d/PatchMaker.py

@@ -23,9 +23,10 @@ class PatchMaker:
             self.printName = None
 
             # The Package object that produces this version, if this
-            # is the current form or the base form, respectively.
+            # is the current, base, or top form, respectively.
             self.packageCurrent = None
             self.packageBase = None
+            self.packageTop = None
 
             # A list of patchfiles that can produce this version.
             self.fromPatches = []
@@ -41,7 +42,7 @@ class PatchMaker:
             if self.tempFile:
                 self.tempFile.unlink()
 
-        def getPatchChain(self, startPv):
+        def getPatchChain(self, startPv, alreadyVisited = []):
             """ Returns a list of patches that, when applied in
             sequence to the indicated PackageVersion object, will
             produce this PackageVersion object.  Returns None if no
@@ -51,11 +52,18 @@ class PatchMaker:
                 # We're already here.  A zero-length patch chain is
                 # therefore the answer.
                 return []
+            if self in alreadyVisited:
+                # We've already been here; this is a loop.  Avoid
+                # infinite recursion.
+                return None
+
+            alreadyVisited = alreadyVisited[:]
+            alreadyVisited.append(self)
 
             bestPatchChain = None
             for patchfile in self.fromPatches:
                 fromPv = patchfile.fromPv
-                patchChain = fromPv.getPatchChain(startPv)
+                patchChain = fromPv.getPatchChain(startPv, alreadyVisited)
                 if patchChain is not None:
                     # There's a path through this patchfile.
                     patchChain = patchChain + [patchfile]
@@ -66,19 +74,27 @@ class PatchMaker:
             # paths found.
             return bestPatchChain
 
-        def getRecreateFilePlan(self):
+        def getRecreateFilePlan(self, alreadyVisited = []):
             """ Returns the tuple (startFile, startPv, plan),
             describing how to recreate the archive file for this
             version.  startFile and startPv is the Filename and
             packageVersion of the file to start with, and plan is a
             list of tuples (patchfile, pv), listing the patches to
             apply in sequence, and the packageVersion object
-            associated with each patch.  Returns (None, None) if there
+            associated with each patch.  Returns (None, None, None) if
-            is no way to recreate this archive file.  """
+            there is no way to recreate this archive file.  """
             
             if self.tempFile:
                 return (self.tempFile, self, [])
 
+            if self in alreadyVisited:
+                # We've already been here; this is a loop.  Avoid
+                # infinite recursion.
+                return (None, None, None)
+
+            alreadyVisited = alreadyVisited[:]
+            alreadyVisited.append(self)
+
             if self.packageCurrent:
                 # This PackageVersion instance represents the current
                 # version of some package.
@@ -97,7 +113,7 @@ class PatchMaker:
             bestStartPv = None
             for patchfile in self.fromPatches:
                 fromPv = patchfile.fromPv
-                startFile, startPv, plan = fromPv.getRecreateFilePlan()
+                startFile, startPv, plan = fromPv.getRecreateFilePlan(alreadyVisited)
                 if plan is not None:
                     # There's a path through this patchfile.
                     plan = plan + [(patchfile, self)]
@@ -279,6 +295,7 @@ class PatchMaker:
             self.patchVersion = 1
             self.currentPv = None
             self.basePv = None
+            self.topPv = None
 
             self.packageName = None
             self.platform = None
@@ -303,14 +320,22 @@ class PatchMaker:
             
             return (self.packageName, self.platform, self.version, self.hostUrl, self.baseFile)
 
+        def getTopKey(self):
+            """ Returns the key to locate the "top" or newest version
+            of this package. """
+            
+            return (self.packageName, self.platform, self.version, self.hostUrl, self.topFile)
+
         def getGenericKey(self, fileSpec):
             """ Returns the key that has the indicated hash. """
             return (self.packageName, self.platform, self.version, self.hostUrl, fileSpec)
 
-        def readDescFile(self):
+        def readDescFile(self, doProcessing = False):
             """ Reads the existing package.xml file and stores it in
-            this class for later rewriting.  Returns true on success,
+            this class for later rewriting.  if doProcessing is true,
-            false on failure. """
+            it may massage the file and the directory contents in
+            preparation for building patches.  Returns true on
+            success, false on failure. """
 
             self.anyChanges = False
 
@@ -361,7 +386,6 @@ class PatchMaker:
                     isNewVersion = False
                 else:
                     # There's a new version this pass.  Update it.
-                    self.topFile = copy.copy(self.currentFile)
                     self.anyChanges = True
                 
             else:
@@ -399,15 +423,18 @@ class PatchMaker:
                 compressedFile.loadXml(xcompressed)
 
                 oldCompressedFilename = compressedFile.filename
-                newCompressedFilename = '%s.%s.pz' % (self.currentFile.filename, self.patchVersion)
+                self.compressedFilename = oldCompressedFilename
-                oldCompressedPathname = Filename(self.packageDir, oldCompressedFilename)
-                newCompressedPathname = Filename(self.packageDir, newCompressedFilename)
-                if oldCompressedPathname.renameTo(newCompressedPathname):
-                    compressedFile.fromFile(self.packageDir, newCompressedFilename)
-                    compressedFile.storeXml(xcompressed)
 
-                self.compressedFilename = newCompressedFilename
+                if doProcessing:
-                self.anyChanges = True
+                    newCompressedFilename = '%s.%s.pz' % (self.currentFile.filename, self.patchVersion)
+                    oldCompressedPathname = Filename(self.packageDir, oldCompressedFilename)
+                    newCompressedPathname = Filename(self.packageDir, newCompressedFilename)
+                    if oldCompressedPathname.renameTo(newCompressedPathname):
+                        compressedFile.fromFile(self.packageDir, newCompressedFilename)
+                        compressedFile.storeXml(xcompressed)
+
+                    self.compressedFilename = newCompressedFilename
+                    self.anyChanges = True
 
             # Get the base_version--the bottom (oldest) of the patch
             # chain.
@@ -429,7 +456,7 @@ class PatchMaker:
                 self.baseFile.filename += '.base'
 
                 # Also duplicate the (compressed) file itself.
-                if self.compressedFilename:
+                if doProcessing and self.compressedFilename:
                     fromPathname = Filename(self.packageDir, self.compressedFilename)
                     toPathname = Filename(self.packageDir, self.baseFile.filename + '.pz')
                     fromPathname.copyTo(toPathname)
@@ -460,7 +487,7 @@ class PatchMaker:
             # Remove all of the old patch entries from the desc file
             # we read earlier.
             xremove = []
-            for value in ['base_version', 'patch']:
+            for value in ['base_version', 'top_version', 'patch']:
                 xpatch = xpackage.FirstChildElement(value)
                 while xpatch:
                     xremove.append(xpatch)
@@ -478,8 +505,9 @@ class PatchMaker:
             self.baseFile.storeXml(xarchive)
             xpackage.InsertEndChild(xarchive)
 
+            # The current version is now the top version.
             xarchive = TiXmlElement('top_version')
-            self.topFile.storeXml(xarchive)
+            self.currentFile.storeXml(xarchive)
             xpackage.InsertEndChild(xarchive)
             
             for patchfile in self.patches:
@@ -554,7 +582,7 @@ class PatchMaker:
         object, or None on failure. """
 
         package = self.Package(Filename(descFilename), self)
-        if not package.readDescFile():
+        if not package.readDescFile(doProcessing = False):
             return None
         
         self.packages.append(package)
@@ -581,7 +609,7 @@ class PatchMaker:
                 if filename and not solo:
                     filename = Filename(filename)
                     package = self.Package(filename, self, xpackage)
-                    package.readDescFile()
+                    package.readDescFile(doProcessing = True)
                     self.packages.append(package)
                     
                 xpackage = xpackage.NextSiblingElement('package')
@@ -638,6 +666,10 @@ class PatchMaker:
             package.basePv = basePv
             basePv.packageBase = package
             basePv.printName = package.baseFile.filename
+
+            topPv = self.getPackageVersion(package.getTopKey())
+            package.topPv = topPv
+            topPv.packageTop = package
             
             for patchfile in package.patches:
                 self.recordPatchfile(patchfile)
@@ -682,22 +714,15 @@ class PatchMaker:
             # No versions.
             return
 
-        # Starting with the package base, how far forward can we go?
+        # What's the current version on the top of the tree?
+        topPv = package.topPv
         currentPv = package.currentPv
-        basePv = package.basePv
-
-        pv = basePv
-        nextPv = pv.getNext(package)
-        while nextPv:
-            pv = nextPv
-            nextPv = pv.getNext(package)
         
-        if pv.packageCurrent != package:
+        if topPv != currentPv:
-            # It doesn't reach all the way to the latest version, so
+            # They're different, so build a new patch.
-            # build a new patch.
             filename = Filename(package.currentFile.filename + '.%s.patch' % (package.patchVersion))
             assert filename not in self.patchFilenames
-            if not self.buildPatch(pv, currentPv, package, filename):
+            if not self.buildPatch(topPv, currentPv, package, filename):
                 raise StandardError, "Couldn't build patch."
 
     def buildPatch(self, v1, v2, package, patchFilename):

direct/src/plugin/p3dPatchFinder.cxx

@@ -40,13 +40,27 @@ PackageVersion(const PackageVersionKey &key) :
 //               false if no chain can be found.
 ////////////////////////////////////////////////////////////////////
 bool P3DPatchFinder::PackageVersion::
-get_patch_chain(Patchfiles &chain, PackageVersion *start_pv) {
+get_patch_chain(Patchfiles &chain, PackageVersion *start_pv,
+                const PackageVersionsList &already_visited_in) {
   chain.clear();
   if (this == start_pv) {
     // We're already here.  A zero-length patch chain is therefore the
     // answer.
     return true;
   }
+  if (::find(already_visited_in.begin(), already_visited_in.end(), this) != already_visited_in.end()) {
+    // We've already been here; this is a loop.  Avoid infinite
+    // recursion.
+    return false;
+  }
+
+  // Yeah, we make a new copy of this vector at each stage of the
+  // recursion.  This could be made much faster with a linked list
+  // instead, but I'm working on the assumption that there will be no
+  // more than a few dozen patchfiles, in which case this naive
+  // approach should be fast enough.
+  PackageVersionsList already_visited = already_visited_in;
+  already_visited.push_back(this);
 
   bool found_any = false;
   Patchfiles::iterator pi;
@@ -55,7 +69,7 @@ get_patch_chain(Patchfiles &chain, PackageVersion *start_pv) {
     PackageVersion *from_pv = patchfile->_from_pv;
     assert(from_pv != NULL);
     Patchfiles this_chain;
-    if (from_pv->get_patch_chain(this_chain, start_pv)) {
+    if (from_pv->get_patch_chain(this_chain, start_pv, already_visited)) {
       // There's a path through this patchfile.
       this_chain.push_back(patchfile);
       if (!found_any || this_chain.size() < chain.size()) {
@@ -341,7 +355,7 @@ get_patch_chain_to_current(Patchfiles &chain, TiXmlDocument *doc,
   PackageVersion *to_pv = package->_current_pv;
 
   if (to_pv != NULL && from_pv != NULL) {
-    return to_pv->get_patch_chain(chain, from_pv);
+    return to_pv->get_patch_chain(chain, from_pv, PackageVersionsList());
   }
 
   return false;

direct/src/plugin/p3dPatchFinder.h

@@ -35,8 +35,10 @@ class P3DPatchFinder {
 public:
   class Package;
   class Patchfile;
+  class PackageVersion;
 
   typedef vector<Patchfile *> Patchfiles;
+  typedef vector<PackageVersion *> PackageVersionsList;
 
   // This class is used to index into a map to locate PackageVersion
   // objects, below.
@@ -65,7 +67,8 @@ public:
   public:
     PackageVersion(const PackageVersionKey &key);
 
-    bool get_patch_chain(Patchfiles &chain, PackageVersion *start_pv);
+    bool get_patch_chain(Patchfiles &chain, PackageVersion *start_pv,
+                         const PackageVersionsList &already_visited_in);
 
   public:
     string _package_name;