Patchwork D9864: rust: Remove hex parsing from the nodemap

login
register
mail settings
Submitter phabricator
Date Jan. 25, 2021, 9:14 p.m.
Message ID <differential-rev-PHID-DREV-vwwuma65nlnnrc3urqkv-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/48174/
State Superseded
Headers show

Comments

phabricator - Jan. 25, 2021, 9:14 p.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  Separating concerns simplifies error types.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/examples/nodemap/main.rs
  rust/hg-core/src/revlog/nodemap.rs
  rust/hg-cpython/src/revlog.rs

CHANGE DETAILS




To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/rust/hg-cpython/src/revlog.rs b/rust/hg-cpython/src/revlog.rs
--- a/rust/hg-cpython/src/revlog.rs
+++ b/rust/hg-cpython/src/revlog.rs
@@ -17,7 +17,7 @@ 
 };
 use hg::{
     nodemap::{Block, NodeMapError, NodeTree},
-    revlog::{nodemap::NodeMap, RevlogIndex},
+    revlog::{nodemap::NodeMap, NodePrefix, RevlogIndex},
     Revision,
 };
 use std::cell::RefCell;
@@ -107,7 +107,9 @@ 
             String::from_utf8_lossy(node.data(py)).to_string()
         };
 
-        nt.find_hex(idx, &node_as_string)
+        let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?;
+
+        nt.find_bin(idx, prefix)
             // TODO make an inner API returning the node directly
             .map(|opt| opt.map(
                 |rev| PyBytes::new(py, idx.node(rev).unwrap().as_bytes())))
@@ -468,9 +470,6 @@ 
     match err {
         NodeMapError::MultipleResults => revlog_error(py),
         NodeMapError::RevisionNotInIndex(r) => rev_not_in_index(py, r),
-        NodeMapError::InvalidNodePrefix => {
-            PyErr::new::<ValueError, _>(py, "Invalid node or prefix")
-        }
     }
 }
 
diff --git a/rust/hg-core/src/revlog/nodemap.rs b/rust/hg-core/src/revlog/nodemap.rs
--- a/rust/hg-core/src/revlog/nodemap.rs
+++ b/rust/hg-core/src/revlog/nodemap.rs
@@ -13,8 +13,7 @@ 
 //! is used in a more abstract context.
 
 use super::{
-    node::NULL_NODE, FromHexError, Node, NodePrefix, Revision, RevlogIndex,
-    NULL_REVISION,
+    node::NULL_NODE, Node, NodePrefix, Revision, RevlogIndex, NULL_REVISION,
 };
 
 use std::cmp::max;
@@ -27,17 +26,10 @@ 
 #[derive(Debug, PartialEq)]
 pub enum NodeMapError {
     MultipleResults,
-    InvalidNodePrefix,
     /// A `Revision` stored in the nodemap could not be found in the index
     RevisionNotInIndex(Revision),
 }
 
-impl From<FromHexError> for NodeMapError {
-    fn from(_: FromHexError) -> Self {
-        NodeMapError::InvalidNodePrefix
-    }
-}
-
 /// Mapping system from Mercurial nodes to revision numbers.
 ///
 /// ## `RevlogIndex` and `NodeMap`
@@ -85,21 +77,6 @@ 
         prefix: NodePrefix,
     ) -> Result<Option<Revision>, NodeMapError>;
 
-    /// Find the unique Revision whose `Node` hexadecimal string representation
-    /// starts with a given prefix
-    ///
-    /// If no Revision matches the given prefix, `Ok(None)` is returned.
-    ///
-    /// If several Revisions match the given prefix, a [`MultipleResults`]
-    /// error is returned.
-    fn find_hex(
-        &self,
-        idx: &impl RevlogIndex,
-        prefix: &str,
-    ) -> Result<Option<Revision>, NodeMapError> {
-        self.find_bin(idx, NodePrefix::from_hex(prefix)?)
-    }
-
     /// Give the size of the shortest node prefix that determines
     /// the revision uniquely.
     ///
@@ -117,16 +94,6 @@ 
         node_prefix: NodePrefix,
     ) -> Result<Option<usize>, NodeMapError>;
 
-    /// Same as `unique_prefix_len_bin`, with the hexadecimal representation
-    /// of the prefix as input.
-    fn unique_prefix_len_hex(
-        &self,
-        idx: &impl RevlogIndex,
-        prefix: &str,
-    ) -> Result<Option<usize>, NodeMapError> {
-        self.unique_prefix_len_bin(idx, NodePrefix::from_hex(prefix)?)
-    }
-
     /// Same as `unique_prefix_len_bin`, with a full `Node` as input
     fn unique_prefix_len_node(
         &self,
@@ -819,6 +786,10 @@ 
         ])
     }
 
