dirstate: Remove the Rust abstraction DirstateMapMethods

This Rust trait used to exist in order to allow the DirstateMap class exposed
to Python to be backed by either of two implementations: one similar to the
Python implementation based on a "flat" `HashMap<HgPathBuf, DirstateEntry>`,
and the newer one based on a tree of nodes matching the directory structure
of tracked files. A boxed trait object was used with dynamic dispatch.

With the flat implementation removed and only the tree one remaining, this
abstraction is not useful anymore and the concrete type can be stored directly.

It remains that the trait was implemented separately for `DirstateMap<'_>`
(which takes a lifetime parameter) and `OwningDirstateMap` (whose job is to
wrap the former and hide the lifetime parameter), with the latter impl only
forwarding calls.

This changeset also removes this forwarding. Instead, the methods formerly of
the `DirstateMapMethods` trait are now inherent methods implemented for
`OwningDirstateMap` (where they will actually be used) but in the module that
defines `DirstateMap`. This unusual setup gives access to the private fields
of `DirstateMap` from those methods.

Differential Revision: https://phab.mercurial-scm.org/D11517

rust/hg-core/src/dirstate_tree.rs

@@ -1,7 +1,5 b''
 pub mod dirstate_map;
-pub mod dispatch;
 pub mod on_disk;
 pub mod owning;
-mod owning_dispatch;
 pub mod path_with_basename;
 pub mod status;

rust/hg-core/src/dirstate_tree/dirstate_map.rs

@@ -6,6 +6,7 b' use std::path::PathBuf;'
 
 use super::on_disk;
 use super::on_disk::DirstateV2ParseError;
+use super::owning::OwningDirstateMap;
 use super::path_with_basename::WithBasename;
 use crate::dirstate::parsers::pack_entry;
 use crate::dirstate::parsers::packed_entry_size;
@@ -728,32 +729,35 b' where'
     })
 }
 
