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

[Kostia Balytskyi]

> -----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. 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?