Patchwork D10555: rust: Remove handling of `parents` in `DirstateMap`

login
register
mail settings
Submitter phabricator
Date May 3, 2021, 10:29 a.m.
Message ID <differential-rev-PHID-DREV-t6bvtkouzi24qcsziuxo-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/48876/
State Superseded
Headers show

Comments

phabricator - May 3, 2021, 10:29 a.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  The Python wrapper class `dirstatemap` can take care of it.
  
  This removes the need to have both `_rustmap` and `_inner_rustmap`.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/dirstate.py
  rust/hg-core/src/dirstate/dirstate_map.rs
  rust/hg-core/src/dirstate_tree/dirstate_map.rs
  rust/hg-core/src/dirstate_tree/dispatch.rs
  rust/hg-cpython/src/dirstate/dirstate_map.rs

CHANGE DETAILS




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

Patch

diff --git a/rust/hg-cpython/src/dirstate/dirstate_map.rs b/rust/hg-cpython/src/dirstate/dirstate_map.rs
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs
@@ -13,8 +13,8 @@ 
 
 use cpython::{
     exc, ObjectProtocol, PyBool, PyBytes, PyClone, PyDict, PyErr, PyList,
-    PyObject, PyResult, PySet, PyString, PyTuple, Python, PythonObject,
-    ToPyObject, UnsafePyLeaked,
+    PyObject, PyResult, PySet, PyString, Python, PythonObject, ToPyObject,
+    UnsafePyLeaked,
 };
 
 use crate::{
@@ -271,27 +271,6 @@ 
             .to_py_object(py))
     }
 
-    def parents(&self, st: PyObject) -> PyResult<PyTuple> {
-        self.inner(py).borrow_mut()
-            .parents(st.extract::<PyBytes>(py)?.data(py))
-            .map(|parents| dirstate_parents_to_pytuple(py, parents))
-            .or_else(|_| {
-                Err(PyErr::new::<exc::OSError, _>(
-                    py,
-                    "Dirstate error".to_string(),
-                ))
-            })
-    }
-
-    def setparents(&self, p1: PyObject, p2: PyObject) -> PyResult<PyObject> {
-        let p1 = extract_node_id(py, &p1)?;
-        let p2 = extract_node_id(py, &p2)?;
-
-        self.inner(py).borrow_mut()
-            .set_parents(&DirstateParents { p1, p2 });
-        Ok(py.None())
-    }
-
     def read(&self, st: PyObject) -> PyResult<Option<PyObject>> {
         match self.inner(py).borrow_mut()
             .read(st.extract::<PyBytes>(py)?.data(py))
diff --git a/rust/hg-core/src/dirstate_tree/dispatch.rs b/rust/hg-core/src/dirstate_tree/dispatch.rs
--- a/rust/hg-core/src/dirstate_tree/dispatch.rs
+++ b/rust/hg-core/src/dirstate_tree/dispatch.rs
@@ -73,13 +73,6 @@ 
         directory: &HgPath,
     ) -> Result<bool, DirstateMapError>;
 
-    fn parents(
-        &mut self,
-        file_contents: &[u8],
-    ) -> Result<&DirstateParents, DirstateError>;
-
-    fn set_parents(&mut self, parents: &DirstateParents);
-
     fn read<'a>(
         &mut self,
         file_contents: &'a [u8],
@@ -223,17 +216,6 @@ 
         self.has_dir(directory)
     }
 
-    fn parents(
-        &mut self,
-        file_contents: &[u8],
-    ) -> Result<&DirstateParents, DirstateError> {
-        self.parents(file_contents)
-    }
-
-    fn set_parents(&mut self, parents: &DirstateParents) {
-        self.set_parents(parents)
-    }
-
     fn read<'a>(
         &mut self,
         file_contents: &'a [u8],
diff --git a/rust/hg-core/src/dirstate_tree/dirstate_map.rs b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
--- a/rust/hg-core/src/dirstate_tree/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate_tree/dirstate_map.rs
@@ -8,10 +8,8 @@ 
 use crate::dirstate::parsers::pack_entry;
 use crate::dirstate::parsers::packed_entry_size;
 use crate::dirstate::parsers::parse_dirstate_entries;
