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

login
register
mail settings
Submitter Kostia Balytskyi
Date April 10, 2017, 11:28 p.m.
Message ID <8e286a85816581cfa4ce.1491866916@devvm1416.lla2.facebook.com>
Download mbox | patch
Permalink /patch/20100/
State Accepted
Headers show

Comments

Kostia Balytskyi - April 10, 2017, 11:28 p.m.
# HG changeset patch
# User Kostia Balytskyi <ikostia@fb.com>
# Date 1491866434 25200
#      Mon Apr 10 16:20:34 2017 -0700
# Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
# Parent  961539160565df5052d1c770788cacb2d76e9752
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, 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.

Test change is needed, because just a few lines below the place of change,
test script does a direct manipulation of shelvestate file by removing a
line in a particular position. Clearly, this relies on the fact that the
file is  is position-based, not key-value.
Ryan McElroy - April 11, 2017, 9:12 a.m.
On 4/11/17 12:28 AM, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1491866434 25200
> #      Mon Apr 10 16:20:34 2017 -0700
> # Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
> # Parent  961539160565df5052d1c770788cacb2d76e9752
> shelve: make shelvestate use simplekeyvaluefile

This series now looks good to me. Kostia has addressed all the concerns 
I had. It's a good cleanup of shelve that makes extension easier and far 
more robust, and the earlier patches that clean up naming are also nice 
to have.

I'll mark as pre-reviewed in patchwork.

>
> 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, 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.
>
> Test change is needed, because just a few lines below the place of change,
> test script does a direct manipulation of shelvestate file by removing a
> line in a particular position. Clearly, this relies on the fact that the
> file is  is position-based, not key-value.
>
> 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,6 +179,18 @@ 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, because old shelvestate file uses it
>           # to detemine values of fields (i.g. version is on the first line,
> @@ -182,12 +198,15 @@ class shelvedstate(object):
>           keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
>                   'nodestoremove', 'branchtorestore', 'keep', 'activebook']
>           d = {}
> -        fp = repo.vfs(cls._filename)
> -        try:
> -            for key in keys:
> -                d[key] = fp.readline().strip()
> -        finally:
> -            fp.close()
> +        if cls.iskeyvaluebased(repo):
> +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> +        else:
> +            fp = repo.vfs(cls._filename)
> +            try:
> +                for key in keys:
> +                    d[key] = fp.readline().strip()
> +            finally:
> +                fp.close()
>   
>           # some basic syntactic verification and transformation
>           try:
> @@ -224,6 +243,22 @@ class shelvedstate(object):
>       @classmethod
>       def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
>                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()]),
> +                "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)
> +            return
>           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
>     unshelving change 'default'
>     temporarily committing pending changes (restore with 'hg unshelve --abort')
>     rebasing shelved changes
>
Yuya Nishihara - April 12, 2017, 3:03 p.m.
On Mon, 10 Apr 2017 16:28:36 -0700, Kostia Balytskyi wrote:
> # HG changeset patch
> # User Kostia Balytskyi <ikostia@fb.com>
> # Date 1491866434 25200
> #      Mon Apr 10 16:20:34 2017 -0700
> # Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
> # Parent  961539160565df5052d1c770788cacb2d76e9752
> 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, 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.
> 
> Test change is needed, because just a few lines below the place of change,
> test script does a direct manipulation of shelvestate file by removing a
> line in a particular position. Clearly, this relies on the fact that the
> file is  is position-based, not key-value.
> 
> 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

I think this is the version 2 of the state file.

And I don't think the config knob is good way to support old state file. If we
want to support writing old state files, we can write two separate files, like
mergestate. If not, we only need to switch parsers depending on the version
field, like histedit.

>      _filename = 'shelvedstate'
> @@ -175,6 +179,18 @@ 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()

Perhaps it's better to compare the version number instead of relying on
heuristic.

