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

login
register
mail settings
Submitter Durham Goode
Date Aug. 30, 2016, 11:40 p.m.
Message ID <8076d529408e97795128.1472600444@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/16499/
State Accepted
Headers show

Comments

Durham Goode - Aug. 30, 2016, 11:40 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1472519937 25200
#      Mon Aug 29 18:18:57 2016 -0700
# Node ID 8076d529408e977951286dbd58e81066f97a9b8d
# Parent  4b26b5cd815a717ee0ee930f4deef0959ef7c492
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 - Aug. 31, 2016, 5:44 p.m.
On Tue, Aug 30, 2016 at 4:40 PM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1472519937 25200
> #      Mon Aug 29 18:18:57 2016 -0700
> # Node ID 8076d529408e977951286dbd58e81066f97a9b8d
> # Parent  4b26b5cd815a717ee0ee930f4deef0959ef7c492
> manifest: change manifestctx to not inherit from manifestdict

Thanks! I like this much better (although I haven't looked at the
consequences later in the series before or after).

>
> 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
> @@ -948,9 +948,9 @@ class manifestlog(object):
>          """
>          if node == revlog.nullid:
>              if self._oldmanifest._treeinmem:
> -                return treemanifest()
> +                return treemanifestctx(self._revlog, node, dir='')
>              else:
> -                return manifestdict()
> +                return manifestctx(self._revlog, node)
>
>          if node in self._mancache:
>              cachemf = self._mancache[node]
> @@ -967,33 +967,38 @@ 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
>          self.p1, self.p2 = revlog.parents(node)
>          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 self._node == revlog.nullid:
> +            return manifestdict()

Might as well cache this value (in _data) just like others? Same for
treemanifestctx.

> +        if not self._data:
> +            data = self._revlog.revision(self._node)
> +            arraytext = array.array('c', data)
> +            self._revlog._fulltextcache[self._node] = arraytext
> +            self._data = manifestdict(data)
> +        return self._data
> +
> +class treemanifestctx(object):
>      def __init__(self, revlog, node, dir=''):
>          revlog = revlog.dirlog(dir)
>          self._revlog = revlog
>          self._dir = dir
> +        self._data = None
>
>          self._node = node
>
> @@ -1004,19 +1009,27 @@ 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:
> -            data = revlog.revision(node)
> -            arraytext = array.array('c', data)
> -            revlog.fulltextcache[node] = arraytext
> -            super(treemanifestctx, self).__init__(dir=dir, text=data)
> +    def read(self):
> +        if self._node == revlog.nullid:
> +            return treemanifest()
> +
> +        if not self._data:
> +            if self._revlog._treeondisk:
> +                treemf = treemanifest(dir=self._dir)

nit: the manifest instances elsewhere are simply called "m" and there
doesn't seem to be any ambiguity that the "tree" resolves here.

> +                def gettext():
> +                    return self._revlog.revision(self._node)
> +                def readsubtree(dir, subm):
> +                    return treemanifestctx(self._revlog, subm, dir).read()
> +                treemf.read(gettext, readsubtree)
> +                treemf.setnode(self._node)
> +                self._data = treemf
> +            else:
> +                data = self._revlog.revision(self._node)

This is really a comment I should have made on an earlier patch, but
it's only clear now: Could you rename "data" to "text"? It's called
"text" elsewhere, and I think it's particularly confusing now that
"data" is a string and "_data" is a manifest.

> +                arraytext = array.array('c', data)
> +                self._revlog.fulltextcache[self._node] = arraytext
> +                self._data = treemanifest(dir=self._dir, text=data)
> +
> +        return self._data
>
>  class manifest(manifestrevlog):
>      def __init__(self, opener, dir='', dirlogcache=None):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - Aug. 31, 2016, 7:45 p.m.
On 8/31/16 10:44 AM, Martin von Zweigbergk wrote:
> On Tue, Aug 30, 2016 at 4:40 PM, Durham Goode <durham@fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1472519937 25200
>> #      Mon Aug 29 18:18:57 2016 -0700
>> # Node ID 8076d529408e977951286dbd58e81066f97a9b8d
>> # Parent  4b26b5cd815a717ee0ee930f4deef0959ef7c492
>> manifest: change manifestctx to not inherit from manifestdict
> Thanks! I like this much better (although I haven't looked at the
> consequences later in the series before or after).
There aren't a ton of consequences.  Some caller code needs to now do 
.read() on the result, but most things had to do that for readdelta and 
readfast anyway.  So this is more consistent.
>
>> 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
>> @@ -948,9 +948,9 @@ class manifestlog(object):
>>           """
>>           if node == revlog.nullid:
>>               if self._oldmanifest._treeinmem:
>> -                return treemanifest()
>> +                return treemanifestctx(self._revlog, node, dir='')
>>               else:
>> -                return manifestdict()
>> +                return manifestctx(self._revlog, node)
>>
>>           if node in self._mancache:
>>               cachemf = self._mancache[node]
>> @@ -967,33 +967,38 @@ 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
>>           self.p1, self.p2 = revlog.parents(node)
>>           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 self._node == revlog.nullid:
>> +            return manifestdict()
> Might as well cache this value (in _data) just like others? Same for
> treemanifestctx.
Will do
>
>> +        if not self._data:
>> +            data = self._revlog.revision(self._node)
>> +            arraytext = array.array('c', data)
>> +            self._revlog._fulltextcache[self._node] = arraytext
>> +            self._data = manifestdict(data)
>> +        return self._data
>> +
>> +class treemanifestctx(object):
>>       def __init__(self, revlog, node, dir=''):
>>           revlog = revlog.dirlog(dir)
>>           self._revlog = revlog
>>           self._dir = dir
>> +        self._data = None
>>
>>           self._node = node
>>
>> @@ -1004,19 +1009,27 @@ 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:
>> -            data = revlog.revision(node)
>> -            arraytext = array.array('c', data)
>> -            revlog.fulltextcache[node] = arraytext
>> -            super(treemanifestctx, self).__init__(dir=dir, text=data)
>> +    def read(self):
>> +        if self._node == revlog.nullid:
>> +            return treemanifest()
>> +
>> +        if not self._data:
>> +            if self._revlog._treeondisk:
>> +                treemf = treemanifest(dir=self._dir)
> nit: the manifest instances elsewhere are simply called "m" and there
> doesn't seem to be any ambiguity that the "tree" resolves here.
Will do
>
>> +                def gettext():
>> +                    return self._revlog.revision(self._node)
>> +                def readsubtree(dir, subm):
>> +                    return treemanifestctx(self._revlog, subm, dir).read()
>> +                treemf.read(gettext, readsubtree)
>> +                treemf.setnode(self._node)
>> +                self._data = treemf
>> +            else:
>> +                data = self._revlog.revision(self._node)
> This is really a comment I should have made on an earlier patch, but
> it's only clear now: Could you rename "data" to "text"? It's called
> "text" elsewhere, and I think it's particularly confusing now that
> "data" is a string and "_data" is a manifest.
Will do
>
>> +                arraytext = array.array('c', data)
>> +                self._revlog.fulltextcache[self._node] = arraytext
>> +                self._data = treemanifest(dir=self._dir, text=data)
>> +
>> +        return self._data
>>
>>   class manifest(manifestrevlog):
>>       def __init__(self, opener, dir='', dirlogcache=None):
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=fHrsiimut8HHCIQZFCfzrhLoOb5vQNCJnJ-gVdk27a8&s=S6bRSEthzAqS_TRss9ei9nAF3i1UhyAqOvocRXEWf0w&e=

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
@@ -948,9 +948,9 @@  class manifestlog(object):
         """
         if node == revlog.nullid:
             if self._oldmanifest._treeinmem:
-                return treemanifest()
+                return treemanifestctx(self._revlog, node, dir='')
             else:
-                return manifestdict()
+                return manifestctx(self._revlog, node)
 
         if node in self._mancache:
             cachemf = self._mancache[node]
@@ -967,33 +967,38 @@  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
         self.p1, self.p2 = revlog.parents(node)
         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 self._node == revlog.nullid:
+            return manifestdict()
+        if not self._data:
+            data = self._revlog.revision(self._node)
+            arraytext = array.array('c', data)
+            self._revlog._fulltextcache[self._node] = arraytext
+            self._data = manifestdict(data)
+        return self._data
+
+class treemanifestctx(object):
     def __init__(self, revlog, node, dir=''):
         revlog = revlog.dirlog(dir)
         self._revlog = revlog
         self._dir = dir
+        self._data = None
 
         self._node = node
 
@@ -1004,19 +1009,27 @@  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:
-            data = revlog.revision(node)
-            arraytext = array.array('c', data)
-            revlog.fulltextcache[node] = arraytext
-            super(treemanifestctx, self).__init__(dir=dir, text=data)
+    def read(self):
+        if self._node == revlog.nullid:
+            return treemanifest()
+
+        if not self._data:
+            if self._revlog._treeondisk:
+                treemf = treemanifest(dir=self._dir)
+                def gettext():
+                    return self._revlog.revision(self._node)
+                def readsubtree(dir, subm):
+                    return treemanifestctx(self._revlog, subm, dir).read()
+                treemf.read(gettext, readsubtree)
+                treemf.setnode(self._node)
+                self._data = treemf
+            else:
+                data = self._revlog.revision(self._node)
+                arraytext = array.array('c', data)
+                self._revlog.fulltextcache[self._node] = arraytext
+                self._data = treemanifest(dir=self._dir, text=data)
+
+        return self._data
 
 class manifest(manifestrevlog):
     def __init__(self, opener, dir='', dirlogcache=None):