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

[Ryan McElroy]

On 4/10/17 5:04 PM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia at fb.com>
> # Date 1491839190 25200
> #      Mon Apr 10 08:46:30 2017 -0700
> # Node ID 73c4ca5490a9b5202ffb00ea0bea76c171491e4a
> # Parent  5dc11b1531f701844001b0723c4b8b97c2e55217
> shelve: make shelvestate use simplekeyvaluefile
>
> Currently shelvestate uses line ordering to differentiate fields. This
> makes it hard for extensions to wrap shelve, since if two alternative
> versions of code add a new line, correct merging is going to be problematic.
> simplekeyvaluefile was introduced fot this purpose specifically.

Great summary!

>
> After this patch:
> - shelve will always write a simplekeyvaluefile, unless 'shelve.oldstatefile'
> is on
> - unshelve will check the first line of the file and if it has '=' sign,
> will treat the file as a simplekeyvalue one. Otherwise, it will try to read
> an old-style statefile.
>
> diff --git a/hgext/shelve.py b/hgext/shelve.py
> --- a/hgext/shelve.py
> +++ b/hgext/shelve.py
> @@ -166,6 +166,10 @@ class shelvedstate(object):
>   
>       Handles saving and restoring a shelved state. Ensures that different
>       versions of a shelved state are possible and handles them appropriately.
> +
> +    By default, simplekeyvaluefile is used. The following config option
> +    allows one to enforce the old position-based state file to be used:
> +    shelve.oldstatefile
>       """
>       _version = 1
>       _filename = 'shelvedstate'
> @@ -175,17 +179,32 @@ class shelvedstate(object):
>       _noactivebook = ':no-active-bookmark'
>   
>       @classmethod
> +    def iskeyvaluebased(cls, repo):
> +        """Determine whether state file is simple lines or simplekeyvaluefile"""
> +        if repo.ui.configbool('shelve', 'oldstatefile', False):
> +            return True
> +        fp = repo.vfs(cls._filename)
> +        try:
> +            firstline = fp.readline()
> +            return '=' in firstline
> +        finally:
> +            fp.close()
> +
> +    @classmethod
>       def load(cls, repo):
>           # order is important, please do not change
>           keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
>                   'nodestoprune', 'branchtorestore', 'keep', 'activebook']
>           st = {}
> -        fp = repo.vfs(cls._filename)
> -        try:
> -            for key in keys:
> -                st[key] = fp.readline().strip()
> -        finally:
> -            fp.close()
> +        if cls.iskeyvaluebased(repo):
> +            st = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()

In this branch, the keys list is never used. Should we validate that we 
got all of the expected keys and fail with a nice error message if we 
did not?

> +        else:
> +            fp = repo.vfs(cls._filename)
> +            try:
> +                for key in keys:
> +                    st[key] = fp.readline().strip()
> +            finally:
> +                fp.close()
>   
>           # some basic syntactic verification and transformation
>           try:
> @@ -222,6 +241,22 @@ class shelvedstate(object):
>       @classmethod
>       def save(cls, repo, name, originalwctx, pendingctx, nodestoprune,
>                branchtorestore, keep=False, activebook=''):
> +        if not repo.ui.configbool('shelve', 'oldstatefile', False):
> +            info = {
> +                "version": str(cls._version),
> +                "name": name,
> +                "originalwctx": nodemod.hex(originalwctx.node()),
> +                "pendingctx": nodemod.hex(pendingctx.node()),
> +                "parents": ' '.join([nodemod.hex(p)
> +                                     for p in repo.dirstate.parents()]),
> +                "nodestoprune": ' '.join([nodemod.hex(n)
> +                                          for n in nodestoprune]),
> +                "branchtorestore": branchtorestore,
> +                "keep": cls._keep if keep else cls._nokeep,
> +                "activebook": activebook or cls._noactivebook
> +            }
> +            scmutil.simplekeyvaluefile(repo.vfs, cls._filename).write(info)
> +            return

I don't love early return, but I don't see a much better way of doing 
it. I think it's okay for now, but I'd like to probably remove the code 
below in the future if we don't intend to write old-style state files 
much longer.

>           fp = repo.vfs(cls._filename, 'wb')
>           fp.write('%i\n' % cls._version)
>           fp.write('%s\n' % name)
> 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

Does this test fail otherwise? It's not really clear why this changed. 
If it's just to make sure that the new code is exercised, then call that 
out in the summary of the patch.

>     unshelving change 'default'
>     temporarily committing pending changes (restore with 'hg unshelve --abort')
>     rebasing shelved changes
>