> +    @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,
> @@ -182,12 +198,15 @@ class shelvedstate(object):
>          keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
>                  'nodestoremove', 'branchtorestore', 'keep', 'activebook']
>          d = {}
> -        fp = repo.vfs(cls._filename)
> -        try:
> -            for key in keys:
> -                d[key] = fp.readline().strip()
> -        finally:
> -            fp.close()
> +        if cls.iskeyvaluebased(repo):
> +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> +        else:
> +            fp = repo.vfs(cls._filename)
> +            try:
> +                for key in keys:
> +                    d[key] = fp.readline().strip()
> +            finally:
> +                fp.close()
>  
>          # some basic syntactic verification and transformation
>          try:
> @@ -224,6 +243,22 @@ class shelvedstate(object):
>      @classmethod
>      def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
>               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()]),
> +                "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)
> +            return
>          fp = repo.vfs(cls._filename, 'wb')
>          fp.write('%i\n' % cls._version)
>          fp.write('%s\n' % name)

The shelvedstate class only provides load(), save() and clear(). They are all
storage functions. I guess new file format was supposed to be in new class.
Thoughts?
Ryan McElroy - April 12, 2017, 3:25 p.m.
On 4/12/17 4:03 PM, Yuya Nishihara wrote:
> Perhaps it's better to compare the version number instead of relying on
> heuristic.

Indeed, that's a better idea. Thanks for catching this Yuya!
Kostia Balytskyi - April 12, 2017, 9:34 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Wednesday, 12 April, 2017 16:03
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Mon, 10 Apr 2017 16:28:36 -0700, Kostia Balytskyi wrote:
> > # HG changeset patch
> > # User Kostia Balytskyi <ikostia@fb.com> # Date 1491866434 25200
> > #      Mon Apr 10 16:20:34 2017 -0700
> > # Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
> > # Parent  961539160565df5052d1c770788cacb2d76e9752
> > 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, 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.
> >
> > Test change is needed, because just a few lines below the place of
> > change, test script does a direct manipulation of shelvestate file by
> > removing a line in a particular position. Clearly, this relies on the
> > fact that the file is  is position-based, not key-value.
> >
> > 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
> 
> I think this is the version 2 of the state file.
> 
> And I don't think the config knob is good way to support old state file. If we
> want to support writing old state files, we can write two separate files, like
> mergestate. If not, we only need to switch parsers depending on the version
> field, like histedit.

The idea behind this was that old state files should almost never be written. And I can't really think of any reason we would want to write them, except for that one test, which can be adjusted.

> 
> >      _filename = 'shelvedstate'
> > @@ -175,6 +179,18 @@ 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()
> 
> Perhaps it's better to compare the version number instead of relying on
> heuristic.
Comparing versions (if I understand you correctly) wouldn't work, see below.

> 
> > +    @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, @@ -182,12 +198,15 @@ class shelvedstate(object):
> >          keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> >                  'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> >          d = {}
> > -        fp = repo.vfs(cls._filename)
> > -        try:
> > -            for key in keys:
> > -                d[key] = fp.readline().strip()
> > -        finally:
> > -            fp.close()
> > +        if cls.iskeyvaluebased(repo):
> > +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> > +        else:
> > +            fp = repo.vfs(cls._filename)
> > +            try:
> > +                for key in keys:
> > +                    d[key] = fp.readline().strip()
> > +            finally:
> > +                fp.close()
> >
> >          # some basic syntactic verification and transformation
> >          try:
> > @@ -224,6 +243,22 @@ class shelvedstate(object):
> >      @classmethod
> >      def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
> >               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()]),
> > +                "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)
> > +            return
> >          fp = repo.vfs(cls._filename, 'wb')
> >          fp.write('%i\n' % cls._version)
> >          fp.write('%s\n' % name)
> 
> The shelvedstate class only provides load(), save() and clear(). They are all
> storage functions. I guess new file format was supposed to be in new class.
> Thoughts?