-use crate::dirstate::parsers::parse_dirstate_parents;
 use crate::dirstate::parsers::Timestamp;
 use crate::matchers::Matcher;
-use crate::revlog::node::NULL_NODE;
 use crate::utils::hg_path::{HgPath, HgPathBuf};
 use crate::CopyMapIter;
 use crate::DirstateEntry;
@@ -27,8 +25,6 @@ 
 use crate::StatusOptions;
 
 pub struct DirstateMap {
-    parents: Option<DirstateParents>,
-    dirty_parents: bool,
     pub(super) root: ChildNodes,
 
     /// Number of nodes anywhere in the tree that have `.entry.is_some()`.
@@ -76,8 +72,6 @@ 
 impl DirstateMap {
     pub fn new() -> Self {
         Self {
-            parents: None,
-            dirty_parents: false,
             root: ChildNodes::default(),
             nodes_with_entry_count: 0,
             nodes_with_copy_source_count: 0,
@@ -288,10 +282,6 @@ 
 
 impl super::dispatch::DirstateMapMethods for DirstateMap {
     fn clear(&mut self) {
-        self.set_parents(&DirstateParents {
-            p1: NULL_NODE,
-            p2: NULL_NODE,
-        });
         self.root.clear();
         self.nodes_with_entry_count = 0;
         self.nodes_with_copy_source_count = 0;
@@ -453,29 +443,6 @@ 
         }
     }
 
-    fn parents(
-        &mut self,
-        file_contents: &[u8],
-    ) -> Result<&DirstateParents, DirstateError> {
-        if self.parents.is_none() {
-            let parents = if !file_contents.is_empty() {
-                parse_dirstate_parents(file_contents)?.clone()
-            } else {
-                DirstateParents {
-                    p1: NULL_NODE,
-                    p2: NULL_NODE,
-                }
-            };
-            self.parents = Some(parents);
-        }
-        Ok(self.parents.as_ref().unwrap())
-    }
-
-    fn set_parents(&mut self, parents: &DirstateParents) {
-        self.parents = Some(parents.clone());
-        self.dirty_parents = true;
-    }
-
     #[timed]
     fn read<'a>(
         &mut self,
@@ -515,10 +482,6 @@ 
             },
         )?;
 
-        if !self.dirty_parents {
-            self.set_parents(parents);
-        }
-
         Ok(Some(parents))
     }
 
@@ -554,7 +517,6 @@ 
                 );
             }
         }
-        self.dirty_parents = false;
         Ok(packed)
     }
 
diff --git a/rust/hg-core/src/dirstate/dirstate_map.rs b/rust/hg-core/src/dirstate/dirstate_map.rs
--- a/rust/hg-core/src/dirstate/dirstate_map.rs
+++ b/rust/hg-core/src/dirstate/dirstate_map.rs
@@ -7,10 +7,8 @@ 
 
 use crate::dirstate::parsers::clear_ambiguous_mtime;
 use crate::dirstate::parsers::Timestamp;
-use crate::errors::HgError;
-use crate::revlog::node::NULL_NODE;
 use crate::{
-    dirstate::{parsers::PARENT_SIZE, EntryState},
+    dirstate::EntryState,
     pack_dirstate, parse_dirstate,
     utils::hg_path::{HgPath, HgPathBuf},
     CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateMapError,
@@ -18,7 +16,6 @@ 
 };
 use micro_timer::timed;
 use std::collections::HashSet;
-use std::convert::TryInto;
 use std::iter::FromIterator;
 use std::ops::Deref;
 
@@ -30,8 +27,6 @@ 
     pub all_dirs: Option<DirsMultiset>,
     non_normal_set: Option<HashSet<HgPathBuf>>,
     other_parent_set: Option<HashSet<HgPathBuf>>,
-    parents: Option<DirstateParents>,
-    dirty_parents: bool,
 }
 
 /// Should only really be used in python interface code, for clarity
@@ -64,10 +59,6 @@ 
         self.copy_map.clear();
         self.non_normal_set = None;
         self.other_parent_set = None;
-        self.set_parents(&DirstateParents {
-            p1: NULL_NODE,
-            p2: NULL_NODE,
-        })
     }
 
     /// Add a tracked file to the dirstate
