Patchwork D11703: rust-nodemap: backed out mitigation for issue 6554

login
register
mail settings
Submitter phabricator
Date Oct. 19, 2021, 10:46 p.m.
Message ID <differential-rev-PHID-DREV-wuvd5dlcnyd2dzjxthng-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/50027/
State Superseded
Headers show

Comments

phabricator - Oct. 19, 2021, 10:46 p.m.
gracinet created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  This is a backout of changeset 3fffb48539ee <https://phab.mercurial-scm.org/rHG3fffb48539ee1ef4ff2cf584fcb506fcbcb3b616>.
  
  Issue 6554 is now considered solved, hence its mitigation
  has to be removed, if only for its performance cost.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-cpython/src/revlog.rs
  tests/test-persistent-nodemap.t

CHANGE DETAILS




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

Patch

diff --git a/tests/test-persistent-nodemap.t b/tests/test-persistent-nodemap.t
--- a/tests/test-persistent-nodemap.t
+++ b/tests/test-persistent-nodemap.t
@@ -435,46 +435,6 @@ 
   data-length: 121088
   data-unused: 0
   data-unused: 0.000%
-
-Sub-case: fallback for corrupted data file
-------------------------------------------
-
-Sabotaging the data file so that nodemap resolutions fail, triggering fallback to
-(non-persistent) C implementation.
-
-
-  $ UUID=`hg debugnodemap --metadata| grep 'uid:' | \
-  > sed 's/uid: //'`
-  $ FILE=.hg/store/00changelog-"${UUID}".nd
-  $ python -c "fobj = open('$FILE', 'r+b'); fobj.write(b'\xff' * 121088); fobj.close()"
-
-The nodemap data file is still considered in sync with the docket. This
-would fail without the fallback to the (non-persistent) C implementation:
-
-  $ hg log -r b355ef8adce0949b8bdf6afc72ca853740d65944 -T '{rev}\n' --traceback
-  5002
-
-The nodemap data file hasn't been fixed, more tests can be inserted:
-
-  $ hg debugnodemap --dump-disk | f --bytes=256 --hexdump --size
-  size=121088
-  0000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0060: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0070: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  0090: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-  00f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
-
   $ mv ../tmp-data-file $FILE
   $ mv ../tmp-docket .hg/store/00changelog.n
 
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
@@ -59,22 +59,12 @@ 
 
     /// Return Revision if found, raises a bare `error.RevlogError`
     /// in case of ambiguity, same as C version does
-    def get_rev(&self, pynode: PyBytes) -> PyResult<Option<Revision>> {
+    def get_rev(&self, node: PyBytes) -> PyResult<Option<Revision>> {
         let opt = self.get_nodetree(py)?.borrow();
         let nt = opt.as_ref().unwrap();
         let idx = &*self.cindex(py).borrow();
-        let node = node_from_py_bytes(py, &pynode)?;
-        match nt.find_bin(idx, node.into())
-        {
-            Ok(None) =>
-                // fallback to C implementation, remove once
-                // https://bz.mercurial-scm.org/show_bug.cgi?id=6554
-                // is fixed (a simple backout should do)
-                self.call_cindex(py, "get_rev", &PyTuple::new(py, &[pynode.into_object()]), None)?
-                .extract(py),
-            Ok(Some(rev)) => Ok(Some(rev)),
-            Err(e) => Err(nodemap_error(py, e)),
-        }
+        let node = node_from_py_bytes(py, &node)?;
+        nt.find_bin(idx, node.into()).map_err(|e| nodemap_error(py, e))
     }
 
     /// same as `get_rev()` but raises a bare `error.RevlogError` if node
@@ -104,34 +94,27 @@ 
         }
     }
 
-    def partialmatch(&self, pynode: PyObject) -> PyResult<Option<PyBytes>> {
+    def partialmatch(&self, node: PyObject) -> PyResult<Option<PyBytes>> {
         let opt = self.get_nodetree(py)?.borrow();
         let nt = opt.as_ref().unwrap();
         let idx = &*self.cindex(py).borrow();
 
         let node_as_string = if cfg!(feature = "python3-sys") {
-            pynode.cast_as::<PyString>(py)?.to_string(py)?.to_string()
+            node.cast_as::<PyString>(py)?.to_string(py)?.to_string()
         }
         else {
-            let node = pynode.extract::<PyBytes>(py)?;
+            let node = node.extract::<PyBytes>(py)?;
             String::from_utf8_lossy(node.data(py)).to_string()
         };
 
         let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?;
 
-        match nt.find_bin(idx, prefix) {
-            Ok(None) =>
-                // fallback to C implementation, remove once
-                // https://bz.mercurial-scm.org/show_bug.cgi?id=6554
-                // is fixed (a simple backout should do)
-                self.call_cindex(
-                    py, "partialmatch",
-                    &PyTuple::new(py, &[pynode]), None
-                )?.extract(py),
-            Ok(Some(rev)) =>
-                Ok(Some(PyBytes::new(py, idx.node(rev).unwrap().as_bytes()))),
-            Err(e) => Err(nodemap_error(py, e)),
-        }
+        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())))
+            .map_err(|e| nodemap_error(py, e))
+
     }
 
     /// append an index entry