I do not like two classes. I think old style should die. Would it be ok if I remove the possibility to write old-style files and leave the ability to read both new and old files. The problem with having separate versions in separate classes is that we still have to maintain some sort of aggregating code to decide which to use and it does not seem worth it to me. 
Basically, what I am proposing is to just remove the config knob, the ability to write old style statefiles and fix the test.
Yuya Nishihara - April 13, 2017, 12:46 p.m.
On Wed, 12 Apr 2017 21:34:23 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Wednesday, 12 April, 2017 16:03
> > To: Kostia Balytskyi <ikostia@fb.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> > simplekeyvaluefile
> > 
> > On Mon, 10 Apr 2017 16:28:36 -0700, Kostia Balytskyi wrote:
> > > # HG changeset patch
> > > # User Kostia Balytskyi <ikostia@fb.com> # Date 1491866434 25200
> > > #      Mon Apr 10 16:20:34 2017 -0700
> > > # Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
> > > # Parent  961539160565df5052d1c770788cacb2d76e9752
> > > 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, 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.
> > >
> > > Test change is needed, because just a few lines below the place of
> > > change, test script does a direct manipulation of shelvestate file by
> > > removing a line in a particular position. Clearly, this relies on the
> > > fact that the file is  is position-based, not key-value.
> > >
> > > 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
> > 
> > I think this is the version 2 of the state file.
> > 
> > And I don't think the config knob is good way to support old state file. If we
> > want to support writing old state files, we can write two separate files, like
> > mergestate. If not, we only need to switch parsers depending on the version
> > field, like histedit.
> 
> The idea behind this was that old state files should almost never be written. And I can't really think of any reason we would want to write them, except for that one test, which can be adjusted.

Then, how about adding undocumented 'debug.<whatever>' config for testing
purpose?

> > > @@ -175,6 +179,18 @@ 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()
> > 
> > Perhaps it's better to compare the version number instead of relying on
> > heuristic.
> Comparing versions (if I understand you correctly) wouldn't work, see below.

I couldn't get why new format can't be version 2. The old format had '1\n'
for future extension. This patch introduces new format, so using '2\n' makes
sense to me.

The current simplekeyvaluefile API would make this a bit harder, but we can
extract functions to serialize and deserialize dict from/to file object.

> > > +    @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, @@ -182,12 +198,15 @@ class shelvedstate(object):
> > >          keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> > >                  'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> > >          d = {}
> > > -        fp = repo.vfs(cls._filename)
> > > -        try:
> > > -            for key in keys:
> > > -                d[key] = fp.readline().strip()
> > > -        finally:
> > > -            fp.close()
> > > +        if cls.iskeyvaluebased(repo):
> > > +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> > > +        else:
> > > +            fp = repo.vfs(cls._filename)
> > > +            try:
> > > +                for key in keys:
> > > +                    d[key] = fp.readline().strip()
> > > +            finally:
> > > +                fp.close()
> > >
> > >          # some basic syntactic verification and transformation
> > >          try:
> > > @@ -224,6 +243,22 @@ class shelvedstate(object):
> > >      @classmethod
> > >      def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
> > >               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()]),
> > > +                "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)
> > > +            return
> > >          fp = repo.vfs(cls._filename, 'wb')
> > >          fp.write('%i\n' % cls._version)
> > >          fp.write('%s\n' % name)
> > 
> > The shelvedstate class only provides load(), save() and clear(). They are all
> > storage functions. I guess new file format was supposed to be in new class.
> > Thoughts?
> 
> I do not like two classes. I think old style should die. Would it be ok if I remove the possibility to write old-style files and leave the ability to read both new and old files. The problem with having separate versions in separate classes is that we still have to maintain some sort of aggregating code to decide which to use and it does not seem worth it to me. 

Yeah. If we don't support writing old state file (except for testing), the
separate classes won't be necessary. So please disregard my idea.

> Basically, what I am proposing is to just remove the config knob, the ability to write old style statefiles and fix the test.

