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

[Yuya Nishihara]

On Thu, 11 May 2017 21:09:35 +, Kostia Balytskyi wrote:
> > -Original Message-
> > From: Yuya Nishihara [mailto:you...@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Thursday, 11 May, 2017 14:48
> > To: Kostia Balytskyi <ikos...@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 <ikos...@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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Kostia Balytskyi]

> -Original Message-
> From: Yuya Nishihara [mailto:you...@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 11 May, 2017 14:48
> To: Kostia Balytskyi <ikos...@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 <ikos...@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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [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 
> # 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
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Kostia Balytskyi]

# HG changeset patch
# User Kostia Balytskyi 
# 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'

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 =