[PATCH 1 of 4 shelve-ext v2] shelve: refactor shelvestate loading

[Ryan McElroy]

On 4/10/17 5:04 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1491840064 25200
> #      Mon Apr 10 09:01:04 2017 -0700
> # Node ID 5dc11b1531f701844001b0723c4b8b97c2e55217
> # Parent  f6d77af84ef3e936b15634759df2718d5363b78a
> 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.
>
>
Nit: two empty lines in the description. Reduce to one please.

> This series is some basic preparatory refactoring to make wrapping
> parts of shelve easier in an extension.
>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -176,38 +176,44 @@ class shelvedstate(object):
>   
>       @classmethod
>       def load(cls, repo):
> +        # order is important, please do not change

Might be worth explaining *why* order is important.

> +        keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> +                'nodestoprune', 'branchtorestore', 'keep', 'activebook']
> +        st = {}

"st" is not a great variable name. Can you call it "state" maybe?

>           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()]
> -            nodestoprune = [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:
> +                st[key] = fp.readline().strip()
>           finally:
>               fp.close()
>   
> +        # some basic syntactic verification and transformation
> +        try:
> +            st['version'] = int(st.get('version'))
> +            if st.get('version') != cls._version:
> +                raise error.Abort(_('this version of shelve is incompatible '
> +                                    'with the version used in this repo'))
> +            st['originalwctx'] = nodemod.bin(st.get('originalwctx'))
> +            st['pendingctx'] = nodemod.bin(st.get('pendingctx'))
> +            st['parents'] = [nodemod.bin(h)
> +                             for h in st.get('parents').split(' ')]
> +            st['nodestoprune'] = [nodemod.bin(h)
> +                                  for h in st.get('nodestoprune').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.nodestoprune = nodestoprune
> -            obj.branchtorestore = branchtorestore
> -            obj.keep = keep
> +            obj.name = st.get('name')
> +            obj.wctx = repo[st.get('originalwctx')]
> +            obj.pendingctx = repo[st.get('pendingctx')]
> +            obj.parents = st.get('parents')
> +            obj.nodestoprune = st.get('nodestoprune')
> +            obj.branchtorestore = st.get('branchtorestore')
> +            obj.keep = st.get('keep') == cls._keep
>               obj.activebookmark = ''
> -            if activebook != cls._noactivebook:
> -                obj.activebookmark = activebook
> +            if st.get('activebook') != cls._noactivebook:
> +                obj.activebookmark = st.get('activebook')
>           except error.RepoLookupError as err:
>               raise error.CorruptedState(str(err))
>   

This looks like a correct mechanical change and a good improvement to 
me. However, with the nitpicks and changes requested to other patches I 
think it's worth one more iteration.