(see above)
Kostia Balytskyi - April 13, 2017, 9:28 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 13 April, 2017 13:47
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Wed, 12 Apr 2017 21:34:23 +0000, Kostia Balytskyi wrote:
> > > -----Original Message-----
> > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > Nishihara
> > > Sent: Wednesday, 12 April, 2017 16:03
> > > To: Kostia Balytskyi <ikostia@fb.com>
> > > Cc: mercurial-devel@mercurial-scm.org
> > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate
> > > use simplekeyvaluefile
> > >
> > > On Mon, 10 Apr 2017 16:28:36 -0700, Kostia Balytskyi wrote:
> > > > # HG changeset patch
> > > > # User Kostia Balytskyi <ikostia@fb.com> # Date 1491866434 25200
> > > > #      Mon Apr 10 16:20:34 2017 -0700
> > > > # Node ID 8e286a85816581cfa4ce44768482cb5722a88bb3
> > > > # Parent  961539160565df5052d1c770788cacb2d76e9752
> > > > 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, 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.
> > > >
> > > > Test change is needed, because just a few lines below the place of
> > > > change, test script does a direct manipulation of shelvestate file
> > > > by removing a line in a particular position. Clearly, this relies
> > > > on the fact that the file is  is position-based, not key-value.
> > > >
> > > > 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
> > >
> > > I think this is the version 2 of the state file.
> > >
> > > And I don't think the config knob is good way to support old state
> > > file. If we want to support writing old state files, we can write
> > > two separate files, like mergestate. If not, we only need to switch
> > > parsers depending on the version field, like histedit.
> >
> > The idea behind this was that old state files should almost never be
> written. And I can't really think of any reason we would want to write them,
> except for that one test, which can be adjusted.
> 
> Then, how about adding undocumented 'debug.<whatever>' config for
> testing purpose?
Test can be rewritten to work with key-value file, it's not a problem.
> 
> > > > @@ -175,6 +179,18 @@ 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()
> > >
> > > Perhaps it's better to compare the version number instead of relying
> > > on heuristic.
> > Comparing versions (if I understand you correctly) wouldn't work, see
> below.
> 
> I couldn't get why new format can't be version 2. The old format had '1\n'
> for future extension. This patch introduces new format, so using '2\n' makes
> sense to me.
> 
> The current simplekeyvaluefile API would make this a bit harder, but we can
> extract functions to serialize and deserialize dict from/to file object.
I do not understand what 'extract functions ...' means in this context? Can you explain?
> 
> > > > +    @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, @@ -182,12 +198,15 @@ class shelvedstate(object):
> > > >          keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
> > > >                  'nodestoremove', 'branchtorestore', 'keep', 'activebook']
> > > >          d = {}
> > > > -        fp = repo.vfs(cls._filename)
> > > > -        try:
> > > > -            for key in keys:
> > > > -                d[key] = fp.readline().strip()
> > > > -        finally:
> > > > -            fp.close()
> > > > +        if cls.iskeyvaluebased(repo):
> > > > +            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
> > > > +        else:
> > > > +            fp = repo.vfs(cls._filename)
> > > > +            try:
> > > > +                for key in keys:
> > > > +                    d[key] = fp.readline().strip()
> > > > +            finally:
> > > > +                fp.close()
> > > >
> > > >          # some basic syntactic verification and transformation
> > > >          try:
> > > > @@ -224,6 +243,22 @@ class shelvedstate(object):
> > > >      @classmethod
> > > >      def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
> > > >               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()]),
> > > > +                "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)
> > > > +            return
> > > >          fp = repo.vfs(cls._filename, 'wb')
> > > >          fp.write('%i\n' % cls._version)
> > > >          fp.write('%s\n' % name)
> > >
> > > The shelvedstate class only provides load(), save() and clear().
> > > They are all storage functions. I guess new file format was supposed to
> be in new class.
> > > Thoughts?
> >
> > I do not like two classes. I think old style should die. Would it be ok if I
> remove the possibility to write old-style files and leave the ability to read
> both new and old files. The problem with having separate versions in
> separate classes is that we still have to maintain some sort of aggregating
> code to decide which to use and it does not seem worth it to me.
> 
> Yeah. If we don't support writing old state file (except for testing), the
> separate classes won't be necessary. So please disregard my idea.
> 
> > Basically, what I am proposing is to just remove the config knob, the ability
> to write old style statefiles and fix the test.
> 
> (see above)
Yuya Nishihara - April 14, 2017, 11:15 a.m.
On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > Comparing versions (if I understand you correctly) wouldn't work, see
> > below.
> > 
> > I couldn't get why new format can't be version 2. The old format had '1\n'
> > for future extension. This patch introduces new format, so using '2\n' makes
> > sense to me.
> > 
> > The current simplekeyvaluefile API would make this a bit harder, but we can
> > extract functions to serialize and deserialize dict from/to file object.
> I do not understand what 'extract functions ...' means in this context? Can you explain?

