Patchwork [3,of,4,shelve-ext,v3] shelve: refactor shelvestate loading

login
register
mail settings
Submitter Kostia Balytskyi
Date April 10, 2017, 11:28 p.m.
Message ID <961539160565df5052d1.1491866915@devvm1416.lla2.facebook.com>
Download mbox | patch
Permalink /patch/20099/
State Accepted
Headers show

Comments

Kostia Balytskyi - April 10, 2017, 11:28 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1491865552 25200
#      Mon Apr 10 16:05:52 2017 -0700
# Node ID 961539160565df5052d1c770788cacb2d76e9752
# Parent  1f3af1e9d43286254237417adf26ecc97efa62e6
shelve: refactor shelvestate loading

This is a preparatory patch which separates file reading from the
minimal validation we have (like turning version into int and
checking that this version is supported). The purpose of this patch
is to be able to read statefile form simplekeyvaluefile, which is
implemented in the following patch.
Yuya Nishihara - April 12, 2017, 2:41 p.m.
On Mon, 10 Apr 2017 16:28:35 -0700, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1491865552 25200
> #      Mon Apr 10 16:05:52 2017 -0700
> # Node ID 961539160565df5052d1c770788cacb2d76e9752
> # Parent  1f3af1e9d43286254237417adf26ecc97efa62e6
> shelve: refactor shelvestate loading
> 
> This is a preparatory patch which separates file reading from the
> minimal validation we have (like turning version into int and
> checking that this version is supported). The purpose of this patch
> is to be able to read statefile form simplekeyvaluefile, which is
> implemented in the following patch.
> 
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -176,38 +176,46 @@ class shelvedstate(object):
>  
>      @classmethod
>      def load(cls, repo):
> +        # Order is important, because old shelvestate file uses it
> +        # to detemine values of fields (i.g. version is on the first line,
> +        # name is on the second and so forth). Please do not change.
> +        keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> +                'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> +        d = {}
>          fp = repo.vfs(cls._filename)
>          try:
> -            version = int(fp.readline().strip())
> -
> -            if version != cls._version:
> -                raise error.Abort(_('this version of shelve is incompatible '
> -                                   'with the version used in this repo'))
> -            name = fp.readline().strip()
> -            wctx = nodemod.bin(fp.readline().strip())
> -            pendingctx = nodemod.bin(fp.readline().strip())
> -            parents = [nodemod.bin(h) for h in fp.readline().split()]
> -            nodestoremove = [nodemod.bin(h) for h in fp.readline().split()]
> -            branchtorestore = fp.readline().strip()
> -            keep = fp.readline().strip() == cls._keep
> -            activebook = fp.readline().strip()
> -        except (ValueError, TypeError) as err:
> -            raise error.CorruptedState(str(err))
> +            for key in keys:
> +                d[key] = fp.readline().strip()
>          finally:
>              fp.close()
>  
> +        # some basic syntactic verification and transformation
> +        try:
> +            d['version'] = int(d.get('version'))
> +            if d.get('version') != cls._version:
> +                raise error.Abort(_('this version of shelve is incompatible '
> +                                    'with the version used in this repo'))
> +            d['originalwctx'] = nodemod.bin(d.get('originalwctx'))
> +            d['pendingctx'] = nodemod.bin(d.get('pendingctx'))
> +            d['parents'] = [nodemod.bin(h)
> +                            for h in d.get('parents').split(' ')]
> +            d['nodestoremove'] = [nodemod.bin(h)
> +                                  for h in d.get('nodestoremove').split(' ')]
> +        except (ValueError, TypeError, AttributeError) as err:

With d.get('parents', '').split(), you can avoid suppressing AttributeError.

>          try:
>              obj = cls()
> -            obj.name = name
> -            obj.wctx = repo[wctx]
> -            obj.pendingctx = repo[pendingctx]
> -            obj.parents = parents
> -            obj.nodestoremove = nodestoremove
> -            obj.branchtorestore = branchtorestore
> -            obj.keep = keep
> +            obj.name = d.get('name')
> +            obj.wctx = repo[d.get('originalwctx')]
> +            obj.pendingctx = repo[d.get('pendingctx')]
> +            obj.parents = d.get('parents')
> +            obj.nodestoremove = d.get('nodestoremove')
> +            obj.branchtorestore = d.get('branchtorestore')

Perhaps we shouldn't ignore missing key here. obj.wctx = repo[None] would
be wrong for example.

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -176,38 +176,46 @@  class shelvedstate(object):
 
     @classmethod
     def load(cls, repo):
+        # Order is important, because old shelvestate file uses it
+        # to detemine values of fields (i.g. version is on the first line,
+        # name is on the second and so forth). Please do not change.
+        keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
+                'nodestoremove', 'branchtorestore', 'keep', 'activebook']
+        d = {}
         fp = repo.vfs(cls._filename)
         try:
-            version = int(fp.readline().strip())
-
-            if version != cls._version:
-                raise error.Abort(_('this version of shelve is incompatible '
-                                   'with the version used in this repo'))
-            name = fp.readline().strip()
-            wctx = nodemod.bin(fp.readline().strip())
-            pendingctx = nodemod.bin(fp.readline().strip())
-            parents = [nodemod.bin(h) for h in fp.readline().split()]
-            nodestoremove = [nodemod.bin(h) for h in fp.readline().split()]
-            branchtorestore = fp.readline().strip()
-            keep = fp.readline().strip() == cls._keep
-            activebook = fp.readline().strip()
-        except (ValueError, TypeError) as err:
-            raise error.CorruptedState(str(err))
+            for key in keys:
+                d[key] = fp.readline().strip()
         finally:
             fp.close()
 
+        # some basic syntactic verification and transformation
+        try:
+            d['version'] = int(d.get('version'))
+            if d.get('version') != cls._version:
+                raise error.Abort(_('this version of shelve is incompatible '
+                                    'with the version used in this repo'))
+            d['originalwctx'] = nodemod.bin(d.get('originalwctx'))
+            d['pendingctx'] = nodemod.bin(d.get('pendingctx'))
+            d['parents'] = [nodemod.bin(h)
+                            for h in d.get('parents').split(' ')]
+            d['nodestoremove'] = [nodemod.bin(h)
+                                  for h in d.get('nodestoremove').split(' ')]
+        except (ValueError, TypeError, AttributeError) as err:
+            raise error.CorruptedState(str(err))
+
         try:
             obj = cls()
-            obj.name = name
-            obj.wctx = repo[wctx]
-            obj.pendingctx = repo[pendingctx]
-            obj.parents = parents
-            obj.nodestoremove = nodestoremove
-            obj.branchtorestore = branchtorestore
-            obj.keep = keep
+            obj.name = d.get('name')
+            obj.wctx = repo[d.get('originalwctx')]
+            obj.pendingctx = repo[d.get('pendingctx')]
+            obj.parents = d.get('parents')
+            obj.nodestoremove = d.get('nodestoremove')
+            obj.branchtorestore = d.get('branchtorestore')
+            obj.keep = d.get('keep') == cls._keep
             obj.activebookmark = ''
-            if activebook != cls._noactivebook:
-                obj.activebookmark = activebook
+            if d.get('activebook') != cls._noactivebook:
+                obj.activebookmark = d.get('activebook')
         except error.RepoLookupError as err:
             raise error.CorruptedState(str(err))