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

Kostia Balytskyi ikostia at fb.com
Wed Apr 12 17:34:23 EDT 2017


> -----Original Message-----
> From: Yuya Nishihara [mailto:youjah at gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Wednesday, 12 April, 2017 16:03
> To: Kostia Balytskyi <ikostia at fb.com>
> Cc: mercurial-devel at 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 at 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.


More information about the Mercurial-devel mailing list