We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
Kostia Balytskyi - April 14, 2017, 4:15 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Friday, 14 April, 2017 12:16
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > Comparing versions (if I understand you correctly) wouldn't work,
> > > > see
> > > below.
> > >
> > > I couldn't get why new format can't be version 2. The old format had '1\n'
> > > for future extension. This patch introduces new format, so using
> > > '2\n' makes sense to me.
> > >
> > > The current simplekeyvaluefile API would make this a bit harder, but
> > > we can extract functions to serialize and deserialize dict from/to file
> object.
> > I do not understand what 'extract functions ...' means in this context? Can
> you explain?
> 
> We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.

I don't like this either. If we go this way, there's no point in having simplekeyvalue file at all. How about the following compromise:
- simplekeyvaluefile looks through the keys it is about to write and if 'version' is one of those keys, it always writes version first. This way, we can always read the first line of any state file and know it's version
- shelvestate.save() always writes a simplekeyvalue file, version=2 and the 'version' attribute of the class if also 2
- shelvestate.load() reads the first line and decides which way of reading to use
- we do not introduce another class, there's only version 2 shelvestate class which knows how to read anything

What do you think?
Yuya Nishihara - April 15, 2017, 8:01 a.m.
On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Friday, 14 April, 2017 12:16
> > To: Kostia Balytskyi <ikostia@fb.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> > simplekeyvaluefile
> > 
> > On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > > Comparing versions (if I understand you correctly) wouldn't work,
> > > > > see
> > > > below.
> > > >
> > > > I couldn't get why new format can't be version 2. The old format had '1\n'
> > > > for future extension. This patch introduces new format, so using
> > > > '2\n' makes sense to me.
> > > >
> > > > The current simplekeyvaluefile API would make this a bit harder, but
> > > > we can extract functions to serialize and deserialize dict from/to file
> > object.
> > > I do not understand what 'extract functions ...' means in this context? Can
> > you explain?
> > 
> > We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> > or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
> 
> I don't like this either. If we go this way, there's no point in having simplekeyvalue file at all.

Why is there no point? If I understand correctly, we want to switch to the
simplekeyvalue file format so that we can easily add new fields.

> How about the following compromise:
> - simplekeyvaluefile looks through the keys it is about to write and if 'version' is one of those keys, it always writes version first. This way, we can always read the first line of any state file and know it's version
> - shelvestate.save() always writes a simplekeyvalue file, version=2 and the 'version' attribute of the class if also 2
> - shelvestate.load() reads the first line and decides which way of reading to use

We could do that for write(), but not clean for read(). We would have to open
a file just for detecting the version, and open again by the simplekeyvaluefile
class.

> - we do not introduce another class, there's only version 2 shelvestate class which knows how to read anything

