D7834: nodemap: have some python code serializing a nodemap

martinvonz (Martin von Zweigbergk) phabricator at mercurial-scm.org
Thu Jan 16 16:44:14 EST 2020


This revision now requires changes to proceed.
martinvonz added inline comments.
martinvonz requested changes to this revision.

INLINE COMMENTS

> nodemap.py:47-49
> +def persistent_data(index):
> +    """return the serialised data of a nodemap for a given index
> +    """

nit: call it `serialize()` to match the description? or maybe `serialize_index()` and rename `_dump_trie()` to `_serialize_trie()`

> nodemap.py:57
> +NO_ENTRY = -1
> +# rev-0 need to be -2 because 0 is used by block, -1 is a special value.
> +REV_OFFSET = 2

nit: replace "rev-0" by "rev 0" so it's not interpreted as subtraction

> nodemap.py:66
> +
> +    Note that this is an involution.
> +    """

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

> nodemap.py:79
> +
> +    The nodemap store revision number for each unique prefix.
> +

s/store/stores/

> nodemap.py:81
> +
> +    Each block is a dictionnary with key in `[0, 15]`. Value are either
> +    another block or a revision number.

s/nn/n/
s/key/keys/
s/Value/Values/

> nodemap.py:85
> +    root = {}
> +    for rev in range(len(index)):
> +        hex = nodemod.hex(index[rev][7])

nit: will `rev, entry in enumerate(index)` help? (i know we don't care about speed)

> nodemap.py:112-118
> +        while current_hex[level] == other_hex[level]:
> +            new = {}
> +            block[_to_int(current_hex[level])] = new
> +            block = new
> +            level += 1
> +        block[_to_int(current_hex[level])] = current_rev
> +        block[_to_int(other_hex[level])] = other_rev

Would it work to replace these lines by the following?

  new = {}
  block[_to_int(current_hex[level])] = new
  _insert_into_block(index, level + 1, new, other_rev, other_hex)
  _insert_into_block(index, level + 1, new, current_rev, current_hex)

> nodemap.py:121
> +
> +def _dump_trie(root):
> +    """serialise a nodemap trie

as mentioned above, i think `_serialize_tree()` would be a clearer name (it matches how you described it in the docstring)

> nodemap.py:145
> +
> +def _dump_block(block_node, block_map):
> +    """serialise a single block

and this could be `_serialize_block()`

> nodemap.py:155
> +
> +def _to_value(item, block_map):
> +    """serialize any value as an integer"""

`serialize_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


More information about the Mercurial-devel mailing list