D7834: nodemap: have some python code serializing a nodemap

[marmoute (Pierre-Yves David)]

marmoute added inline comments.

INLINE COMMENTS

> martinvonz wrote in nodemap.py:47-49
> nit: call it `serialize()` to match the description? or maybe `serialize_index()` and rename `_dump_trie()` to `_serialize_trie()`

I don't really like `serialize` because we won't have a `deserialize` step. The data should be usable directly with a mmap from disk. So if we standardize function name on something I would rather go toward "persist" (and "parse" for the python function that checks it).

This can apply to the "dump" function and docstring.

> martinvonz wrote in nodemap.py:57
> nit: replace "rev-0" by "rev 0" so it's not interpreted as subtraction

subtraction by zero would be odd, but why not.

> martinvonz wrote in nodemap.py:66
> nit: maybe "... involution (is its own inverse)" because i don't think most people know what an involution is
> 
> Also, you could rename the function to `encode_rev()`. You could then do:
> 
>   # encode_rev is its own inverse
>   decode_rev = encode_rev
> 
> Or not even define that since it's not used

I am not convinced by the encode_rev/decode_rev form. I don't think it very important to define `involution`, people can look for it if they need to. The reste of the docstring already explain the functionworks both way.

> martinvonz wrote in nodemap.py:85
> nit: will `rev, entry in enumerate(index)` help? (i know we don't care about speed)

We need to index the entry anyway, so that won't change the line much. I find the current form clearer.

> martinvonz wrote in nodemap.py:112-118
> The test case still passes with that change, though....

With more eyes, I am seeing what you are doing now. Yeah I think if would work. And it is more consistent with the recursivity used int he previous conditional block. Updating the patch,

> martinvonz wrote in nodemap.py:121
> as mentioned above, i think `_serialize_tree()` would be a clearer name (it matches how you described it in the docstring)

I would rather move to `_persist_trie`. What do you think?

> martinvonz wrote in nodemap.py:145
> and this could be `_serialize_block()`

so it would be `_persist_block()`

> martinvonz wrote in nodemap.py:155
> `serialize_value()`?

and `_persist_value()`

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7834/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7834

To: marmoute, #hg-reviewers, martinvonz
Cc: martinvonz, mjpieters, mercurial-devel