Sure.
Kostia Balytskyi - April 15, 2017, 5:43 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Saturday, 15 April, 2017 09:02
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > > -----Original Message-----
> > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > Nishihara
> > > Sent: Friday, 14 April, 2017 12:16
> > > To: Kostia Balytskyi <ikostia@fb.com>
> > > Cc: mercurial-devel@mercurial-scm.org
> > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate
> > > use simplekeyvaluefile
> > >
> > > On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > > > Comparing versions (if I understand you correctly) wouldn't
> > > > > > work, see
> > > > > below.
> > > > >
> > > > > I couldn't get why new format can't be version 2. The old format had
> '1\n'
> > > > > for future extension. This patch introduces new format, so using
> > > > > '2\n' makes sense to me.
> > > > >
> > > > > The current simplekeyvaluefile API would make this a bit harder,
> > > > > but we can extract functions to serialize and deserialize dict
> > > > > from/to file
> > > object.
> > > > I do not understand what 'extract functions ...' means in this
> > > > context? Can
> > > you explain?
> > >
> > > We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> > > or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
> >
> > I don't like this either. If we go this way, there's no point in having
> simplekeyvalue file at all.
> 
> Why is there no point? If I understand correctly, we want to switch to the
> simplekeyvalue file format so that we can easily add new fields.
By "no point" I meant that I don't want to break the simplekeyvalue promise, e.g. don't want to write non-key-value data.

> 
> > How about the following compromise:
> > - simplekeyvaluefile looks through the keys it is about to write and
> > if 'version' is one of those keys, it always writes version first.
> > This way, we can always read the first line of any state file and know
> > it's version
> > - shelvestate.save() always writes a simplekeyvalue file, version=2
> > and the 'version' attribute of the class if also 2
> > - shelvestate.load() reads the first line and decides which way of
> > reading to use
> 
> We could do that for write(), but not clean for read(). We would have to
> open a file just for detecting the version, and open again by the
> simplekeyvaluefile class.
I do not think that opening file 2 times is too bad: state files are small, read rarely. Sure, it is slightly ugly, but IMO worth doing.

> 
> > - we do not introduce another class, there's only version 2
> > shelvestate class which knows how to read anything
> 
> Sure.
Yuya Nishihara - April 16, 2017, 10:52 a.m.
On Sat, 15 Apr 2017 17:43:22 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Saturday, 15 April, 2017 09:02
> > To: Kostia Balytskyi <ikostia@fb.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> > simplekeyvaluefile
> > 
> > On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > > > -----Original Message-----
> > > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > > Nishihara
> > > > Sent: Friday, 14 April, 2017 12:16
> > > > To: Kostia Balytskyi <ikostia@fb.com>
> > > > Cc: mercurial-devel@mercurial-scm.org
> > > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate
> > > > use simplekeyvaluefile
> > > >
> > > > On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > > > > Comparing versions (if I understand you correctly) wouldn't
> > > > > > > work, see
> > > > > > below.
> > > > > >
> > > > > > I couldn't get why new format can't be version 2. The old format had
> > '1\n'
> > > > > > for future extension. This patch introduces new format, so using
> > > > > > '2\n' makes sense to me.
> > > > > >
> > > > > > The current simplekeyvaluefile API would make this a bit harder,
> > > > > > but we can extract functions to serialize and deserialize dict
> > > > > > from/to file
> > > > object.
> > > > > I do not understand what 'extract functions ...' means in this
> > > > > context? Can
> > > > you explain?
> > > >
> > > > We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> > > > or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
> > >
> > > I don't like this either. If we go this way, there's no point in having
> > simplekeyvalue file at all.
> > 
> > Why is there no point? If I understand correctly, we want to switch to the
> > simplekeyvalue file format so that we can easily add new fields.
> By "no point" I meant that I don't want to break the simplekeyvalue promise, e.g. don't want to write non-key-value data.

Then, how about changing the filename? I prefer using a version header '%d\n'
for the maximum backward/forward compatibility, but you don't want to put it
into the simplekeyvalue file. This probably means a simplekeyvalue file must
always be in the simplekeyvalue format.