-impl<'on_disk> super::dispatch::DirstateMapMethods for DirstateMap<'on_disk> {
-    fn clear(&mut self) {
-        self.root = Default::default();
-        self.nodes_with_entry_count = 0;
-        self.nodes_with_copy_source_count = 0;
+impl OwningDirstateMap {
+    pub fn clear(&mut self) {
+        let map = self.get_map_mut();
+        map.root = Default::default();
+        map.nodes_with_entry_count = 0;
+        map.nodes_with_copy_source_count = 0;
     }
 
-    fn set_entry(
+    pub fn set_entry(
         &mut self,
         filename: &HgPath,
         entry: DirstateEntry,
     ) -> Result<(), DirstateV2ParseError> {
-        self.get_or_insert(&filename)?.data = NodeData::Entry(entry);
+        let map = self.get_map_mut();
+        map.get_or_insert(&filename)?.data = NodeData::Entry(entry);
         Ok(())
     }
 
-    fn add_file(
+    pub fn add_file(
         &mut self,
         filename: &HgPath,
         entry: DirstateEntry,
     ) -> Result<(), DirstateError> {
         let old_state = self.get(filename)?.map(|e| e.state());
-        Ok(self.add_or_remove_file(filename, old_state, entry)?)
+        let map = self.get_map_mut();
+        Ok(map.add_or_remove_file(filename, old_state, entry)?)
     }
 
-    fn remove_file(
+    pub fn remove_file(
         &mut self,
         filename: &HgPath,
         in_merge: bool,
@@ -781,17 +785,19 b" impl<'on_disk> super::dispatch::Dirstate"
         if size == 0 {
             self.copy_map_remove(filename)?;
         }
+        let map = self.get_map_mut();
         let entry = DirstateEntry::new_removed(size);
-        Ok(self.add_or_remove_file(filename, old_state, entry)?)
+        Ok(map.add_or_remove_file(filename, old_state, entry)?)
     }
 
-    fn drop_entry_and_copy_source(
+    pub fn drop_entry_and_copy_source(
         &mut self,
         filename: &HgPath,
     ) -> Result<(), DirstateError> {
         let was_tracked = self
             .get(filename)?
             .map_or(false, |e| e.state().is_tracked());
+        let map = self.get_map_mut();
         struct Dropped {
             was_tracked: bool,
             had_entry: bool,
@@ -879,16 +885,16 b" impl<'on_disk> super::dispatch::Dirstate"
         }
 
         if let Some((dropped, _removed)) = recur(
-            self.on_disk,
-            &mut self.unreachable_bytes,
-            &mut self.root,
+            map.on_disk,
+            &mut map.unreachable_bytes,
+            &mut map.root,
             filename,
         )? {
             if dropped.had_entry {
-                self.nodes_with_entry_count -= 1
+                map.nodes_with_entry_count -= 1
             }
             if dropped.had_copy_source {
-                self.nodes_with_copy_source_count -= 1
+                map.nodes_with_copy_source_count -= 1
             }
         } else {
             debug_assert!(!was_tracked);
@@ -896,11 +902,12 b" impl<'on_disk> super::dispatch::Dirstate"
         Ok(())
     }
 
-    fn has_tracked_dir(
+    pub fn has_tracked_dir(
         &mut self,
         directory: &HgPath,
     ) -> Result<bool, DirstateError> {
-        if let Some(node) = self.get_node(directory)? {
+        let map = self.get_map_mut();
+        if let Some(node) = map.get_node(directory)? {
             // A node without a `DirstateEntry` was created to hold child
             // nodes, and is therefore a directory.
             let state = node.state()?;
@@ -910,8 +917,12 b" impl<'on_disk> super::dispatch::Dirstate"
         }
     }
 
-    fn has_dir(&mut self, directory: &HgPath) -> Result<bool, DirstateError> {
-        if let Some(node) = self.get_node(directory)? {
+    pub fn has_dir(
+        &mut self,
+        directory: &HgPath,
+    ) -> Result<bool, DirstateError> {
+        let map = self.get_map_mut();
+        if let Some(node) = map.get_node(directory)? {
             // A node without a `DirstateEntry` was created to hold child
             // nodes, and is therefore a directory.
             let state = node.state()?;
@@ -922,43 +933,44 b" impl<'on_disk> super::dispatch::Dirstate"
     }
 
     #[timed]
-    fn pack_v1(
+    pub fn pack_v1(
         &mut self,
         parents: DirstateParents,
         now: Timestamp,
     ) -> Result<Vec<u8>, DirstateError> {
+        let map = self.get_map_mut();
         let now: i32 = now.0.try_into().expect("time overflow");
         let mut ambiguous_mtimes = Vec::new();
         // Optizimation (to be measured?): pre-compute size to avoid `Vec`
         // reallocations
         let mut size = parents.as_bytes().len();
-        for node in self.iter_nodes() {
+        for node in map.iter_nodes() {
             let node = node?;
             if let Some(entry) = node.entry()? {
                 size += packed_entry_size(
-                    node.full_path(self.on_disk)?,
-                    node.copy_source(self.on_disk)?,
+                    node.full_path(map.on_disk)?,
+                    node.copy_source(map.on_disk)?,
                 );
                 if entry.mtime_is_ambiguous(now) {
                     ambiguous_mtimes.push(
-                        node.full_path_borrowed(self.on_disk)?
+                        node.full_path_borrowed(map.on_disk)?
                             .detach_from_tree(),
                     )
                 }
             }
         }
-        self.clear_known_ambiguous_mtimes(&ambiguous_mtimes)?;
+        map.clear_known_ambiguous_mtimes(&ambiguous_mtimes)?;
 
         let mut packed = Vec::with_capacity(size);
         packed.extend(parents.as_bytes());
 
-        for node in self.iter_nodes() {
+        for node in map.iter_nodes() {
             let node = node?;
             if let Some(entry) = node.entry()? {
                 pack_entry(
-                    node.full_path(self.on_disk)?,
+                    node.full_path(map.on_disk)?,
                     &entry,
-                    node.copy_source(self.on_disk)?,
+                    node.copy_source(map.on_disk)?,
                     &mut packed,
                 );
             }
@@ -968,23 +980,24 b" impl<'on_disk> super::dispatch::Dirstate"
 
     /// Returns new data and metadata together with whether that data should be
     /// appended to the existing data file whose content is at
-    /// `self.on_disk` (true), instead of written to a new data file
+    /// `map.on_disk` (true), instead of written to a new data file
     /// (false).
     #[timed]
-    fn pack_v2(
+    pub fn pack_v2(
         &mut self,
         now: Timestamp,
         can_append: bool,
     ) -> Result<(Vec<u8>, Vec<u8>, bool), DirstateError> {
+        let map = self.get_map_mut();
         // TODO: how do we want to handle this in 2038?
         let now: i32 = now.0.try_into().expect("time overflow");
         let mut paths = Vec::new();
-        for node in self.iter_nodes() {
+        for node in map.iter_nodes() {
             let node = node?;
             if let Some(entry) = node.entry()? {
                 if entry.mtime_is_ambiguous(now) {
                     paths.push(
-                        node.full_path_borrowed(self.on_disk)?
+                        node.full_path_borrowed(map.on_disk)?
                             .detach_from_tree(),
                     )
                 }
@@ -992,12 +1005,12 b" impl<'on_disk> super::dispatch::Dirstate"
         }
         // Borrow of `self` ends here since we collect cloned paths
 
-        self.clear_known_ambiguous_mtimes(&paths)?;
+        map.clear_known_ambiguous_mtimes(&paths)?;
 
-        on_disk::write(self, can_append)
+        on_disk::write(map, can_append)
     }
 
-    fn status<'a>(
+    pub fn status<'a>(
         &'a mut self,
         matcher: &'a (dyn Matcher + Sync),
         root_dir: PathBuf,
@@ -1005,119 +1018,129 b" impl<'on_disk> super::dispatch::Dirstate"
         options: StatusOptions,
     ) -> Result<(DirstateStatus<'a>, Vec<PatternFileWarning>), StatusError>
     {
-        super::status::status(self, matcher, root_dir, ignore_files, options)
+        let map = self.get_map_mut();
+        super::status::status(map, matcher, root_dir, ignore_files, options)
     }
 
-    fn copy_map_len(&self) -> usize {
-        self.nodes_with_copy_source_count as usize
+    pub fn copy_map_len(&self) -> usize {
+        let map = self.get_map();
+        map.nodes_with_copy_source_count as usize
     }
 
-    fn copy_map_iter(&self) -> CopyMapIter<'_> {
-        Box::new(filter_map_results(self.iter_nodes(), move |node| {
-            Ok(if let Some(source) = node.copy_source(self.on_disk)? {
-                Some((node.full_path(self.on_disk)?, source))
+    pub fn copy_map_iter(&self) -> CopyMapIter<'_> {
+        let map = self.get_map();
+        Box::new(filter_map_results(map.iter_nodes(), move |node| {
+            Ok(if let Some(source) = node.copy_source(map.on_disk)? {
+                Some((node.full_path(map.on_disk)?, source))
             } else {
                 None
             })
         }))
     }
 
-    fn copy_map_contains_key(
+    pub fn copy_map_contains_key(
         &self,
         key: &HgPath,
     ) -> Result<bool, DirstateV2ParseError> {
-        Ok(if let Some(node) = self.get_node(key)? {
+        let map = self.get_map();
+        Ok(if let Some(node) = map.get_node(key)? {
             node.has_copy_source()
         } else {
             false
         })
     }
 
-    fn copy_map_get(
+    pub fn copy_map_get(
         &self,
         key: &HgPath,
     ) -> Result<Option<&HgPath>, DirstateV2ParseError> {
-        if let Some(node) = self.get_node(key)? {
-            if let Some(source) = node.copy_source(self.on_disk)? {
+        let map = self.get_map();
+        if let Some(node) = map.get_node(key)? {
+            if let Some(source) = node.copy_source(map.on_disk)? {
                 return Ok(Some(source));
             }
         }
         Ok(None)
     }
 
-    fn copy_map_remove(
+    pub fn copy_map_remove(
         &mut self,
         key: &HgPath,
     ) -> Result<Option<HgPathBuf>, DirstateV2ParseError> {
-        let count = &mut self.nodes_with_copy_source_count;
-        let unreachable_bytes = &mut self.unreachable_bytes;
-        Ok(Self::get_node_mut(
-            self.on_disk,
+        let map = self.get_map_mut();
+        let count = &mut map.nodes_with_copy_source_count;
+        let unreachable_bytes = &mut map.unreachable_bytes;
+        Ok(DirstateMap::get_node_mut(
+            map.on_disk,
             unreachable_bytes,
-            &mut self.root,
+            &mut map.root,
             key,
         )?
         .and_then(|node| {
             if let Some(source) = &node.copy_source {
                 *count -= 1;
-                Self::count_dropped_path(unreachable_bytes, source);
+                DirstateMap::count_dropped_path(unreachable_bytes, source);
             }
             node.copy_source.take().map(Cow::into_owned)
         }))
     }
 
-    fn copy_map_insert(
+    pub fn copy_map_insert(
         &mut self,
         key: HgPathBuf,
         value: HgPathBuf,
     ) -> Result<Option<HgPathBuf>, DirstateV2ParseError> {
-        let node = Self::get_or_insert_node(
-            self.on_disk,
-            &mut self.unreachable_bytes,
-            &mut self.root,
+        let map = self.get_map_mut();
+        let node = DirstateMap::get_or_insert_node(
+            map.on_disk,
+            &mut map.unreachable_bytes,
+            &mut map.root,
             &key,
             WithBasename::to_cow_owned,
             |_ancestor| {},
         )?;
         if node.copy_source.is_none() {
-            self.nodes_with_copy_source_count += 1
+            map.nodes_with_copy_source_count += 1
         }
         Ok(node.copy_source.replace(value.into()).map(Cow::into_owned))
     }
 
-    fn len(&self) -> usize {
-        self.nodes_with_entry_count as usize
+    pub fn len(&self) -> usize {
+        let map = self.get_map();
+        map.nodes_with_entry_count as usize
     }
 
-    fn contains_key(
+    pub fn contains_key(
         &self,
         key: &HgPath,
     ) -> Result<bool, DirstateV2ParseError> {
         Ok(self.get(key)?.is_some())
     }
 
-    fn get(
+    pub fn get(
         &self,
         key: &HgPath,
     ) -> Result<Option<DirstateEntry>, DirstateV2ParseError> {
-        Ok(if let Some(node) = self.get_node(key)? {
+        let map = self.get_map();
+        Ok(if let Some(node) = map.get_node(key)? {
             node.entry()?
         } else {
             None
         })
     }
 
-    fn iter(&self) -> StateMapIter<'_> {
-        Box::new(filter_map_results(self.iter_nodes(), move |node| {
+    pub fn iter(&self) -> StateMapIter<'_> {
+        let map = self.get_map();
+        Box::new(filter_map_results(map.iter_nodes(), move |node| {
             Ok(if let Some(entry) = node.entry()? {
-                Some((node.full_path(self.on_disk)?, entry))
+                Some((node.full_path(map.on_disk)?, entry))
             } else {
                 None
             })
         }))
     }
 
-    fn iter_tracked_dirs(
+    pub fn iter_tracked_dirs(
         &mut self,
     ) -> Result<
         Box<
@@ -1127,9 +1150,10 b" impl<'on_disk> super::dispatch::Dirstate"
         >,
         DirstateError,
     > {
-        let on_disk = self.on_disk;
+        let map = self.get_map_mut();
+        let on_disk = map.on_disk;
         Ok(Box::new(filter_map_results(
-            self.iter_nodes(),
+            map.iter_nodes(),
             move |node| {
                 Ok(if node.tracked_descendants_count() > 0 {
                     Some(node.full_path(on_disk)?)
@@ -1140,7 +1164,7 b" impl<'on_disk> super::dispatch::Dirstate"
         )))
     }
 
-    fn debug_iter(
+    pub fn debug_iter(
         &self,
         all: bool,
     ) -> Box<
@@ -1152,7 +1176,8 b" impl<'on_disk> super::dispatch::Dirstate"
             > + Send
             + '_,
     > {
-        Box::new(filter_map_results(self.iter_nodes(), move |node| {
+        let map = self.get_map();
+        Box::new(filter_map_results(map.iter_nodes(), move |node| {
             let debug_tuple = if let Some(entry) = node.entry()? {
                 entry.debug_tuple()
             } else if !all {
@@ -1162,7 +1187,7 b" impl<'on_disk> super::dispatch::Dirstate"
             } else {
                 (b' ', 0, -1, -1)
             };
-            Ok(Some((node.full_path(self.on_disk)?, debug_tuple)))
+            Ok(Some((node.full_path(map.on_disk)?, debug_tuple)))
         }))
     }
 }

rust/hg-core/src/dirstate_tree/owning.rs

@@ -44,7 +44,7 b' impl OwningDirstateMap {'
         Self { on_disk, ptr }
     }
 
-    pub fn get_mut_pair<'a>(
+    pub fn get_pair_mut<'a>(
         &'a mut self,
     ) -> (&'a [u8], &'a mut DirstateMap<'a>) {
         // SAFETY: We cast the type-erased pointer back to the same type it had
@@ -60,11 +60,11 b' impl OwningDirstateMap {'
         (&self.on_disk, unsafe { &mut *ptr })
     }
 
-    pub fn get_mut<'a>(&'a mut self) -> &'a mut DirstateMap<'a> {
-        self.get_mut_pair().1
+    pub fn get_map_mut<'a>(&'a mut self) -> &'a mut DirstateMap<'a> {
+        self.get_pair_mut().1
     }
 
-    pub fn get<'a>(&'a self) -> &'a DirstateMap<'a> {
+    pub fn get_map<'a>(&'a self) -> &'a DirstateMap<'a> {
         // SAFETY: same reasoning as in `get_mut` above.
         let ptr: *mut DirstateMap<'a> = self.ptr.cast();
         unsafe { &*ptr }

rust/hg-core/src/repo.rs

@@ -294,12 +294,12 b' impl Repo {'
             } else {
                 OwningDirstateMap::new_empty(Vec::new())
             };
-            let (on_disk, placeholder) = map.get_mut_pair();
+            let (on_disk, placeholder) = map.get_pair_mut();
             *placeholder = DirstateMap::new_v2(on_disk, data_size, metadata)?;
             Ok(map)
         } else {
             let mut map = OwningDirstateMap::new_empty(dirstate_file_contents);
-            let (on_disk, placeholder) = map.get_mut_pair();
+            let (on_disk, placeholder) = map.get_pair_mut();
             let (inner, parents) = DirstateMap::new_v1(on_disk)?;
             self.dirstate_parents
                 .set(Some(parents.unwrap_or(DirstateParents::NULL)));

rust/hg-cpython/src/dirstate/dirstate_map.rs

@@ -25,7 +25,6 b' use hg::{'
     dirstate::parsers::Timestamp,
     dirstate::StateMapIter,
     dirstate_tree::dirstate_map::DirstateMap as TreeDirstateMap,
-    dirstate_tree::dispatch::DirstateMapMethods,
     dirstate_tree::on_disk::DirstateV2ParseError,
     dirstate_tree::owning::OwningDirstateMap,
     revlog::Node,
@@ -47,7 +46,7 b' use hg::{'
 //     All attributes also have to have a separate refcount data attribute for
 //     leaks, with all methods that go along for reference sharing.
 py_class!(pub class DirstateMap |py| {
-    @shared data inner: Box<dyn DirstateMapMethods + Send>;
+    @shared data inner: OwningDirstateMap;
 
     /// Returns a `(dirstate_map, parents)` tuple
     @staticmethod
@@ -56,12 +55,12 b' py_class!(pub class DirstateMap |py| {'
     ) -> PyResult<PyObject> {
         let on_disk = PyBytesDeref::new(py, on_disk);
         let mut map = OwningDirstateMap::new_empty(on_disk);
-        let (on_disk, map_placeholder) = map.get_mut_pair();
+        let (on_disk, map_placeholder) = map.get_pair_mut();
 
         let (actual_map, parents) = TreeDirstateMap::new_v1(on_disk)
             .map_err(|e| dirstate_error(py, e))?;
         *map_placeholder = actual_map;
-        let map = Self::create_instance(py, Box::new(map))?;
+        let map = Self::create_instance(py, map)?;
         let parents = parents.map(|p| {
             let p1 = PyBytes::new(py, p.p1.as_bytes());
             let p2 = PyBytes::new(py, p.p2.as_bytes());
@@ -82,11 +81,11 b' py_class!(pub class DirstateMap |py| {'
         };
         let on_disk = PyBytesDeref::new(py, on_disk);
         let mut map = OwningDirstateMap::new_empty(on_disk);
-        let (on_disk, map_placeholder) = map.get_mut_pair();
+        let (on_disk, map_placeholder) = map.get_pair_mut();
         *map_placeholder = TreeDirstateMap::new_v2(
             on_disk, data_size, tree_metadata.data(py),
         ).map_err(dirstate_error)?;
-        let map = Self::create_instance(py, Box::new(map))?;
+        let map = Self::create_instance(py, map)?;
         Ok(map.into_object())
     }
 
@@ -452,7 +451,7 b' impl DirstateMap {'
     pub fn get_inner_mut<'a>(
         &'a self,
         py: Python<'a>,
-    ) -> RefMut<'a, Box<dyn DirstateMapMethods + Send>> {
+    ) -> RefMut<'a, OwningDirstateMap> {
         self.inner(py).borrow_mut()
     }
     fn translate_key(

rust/rhg/src/commands/status.rs

@@ -9,7 +9,6 b' use crate::error::CommandError;'
 use crate::ui::Ui;
 use clap::{Arg, SubCommand};
 use hg;
-use hg::dirstate_tree::dispatch::DirstateMapMethods;
 use hg::errors::HgError;
 use hg::manifest::Manifest;
 use hg::matchers::AlwaysMatcher;