[PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use simplekeyvaluefile

[Yuya Nishihara]

On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1494162279 25200
> #      Sun May 07 06:04:39 2017 -0700
> # Node ID 9f24868156f15473b08a418765411341c96e892b
> # Parent  497904bddbaa75b9086c168ab2e03381dfaff165
> shelve: make shelvestate use simplekeyvaluefile

This looks good to me.

> +    @classmethod
> +    def _getversion(cls, repo):
> +        """Read version information from shelvestate file"""
> +        fp = repo.vfs(cls._filename)
> +        try:
> +            version = int(fp.readline().strip())
> +        except ValueError as err:
> +            raise error.CorruptedState(str(err))
> +        finally:
> +            fp.close()
> +        return version
> +
> +    @classmethod
> +    def _readold(cls, repo):
> +        """Read the old position-based version of a shelvestate file"""
> +        # Order is important, because old shelvestate file uses it
> +        # to detemine values of fields (i.g. name is on the second line,
> +        # originalwctx is on the third and so forth). Please do not change.
> +        keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> +                'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> +        # this is executed only seldomly, so it is not a big deal
> +        # that we open this file twice
> +        fp = repo.vfs(cls._filename)
> +        d = {}
> +        try:
> +            for key in keys:
> +                d[key] = fp.readline().strip()
> +        finally:
> +            fp.close()
> +        return d
> +
> +    @classmethod
> +    def load(cls, repo):
> +        version = cls._getversion(repo)
> +        if version < cls._version:
> +            d = cls._readold(repo)
> +        elif version == cls._version:
> +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename)\
> +                       .read(firstlinenonkeyval=True)

This is okay, but I really don't understand why the simplekeyvaluefile
has to:

 - be a class instead of load/dump functions (like json)
 - take a vfs instead of a file object

IMHO, these design decisions just make things complicated.

> diff --git a/tests/test-shelve.t b/tests/test-shelve.t
> --- a/tests/test-shelve.t
> +++ b/tests/test-shelve.t
> @@ -1578,7 +1578,7 @@ shelve on new branch, conflict with prev
>    $ echo "ccc" >> a
>    $ hg status
>    M a
> -  $ hg unshelve
> +  $ hg unshelve --config shelve.oldstatefile=on

Maybe this has to be updated to use a pre-generated v1 state file?

>    unshelving change 'default'
>    temporarily committing pending changes (restore with 'hg unshelve --abort')
>    rebasing shelved changes
> @@ -1591,9 +1591,8 @@ shelve on new branch, conflict with prev
>  Removing restore branch information from shelvedstate file(making it looks like
>  in previous versions) and running unshelve --continue
>  
> -  $ head -n 6 < .hg/shelvedstate > .hg/shelvedstate_oldformat
> -  $ rm .hg/shelvedstate
> -  $ mv .hg/shelvedstate_oldformat .hg/shelvedstate
> +  $ cp .hg/shelvedstate .hg/shelvedstate_old
> +  $ cat .hg/shelvedstate_old | grep -v 'branchtorestore' > .hg/shelvedstate