Since old clients expect the first line can be parsed as an int, we'll need
two files, e.g. 'shelvedstate' says '2\n', and 'shelvedstate2' contains data
in the simplekeyvalue format.
Kostia Balytskyi - April 16, 2017, 10:51 p.m.
> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Sunday, 16 April, 2017 11:52
> To: Kostia Balytskyi <ikostia@fb.com>
> Cc: mercurial-devel@mercurial-scm.org
> Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> simplekeyvaluefile
> 
> On Sat, 15 Apr 2017 17:43:22 +0000, Kostia Balytskyi wrote:
> > > -----Original Message-----
> > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > Nishihara
> > > Sent: Saturday, 15 April, 2017 09:02
> > > To: Kostia Balytskyi <ikostia@fb.com>
> > > Cc: mercurial-devel@mercurial-scm.org
> > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate
> > > use simplekeyvaluefile
> > >
> > > On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > > > > -----Original Message-----
> > > > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > > > Nishihara
> > > > > Sent: Friday, 14 April, 2017 12:16
> > > > > To: Kostia Balytskyi <ikostia@fb.com>
> > > > > Cc: mercurial-devel@mercurial-scm.org
> > > > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make
> > > > > shelvestate use simplekeyvaluefile
> > > > >
> > > > > On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > > > > > Comparing versions (if I understand you correctly)
> > > > > > > > wouldn't work, see
> > > > > > > below.
> > > > > > >
> > > > > > > I couldn't get why new format can't be version 2. The old
> > > > > > > format had
> > > '1\n'
> > > > > > > for future extension. This patch introduces new format, so
> > > > > > > using '2\n' makes sense to me.
> > > > > > >
> > > > > > > The current simplekeyvaluefile API would make this a bit
> > > > > > > harder, but we can extract functions to serialize and
> > > > > > > deserialize dict from/to file
> > > > > object.
> > > > > > I do not understand what 'extract functions ...' means in this
> > > > > > context? Can
> > > > > you explain?
> > > > >
> > > > > We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> > > > > or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
> > > >
> > > > I don't like this either. If we go this way, there's no point in
> > > > having
> > > simplekeyvalue file at all.
> > >
> > > Why is there no point? If I understand correctly, we want to switch
> > > to the simplekeyvalue file format so that we can easily add new fields.
> > By "no point" I meant that I don't want to break the simplekeyvalue
> promise, e.g. don't want to write non-key-value data.
> 
> Then, how about changing the filename? I prefer using a version header
> '%d\n'
> for the maximum backward/forward compatibility, but you don't want to put
> it into the simplekeyvalue file. This probably means a simplekeyvalue file
> must always be in the simplekeyvalue format.
> 
> Since old clients expect the first line can be parsed as an int, we'll need two
> files, e.g. 'shelvedstate' says '2\n', and 'shelvedstate2' contains data in the
> simplekeyvalue format.

