[remotefilelog] don't crash on invalid pack files

Summary:
We have run into some cases where users ended up with empty pack file in their
packs directory.  Just log a warning in this case and skip this pack file,
rather than letting the exception propagate up and crashing the command.

Test Plan:
Created empty 0000000000000000000000000000000000000000.histpack and
0000000000000000000000000000000000000000.histidx files in my repository's
hgcache directory, and confirmed that "hg log" now simply warns about them
instead of crashing.

I didn't really test the perftest.py or treemanifest_correctness.py extensions
much.  They seem to throw exceptions, and look like they have maybe gotten a
bit stale.  I fixed one minor typo, but didn't dig into the other exceptions
too much.

Reviewers: durham

Reviewed By: durham

Subscribers: net-systems-diffs@, yogeshwer, mjpieters

Differential Revision: https://phabricator.intern.facebook.com/D4402516

Tasks: 15428659

Signature: t1:4402516:1484155254:96d2980efcec2d735257b08910e1ca437ef1dad6

remotefilelog/basepack.py

@@ -1,5 +1,6 @@
 import errno, hashlib, mmap, os, struct, time
 from mercurial import osutil
+from mercurial.i18n import _
 
 import shallowutil
 
@@ -37,7 +38,7 @@ SMALLFANOUTCUTOFF = 2**16 / 8
 REFRESHRATE = 0.1
 
 class basepackstore(object):
-    def __init__(self, path):
+    def __init__(self, ui, path):
         self.path = path
         self.packs = []
         # lastrefesh is 0 so we'll immediately check for new packs on the first
@@ -47,12 +48,17 @@ class basepackstore(object):
         for filepath in self._getavailablepackfiles():
             try:
                 pack = self.getpack(filepath)
-            except (OSError, IOError) as ex:
+            except Exception as ex:
-                # someone could have removed the file since we retrieved the
+                # An exception may be thrown if the pack file is corrupted
-                # list of paths.  if that happens, just don't add the pack to
+                # somehow.  Log a warning but keep going in this case, just
-                # the list of available packs.
+                # skipping this pack file.
-                if ex.errno != errno.ENOENT:
+                #
-                    raise
+                # If this is an ENOENT error then don't even bother logging.
+                # Someone could have removed the file since we retrieved the
+                # list of paths.
+                if getattr(ex, 'errno', None) != errno.ENOENT:
+                    ui.warn(_('unable to load pack %s: %s\n') % (filepath, ex))
+                    pass
                 continue
             self.packs.append(pack)
 

remotefilelog/datapack.py

@@ -22,9 +22,9 @@ class datapackstore(basepack.basepackstore):
     INDEXSUFFIX = INDEXSUFFIX
     PACKSUFFIX = PACKSUFFIX
 
-    def __init__(self, path, usecdatapack=False):
+    def __init__(self, ui, path, usecdatapack=False):
         self.usecdatapack = usecdatapack
-        super(datapackstore, self).__init__(path)
+        super(datapackstore, self).__init__(ui, path)
 
     def getpack(self, path):
         if self.usecdatapack:

remotefilelog/shallowrepo.py

@@ -184,9 +184,10 @@ def wraprepo(repo):
     # Instantiate pack stores
     packpath = shallowutil.getcachepackpath(repo, constants.FILEPACK_CATEGORY)
     packcontentstore = datapackstore(
+        repo.ui,
         packpath,
         usecdatapack=repo.ui.configbool('remotefilelog', 'fastdatapack'))
-    packmetadatastore = historypackstore(packpath)
+    packmetadatastore = historypackstore(repo.ui, packpath)
 
     # Instantiate union stores
     repo.contentstore = unioncontentstore(packcontentstore, cachecontent,

tests/perftest.py

@@ -39,7 +39,7 @@ def testpackedtrees(ui, repo, *args, **opts):
             buildtreepack(repo, newpack, opts.get('build'))
             newpack.close()
 
-    packstore = datapack.datapackstore(opener.base,
+    packstore = datapack.datapackstore(ui, opener.base,
             usecdatapack=ui.configbool('remotefilelog', 'fastdatapack'))
     unionstore = contentstore.unioncontentstore(packstore)
 
@@ -63,7 +63,7 @@ class cachestore(object):
 def buildtreepack(repo, pack, revs):
     mf = repo.manifest
     cache = cachestore()
-    packstore = datapack.datapackstore(pack.opener.base)
+    packstore = datapack.datapackstore(repo.ui, pack.opener.base)
     store = contentstore.unioncontentstore(cache, packstore)
     ctxs = list(repo.set(revs))
     for count, ctx in enumerate(ctxs):

tests/treemanifest_correctness.py

@@ -26,7 +26,7 @@ testedwith = 'ships-with-fb-hgext'
     ('', 'revs', 'master + master~5', ''),
     ], '')
 def testpackedtrees(ui, repo, *args, **opts):
-    packpath = shallowutil.getcacheackpath(repo, 'manifest')
+    packpath = shallowutil.getcachepackpath(repo, 'manifest')
     if not os.path.exists(packpath):
         os.mkdir(packpath)
     opener = scmutil.vfs(packpath)
@@ -35,7 +35,7 @@ def testpackedtrees(ui, repo, *args, **opts):
             buildtreepack(repo, newpack, opts.get('build'))
             newpack.close()
 
-    packstore = datapack.datapackstore(opener.base,
+    packstore = datapack.datapackstore(ui, opener.base,
             usecdatapack=ui.configbool('remotefilelog', 'fastdatapack'))
     unionstore = contentstore.unioncontentstore(packstore)
 
@@ -59,7 +59,7 @@ class cachestore(object):
 def buildtreepack(repo, pack, revs):
     mf = repo.manifest
     cache = cachestore()
-    packstore = datapack.datapackstore(pack.opener.base)
+    packstore = datapack.datapackstore(repo.ui, pack.opener.base)
     store = contentstore.unioncontentstore(cache, packstore)
     ctxs = list(repo.set(revs))
     for count, ctx in enumerate(ctxs):

treemanifest/__init__.py

@@ -57,11 +57,12 @@ def wraprepo(repo):
     usecdatapack = repo.ui.configbool('remotefilelog', 'fastdatapack')
 
     packpath = shallowutil.getcachepackpath(repo, PACK_CATEGORY)
-    datastore = datapackstore(packpath, usecdatapack=usecdatapack)
+    datastore = datapackstore(repo.ui, packpath, usecdatapack=usecdatapack)
 
     localpackpath = shallowutil.getlocalpackpath(repo.svfs.vfs.base,
                                                  PACK_CATEGORY)
-    localdatastore = datapackstore(localpackpath, usecdatapack=usecdatapack)
+    localdatastore = datapackstore(repo.ui, localpackpath,
+                                   usecdatapack=usecdatapack)
 
     repo.svfs.sharedmanifestdatastores = [datastore]
     repo.svfs.localmanifestdatastores = [localdatastore]