Patchwork D11099: dirstate-v2: Separate iterators for dirfoldmap and debugdirstate

login
register
mail settings
Submitter phabricator
Date July 16, 2021, 12:40 p.m.
Message ID <differential-rev-PHID-DREV-za4mmloc32m5prjvbwbk-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49413/
State Superseded
Headers show

Comments

phabricator - July 16, 2021, 12:40 p.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  `dirstatemap.dirfoldmap` was recently changed to re-use a Rust iterator
  that was added for the `hg debugdirstate` command.
  
  That iterator was based on all nodes in the tree dirstate without an entry
  only existing to hold child nodes, and therefore being directories.
  However to optimize status further we may want to store additional nodes
  for unknown or ignored files and directories. At that point the two users
  of this iterator will want different things, so let’s make two iterators
  instead.
  
  See doc-comments in `dispatch.rs`.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/debugcommands.py
  mercurial/dirstatemap.py
  rust/hg-core/src/dirstate.rs
  rust/hg-core/src/dirstate_tree/dirstate_map.rs
  rust/hg-core/src/dirstate_tree/dispatch.rs
  rust/hg-cpython/src/dirstate.rs
  rust/hg-cpython/src/dirstate/dirstate_map.rs
  rust/hg-cpython/src/dirstate/dispatch.rs
  tests/test-completion.t
  tests/test-status.t

CHANGE DETAILS




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

Patch

diff --git a/tests/test-status.t b/tests/test-status.t
--- a/tests/test-status.t
+++ b/tests/test-status.t
@@ -929,23 +929,23 @@ 
 
 The cached mtime is initially unset
 
-  $ hg debugdirstate --dirs --no-dates | grep '^d'
-  d   0          0 unset               subdir
+  $ hg debugdirstate --all --no-dates | grep '^ '
+      0         -1 unset               subdir
 
 It is still not set when there are unknown files
 
   $ touch subdir/unknown
   $ hg status
   ? subdir/unknown
-  $ hg debugdirstate --dirs --no-dates | grep '^d'
-  d   0          0 unset               subdir
+  $ hg debugdirstate --all --no-dates | grep '^ '
+      0         -1 unset               subdir
 
 Now the directory is eligible for caching, so its mtime is save in the dirstate
 
   $ rm subdir/unknown
   $ hg status
-  $ hg debugdirstate --dirs --no-dates | grep '^d'
-  d   0          0 set                 subdir
+  $ hg debugdirstate --all --no-dates | grep '^ '
+      0         -1 set                 subdir
 
 This time the command should be ever so slightly faster since it does not need `read_dir("subdir")`
 
@@ -963,11 +963,11 @@ 
 Removing a node from the dirstate resets the cache for its parent directory
 
   $ hg forget subdir/a
-  $ hg debugdirstate --dirs --no-dates | grep '^d'
-  d   0          0 set                 subdir
+  $ hg debugdirstate --all --no-dates | grep '^ '
+      0         -1 set                 subdir
   $ hg ci -qm '#1'
-  $ hg debugdirstate --dirs --no-dates | grep '^d'
-  d   0          0 unset               subdir
+  $ hg debugdirstate --all --no-dates | grep '^ '
+      0         -1 unset               subdir
   $ hg status
   ? subdir/a
 
diff --git a/tests/test-completion.t b/tests/test-completion.t
--- a/tests/test-completion.t
+++ b/tests/test-completion.t
@@ -284,7 +284,7 @@ 
   debugdate: extended
   debugdeltachain: changelog, manifest, dir, template
   debugdirstateignorepatternshash: 
-  debugdirstate: nodates, dates, datesort, dirs
+  debugdirstate: nodates, dates, datesort, all
   debugdiscovery: old, nonheads, rev, seed, local-as-revs, remote-as-revs, ssh, remotecmd, insecure, template
   debugdownload: output
   debugextensions: template
diff --git a/rust/hg-cpython/src/dirstate/dispatch.rs b/rust/hg-cpython/src/dirstate/dispatch.rs
--- a/rust/hg-cpython/src/dirstate/dispatch.rs
+++ b/rust/hg-cpython/src/dirstate/dispatch.rs
@@ -203,17 +203,30 @@ 
         self.get().iter()
     }
 
