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

Yuya Nishihara yuya at tcha.org
Mon Apr 17 09:25:01 EDT 2017


On Sun, 16 Apr 2017 22:51:39 +0000, Kostia Balytskyi wrote:
> > -----Original Message-----
> > From: Yuya Nishihara [mailto:youjah at gmail.com] On Behalf Of Yuya
> > Nishihara
> > Sent: Sunday, 16 April, 2017 11:52
> > 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 Sat, 15 Apr 2017 17:43:22 +0000, Kostia Balytskyi wrote:
> > > > -----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.
> > 
> > Then, how about changing the filename? I prefer using a version header
> > '%d\n'
> > for the maximum backward/forward compatibility, but you don't want to put
> > it into the simplekeyvalue file. This probably means a simplekeyvalue file
> > must always be in the simplekeyvalue format.
> > 
> > Since old clients expect the first line can be parsed as an int, we'll need two
> > files, e.g. 'shelvedstate' says '2\n', and 'shelvedstate2' contains data in the
> > simplekeyvalue format.
> 
> I realized the point I was missing. I assumed that if shelvestate file was written by Mercurial of version X, it can only be read by version >=X. This is not true and touches not only shelvestate files, but any file generally. I think this is worth sacrificing the completeness of simplekeyvaluefile protocol and allowing the very first line to be a simple integer. I will change it first and shelve patches later.

Thanks. Another unfortunate thing is that a version isn't always an integer.
histedit uses 'v1\n' for example. That's why I prefer somewhat low-level
dump/load(fp) API.

Anyway, the code freeze is about to start. It wouldn't make sense to introduce
the new file format before the freeze.


More information about the Mercurial-devel mailing list