+    fn hex(s: &str) -> NodePrefix {
+        NodePrefix::from_hex(s).unwrap()
+    }
+
     #[test]
     fn test_nt_debug() {
         let nt = sample_nodetree();
@@ -837,11 +808,11 @@ 
         pad_insert(&mut idx, 1, "1234deadcafe");
 
         let nt = NodeTree::from(vec![block! {1: Rev(1)}]);
-        assert_eq!(nt.find_hex(&idx, "1")?, Some(1));
-        assert_eq!(nt.find_hex(&idx, "12")?, Some(1));
-        assert_eq!(nt.find_hex(&idx, "1234de")?, Some(1));
-        assert_eq!(nt.find_hex(&idx, "1a")?, None);
-        assert_eq!(nt.find_hex(&idx, "ab")?, None);
+        assert_eq!(nt.find_bin(&idx, hex("1"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("12"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("1234de"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("1a"))?, None);
+        assert_eq!(nt.find_bin(&idx, hex("ab"))?, None);
 
         // and with full binary Nodes
         assert_eq!(nt.find_node(&idx, idx.get(&1).unwrap())?, Some(1));
@@ -858,12 +829,12 @@ 
 
         let nt = sample_nodetree();
 
-        assert_eq!(nt.find_hex(&idx, "0"), Err(MultipleResults));
-        assert_eq!(nt.find_hex(&idx, "01"), Ok(Some(9)));
-        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
-        assert_eq!(nt.find_hex(&idx, "00a"), Ok(Some(0)));
-        assert_eq!(nt.unique_prefix_len_hex(&idx, "00a"), Ok(Some(3)));
-        assert_eq!(nt.find_hex(&idx, "000"), Ok(Some(NULL_REVISION)));
+        assert_eq!(nt.find_bin(&idx, hex("0")), Err(MultipleResults));
+        assert_eq!(nt.find_bin(&idx, hex("01")), Ok(Some(9)));
+        assert_eq!(nt.find_bin(&idx, hex("00")), Err(MultipleResults));
+        assert_eq!(nt.find_bin(&idx, hex("00a")), Ok(Some(0)));
+        assert_eq!(nt.unique_prefix_len_bin(&idx, hex("00a")), Ok(Some(3)));
+        assert_eq!(nt.find_bin(&idx, hex("000")), Ok(Some(NULL_REVISION)));
     }
 
     #[test]
@@ -881,13 +852,13 @@ 
             root: block![0: Block(1), 1:Block(3), 12: Rev(2)],
             masked_inner_blocks: 1,
         };
-        assert_eq!(nt.find_hex(&idx, "10")?, Some(1));
-        assert_eq!(nt.find_hex(&idx, "c")?, Some(2));
-        assert_eq!(nt.unique_prefix_len_hex(&idx, "c")?, Some(1));
-        assert_eq!(nt.find_hex(&idx, "00"), Err(MultipleResults));
-        assert_eq!(nt.find_hex(&idx, "000")?, Some(NULL_REVISION));
-        assert_eq!(nt.unique_prefix_len_hex(&idx, "000")?, Some(3));
-        assert_eq!(nt.find_hex(&idx, "01")?, Some(9));
+        assert_eq!(nt.find_bin(&idx, hex("10"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("c"))?, Some(2));
+        assert_eq!(nt.unique_prefix_len_bin(&idx, hex("c"))?, Some(1));
+        assert_eq!(nt.find_bin(&idx, hex("00")), Err(MultipleResults));
+        assert_eq!(nt.find_bin(&idx, hex("000"))?, Some(NULL_REVISION));
+        assert_eq!(nt.unique_prefix_len_bin(&idx, hex("000"))?, Some(3));
+        assert_eq!(nt.find_bin(&idx, hex("01"))?, Some(9));
         assert_eq!(nt.masked_readonly_blocks(), 2);
         Ok(())
     }
@@ -920,14 +891,14 @@ 
             &self,
             prefix: &str,
         ) -> Result<Option<Revision>, NodeMapError> {
-            self.nt.find_hex(&self.index, prefix)
+            self.nt.find_bin(&self.index, hex(prefix))
         }
 
         fn unique_prefix_len_hex(
             &self,
             prefix: &str,
         ) -> Result<Option<usize>, NodeMapError> {
-            self.nt.unique_prefix_len_hex(&self.index, prefix)
+            self.nt.unique_prefix_len_bin(&self.index, hex(prefix))
         }
 
         /// Drain `added` and restart a new one
diff --git a/rust/hg-core/examples/nodemap/main.rs b/rust/hg-core/examples/nodemap/main.rs
--- a/rust/hg-core/examples/nodemap/main.rs
+++ b/rust/hg-core/examples/nodemap/main.rs
@@ -49,7 +49,7 @@ 
 
 fn query(index: &Index, nm: &NodeTree, prefix: &str) {
     let start = Instant::now();
-    let res = nm.find_hex(index, prefix);
+    let res = NodePrefix::from_hex(prefix).map(|p| nm.find_bin(index, p));
     println!("Result found in {:?}: {:?}", start.elapsed(), res);
 }