-    fn iter_directories(
+    fn iter_tracked_dirs(
+        &mut self,
+    ) -> Result<
+        Box<
+            dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>>
+                + Send
+                + '_,
+        >,
+        DirstateError,
+    > {
+        self.get_mut().iter_tracked_dirs()
+    }
+
+    fn debug_iter(
         &self,
     ) -> Box<
         dyn Iterator<
                 Item = Result<
-                    (&HgPath, Option<Timestamp>),
+                    (&HgPath, (u8, i32, i32, i32)),
                     DirstateV2ParseError,
                 >,
             > + Send
             + '_,
     > {
-        self.get().iter_directories()
+        self.get().debug_iter()
     }
 }
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
@@ -19,8 +19,8 @@ 
 
 use crate::{
     dirstate::copymap::{CopyMap, CopyMapItemsIterator, CopyMapKeysIterator},
-    dirstate::make_directory_item,
     dirstate::make_dirstate_item,
+    dirstate::make_dirstate_item_raw,
     dirstate::non_normal_entries::{
         NonNormalEntries, NonNormalEntriesIterator,
     },
@@ -61,17 +61,14 @@ 
         use_dirstate_tree: bool,
         on_disk: PyBytes,
     ) -> PyResult<PyObject> {
-        let dirstate_error = |e: DirstateError| {
-            PyErr::new::<exc::OSError, _>(py, format!("Dirstate error: {:?}", e))
-        };
         let (inner, parents) = if use_dirstate_tree {
             let (map, parents) = OwningDirstateMap::new_v1(py, on_disk)
-                .map_err(dirstate_error)?;
+                .map_err(|e| dirstate_error(py, e))?;
             (Box::new(map) as _, parents)
         } else {
             let bytes = on_disk.data(py);
             let mut map = RustDirstateMap::default();
-            let parents = map.read(bytes).map_err(dirstate_error)?;
+            let parents = map.read(bytes).map_err(|e| dirstate_error(py, e))?;
             (Box::new(map) as _, parents)
         };
         let map = Self::create_instance(py, inner)?;
@@ -550,19 +547,29 @@ 
         )
     }
 
