Patchwork D3116: simplestore: correctly implement flag processors

login
register
mail settings
Submitter phabricator
Date April 5, 2018, 4:39 a.m.
Message ID <differential-rev-PHID-DREV-wurwhusk77euine43ckv-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30343/
State Superseded
Headers show

Comments

phabricator - April 5, 2018, 4:39 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  There were a couple of bugs around the implementation of
  flags processing with the simple store. After these changes,
  test-flagprocessor.t now passes!
  
  test-flagprocessor.t was also updated to include explicit test
  coverage that pushed data is as expected on the server.
  
  The test extension used by test-flagprocessor.t has been updated
  so it monkeypatches the object returned from repo.file() instead
  of monkeypatching filelog.filelog. This allows it to work with
  extensions that return custom types from repo.file().
  
  The monkeypatching is rather hacky and probably is performance
  prohibitive for real repos. We should probably come up with a
  better mechanism for registering flag processors so monkeypatching
  isn't needed.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3116

AFFECTED FILES
  tests/flagprocessorext.py
  tests/simplestorerepo.py
  tests/test-flagprocessor.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/test-flagprocessor.t b/tests/test-flagprocessor.t
--- a/tests/test-flagprocessor.t
+++ b/tests/test-flagprocessor.t
@@ -88,6 +88,44 @@ 
   adding file changes
   added 7 changesets with 7 changes to 7 files
 
+Ensure the data got to the server OK
+
+  $ cd ../server
+  $ hg cat -r 6e48f4215d24 noop
+  [NOOP]
+  $ hg debugdata noop 0
+  [NOOP]
+
+  $ hg cat -r 6e48f4215d24 base64
+  [BASE64]
+  $ hg debugdata base64 0
+  W0JBU0U2NF0K (no-eol)
+
+  $ hg cat -r 6e48f4215d24 gzip
+  [GZIP]
+  $ hg debugdata gzip 0
+  x\x9c\x8bv\x8f\xf2\x0c\x88\xe5\x02\x00\x08\xc8\x01\xfd (no-eol) (esc)
+
+  $ hg cat -r 6e48f4215d24 noop-base64
+  [NOOP][BASE64]
+  $ hg debugdata noop-base64 0
+  W05PT1BdW0JBU0U2NF0K (no-eol)
+
+  $ hg cat -r 6e48f4215d24 noop-gzip
+  [NOOP][GZIP]
+  $ hg debugdata noop-gzip 0
+  x\x9c\x8b\xf6\xf3\xf7\x0f\x88\x8dv\x8f\xf2\x0c\x88\xe5\x02\x00\x1dH\x03\xf1 (no-eol) (esc)
+
+  $ hg cat -r 6e48f4215d24 base64-gzip
+  [BASE64][GZIP]
+  $ hg debugdata base64-gzip 0
+  eJyLdnIMdjUziY12j/IMiOUCACLBBDo= (no-eol)
+
+  $ hg cat -r 6e48f4215d24 base64-gzip-noop
+  [BASE64][GZIP][NOOP]
+  $ hg debugdata base64-gzip-noop 0
+  eJyLdnIMdjUziY12j/IMiI328/cPiOUCAESjBi4= (no-eol)
+
 # Initialize new client (not cloning) and setup extension
   $ cd ..
   $ hg init client2
@@ -197,6 +235,7 @@ 
   $ echo '[BASE64]a-bit-longer-branching' > base64
   $ hg commit -q -m branching
 
+#if repobundlerepo
   $ hg bundle --base 1 bundle.hg
   4 changesets found
   $ hg --config extensions.strip= strip -r 2 --no-backup --force -q
@@ -253,6 +292,7 @@ 
    1 files changed, 1 insertions(+), 0 deletions(-)
   
   $ rm bundle.hg bundle-again.hg
+#endif
 
 # TEST: hg status
 
diff --git a/tests/simplestorerepo.py b/tests/simplestorerepo.py
--- a/tests/simplestorerepo.py
+++ b/tests/simplestorerepo.py
@@ -243,6 +243,10 @@ 
         if flags == 0:
             return text, True
 
+        if flags & ~revlog.REVIDX_KNOWN_FLAGS:
+            raise error.RevlogError(_("incompatible revision flag '%#x'") %
+                                    (flags & ~revlog.REVIDX_KNOWN_FLAGS))
+
         validatehash = True
         # Depending on the operation (read or write), the order might be
         # reversed due to non-commutative transforms.
@@ -405,15 +409,13 @@ 
         return 0, 0
 
     def add(self, text, meta, transaction, linkrev, p1, p2):
-        transaction.addbackup(self._indexpath)
-
         if meta or text.startswith(b'\1\n'):
             text = filelog.packmeta(meta, text)
 
         return self.addrevision(text, transaction, linkrev, p1, p2)
 
     def addrevision(self, text, transaction, linkrev, p1, p2, node=None,
-                    flags=0):
+                    flags=revlog.REVIDX_DEFAULT_FLAGS, cachedelta=None):
         validatenode(p1)
         validatenode(p2)
 
@@ -430,15 +432,21 @@ 
         if validatehash:
             self.checkhash(rawtext, node, p1=p1, p2=p2)
 
