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

[Bryan O'Sullivan]

On Tue, Jan 26, 2016 at 3:43 PM, Shusen LIU <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.


> +#
> +# 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?


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


> +    return data
>

This is redundant.


> +# 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.

+
> +def _evolvestatewrite(repo, data, version=defaultversion):
> +    formats[version][1](repo, data)
>

And here's why I don't like the tuple: now we've some indexing into a
nested structure, instead of a reference to a name.

+def _evolvestateread(repo):
> +    diskversion = int(repo.vfs.read('evolvestate').split('\n')[0])
>

If the evolve state is big, then reading the first line is going to become
very expensive because you're reading the entire file.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20160126/ea56b992/attachment.html>