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

login
register
mail settings
Submitter Kostia Balytskyi
Date May 7, 2017, 1:07 p.m.
Message ID <9f24868156f15473b08a.1494162426@devvm1416.lla2.facebook.com>
Download mbox | patch
Permalink /patch/20519/
State Superseded
Headers show

Comments

Kostia Balytskyi - May 7, 2017, 1:07 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1494162279 25200
#      Sun May 07 06:04:39 2017 -0700
# Node ID 9f24868156f15473b08a418765411341c96e892b
# Parent  497904bddbaa75b9086c168ab2e03381dfaff165
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.

After this patch:
- shelve will always write a simplekeyvaluefile
- unshelve will check the first line of the file for a version, and if the
  version is 1, will read it in a position-based way, if the version is 2,
  will read it in a key-value way

As discussed with Yuya previously, this will be able to handle old-style
shelvedstate files, but old Mercurial versions will fail on the attempt to
read shelvedstate file of version 2 with a self-explanatory message:
  'abort: this version of shelve is incompatible with the version used
  in this repo'
Yuya Nishihara - May 11, 2017, 1:48 p.m.
On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@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
Kostia Balytskyi - May 11, 2017, 9:09 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 11 May, 2017 14:48
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia@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.

I like it being a class because it allows inheritance, which allows simplekeyvalue users
to add validation. Also, it can serve as a container for firstlinekey.
vfs vs a file object - I don't have an answer to this, maybe file object would've been better, but I'd prefer to not rewrite it.

> 
> > 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
Yuya Nishihara - May 13, 2017, 10:51 a.m.
On Thu, 11 May 2017 21:09:35 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Thursday, 11 May, 2017 14:48
> > To: Kostia Balytskyi <ikostia@fb.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH 4 of 4 shelve-ext v4] shelve: make shelvestate use
> > simplekeyvaluefile
> > 
> > On Sun, 7 May 2017 06:07:06 -0700, Kostia Balytskyi wrote:
> > > # HG changeset patch
> > > # User Kostia Balytskyi <ikostia@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.
> 
> I like it being a class because it allows inheritance, which allows simplekeyvalue users
> to add validation. Also, it can serve as a container for firstlinekey.
> vfs vs a file object - I don't have an answer to this, maybe file object would've been better, but I'd prefer to not rewrite it.

Thanks for clarification. I'm not a fan of using inheritance excessively,
but yeah that's one way.

Patch

diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -167,7 +167,7 @@  class shelvedstate(object):
     Handles saving and restoring a shelved state. Ensures that different
     versions of a shelved state are possible and handles them appropriately.
     """
-    _version = 1
+    _version = 2
     _filename = 'shelvedstate'
     _keep = 'keep'
     _nokeep = 'nokeep'
@@ -175,26 +175,9 @@  class shelvedstate(object):
     _noactivebook = ':no-active-bookmark'
 
     @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)
+    def _verifyandtransform(cls, d):
+        """Some basic shelvestate syntactic verification and transformation"""
         try:
-            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)
@@ -204,6 +187,50 @@  class shelvedstate(object):
         except (ValueError, TypeError, AttributeError) as err:
             raise error.CorruptedState(str(err))
 
+    @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)
+        else:
+            raise error.Abort(_('this version of shelve is incompatible '
+                                'with the version used in this repo'))
+
+        cls._verifyandtransform(d)
         try:
             obj = cls()
             obj.name = d.get('name')
@@ -224,19 +251,20 @@  class shelvedstate(object):
     @classmethod
     def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
              branchtorestore, keep=False, activebook=''):
-        fp = repo.vfs(cls._filename, 'wb')
-        fp.write('%i\n' % cls._version)
-        fp.write('%s\n' % name)
-        fp.write('%s\n' % nodemod.hex(originalwctx.node()))
-        fp.write('%s\n' % nodemod.hex(pendingctx.node()))
-        fp.write('%s\n' %
-                 ' '.join([nodemod.hex(p) for p in repo.dirstate.parents()]))
-        fp.write('%s\n' %
-                 ' '.join([nodemod.hex(n) for n in nodestoremove]))
-        fp.write('%s\n' % branchtorestore)
-        fp.write('%s\n' % (cls._keep if keep else cls._nokeep))
-        fp.write('%s\n' % (activebook or cls._noactivebook))
-        fp.close()
+        info = {
+            "name": name,
+            "originalwctx": nodemod.hex(originalwctx.node()),
+            "pendingctx": nodemod.hex(pendingctx.node()),
+            "parents": ' '.join([nodemod.hex(p)
+                                 for p in repo.dirstate.parents()]),
+            "nodestoremove": ' '.join([nodemod.hex(n)
+                                      for n in nodestoremove]),
+            "branchtorestore": branchtorestore,
+            "keep": cls._keep if keep else cls._nokeep,
+            "activebook": activebook or cls._noactivebook
+        }
+        scmutil.simplekeyvaluefile(repo.vfs, cls._filename)\
+               .write(info, firstline=str(cls._version))
 
     @classmethod
     def clear(cls, repo):
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
   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
 
   $ echo "aaabbbccc" > a
   $ rm a.orig