[PATCH 3 of 4 shelve-ext v4] shelve: refactor shelvestate loading

Kostia Balytskyi ikostia at fb.com
Thu May 11 12:23:14 EDT 2017



> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah at gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 11 May, 2017 14:38
> To: Kostia Balytskyi <ikostia at fb.com>
> Cc: mercurial-devel at mercurial-scm.org
> Subject: Re: [PATCH 3 of 4 shelve-ext v4] shelve: refactor shelvestate loading
> 
> On Sun, 7 May 2017 06:07:05 -0700, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia at fb.com> # Date 1494113427 25200
> > #      Sat May 06 16:30:27 2017 -0700
> > # Node ID 497904bddbaa75b9086c168ab2e03381dfaff165
> > # Parent  d78507e5b31ac9d5dbf3b8ab45c0c94b01491a0b
> > 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:
> > +            raise error.CorruptedState(str(err))
> 
> Perhaps it's better to catch KeyError instead of suppressing implicit
> AttributeError caused by d.get(key).
> 
> > +
> >          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')
> 
> Are these d.get(key) uses valid? For example, repo[d.get('pendingctx')]
> would be a workingctx object if 'pendingctx=' is missing.
You are right. I will change the stuff above to use [] instead of .get(), and here will only change d.get('activebook') to d.get('activebook', ''). Together these things should handle all the cases.


More information about the Mercurial-devel mailing list