Patchwork [2,of,2,V4] manifest: change manifestctx to not inherit from manifestdict

login
register
mail settings
Submitter Durham Goode
Date Sept. 12, 2016, 9:33 p.m.
Message ID <0291745860223e276e4b.1473716021@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/16598/
State Accepted
Headers show

Comments

Durham Goode - Sept. 12, 2016, 9:33 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1473702943 25200
#      Mon Sep 12 10:55:43 2016 -0700
# Node ID 0291745860223e276e4be73a84eb7c097640388e
# Parent  3a45359ff38aee2b4da284186024c3a83cc2b3ae
manifest: change manifestctx to not inherit from manifestdict

If manifestctx inherits from manifestdict, it requires some weird logic to
lazily load the dict if a piece of information is asked for. This ended up being
complicated and unintuitive to use.

Let's move the dict creation to .read(). This will make even more sense once we
start adding readdelta() and other similar methods to manifestctx.
via Mercurial-devel - Sept. 12, 2016, 11:40 p.m.
Pushed to committed, thanks.

On Mon, Sep 12, 2016 at 2:33 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1473702943 25200
> #      Mon Sep 12 10:55:43 2016 -0700
> # Node ID 0291745860223e276e4be73a84eb7c097640388e
> # Parent  3a45359ff38aee2b4da284186024c3a83cc2b3ae
> manifest: change manifestctx to not inherit from manifestdict
>
> If manifestctx inherits from manifestdict, it requires some weird logic to
> lazily load the dict if a piece of information is asked for. This ended up being
> complicated and unintuitive to use.
>
> Let's move the dict creation to .read(). This will make even more sense once we
> start adding readdelta() and other similar methods to manifestctx.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -528,7 +528,7 @@ class changectx(basectx):
>
>      @propertycache
>      def _manifest(self):
> -        return self._repo.manifestlog[self._changeset.manifest]
> +        return self._repo.manifestlog[self._changeset.manifest].read()
>
>      @propertycache
>      def _manifestdelta(self):
> diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> --- a/mercurial/manifest.py
> +++ b/mercurial/manifest.py
> @@ -962,12 +962,13 @@ class manifestlog(object):
>              self._mancache[node] = m
>          return m
>
> -class manifestctx(manifestdict):
> +class manifestctx(object):
>      """A class representing a single revision of a manifest, including its
>      contents, its parent revs, and its linkrev.
>      """
>      def __init__(self, revlog, node):
>          self._revlog = revlog
> +        self._data = None
>
>          self._node = node
>
> @@ -978,21 +979,26 @@ class manifestctx(manifestdict):
>          #rev = revlog.rev(node)
>          #self.linkrev = revlog.linkrev(rev)
>
> -        # This should eventually be made lazy loaded, so consumers can access
> -        # the node/p1/linkrev data without having to parse the whole manifest.
> -        data = revlog.revision(node)
> -        arraytext = array.array('c', data)
> -        revlog._fulltextcache[node] = arraytext
> -        super(manifestctx, self).__init__(data)
> -
>      def node(self):
>          return self._node
>
> -class treemanifestctx(treemanifest):
> +    def read(self):
> +        if not self._data:
> +            if self._node == revlog.nullid:
> +                self._data = manifestdict()
> +            else:
> +                text = self._revlog.revision(self._node)
> +                arraytext = array.array('c', text)
> +                self._revlog._fulltextcache[self._node] = arraytext
> +                self._data = manifestdict(text)
> +        return self._data
> +
> +class treemanifestctx(object):
>      def __init__(self, revlog, dir, node):
>          revlog = revlog.dirlog(dir)
>          self._revlog = revlog
>          self._dir = dir
> +        self._data = None
>
>          self._node = node
>
> @@ -1003,19 +1009,26 @@ class treemanifestctx(treemanifest):
>          #rev = revlog.rev(node)
>          #self.linkrev = revlog.linkrev(rev)
>
> -        if revlog._treeondisk:
> -            super(treemanifestctx, self).__init__(dir=dir)
> -            def gettext():
> -                return revlog.revision(node)
> -            def readsubtree(dir, subm):
> -                return revlog.dirlog(dir).read(subm)
> -            self.read(gettext, readsubtree)
> -            self.setnode(node)
> -        else:
> -            text = revlog.revision(node)
> -            arraytext = array.array('c', text)
> -            revlog.fulltextcache[node] = arraytext
> -            super(treemanifestctx, self).__init__(dir=dir, text=text)
> +    def read(self):
> +        if not self._data:
> +            if self._node == revlog.nullid:
> +                self._data = treemanifest()
> +            elif self._revlog._treeondisk:
> +                m = treemanifest(dir=self._dir)
> +                def gettext():
> +                    return self._revlog.revision(self._node)
> +                def readsubtree(dir, subm):
> +                    return treemanifestctx(self._revlog, dir, subm).read()
> +                m.read(gettext, readsubtree)
> +                m.setnode(self._node)
> +                self._data = m
> +            else:
> +                text = self._revlog.revision(self._node)
> +                arraytext = array.array('c', text)
> +                self._revlog.fulltextcache[self._node] = arraytext
> +                self._data = treemanifest(dir=self._dir, text=text)
> +
> +        return self._data
>
>      def node(self):
>          return self._node
> @@ -1130,7 +1143,11 @@ class manifest(manifestrevlog):
>          if node == revlog.nullid:
>              return self._newmanifest() # don't upset local cache
>          if node in self._mancache:
> -            return self._mancache[node]
> +            cached = self._mancache[node]
> +            if (isinstance(cached, manifestctx) or
> +                isinstance(cached, treemanifestctx)):
> +                cached = cached.read()
> +            return cached
>          if self._treeondisk:
>              def gettext():
>                  return self.revision(node)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -528,7 +528,7 @@  class changectx(basectx):
 
     @propertycache
     def _manifest(self):