@@ -292,41 +283,6 @@ 
         Ok(self.all_dirs.as_ref().unwrap().contains(directory))
     }
 
-    pub fn parents(
-        &mut self,
-        file_contents: &[u8],
-    ) -> Result<&DirstateParents, DirstateError> {
-        if let Some(ref parents) = self.parents {
-            return Ok(parents);
-        }
-        let parents;
-        if file_contents.len() == PARENT_SIZE * 2 {
-            parents = DirstateParents {
-                p1: file_contents[..PARENT_SIZE].try_into().unwrap(),
-                p2: file_contents[PARENT_SIZE..PARENT_SIZE * 2]
-                    .try_into()
-                    .unwrap(),
-            };
-        } else if file_contents.is_empty() {
-            parents = DirstateParents {
-                p1: NULL_NODE,
-                p2: NULL_NODE,
-            };
-        } else {
-            return Err(
-                HgError::corrupted("Dirstate appears to be damaged").into()
-            );
-        }
-
-        self.parents = Some(parents);
-        Ok(self.parents.as_ref().unwrap())
-    }
-
-    pub fn set_parents(&mut self, parents: &DirstateParents) {
-        self.parents = Some(parents.clone());
-        self.dirty_parents = true;
-    }
-
     #[timed]
     pub fn read<'a>(
         &mut self,
@@ -347,11 +303,6 @@ 
                 .into_iter()
                 .map(|(path, copy)| (path.to_owned(), copy.to_owned())),
         );
-
-        if !self.dirty_parents {
-            self.set_parents(&parents);
-        }
-
         Ok(Some(parents))
     }
 
@@ -363,8 +314,6 @@ 
         let packed =
             pack_dirstate(&mut self.state_map, &self.copy_map, parents, now)?;
 
-        self.dirty_parents = false;
-
         self.set_non_normal_other_parent_entries(true);
         Ok(packed)
     }
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -1750,6 +1750,7 @@ 
             self._opener = opener
             self._root = root
             self._filename = b'dirstate'
+            self._nodelen = 20
             self._parents = None
             self._dirtyparents = False
 
@@ -1778,25 +1779,15 @@ 
         def _rustmap(self):
             """
             Fills the Dirstatemap when called.
-            Use `self._inner_rustmap` if reading the dirstate is not necessary.
-            """
-            self._rustmap = self._inner_rustmap
-            self.read()
-            return self._rustmap
-
-        @propertycache
-        def _inner_rustmap(self):
-            """
-            Does not fill the Dirstatemap when called. This allows for
-            optimizations where only setting/getting the parents is needed.
             """
             use_dirstate_tree = self._ui.configbool(
                 b"experimental",
                 b"dirstate-tree.in-memory",
                 False,
             )
-            self._inner_rustmap = rustmod.DirstateMap(use_dirstate_tree)
-            return self._inner_rustmap
+            self._rustmap = rustmod.DirstateMap(use_dirstate_tree)
+            self.read()
+            return self._rustmap
 
         @property
         def copymap(self):
@@ -1807,7 +1798,6 @@ 
 
         def clear(self):
             self._rustmap.clear()
-            self._inner_rustmap.clear()
             self.setparents(
                 self._nodeconstants.nullid, self._nodeconstants.nullid
             )
@@ -1849,7 +1839,6 @@ 
             return fp
 
         def setparents(self, p1, p2):
-            self._rustmap.setparents(p1, p2)
             self._parents = (p1, p2)
             self._dirtyparents = True
 
@@ -1865,9 +1854,18 @@ 
                     # File doesn't exist, so the current state is empty
                     st = b''
 
-                try:
-                    self._parents = self._inner_rustmap.parents(st)
-                except ValueError:
+                l = len(st)
+                if l == self._nodelen * 2:
+                    self._parents = (
+                        st[: self._nodelen],
+                        st[self._nodelen : 2 * self._nodelen],
+                    )
+                elif l == 0:
+                    self._parents = (
+                        self._nodeconstants.nullid,
+                        self._nodeconstants.nullid,
+                    )
+                else:
                     raise error.Abort(
                         _(b'working directory state appears damaged!')
                     )