-    def directories(&self) -> PyResult<PyList> {
+    def tracked_dirs(&self) -> PyResult<PyList> {
         let dirs = PyList::new(py, &[]);
-        for item in self.inner(py).borrow().iter_directories() {
-            let (path, mtime) = item.map_err(|e| v2_error(py, e))?;
+        for path in self.inner(py).borrow_mut().iter_tracked_dirs()
+            .map_err(|e |dirstate_error(py, e))?
+        {
+            let path = path.map_err(|e| v2_error(py, e))?;
             let path = PyBytes::new(py, path.as_bytes());
-            let mtime = mtime.map(|t| t.0).unwrap_or(-1);
-            let item = make_directory_item(py, mtime as i32)?;
-            let tuple = (path, item);
-            dirs.append(py, tuple.to_py_object(py).into_object())
+            dirs.append(py, path.into_object())
         }
         Ok(dirs)
     }
 
+    def debug_iter(&self) -> PyResult<PyList> {
+        let dirs = PyList::new(py, &[]);
+        for item in self.inner(py).borrow().debug_iter() {
+            let (path, (state, mode, size, mtime)) =
+                item.map_err(|e| v2_error(py, e))?;
+            let path = PyBytes::new(py, path.as_bytes());
+            let item = make_dirstate_item_raw(py, state, mode, size, mtime)?;
+            dirs.append(py, (path, item).to_py_object(py).into_object())
+        }
+        Ok(dirs)
+    }
 });
 
 impl DirstateMap {
@@ -616,3 +623,7 @@ 
 pub(super) fn v2_error(py: Python<'_>, _: DirstateV2ParseError) -> PyErr {
     PyErr::new::<exc::ValueError, _>(py, "corrupted dirstate-v2")
 }
+
+fn dirstate_error(py: Python<'_>, e: DirstateError) -> PyErr {
+    PyErr::new::<exc::OSError, _>(py, format!("Dirstate error: {:?}", e))
+}
diff --git a/rust/hg-cpython/src/dirstate.rs b/rust/hg-cpython/src/dirstate.rs
--- a/rust/hg-cpython/src/dirstate.rs
+++ b/rust/hg-cpython/src/dirstate.rs
@@ -52,9 +52,6 @@ 
     py: Python,
     entry: &DirstateEntry,
 ) -> PyResult<PyObject> {
-    // might be silly to retrieve capsule function in hot loop
-    let make = make_dirstate_item_capi::retrieve(py)?;
-
     let &DirstateEntry {
         state,
         mode,
@@ -65,22 +62,19 @@ 
     // because Into<u8> has a specific implementation while `as c_char` would
     // just do a naive enum cast.
     let state_code: u8 = state.into();
-
-    let maybe_obj = unsafe {
-        let ptr = make(state_code as c_char, mode, size, mtime);
-        PyObject::from_owned_ptr_opt(py, ptr)
-    };
-    maybe_obj.ok_or_else(|| PyErr::fetch(py))
+    make_dirstate_item_raw(py, state_code, mode, size, mtime)
 }
 
-// XXX a bit strange to have a dedicated function, but directory are not
-// treated as dirstate node by hg-core for now so…
-pub fn make_directory_item(py: Python, mtime: i32) -> PyResult<PyObject> {
-    // might be silly to retrieve capsule function in hot loop
+pub fn make_dirstate_item_raw(
+    py: Python,
+    state: u8,
+    mode: i32,
+    size: i32,
+    mtime: i32,
+) -> PyResult<PyObject> {
     let make = make_dirstate_item_capi::retrieve(py)?;
-
     let maybe_obj = unsafe {
-        let ptr = make(b'd' as c_char, 0 as i32, 0 as i32, mtime);
+        let ptr = make(state as c_char, mode, size, mtime);
         PyObject::from_owned_ptr_opt(py, ptr)
     };
     maybe_obj.ok_or_else(|| PyErr::fetch(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
@@ -259,20 +259,40 @@ 
     /// are `Result`s.
     fn iter(&self) -> StateMapIter<'_>;
 
-    /// In the tree dirstate, return an iterator of "directory" (entry-less)
-    /// nodes with the data stored for them. This is for `hg debugdirstate
-    /// --dirs`.
+    /// Returns an iterator of tracked directories.
     ///
-    /// In the flat dirstate, returns an empty iterator.
+    /// This is the paths for which `has_tracked_dir` would return true.
+    /// Or, in other words, the union of ancestor paths of all paths that have
+    /// an associated entry in a "tracked" state in this dirstate map.
     ///
     /// Because parse errors can happen during iteration, the iterated items
     /// are `Result`s.
-    fn iter_directories(
+    fn iter_tracked_dirs(
+        &mut self,
+    ) -> Result<
+        Box<
+            dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>>
+                + Send
+                + '_,
+        >,
+        DirstateError,
+    >;
+
+    /// Return an iterator of `(path, (state, mode, size, mtime))` for every
+    /// node stored in this dirstate map, for the purpose of the `hg
+    /// debugdirstate` command.
+    ///
+    /// For nodes that don’t have an entry, `state` is the ASCII space.
+    /// An `mtime` may still be present. It is used to optimize `status`.
+    ///
+    /// Because parse errors can happen during iteration, the iterated items
+    /// are `Result`s.
+    fn debug_iter(
         &self,
     ) -> Box<
         dyn Iterator<
                 Item = Result<
-                    (&HgPath, Option<Timestamp>),
+                    (&HgPath, (u8, i32, i32, i32)),
                     DirstateV2ParseError,
                 >,
             > + Send
@@ -476,17 +496,41 @@ 
         Box::new((&**self).iter().map(|(key, value)| Ok((&**key, *value))))
     }
 
-    fn iter_directories(
+    fn iter_tracked_dirs(
+        &mut self,
+    ) -> Result<
+        Box<
+            dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>>
+                + Send
+                + '_,
+        >,
+        DirstateError,
+    > {
+        self.set_all_dirs()?;
+        Ok(Box::new(
+            self.all_dirs
+                .as_ref()
+                .unwrap()
+                .iter()
+                .map(|path| Ok(&**path)),
+        ))
+    }
+
+    fn debug_iter(
         &self,
     ) -> Box<
         dyn Iterator<
                 Item = Result<
-                    (&HgPath, Option<Timestamp>),
+                    (&HgPath, (u8, i32, i32, i32)),
                     DirstateV2ParseError,
                 >,
             > + Send
             + '_,
     > {
-        Box::new(std::iter::empty())
+        Box::new(
+            (&**self)
+                .iter()
+                .map(|(path, entry)| Ok((&**path, entry.debug_tuple()))),
+        )
     }
 }
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
@@ -1246,27 +1246,50 @@ 
         }))
     }
 
-    fn iter_directories(
+    fn iter_tracked_dirs(
+        &mut self,
+    ) -> Result<
+        Box<
+            dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>>
+                + Send
+                + '_,
+        >,
+        DirstateError,
+    > {
+        let on_disk = self.on_disk;
+        Ok(Box::new(filter_map_results(
+            self.iter_nodes(),
+            move |node| {
+                Ok(if node.tracked_descendants_count() > 0 {
+                    Some(node.full_path(on_disk)?)
+                } else {
+                    None
+                })
+            },
+        )))
+    }
+
+    fn debug_iter(
         &self,
     ) -> Box<
         dyn Iterator<
                 Item = Result<
-                    (&HgPath, Option<Timestamp>),
+                    (&HgPath, (u8, i32, i32, i32)),
                     DirstateV2ParseError,
                 >,
             > + Send
             + '_,
     > {
-        Box::new(filter_map_results(self.iter_nodes(), move |node| {
-            Ok(if node.state()?.is_none() {
-                Some((
-                    node.full_path(self.on_disk)?,
-                    node.cached_directory_mtime()
-                        .map(|mtime| Timestamp(mtime.seconds())),
-                ))
+        Box::new(self.iter_nodes().map(move |node| {
+            let node = node?;
+            let debug_tuple = if let Some(entry) = node.entry()? {
+                entry.debug_tuple()
+            } else if let Some(mtime) = node.cached_directory_mtime() {
+                (b' ', 0, -1, mtime.seconds() as i32)
             } else {
-                None
-            })
+                (b' ', 0, -1, -1)
+            };
+            Ok((node.full_path(self.on_disk)?, debug_tuple))
         }))
     }
 }
diff --git a/rust/hg-core/src/dirstate.rs b/rust/hg-core/src/dirstate.rs
--- a/rust/hg-core/src/dirstate.rs
+++ b/rust/hg-core/src/dirstate.rs
@@ -65,6 +65,12 @@ 
         let fs_exec_bit = filesystem_metadata.mode() & EXEC_BIT_MASK;
         dirstate_exec_bit != fs_exec_bit
     }
+
+    /// Returns a `(state, mode, size, mtime)` tuple as for
+    /// `DirstateMapMethods::debug_iter`.
+    pub fn debug_tuple(&self) -> (u8, i32, i32, i32) {
+        (self.state.into(), self.mode, self.size, self.mtime)
+    }
 }
 
 #[derive(BytesCast)]
diff --git a/mercurial/dirstatemap.py b/mercurial/dirstatemap.py
--- a/mercurial/dirstatemap.py
+++ b/mercurial/dirstatemap.py
@@ -105,10 +105,6 @@ 
         self._map
         return self.copymap
 
-    def directories(self):
-        # Rust / dirstate-v2 only
-        return []
-
     def clear(self):
         self._map.clear()
         self.copymap.clear()
@@ -126,6 +122,8 @@ 
     # forward for python2,3 compat
     iteritems = items
 
+    debug_iter = items
+
     def __len__(self):
         return len(self._map)
 
@@ -527,6 +525,9 @@ 
         def directories(self):
             return self._rustmap.directories()
 
+        def debug_iter(self):
+            return self._rustmap.debug_iter()
+
         def preload(self):
             self._rustmap
 
@@ -737,6 +738,6 @@ 
         def dirfoldmap(self):
             f = {}
             normcase = util.normcase
-            for name, _pseudo_entry in self.directories():
+            for name in self._rustmap.tracked_dirs():
                 f[normcase(name)] = name
             return f
diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -942,7 +942,7 @@ 
         ),
         (b'', b'dates', True, _(b'display the saved mtime')),
         (b'', b'datesort', None, _(b'sort by saved mtime')),
-        (b'', b'dirs', False, _(b'display directories')),
+        (b'', b'all', False, _(b'display dirstate-v2 tree nodes that would not exist in v1')),
     ],
     _(b'[OPTION]...'),
 )
@@ -961,9 +961,10 @@ 
         )  # sort by mtime, then by filename
     else:
         keyfunc = None  # sort by filename
-    entries = list(pycompat.iteritems(repo.dirstate))
-    if opts['dirs']:
-        entries.extend(repo.dirstate.directories())
+    if opts['all']:
+        entries = list(repo.dirstate._map.debug_iter())
+    else:
+        entries = list(pycompat.iteritems(repo.dirstate))
     entries.sort(key=keyfunc)
     for file_, ent in entries:
         if ent.v1_mtime() == -1: