changeset 52068:0cd16b1d613f

rust-dirstate: use a struct as arguments for the high-level `reset_state` This makes the interface a lot clearer at the call site and prevents silly mistakes, as an API with a bunch of booleans is prone to errors. This refactor adds a `from_empty` parameter for a fast-path when resetting and entry we're sure does not exist. It will be used in the upcoming update Rust fastpath, and was not split to prevent more churn.
author Raphaël Gomès <rgomes@octobus.net>
date Mon, 30 Sep 2024 17:19:35 +0200
parents ae1ab6d71f4a
children 652149ed64f0
files rust/hg-core/src/dirstate_tree/dirstate_map.rs rust/hg-cpython/src/dirstate/dirstate_map.rs
diffstat 2 files changed, 199 insertions(+), 78 deletions(-) [+]
line wrap: on
line diff
--- a/rust/hg-core/src/dirstate_tree/dirstate_map.rs	Mon Sep 30 16:55:11 2024 +0200
+++ b/rust/hg-core/src/dirstate_tree/dirstate_map.rs	Mon Sep 30 17:19:35 2024 +0200
@@ -962,6 +962,26 @@
     }
 }
 
+/// Sets the parameters for resetting a dirstate entry
+pub struct DirstateEntryReset<'a> {
+    /// Which entry are we resetting
+    pub filename: &'a HgPath,
+    /// Whether the entry is tracked in the working copy
+    pub wc_tracked: bool,
+    /// Whether the entry is tracked in p1
+    pub p1_tracked: bool,
+    /// Whether the entry has merge information
+    pub p2_info: bool,
+    /// Whether the entry's mtime should be trusted
+    pub has_meaningful_mtime: bool,
+    /// Information from the parent file data (from the manifest)
+    pub parent_file_data_opt: Option<ParentFileData>,
+    /// Set this to `true` if you are *certain* that there is no old entry for
+    /// this filename. Yield better performance in cases where we do a lot
+    /// of additions to the dirstate.
+    pub from_empty: bool,
+}
+
 type DebugDirstateTuple<'a> = (&'a HgPath, (u8, i32, i32, i32));
 
 impl OwningDirstateMap {
@@ -1048,28 +1068,31 @@
 
     pub fn reset_state(
         &mut self,
-        filename: &HgPath,
-        wc_tracked: bool,
-        p1_tracked: bool,
-        p2_info: bool,
-        has_meaningful_mtime: bool,
-        parent_file_data_opt: Option<ParentFileData>,
+        reset: DirstateEntryReset,
     ) -> Result<(), DirstateError> {
-        if !(p1_tracked || p2_info || wc_tracked) {
-            self.drop_entry_and_copy_source(filename)?;
+        if !(reset.p1_tracked || reset.p2_info || reset.wc_tracked) {
+            self.drop_entry_and_copy_source(reset.filename)?;
             return Ok(());
         }
-        self.copy_map_remove(filename)?;
-        let old_entry_opt = self.get(filename)?;
+        if !reset.from_empty {
+            self.copy_map_remove(reset.filename)?;
+        }
+
+        let old_entry_opt = if reset.from_empty {
+            None
+        } else {
+            self.get(reset.filename)?
+        };
+
         self.with_dmap_mut(|map| {
             map.reset_state(
-                filename,
+                reset.filename,
                 old_entry_opt,
-                wc_tracked,
-                p1_tracked,
-                p2_info,
-                has_meaningful_mtime,
-                parent_file_data_opt,
+                reset.wc_tracked,
+                reset.p1_tracked,
+                reset.p2_info,
+                reset.has_meaningful_mtime,
+                reset.parent_file_data_opt,
             )
         })
     }
@@ -1643,39 +1666,79 @@
         // A file that was just added
         map.set_tracked(p(b"some/nested/path"))?;
         // This has no information, the dirstate should ignore it
-        map.reset_state(p(b"some/file"), false, false, false, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/file"),
+            wc_tracked: false,
+            p1_tracked: false,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert_does_not_exist(&map, b"some/file");
 
         // A file that was removed
-        map.reset_state(
-            p(b"some/nested/file"),
-            false,
-            true,
-            false,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/nested/file"),
+            wc_tracked: false,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert!(!map.get(p(b"some/nested/file"))?.unwrap().tracked());
         // Only present in p2
-        map.reset_state(p(b"some/file3"), false, false, true, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/file3"),
+            wc_tracked: false,
+            p1_tracked: false,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert!(!map.get(p(b"some/file3"))?.unwrap().tracked());
         // A file that was merged
-        map.reset_state(p(b"root_file"), true, true, true, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"root_file"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert!(map.get(p(b"root_file"))?.unwrap().tracked());
         // A file that is added, with info from p2
         // XXX is that actually possible?
-        map.reset_state(p(b"some/file2"), true, false, true, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/file2"),
+            wc_tracked: true,
+            p1_tracked: false,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert!(map.get(p(b"some/file2"))?.unwrap().tracked());
         // A clean file
         // One layer without any files to test deletion cascade
-        map.reset_state(
-            p(b"some/other/nested/path"),
-            true,
-            true,
-            false,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/other/nested/path"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         assert!(map.get(p(b"some/other/nested/path"))?.unwrap().tracked());
 
         assert_eq!(map.len(), 6);
@@ -1738,13 +1801,49 @@
         let mut map = OwningDirstateMap::new_empty(vec![], None);
 
         // Clean file
-        map.reset_state(p(b"files/clean"), true, true, false, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"files/clean"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // Merged file
-        map.reset_state(p(b"files/from_p2"), true, true, true, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"files/from_p2"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // Removed file
-        map.reset_state(p(b"removed"), false, true, false, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"removed"),
+            wc_tracked: false,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // Added file
-        map.reset_state(p(b"files/added"), true, false, false, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"files/added"),
+            wc_tracked: true,
+            p1_tracked: false,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // Add copy
         map.copy_map_insert(p(b"files/clean"), p(b"clean_copy_source"))?;
         assert_eq!(map.copy_map_len(), 1);
@@ -1799,49 +1898,66 @@
         map.copy_map_insert(p(b"some/nested/added"), p(b"added_copy_source"))?;
 
         // A file that was removed
-        map.reset_state(
-            p(b"some/nested/removed"),
-            false,
-            true,
-            false,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/nested/removed"),
+            wc_tracked: false,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // Only present in p2
-        map.reset_state(
-            p(b"other/p2_info_only"),
-            false,
-            false,
-            true,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"other/p2_info_only"),
+            wc_tracked: false,
+            p1_tracked: false,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         map.copy_map_insert(
             p(b"other/p2_info_only"),
             p(b"other/p2_info_copy_source"),
         )?;
         // A file that was merged
-        map.reset_state(p(b"merged"), true, true, true, false, None)?;
+        let reset = DirstateEntryReset {
+            filename: p(b"merged"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // A file that is added, with info from p2
         // XXX is that actually possible?
-        map.reset_state(
-            p(b"other/added_with_p2"),
-            true,
-            false,
-            true,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"other/added_with_p2"),
+            wc_tracked: true,
+            p1_tracked: false,
+            p2_info: true,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
         // One layer without any files to test deletion cascade
         // A clean file
-        map.reset_state(
-            p(b"some/other/nested/clean"),
-            true,
-            true,
-            false,
-            false,
-            None,
-        )?;
+        let reset = DirstateEntryReset {
+            filename: p(b"some/other/nested/clean"),
+            wc_tracked: true,
+            p1_tracked: true,
+            p2_info: false,
+            has_meaningful_mtime: false,
+            parent_file_data_opt: None,
+            from_empty: false,
+        };
+        map.reset_state(reset)?;
 
         let (packed, metadata, _should_append, _old_data_size) =
             map.pack_v2(DirstateMapWriteMode::ForceNewDataFile)?;
--- a/rust/hg-cpython/src/dirstate/dirstate_map.rs	Mon Sep 30 16:55:11 2024 +0200
+++ b/rust/hg-cpython/src/dirstate/dirstate_map.rs	Mon Sep 30 17:19:35 2024 +0200
@@ -14,7 +14,10 @@
     exc, PyBool, PyBytes, PyClone, PyDict, PyErr, PyList, PyNone, PyObject,
     PyResult, Python, PythonObject, ToPyObject, UnsafePyLeaked,
 };
-use hg::dirstate::{ParentFileData, TruncatedTimestamp};
+use hg::{
+    dirstate::{ParentFileData, TruncatedTimestamp},
+    dirstate_tree::dirstate_map::DirstateEntryReset,
+};
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
@@ -196,14 +199,16 @@
         };
         let bytes = f.extract::<PyBytes>(py)?;
         let path = HgPath::new(bytes.data(py));
-        let res = self.inner(py).borrow_mut().reset_state(
-            path,
+        let reset = DirstateEntryReset {
+            filename: path,
             wc_tracked,
             p1_tracked,
             p2_info,
             has_meaningful_mtime,
-            parent_file_data,
-        );
+            parent_file_data_opt: parent_file_data,
+            from_empty: false
+        };
+        let res = self.inner(py).borrow_mut().reset_state(reset);
         res.map_err(|_| PyErr::new::<exc::OSError, _>(py, "Dirstate error".to_string()))?;
         Ok(PyNone)
     }