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

[Yuya Nishihara]

On Fri, 14 Apr 2017 16:15:01 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah at gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Friday, 14 April, 2017 12:16
> > 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 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.