I realized the point I was missing. I assumed that if shelvestate file was written by Mercurial of version X, it can only be read by version >=X. This is not true and touches not only shelvestate files, but any file generally. I think this is worth sacrificing the completeness of simplekeyvaluefile protocol and allowing the very first line to be a simple integer. I will change it first and shelve patches later.
Yuya Nishihara - April 17, 2017, 1:25 p.m.
On Sun, 16 Apr 2017 22:51:39 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Sunday, 16 April, 2017 11:52
> > To: Kostia Balytskyi <ikostia@fb.com>
> > Cc: mercurial-devel@mercurial-scm.org
> > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate use
> > simplekeyvaluefile
> > 
> > On Sat, 15 Apr 2017 17:43:22 +0000, Kostia Balytskyi wrote:
> > > > -----Original Message-----
> > > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > > Nishihara
> > > > Sent: Saturday, 15 April, 2017 09:02
> > > > To: Kostia Balytskyi <ikostia@fb.com>
> > > > Cc: mercurial-devel@mercurial-scm.org
> > > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make shelvestate
> > > > use simplekeyvaluefile
> > > >
> > > > On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > > > > > -----Original Message-----
> > > > > > From: Yuya Nishihara [mailto:youjah@gmail.com] On Behalf Of Yuya
> > > > > > Nishihara
> > > > > > Sent: Friday, 14 April, 2017 12:16
> > > > > > To: Kostia Balytskyi <ikostia@fb.com>
> > > > > > Cc: mercurial-devel@mercurial-scm.org
> > > > > > Subject: Re: [PATCH 4 of 4 shelve-ext v3] shelve: make
> > > > > > shelvestate use simplekeyvaluefile
> > > > > >
> > > > > > On Thu, 13 Apr 2017 21:28:52 +0000, Kostia Balytskyi wrote:
> > > > > > > > > Comparing versions (if I understand you correctly)
> > > > > > > > > wouldn't work, see
> > > > > > > > below.
> > > > > > > >
> > > > > > > > I couldn't get why new format can't be version 2. The old
> > > > > > > > format had
> > > > '1\n'
> > > > > > > > for future extension. This patch introduces new format, so
> > > > > > > > using '2\n' makes sense to me.
> > > > > > > >
> > > > > > > > The current simplekeyvaluefile API would make this a bit
> > > > > > > > harder, but we can extract functions to serialize and
> > > > > > > > deserialize dict from/to file
> > > > > > object.
> > > > > > > I do not understand what 'extract functions ...' means in this
> > > > > > > context? Can
> > > > > > you explain?
> > > > > >
> > > > > > We can move deserialization function (e.g. 'loadkv(fp: file) -> dict'
> > > > > > or 'loadkv(s: str) -> dict') from simplekeyvaluefile.read() for example.
> > > > >
> > > > > I don't like this either. If we go this way, there's no point in
> > > > > having
> > > > simplekeyvalue file at all.
> > > >
> > > > Why is there no point? If I understand correctly, we want to switch
> > > > to the simplekeyvalue file format so that we can easily add new fields.
> > > By "no point" I meant that I don't want to break the simplekeyvalue
> > promise, e.g. don't want to write non-key-value data.
> > 
> > Then, how about changing the filename? I prefer using a version header
> > '%d\n'
> > for the maximum backward/forward compatibility, but you don't want to put
> > it into the simplekeyvalue file. This probably means a simplekeyvalue file
> > must always be in the simplekeyvalue format.
> > 
> > Since old clients expect the first line can be parsed as an int, we'll need two
> > files, e.g. 'shelvedstate' says '2\n', and 'shelvedstate2' contains data in the
> > simplekeyvalue format.
> 
> I realized the point I was missing. I assumed that if shelvestate file was written by Mercurial of version X, it can only be read by version >=X. This is not true and touches not only shelvestate files, but any file generally. I think this is worth sacrificing the completeness of simplekeyvaluefile protocol and allowing the very first line to be a simple integer. I will change it first and shelve patches later.

Thanks. Another unfortunate thing is that a version isn't always an integer.
histedit uses 'v1\n' for example. That's why I prefer somewhat low-level
dump/load(fp) API.

Anyway, the code freeze is about to start. It wouldn't make sense to introduce
the new file format before the freeze.

Patch

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,6 +179,18 @@  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, because old shelvestate file uses it
         # to detemine values of fields (i.g. version is on the first line,
@@ -182,12 +198,15 @@  class shelvedstate(object):
         keys = ['version', 'name', 'originalwctx', 'pendingctx', 'parents',
                 'nodestoremove', 'branchtorestore', 'keep', 'activebook']
         d = {}
-        fp = repo.vfs(cls._filename)
-        try:
-            for key in keys:
-                d[key] = fp.readline().strip()
-        finally:
-            fp.close()
+        if cls.iskeyvaluebased(repo):
+            d = scmutil.simplekeyvaluefile(repo.vfs, cls._filename).read()
+        else:
+            fp = repo.vfs(cls._filename)
+            try:
+                for key in keys:
+                    d[key] = fp.readline().strip()
+            finally:
+                fp.close()
 
         # some basic syntactic verification and transformation
         try:
@@ -224,6 +243,22 @@  class shelvedstate(object):
     @classmethod
     def save(cls, repo, name, originalwctx, pendingctx, nodestoremove,
              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()]),
+                "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)
+            return
         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
   unshelving change 'default'
   temporarily committing pending changes (restore with 'hg unshelve --abort')
   rebasing shelved changes