> -----Original Message-----
> From: Yuya Nishihara [mailto:you...@gmail.com] On Behalf Of Yuya
> Nishihara
> Sent: Thursday, 13 April, 2017 13:47
> To: Kostia Balytskyi <ikos...@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:you...@gmail.com] On Behalf Of Yuya
> > > Nishihara
> > > Sent: Wednesday, 12 April, 2017 16:03
> > > To: Kostia Balytskyi <ikos...@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 <ikos...@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)
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
