D7788: rust-node: binary Node ID and conversion utilities

[gracinet (Georges Racinet)]

gracinet added a comment.
gracinet marked 6 inline comments as done.


  As the new commit description explains, I've done all I could to make the change of hash format easier
  
  I should add that my other colleagues at Octobus seem also to be involved in the improvement of hashing, there's no risk we would forget this one.

INLINE COMMENTS

> martinvonz wrote in dirs_multiset.rs:111
> please revert unrelated change

ah yes, sorry must have been a rebase artifact

> martinvonz wrote in node.rs:31
> Or even use `hex::decode()` from the `hex` crate? Can also use `hex::encode()` in `node_to_hex` if we're okay with that.

Yes, `hex` is nice, it's able to produce arrays without further copies and does length checks. Its error type does not repeat the input string, though, which in my experience is really very useful to understand problems when they occur.

`hex` does not support hexadecimal string representations with odd numbers of
digits, which will be easy to work around in the next patch.

> kevincox wrote in node.rs:39
> If we want to avoid a number of allocations we can do:
> 
>   pub fn node_to_hex(n: &Node) -> String {
>       let mut r = String::with_capacity(n.len() * 2);
>       for byte in n {
>           write!(r, "{:02x}", byte).unwrap();
>       }
>       debug_assert_eq!(r.len(), n.len() * 2);
>       r
>   }
> 
> The generated code for `write!()` doesn't look great but if hex printing shows up in benchmarks it would be trivial to write a custom hex-formatter.

Finally used the `hex` crate here too.

> martinvonz wrote in node.rs:41-51
> This feels like something that will only be used in nodemap.rs, so put it there instead? Or do you think (or know) that it will be used elsewhere soon?

Well it's become a public method in the new `Node` struct.
`NodePrefix` will be another user.

I tend to prefer encapsulation over where it's used for these choices. This way, there's no tempation for nodemap.rs to play with the binary data directly.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers, kevincox, martinvonz
Cc: martinvonz, durin42, kevincox, mercurial-devel