D7790: rust-node: handling binary Node prefix

2020-01-27 Thread martinvonz (Martin von Zweigbergk)
martinvonz added inline comments.

INLINE COMMENTS

> gracinet wrote in node.rs:226
> yes, indeed.
> 
> current callers from `nodemap` work either on full Nodes or (the visitor) by 
> explicitely bounding with `prefix.len()`, but it's safer to protect it 
> inconditonally.
> 
> I think a simple `assert!` is enough though: that's already what slicing does 
> anyway.

Yes, an `assert!` is probably what I had in mind. I've already queued this, so 
please send in a follow-up.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers, martinvonz
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-27 Thread gracinet (Georges Racinet)
gracinet added inline comments.

INLINE COMMENTS

> martinvonz wrote in node.rs:226
> Should we check here that `i < self.len()`? I'm especially thinking of the 
> case of an odd-length prefix where we would otherwise silently return 0.

yes, indeed.

current callers from `nodemap` work either on full Nodes or (the visitor) by 
explicitely bounding with `prefix.len()`, but it's safer to protect it 
inconditonally.

I think a simple `assert!` is enough though: that's already what slicing does 
anyway.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers, martinvonz
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-27 Thread gracinet (Georges Racinet)
Closed by commit rHG9896a8d0d3d2: rust-node: handling binary Node prefix 
(authored by gracinet).
This revision was automatically updated to reflect the committed changes.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7790?vs=19630=19643

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

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

AFFECTED FILES
  rust/hg-core/src/revlog.rs
  rust/hg-core/src/revlog/node.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/node.rs b/rust/hg-core/src/revlog/node.rs
--- a/rust/hg-core/src/revlog/node.rs
+++ b/rust/hg-core/src/revlog/node.rs
@@ -62,6 +62,7 @@
 #[derive(Debug, PartialEq)]
 pub enum NodeError {
 ExactLengthRequired(usize, String),
+PrefixTooLong(String),
 HexError(FromHexError, String),
 }
 
@@ -119,17 +120,119 @@
 }
 }
 
-impl From<(FromHexError, )> for NodeError {
-fn from(err_offender: (FromHexError, )) -> Self {
+impl> From<(FromHexError, T)> for NodeError {
+fn from(err_offender: (FromHexError, T)) -> Self {
 let (err, offender) = err_offender;
 match err {
 FromHexError::InvalidStringLength => {
 NodeError::ExactLengthRequired(
 NODE_NYBBLES_LENGTH,
-offender.to_string(),
+offender.as_ref().to_owned(),
 )
 }
-_ => NodeError::HexError(err, offender.to_string()),
+_ => NodeError::HexError(err, offender.as_ref().to_owned()),
+}
+}
+}
+
+/// The beginning of a binary revision SHA.
+///
+/// Since it can potentially come from an hexadecimal representation with
+/// odd length, it needs to carry around whether the last 4 bits are relevant
+/// or not.
+#[derive(Debug, PartialEq)]
+pub struct NodePrefix {
+buf: Vec,
+is_odd: bool,
+}
+
+impl NodePrefix {
+/// Convert from hexadecimal string representation
+///
+/// Similarly to `hex::decode`, can be used with Unicode string types
+/// (`String`, ``) as well as bytes.
+///
+/// To be used in FFI and I/O only, in order to facilitate future
+/// changes of hash format.
+pub fn from_hex(hex: impl AsRef<[u8]>) -> Result {
+let hex = hex.as_ref();
+let len = hex.len();
+if len > NODE_NYBBLES_LENGTH {
+return Err(NodeError::PrefixTooLong(
+String::from_utf8_lossy(hex).to_owned().to_string(),
+));
+}
+
+let is_odd = len % 2 == 1;
+let even_part = if is_odd { [..len - 1] } else { hex };
+let mut buf: Vec = Vec::from_hex(_part)
+.map_err(|e| (e, String::from_utf8_lossy(hex)))?;
+
+if is_odd {
+let latest_char = char::from(hex[len - 1]);
+let latest_nybble = latest_char.to_digit(16).ok_or_else(|| {
+(
+FromHexError::InvalidHexCharacter {
+c: latest_char,
+index: len - 1,
+},
+String::from_utf8_lossy(hex),
+)
+})? as u8;
+buf.push(latest_nybble << 4);
+}
+Ok(NodePrefix { buf, is_odd })
+}
+
+pub fn borrow() -> NodePrefixRef {
+NodePrefixRef {
+buf: ,
+is_odd: self.is_odd,
+}
+}
+}
+
+#[derive(Clone, Debug, PartialEq)]
+pub struct NodePrefixRef<'a> {
+buf: &'a [u8],
+is_odd: bool,
+}
+
+impl<'a> NodePrefixRef<'a> {
+pub fn len() -> usize {
+if self.is_odd {
+self.buf.len() * 2 - 1
+} else {
+self.buf.len() * 2
+}
+}
+
+pub fn is_prefix_of(, node: ) -> bool {
+if self.is_odd {
+let buf = self.buf;
+let last_pos = buf.len() - 1;
+node.data.starts_with(buf.split_at(last_pos).0)
+&& node.data[last_pos] >> 4 == buf[last_pos] >> 4
+} else {
+node.data.starts_with(self.buf)
+}
+}
+
+/// Retrieve the `i`th half-byte from the prefix.
+///
+/// This is also the `i`th hexadecimal digit in numeric form,
+/// also called a [nybble](https://en.wikipedia.org/wiki/Nibble).
+pub fn get_nybble(, i: usize) -> u8 {
+get_nybble(self.buf, i)
+}
+}
+
+/// A shortcut for full `Node` references
+impl<'a> From<&'a Node> for NodePrefixRef<'a> {
+fn from(node: &'a Node) -> Self {
+NodePrefixRef {
+buf: ,
+is_odd: false,
 }
 }
 }