+        return self._addrawrevision(node, rawtext, transaction, linkrev, p1, p2,
+                                    flags)
+
+    def _addrawrevision(self, node, rawtext, transaction, link, p1, p2, flags):
+        transaction.addbackup(self._indexpath)
+
         path = b'/'.join([self._storepath, hex(node)])
 
-        self._svfs.write(path, text)
+        self._svfs.write(path, rawtext)
 
         self._indexdata.append({
             b'node': node,
             b'p1': p1,
             b'p2': p2,
-            b'linkrev': linkrev,
+            b'linkrev': link,
             b'flags': flags,
         })
 
@@ -457,6 +465,7 @@ 
 
         for node, p1, p2, linknode, deltabase, delta, flags in deltas:
             linkrev = linkmapper(linknode)
+            flags = flags or revlog.REVIDX_DEFAULT_FLAGS
 
             nodes.append(node)
 
@@ -469,7 +478,8 @@ 
             else:
                 text = mdiff.patch(self.revision(deltabase), delta)
 
-            self.addrevision(text, transaction, linkrev, p1, p2, flags)
+            self._addrawrevision(node, text, transaction, linkrev, p1, p2,
+                                 flags)
 
             if addrevisioncb:
                 addrevisioncb(self, node)
diff --git a/tests/flagprocessorext.py b/tests/flagprocessorext.py
--- a/tests/flagprocessorext.py
+++ b/tests/flagprocessorext.py
@@ -9,7 +9,6 @@ 
     changegroup,
     exchange,
     extensions,
-    filelog,
     revlog,
     util,
 )
@@ -55,39 +54,41 @@ 
     versions.add(b'03')
     return versions
 
-def noopaddrevision(orig, self, text, transaction, link, p1, p2,
-                    cachedelta=None, node=None,
-                    flags=revlog.REVIDX_DEFAULT_FLAGS):
-    if b'[NOOP]' in text:
-        flags |= REVIDX_NOOP
-    return orig(self, text, transaction, link, p1, p2, cachedelta=cachedelta,
-                node=node, flags=flags)
+def makewrappedfile(obj):
+    class wrappedfile(obj.__class__):
+        def addrevision(self, text, transaction, link, p1, p2,
+                        cachedelta=None, node=None,
+                        flags=revlog.REVIDX_DEFAULT_FLAGS):
+            if b'[NOOP]' in text:
+                flags |= REVIDX_NOOP
 
-def b64addrevision(orig, self, text, transaction, link, p1, p2,
-                   cachedelta=None, node=None,
-                   flags=revlog.REVIDX_DEFAULT_FLAGS):
-    if b'[BASE64]' in text:
-        flags |= REVIDX_BASE64
-    return orig(self, text, transaction, link, p1, p2, cachedelta=cachedelta,
-                node=node, flags=flags)
+            if b'[BASE64]' in text:
+                flags |= REVIDX_BASE64
+
+            if b'[GZIP]' in text:
+                flags |= REVIDX_GZIP
 
-def gzipaddrevision(orig, self, text, transaction, link, p1, p2,
-                    cachedelta=None, node=None,
-                    flags=revlog.REVIDX_DEFAULT_FLAGS):
-    if b'[GZIP]' in text:
-        flags |= REVIDX_GZIP
-    return orig(self, text, transaction, link, p1, p2, cachedelta=cachedelta,
-                node=node, flags=flags)
+            # This addrevision wrapper is meant to add a flag we will not have
+            # transforms registered for, ensuring we handle this error case.
+            if b'[FAIL]' in text:
+                flags |= REVIDX_FAIL
+
+            return super(wrappedfile, self).addrevision(text, transaction, link,
+                                                        p1, p2,
+                                                        cachedelta=cachedelta,
+                                                        node=node,
+                                                        flags=flags)
 
-def failaddrevision(orig, self, text, transaction, link, p1, p2,
-                    cachedelta=None, node=None,
-                    flags=revlog.REVIDX_DEFAULT_FLAGS):
-    # This addrevision wrapper is meant to add a flag we will not have
-    # transforms registered for, ensuring we handle this error case.
-    if b'[FAIL]' in text:
-        flags |= REVIDX_FAIL
-    return orig(self, text, transaction, link, p1, p2, cachedelta=cachedelta,
-                node=node, flags=flags)
+    obj.__class__ = wrappedfile
+
+def reposetup(ui, repo):
+    class wrappingflagprocessorrepo(repo.__class__):
+        def file(self, f):
+            orig = super(wrappingflagprocessorrepo, self).file(f)
+            makewrappedfile(orig)
+            return orig
+
+    repo.__class__ = wrappingflagprocessorrepo
 
 def extsetup(ui):
     # Enable changegroup3 for flags to be sent over the wire
@@ -108,13 +109,6 @@ 
     for k in exchange._bundlespeccontentopts.keys():
         exchange._bundlespeccontentopts[k]["cg.version"] = "03"
 
-    # Add wrappers for addrevision, responsible to set flags depending on the
-    # revision data contents.
-    wrapfunction(filelog.filelog, 'addrevision', noopaddrevision)
-    wrapfunction(filelog.filelog, 'addrevision', b64addrevision)
-    wrapfunction(filelog.filelog, 'addrevision', gzipaddrevision)
-    wrapfunction(filelog.filelog, 'addrevision', failaddrevision)
-
     # Register flag processors for each extension
     revlog.addflagprocessor(
         REVIDX_NOOP,