-        return self._repo.manifestlog[self._changeset.manifest]
+        return self._repo.manifestlog[self._changeset.manifest].read()
 
     @propertycache
     def _manifestdelta(self):
diff --git a/mercurial/manifest.py b/mercurial/manifest.py
--- a/mercurial/manifest.py
+++ b/mercurial/manifest.py
@@ -962,12 +962,13 @@  class manifestlog(object):
             self._mancache[node] = m
         return m
 
-class manifestctx(manifestdict):
+class manifestctx(object):
     """A class representing a single revision of a manifest, including its
     contents, its parent revs, and its linkrev.
     """
     def __init__(self, revlog, node):
         self._revlog = revlog
+        self._data = None
 
         self._node = node
 
@@ -978,21 +979,26 @@  class manifestctx(manifestdict):
         #rev = revlog.rev(node)
         #self.linkrev = revlog.linkrev(rev)
 
-        # This should eventually be made lazy loaded, so consumers can access
-        # the node/p1/linkrev data without having to parse the whole manifest.
-        data = revlog.revision(node)
-        arraytext = array.array('c', data)
-        revlog._fulltextcache[node] = arraytext
-        super(manifestctx, self).__init__(data)
-
     def node(self):
         return self._node
 
-class treemanifestctx(treemanifest):
+    def read(self):
+        if not self._data:
+            if self._node == revlog.nullid:
+                self._data = manifestdict()
+            else:
+                text = self._revlog.revision(self._node)
+                arraytext = array.array('c', text)
+                self._revlog._fulltextcache[self._node] = arraytext
+                self._data = manifestdict(text)
+        return self._data
+
+class treemanifestctx(object):
     def __init__(self, revlog, dir, node):
         revlog = revlog.dirlog(dir)
         self._revlog = revlog
         self._dir = dir
+        self._data = None
 
         self._node = node
 
@@ -1003,19 +1009,26 @@  class treemanifestctx(treemanifest):
         #rev = revlog.rev(node)
         #self.linkrev = revlog.linkrev(rev)
 
-        if revlog._treeondisk:
-            super(treemanifestctx, self).__init__(dir=dir)
-            def gettext():
-                return revlog.revision(node)
-            def readsubtree(dir, subm):
-                return revlog.dirlog(dir).read(subm)
-            self.read(gettext, readsubtree)
-            self.setnode(node)
-        else:
-            text = revlog.revision(node)
-            arraytext = array.array('c', text)
-            revlog.fulltextcache[node] = arraytext
-            super(treemanifestctx, self).__init__(dir=dir, text=text)
+    def read(self):
+        if not self._data:
+            if self._node == revlog.nullid:
+                self._data = treemanifest()
+            elif self._revlog._treeondisk:
+                m = treemanifest(dir=self._dir)
+                def gettext():
+                    return self._revlog.revision(self._node)
+                def readsubtree(dir, subm):
+                    return treemanifestctx(self._revlog, dir, subm).read()
+                m.read(gettext, readsubtree)
+                m.setnode(self._node)
+                self._data = m
+            else:
+                text = self._revlog.revision(self._node)
+                arraytext = array.array('c', text)
+                self._revlog.fulltextcache[self._node] = arraytext
+                self._data = treemanifest(dir=self._dir, text=text)
+
+        return self._data
 
     def node(self):
         return self._node
@@ -1130,7 +1143,11 @@  class manifest(manifestrevlog):
         if node == revlog.nullid:
             return self._newmanifest() # don't upset local cache
         if node in self._mancache:
-            return self._mancache[node]
+            cached = self._mancache[node]
+            if (isinstance(cached, manifestctx) or
+                isinstance(cached, treemanifestctx)):
+                cached = cached.read()
+            return cached
         if self._treeondisk:
             def gettext():
                 return self.revision(node)