[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: Saturday, 15 April, 2017 09:02
> 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 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.
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.