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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