@@ -188,4 +291,74 @@
 fn test_node_encode_hex() {
 assert_eq!(sample_node().encode_hex(), sample_node_hex());
 }
+
+#[test]
+fn test_prefix_from_hex() -> Result<(), NodeError> {
+assert_eq!(
+NodePrefix::from_hex("0e1")?,
+NodePrefix {
+buf: vec![14, 16],
+is_odd: true
+}
+);
+assert_eq!(
+

D7790: rust-node: handling binary Node prefix

2020-01-27 Thread martinvonz (Martin von Zweigbergk)
This revision is now accepted and ready to land.
martinvonz added inline comments.
martinvonz accepted this revision.

INLINE COMMENTS

> node.rs:103
> +
> +impl<'a> NodePrefixRef<'a> {
> +pub fn len() -> usize {

The lifetime parameter doesn't seem to be used, so make it anonymous? (I.e., 
remove it from `impl` and use `<'_>` on the type.

> node.rs:226
> +pub fn get_nybble(, i: usize) -> u8 {
> +get_nybble(self.buf, i)
> +}

Should we check here that `i < self.len()`? I'm especially thinking of the case 
of an odd-length prefix where we would otherwise silently return 0.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: gracinet, #hg-reviewers, martinvonz
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-27 Thread gracinet (Georges Racinet)
gracinet added a comment.


  > Depends on the definition of NodePrefixRef. I don't think there would be 
any extra allocation if you define it like this:
  
  
  
pub enum NodePrefixRef<'a> {
Full(&'a Node),
Prefix(&'a NodePrefix),
}
  
  That's an interesting idea, another possibility would be to define a trait 
(essentially for `get_nybble`) and implement it for `` and ``. 
We'll see when we're pass preliminary tests, thanks.

INLINE COMMENTS

> martinvonz wrote in node.rs:79
> Why not use `(len + 1) / 2` as capacity?

Just thought 20 would be the simplest one-size-fit-all. With the preparations 
for different hash length (and potentially even somewhat dynamic), it's really 
obsolete now (switched to actual len based, yes).

> martinvonz wrote in node.rs:89
> Is this lifetime parameter needed?

ah yes indeed the compiler seems to be able to infer that `` and 
`Nodeprefix.buf` have the same lifteime, and that it must then be equal to the 
lifetime parameter of `NodePrefix` itself.

> martinvonz wrote in node.rs:136
> What does the `&*` do? Specifically, what's different if you drop that part?

These are conversions that need to be explicit when the compiler doesn't insert 
them magically. Usually, I try to avoid them, but it can happen that they 
become non necessary in the final form after some changes.

Not needed in the new form with a real struct.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-27 Thread gracinet (Georges Racinet)
gracinet edited the summary of this revision.
gracinet updated this revision to Diff 19630.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D7790?vs=19039=19630

BRANCH
  default

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

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

AFFECTED FILES
  rust/hg-core/src/revlog.rs
  rust/hg-core/src/revlog/node.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/node.rs b/rust/hg-core/src/revlog/node.rs
--- a/rust/hg-core/src/revlog/node.rs
+++ b/rust/hg-core/src/revlog/node.rs
@@ -62,6 +62,7 @@
 #[derive(Debug, PartialEq)]
 pub enum NodeError {
 ExactLengthRequired(usize, String),
+PrefixTooLong(String),
 HexError(FromHexError, String),
 }
 
@@ -119,17 +120,119 @@
 }
 }
 
-impl From<(FromHexError, )> for NodeError {
-fn from(err_offender: (FromHexError, )) -> Self {
+impl> From<(FromHexError, T)> for NodeError {
+fn from(err_offender: (FromHexError, T)) -> Self {
 let (err, offender) = err_offender;
 match err {
 FromHexError::InvalidStringLength => {
 NodeError::ExactLengthRequired(
 NODE_NYBBLES_LENGTH,
-offender.to_string(),
+offender.as_ref().to_owned(),
 )
 }
-_ => NodeError::HexError(err, offender.to_string()),
+_ => NodeError::HexError(err, offender.as_ref().to_owned()),
+}
+}
+}
+
+/// The beginning of a binary revision SHA.
+///
+/// Since it can potentially come from an hexadecimal representation with
+/// odd length, it needs to carry around whether the last 4 bits are relevant
+/// or not.
+#[derive(Debug, PartialEq)]
+pub struct NodePrefix {
+buf: Vec,
+is_odd: bool,
+}
+
+impl NodePrefix {
+/// Convert from hexadecimal string representation
+///
+/// Similarly to `hex::decode`, can be used with Unicode string types
+/// (`String`, ``) as well as bytes.
+///
+/// To be used in FFI and I/O only, in order to facilitate future
+/// changes of hash format.
+pub fn from_hex(hex: impl AsRef<[u8]>) -> Result {
+let hex = hex.as_ref();
+let len = hex.len();
+if len > NODE_NYBBLES_LENGTH {
+return Err(NodeError::PrefixTooLong(
+String::from_utf8_lossy(hex).to_owned().to_string(),
+));
+}
+
+let is_odd = len % 2 == 1;
+let even_part = if is_odd { [..len - 1] } else { hex };
+let mut buf: Vec = Vec::from_hex(_part)
+.map_err(|e| (e, String::from_utf8_lossy(hex)))?;
+
+if is_odd {
+let latest_char = char::from(hex[len - 1]);
+let latest_nybble = latest_char.to_digit(16).ok_or_else(|| {
+(
+FromHexError::InvalidHexCharacter {
+c: latest_char,
+index: len - 1,
+},
+String::from_utf8_lossy(hex),
+)
+})? as u8;
+buf.push(latest_nybble << 4);
+}
+Ok(NodePrefix { buf, is_odd })
+}
+
+pub fn borrow() -> NodePrefixRef {
+NodePrefixRef {
+buf: ,
+is_odd: self.is_odd,
+}
+}
+}
+
+#[derive(Clone, Debug, PartialEq)]
+pub struct NodePrefixRef<'a> {
+buf: &'a [u8],
+is_odd: bool,
+}
+
+impl<'a> NodePrefixRef<'a> {
+pub fn len() -> usize {
+if self.is_odd {
+self.buf.len() * 2 - 1
+} else {
+self.buf.len() * 2
+}
+}
+
+pub fn is_prefix_of(, node: ) -> bool {
+if self.is_odd {
+let buf = self.buf;
+let last_pos = buf.len() - 1;
+node.data.starts_with(buf.split_at(last_pos).0)
+&& node.data[last_pos] >> 4 == buf[last_pos] >> 4
+} else {
+node.data.starts_with(self.buf)
+}
+}
+
+/// Retrieve the `i`th half-byte from the prefix.
+///
+/// This is also the `i`th hexadecimal digit in numeric form,
+/// also called a [nybble](https://en.wikipedia.org/wiki/Nibble).
+pub fn get_nybble(, i: usize) -> u8 {
+get_nybble(self.buf, i)
+}
+}
+
+/// A shortcut for full `Node` references
+impl<'a> From<&'a Node> for NodePrefixRef<'a> {
+fn from(node: &'a Node) -> Self {
+NodePrefixRef {
+buf: ,
+is_odd: false,
 }
 }
 }
@@ -188,4 +291,74 @@
 fn test_node_encode_hex() {
 assert_eq!(sample_node().encode_hex(), sample_node_hex());
 }
+
+#[test]
+fn test_prefix_from_hex() -> Result<(), NodeError> {
+assert_eq!(
+NodePrefix::from_hex("0e1")?,
+NodePrefix {
+buf: vec![14, 16],
+is_odd: true
+}
+);
+assert_eq!(
+NodePrefix::from_hex("0e1a")?,
+NodePrefix {
+  

D7790: rust-node: handling binary Node prefix

2020-01-21 Thread martinvonz (Martin von Zweigbergk)
martinvonz added a comment.


  In D7790#116757 , @gracinet 
wrote:
  
  > @martinvonz in this code, we're in competition with the C implementation, 
which does something similar.
  > Switching to a full u8 per nybble is more than a few minutes, it means 
changing the ownership model completely. Also, it introduces a new allocation 
and a copy instead of merely taking a reference.
  
  Depends on the definition of `NodePrefixRef`. I don't think there would be 
any extra allocation if you define it like this:
  
pub enum NodePrefixRef<'a> {
Full(&'a Node),
Prefix(&'a NodePrefix),
}
  
  
  
  > So, it's more work and has performance impact that we have no means to 
measure. 
  > The odd/even thing is not *that* complicated. It's very bad for readability 
in the C code, but it's nicely encapsulated in the Rust case and fully tested. 
I'd be more comfortable trying what you're suggesting once we have real-life 
measurements.
  
  Sure, please revisit it when you can do measurements.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-21 Thread martinvonz (Martin von Zweigbergk)
martinvonz added inline comments.

INLINE COMMENTS

> node.rs:79
> +let is_odd = len % 2 == 1;
> +let mut buf: Vec = Vec::with_capacity(20);
> +for i in 0..len / 2 {

Why not use `(len + 1) / 2` as capacity?

> node.rs:89
> +
> +pub fn borrow<'a>(&'a self) -> NodePrefixRef<'a> {
> +NodePrefixRef {

Is this lifetime parameter needed?

> node.rs:136
> +NodePrefixRef {
> +buf: &*node,
> +is_odd: false,

What does the `&*` do? Specifically, what's different if you drop that part?

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-20 Thread gracinet (Georges Racinet)
gracinet added a comment.


  @martinvonz in this code, we're in competition with the C implementation, 
which does something similar.
  
  Switching to a full u8 per nybble is more than a few minutes, it means 
changing the ownership model completely. Also, it introduces a new allocation 
and a copy instead of merely taking a reference.
  
  So, it's more work and has performance impact that we have no means to 
measure.
  
  The odd/even thing is not *that* complicated. It's very bad for readability 
in the C code, but it's nicely encapsulated in the Rust case and fully tested. 
I'd be more comfortable trying what you're suggesting once we have real-life 
measurements.

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-17 Thread martinvonz (Martin von Zweigbergk)
martinvonz added inline comments.

INLINE COMMENTS

> node.rs:62-64
> +/// Since it can potentially come from an hexadecimal representation with
> +/// odd length, it needs to carry around whether the last 4 bits are relevant
> +/// or not.

Did you consider using a full u8 for each nibble instead? It seems like that 
would be simpler. I'd appreciate it if you could spend a few minutes to try 
that (if you haven't already).

REPOSITORY
  rHG Mercurial

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

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

To: gracinet, #hg-reviewers
Cc: martinvonz, durin42, kevincox, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel


D7790: rust-node: handling binary Node prefix

2020-01-06 Thread gracinet (Georges Racinet)
gracinet created this revision.
Herald added subscribers: mercurial-devel, kevincox, durin42.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Parallel to the inner signatures of the nodetree functions in
  revlog.c, we'll have to handle prefixes of `Node` in binary
  form.
  
  There's a complication due to the fact that we'll be sometimes
  interested in prefixes with an odd number of hexadecimal digits,
  which translates in binary form by a last byte in which only the
  highest weight 4 bits are considered.
  
  There are a few candidates for inlining here, but we refrain from
  such premature optimizations, letting the compiler decide.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/revlog.rs
  rust/hg-core/src/revlog/node.rs

CHANGE DETAILS

diff --git a/rust/hg-core/src/revlog/node.rs b/rust/hg-core/src/revlog/node.rs
--- a/rust/hg-core/src/revlog/node.rs
+++ b/rust/hg-core/src/revlog/node.rs
@@ -19,6 +19,7 @@
 #[derive(Debug, PartialEq)]
 pub enum NodeError {
 ExactLengthRequired(String),
+PrefixTooLong(String),
 NotHexadecimal,
 }
 
@@ -56,6 +57,88 @@
 }
 }
 
+/// The beginning of a binary revision SHA.
+///
+/// Since it can potentially come from an hexadecimal representation with
+/// odd length, it needs to carry around whether the last 4 bits are relevant
+/// or not.
+#[derive(Debug, PartialEq)]
+pub struct NodePrefix {
+buf: Vec,
+is_odd: bool,
+}
+
+impl NodePrefix {
+/// Conversion from hexadecimal string representation
+pub fn from_hex(hex: ) -> Result {
+let len = hex.len();
+if len > 40 {
+return Err(NodeError::PrefixTooLong(hex.to_string()));
+}
+let is_odd = len % 2 == 1;
+let mut buf: Vec = Vec::with_capacity(20);
+for i in 0..len / 2 {
+buf.push(u8::from_str_radix([i * 2..i * 2 + 2], 16)?);
+}
+if is_odd {
+buf.push(u8::from_str_radix([len - 1..], 16)? << 4);
+}
+Ok(NodePrefix { buf, is_odd })
+}
+
+pub fn borrow<'a>(&'a self) -> NodePrefixRef<'a> {
+NodePrefixRef {
+buf: ,
+is_odd: self.is_odd,
+}
+}
+}
+
+#[derive(Clone, Debug, PartialEq)]
+pub struct NodePrefixRef<'a> {
+buf: &'a [u8],
+is_odd: bool,
+}
+
+impl<'a> NodePrefixRef<'a> {
+pub fn len() -> usize {
+if self.is_odd {
+self.buf.len() * 2 - 1
+} else {
+self.buf.len() * 2
+}
+}
+
+pub fn is_prefix_of(, node: ) -> bool {
+if self.is_odd {
+let buf = self.buf;
+let last_pos = buf.len() - 1;
+node.starts_with(buf.split_at(last_pos).0)
+&& node[last_pos] >> 4 == buf[last_pos] >> 4
+} else {
+node.starts_with(self.buf)
+}
+}
+
+/// Retrieve the `i`th half-byte from the prefix.
+///
+/// This is also the `i`th hexadecimal digit in numeric form,
+/// also called a [nybble](https://en.wikipedia.org/wiki/Nibble).
+pub fn get_nybble(, i: usize) -> u8 {
+get_nybble(i, self.buf)
+}
+}
+
+/// A shortcut for full `Node` references
+impl<'a> From<&'a Node> for NodePrefixRef<'a> {
+fn from(node: &'a Node) -> Self {
+NodePrefixRef {
+buf: &*node,
+is_odd: false,
+}
+}
+}
+
 #[cfg(test)]
 mod tests {
 use super::*;
@@ -88,4 +171,68 @@
 SAMPLE_NODE_HEX
 );
 }
+
+#[test]
+fn test_prefix_from_hex() -> Result<(), NodeError> {
+assert_eq!(
+NodePrefix::from_hex("0e1")?,
+NodePrefix {
+buf: vec![14, 16],
+is_odd: true
+}
+);
+assert_eq!(
+NodePrefix::from_hex("0e1a")?,
+NodePrefix {
+buf: vec![14, 26],
+is_odd: false
+}
+);
+
+// checking limit case
+assert_eq!(
+NodePrefix::from_hex(SAMPLE_NODE_HEX)?,
+NodePrefix {
+buf: node_from_hex(SAMPLE_NODE_HEX)?.iter().cloned().collect(),
+is_odd: false
+}
+);
+
+Ok(())
+}
+
+#[test]
+fn test_prefix_from_hex_errors() {
+assert_eq!(
+NodePrefix::from_hex("testgr"),
+Err(NodeError::NotHexadecimal)
+);
+let long = "0";
+match NodePrefix::from_hex(long)
+.expect_err("should be refused as too long")
+{
+NodeError::PrefixTooLong(s) => assert_eq!(s, long),
+err => panic!(format!("Should have been TooLong, got {:?}", err)),
+}
+}
+
+#[test]
+fn test_is_prefix_of() -> Result<(), NodeError> {
+let mut node: Node = [0; 20];
+node[0] = 0x12;
+node[1] = 0xca;
+