[PATCH evolve-ext-V5] evolve: add new methods _evolvestatewrite, _evolvestateread, _evolvestatedelete

[Pierre-Yves David]

On 01/27/2016 01:01 AM, Bryan O'Sullivan wrote:
> On Tue, Jan 26, 2016 at 3:43 PM, Shusen LIU <liushusen at fb.com
> <mailto:liushusen at fb.com>> wrote:
>
>     +## Parsing and writing of version "0"
>
>
> We have historically avoided numbered versions in favour of
> capabilities. I don't have a strong opinion on whether to do that here,
> just raising this as diverging from that practice.

Actually, we have been using version number extensively in the context 
of file format recently. Binary formats are a quite rigid structure and 
the one disk format have limited scope and change enough that having 
version number is good enough. Especially because the version number is 
usually just used to decide which parser to use. Nowaday the content of 
the file itself already account for some extensibility.

(so, yes, we want a version number in this on disk file)

>     +# The file contains two line.
>     +# First line is the version number.
>     +# Second line is the the data in json format
>     +
>     +_fm0evolvestateversion = 0
>
>
> What does _fm indicate here?

_fm0 is used elsewhere in the code to distinct function that handle 
obsstore format-0 and obsstore format-1.

This is not very critical here (since we don't have multiple evolve 
state version yet). But it does not hurt.

>
>     +
>     +def _fm0evolvestateread(repo):
>     +    serialized_data = repo.vfs.read('evolvestate').split('\n')[1]
>     +    data = dict([(str(k), str(v))
>     +                 for k,v in json.loads(serialized_data).iteritems()])
>
>
> This doesn't make sense to me. Why are you turning everything in the
> serialised json into strings? Wouldn't they already be in the desired
> form if you wrote them out?

json is unicode, and Mercurial wants (good old) string. We serialise 
string, but they come back as unicode object. We don't want unicode object.

That said, this approach is probably too naive because we'll want to be 
able to store something else that just string as value (list, other 
dict, etc), so this blind conversion if doing to break.

>     +# mapping to read/write various evolvestate formats
>     +# <version> -> (reader, writer)
>     +formats = {_fm0evolvestateversion: (_fm0evolvestateread,
>     _fm0evolvestatewrite),}
>     +defaultversion = _fm0evolvestateversion
>
>
> I think you should really put these functions in a class and refer to
> that class. My feeling is that using a tuple is less elegant.

Matt have quite strong feeling about class as namespace. In my opinion, 
as we only have two elements, using a tuple is not too problematic.

-- 
Pierre-Yves David