Patchwork D11134: dirstate-map: move most of `dirstate.update_file` logic in the dsmap

login
register
mail settings
Submitter phabricator
Date July 19, 2021, 10:43 a.m.
Message ID <differential-rev-PHID-DREV-6uycxy7zvzgnpibjpwij-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49455/
State Superseded
Headers show

Comments

phabricator - July 19, 2021, 10:43 a.m.
marmoute created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  A new `reset_state` method is introduced to deal with most of that logic. This
  move things one layer lower, but the ultimate goal is to deal with most of this
  at the DirstateItem level.
  
  This reveal various imperfection with the data passed to update_file by
  `mergestate.recordupdates`, however this is orthogonal to this patch and should
  be dealt with at a higher level.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/dirstate.py
  mercurial/dirstatemap.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
  rust/hg-cpython/src/dirstate/dispatch.rs
  rust/hg-cpython/src/dirstate/non_normal_entries.rs

CHANGE DETAILS




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

Patch

diff --git a/rust/hg-cpython/src/dirstate/non_normal_entries.rs b/rust/hg-cpython/src/dirstate/non_normal_entries.rs
--- a/rust/hg-cpython/src/dirstate/non_normal_entries.rs
+++ b/rust/hg-cpython/src/dirstate/non_normal_entries.rs
@@ -26,6 +26,12 @@ 
     def remove(&self, key: PyObject) -> PyResult<PyObject> {
         self.dmap(py).non_normal_entries_remove(py, key)
     }
+    def add(&self, key: PyObject) -> PyResult<PyObject> {
+        self.dmap(py).non_normal_entries_add(py, key)
+    }
+    def discard(&self, key: PyObject) -> PyResult<PyObject> {
+        self.dmap(py).non_normal_entries_discard(py, key)
+    }
     def __richcmp__(&self, other: PyObject, op: CompareOp) -> PyResult<bool> {
         match op {
             CompareOp::Eq => self.is_equal_to(py, other),
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
@@ -20,6 +20,10 @@ 
         self.get_mut().clear()
     }
 
+    fn set_v1(&mut self, filename: &HgPath, entry: DirstateEntry) {
+        self.get_mut().set_v1(filename, entry)
+    }
+
     fn add_file(
         &mut self,
         filename: &HgPath,
@@ -66,10 +70,14 @@ 
         self.get_mut().non_normal_entries_contains(key)
     }
 
-    fn non_normal_entries_remove(&mut self, key: &HgPath) {
+    fn non_normal_entries_remove(&mut self, key: &HgPath) -> bool {
         self.get_mut().non_normal_entries_remove(key)
     }
 
+    fn non_normal_entries_add(&mut self, key: &HgPath) {
+        self.get_mut().non_normal_entries_add(key)
+    }
+
     fn non_normal_or_other_parent_paths(
         &mut self,
     ) -> Box<dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>> + '_>
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
@@ -105,6 +105,21 @@ 
         }
     }
 
+    def set_v1(&self, path: PyObject, item: PyObject) -> PyResult<PyObject> {
+        let f = path.extract::<PyBytes>(py)?;
+        let filename = HgPath::new(f.data(py));
+        let state = item.getattr(py, "state")?.extract::<PyBytes>(py)?;
+        let state = state.data(py)[0];
+        let entry = DirstateEntry {
+            state: state.try_into().expect("state is always valid"),
+            mtime: item.getattr(py, "mtime")?.extract(py)?,
+            size: item.getattr(py, "size")?.extract(py)?,
+            mode: item.getattr(py, "mode")?.extract(py)?,
+        };
+        self.inner(py).borrow_mut().set_v1(filename, entry);
+        Ok(py.None())
+    }
+
     def addfile(
         &self,
         f: PyObject,
@@ -249,6 +264,22 @@ 
 
     def non_normal_entries_remove(&self, key: PyObject) -> PyResult<PyObject> {
         let key = key.extract::<PyBytes>(py)?;
+        let key = key.data(py);
+        let was_present = self
+            .inner(py)
+            .borrow_mut()
+            .non_normal_entries_remove(HgPath::new(key));
+        if !was_present {
+            let msg = String::from_utf8_lossy(key);
+            Err(PyErr::new::<exc::KeyError, _>(py, msg))
+        } else {
+            Ok(py.None())
+        }
+    }
+
+    def non_normal_entries_discard(&self, key: PyObject) -> PyResult<PyObject>
+    {
+        let key = key.extract::<PyBytes>(py)?;
         self
             .inner(py)
             .borrow_mut()
@@ -256,6 +287,15 @@ 
         Ok(py.None())
     }
 
+    def non_normal_entries_add(&self, key: PyObject) -> PyResult<PyObject> {
+        let key = key.extract::<PyBytes>(py)?;
+        self
+            .inner(py)
+            .borrow_mut()
+            .non_normal_entries_add(HgPath::new(key.data(py)));
+        Ok(py.None())
+    }
+
     def non_normal_or_other_parent_paths(&self) -> PyResult<PyList> {
         let mut inner = self.inner(py).borrow_mut();
 
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
@@ -37,6 +37,8 @@ 
     /// Remove information about all files in this map
     fn clear(&mut self);
 
+    fn set_v1(&mut self, filename: &HgPath, entry: DirstateEntry);
+
     /// Add or change the information associated to a given file.
     ///
     /// `old_state` is the state in the entry that `get` would have returned
@@ -95,7 +97,13 @@ 
     /// Mark the given path as "normal" file. This is only relevant in the flat
     /// dirstate map where there is a separate `HashSet` that needs to be kept
     /// up to date.
-    fn non_normal_entries_remove(&mut self, key: &HgPath);
+    /// Returns whether the key was present in the set.
+    fn non_normal_entries_remove(&mut self, key: &HgPath) -> bool;
+
+    /// Mark the given path as "non-normal" file.
+    /// This is only relevant in the flat dirstate map where there is a
+    /// separate `HashSet` that needs to be kept up to date.
+    fn non_normal_entries_add(&mut self, key: &HgPath);
 
     /// Return an iterator of paths whose respective entry are either
     /// "non-normal" (see `non_normal_entries_contains`) or "from other
@@ -280,6 +288,14 @@ 
         self.clear()
     }
 
+    /// Used to set a value directory.
+    ///
+    /// XXX Is temporary during a refactor of V1 dirstate and will disappear
+    /// shortly.
+    fn set_v1(&mut self, filename: &HgPath, entry: DirstateEntry) {
+        self.set_v1_inner(&filename, entry)
+    }
+
     fn add_file(
         &mut self,
         filename: &HgPath,
@@ -321,10 +337,14 @@ 
         Ok(non_normal.contains(key))
     }
 
-    fn non_normal_entries_remove(&mut self, key: &HgPath) {
+    fn non_normal_entries_remove(&mut self, key: &HgPath) -> bool {
         self.non_normal_entries_remove(key)
     }
 
+    fn non_normal_entries_add(&mut self, key: &HgPath) {
+        self.non_normal_entries_add(key)
+    }
+
     fn non_normal_or_other_parent_paths(
         &mut self,
     ) -> Box<dyn Iterator<Item = Result<&HgPath, DirstateV2ParseError>> + '_>
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
@@ -718,6 +718,16 @@ 
         self.nodes_with_copy_source_count = 0;
     }
 
+    fn set_v1(&mut self, filename: &HgPath, entry: DirstateEntry) {
+        let node =
+            self.get_or_insert(&filename).expect("no parse error in v1");
+        node.data = NodeData::Entry(entry);
+        node.children = ChildNodes::default();
+        node.copy_source = None;
+        node.descendants_with_entry_count = 0;
+        node.tracked_descendants_count = 0;
+    }
+
     fn add_file(
         &mut self,
         filename: &HgPath,
@@ -925,7 +935,16 @@ 
         })
     }
 
-    fn non_normal_entries_remove(&mut self, _key: &HgPath) {
+    fn non_normal_entries_remove(&mut self, key: &HgPath) -> bool {
+        // Do nothing, this `DirstateMap` does not have a separate "non normal
+        // entries" set that need to be kept up to date.
+        if let Ok(Some(v)) = self.get(key) {
+            return v.is_non_normal();
+        }
+        false
+    }
+
+    fn non_normal_entries_add(&mut self, _key: &HgPath) {
         // Do nothing, this `DirstateMap` does not have a separate "non normal
         // entries" set that need to be kept up to date
     }
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
@@ -64,6 +64,10 @@ 
         self.other_parent_set = None;
     }
 
+    pub fn set_v1_inner(&mut self, filename: &HgPath, entry: DirstateEntry) {
+        self.state_map.insert(filename.to_owned(), entry);
+    }
+
     /// Add a tracked file to the dirstate
     pub fn add_file(
         &mut self,
@@ -245,10 +249,19 @@ 
         }
     }
 
-    pub fn non_normal_entries_remove(&mut self, key: impl AsRef<HgPath>) {
+    pub fn non_normal_entries_remove(
+        &mut self,
+        key: impl AsRef<HgPath>,
+    ) -> bool {
         self.get_non_normal_other_parent_entries()
             .0
-            .remove(key.as_ref());
+            .remove(key.as_ref())
+    }
+
+    pub fn non_normal_entries_add(&mut self, key: impl AsRef<HgPath>) {
+        self.get_non_normal_other_parent_entries()
+            .0
+            .insert(key.as_ref().into());
     }
 
     pub fn non_normal_entries_union(
diff --git a/mercurial/dirstatemap.py b/mercurial/dirstatemap.py
--- a/mercurial/dirstatemap.py
+++ b/mercurial/dirstatemap.py
@@ -217,6 +217,83 @@ 
         if e.dm_otherparent:
             self.otherparentset.add(f)
 
+    def reset_state(
+        self,
+        filename,
+        wc_tracked,
+        p1_tracked,
+        p2_tracked=False,
+        merged=False,
+        clean_p1=False,
+        clean_p2=False,
+        possibly_dirty=False,
+        parentfiledata=None,
+    ):
+        """Set a entry to a given state, diregarding all previous state
+
+        This is to be used by the part of the dirstate API dedicated to
+        adjusting the dirstate after a update/merge.
+
+        note: calling this might result to no entry existing at all if the
+        dirstate map does not see any point at having one for this file
+        anymore.
+        """
+        if merged and (clean_p1 or clean_p2):
+            msg = b'`merged` argument incompatible with `clean_p1`/`clean_p2`'
+            raise error.ProgrammingError(msg)
+        # copy information are now outdated
+        # (maybe new information should be in directly passed to this function)
+        self.copymap.pop(filename, None)
+
+        if not (p1_tracked or p2_tracked or wc_tracked):
+            self.dropfile(filename)
+        elif merged:
+            # XXX might be merged and removed ?
+            entry = self.get(filename)
+            if entry is not None and entry.tracked:
+                # XXX mostly replicate dirstate.other parent.  We should get
+                # the higher layer to pass us more reliable data where `merged`
+                # actually mean merged. Dropping the else clause will show
+                # failure in `test-graft.t`
+                self.addfile(filename, merged=True)
+            else:
+                self.addfile(filename, from_p2=True)
+        elif not (p1_tracked or p2_tracked) and wc_tracked:
+            self.addfile(filename, added=True, possibly_dirty=possibly_dirty)
+        elif (p1_tracked or p2_tracked) and not wc_tracked:
+            # XXX might be merged and removed ?
+            old_entry = self._map.get(filename)
+            self._dirs_decr(filename, old_entry=old_entry, remove_variant=True)
+            self._map[filename] = DirstateItem(b'r', 0, 0, 0)
+            self.nonnormalset.add(filename)
+        elif clean_p2 and wc_tracked:
+            if p1_tracked or self.get(filename) is not None:
+                # XXX the `self.get` call is catching some case in
+                # `test-merge-remove.t` where the file is tracked in p1, the
+                # p1_tracked argument is False.
+                #
+                # In addition, this seems to be a case where the file is marked
+                # as merged without actually being the result of a merge
+                # action. So thing are not ideal here.
+                self.addfile(filename, merged=True)
+            else:
+                self.addfile(filename, from_p2=True)
+        elif not p1_tracked and p2_tracked and wc_tracked:
+            self.addfile(filename, from_p2=True, possibly_dirty=possibly_dirty)
+        elif possibly_dirty:
+            self.addfile(filename, possibly_dirty=possibly_dirty)
+        elif wc_tracked:
+            # this is a "normal" file
+            if parentfiledata is None:
+                msg = b'failed to pass parentfiledata for a normal file: %s'
+                msg %= filename
+                raise error.ProgrammingError(msg)
+            mode, size, mtime = parentfiledata
+            self.addfile(filename, mode=mode, size=size, mtime=mtime)
+            self.nonnormalset.discard(filename)
+        else:
+            assert False, 'unreachable'
+
     def removefile(self, f, in_merge=False):
         """
         Mark a file as removed in the dirstate.
@@ -496,6 +573,87 @@ 
                 possibly_dirty,
             )
 
+        def reset_state(
+            self,
+            filename,
+            wc_tracked,
+            p1_tracked,
+            p2_tracked=False,
+            merged=False,
+            clean_p1=False,
+            clean_p2=False,
+            possibly_dirty=False,
+            parentfiledata=None,
+        ):
+            """Set a entry to a given state, disregarding all previous state
+
+            This is to be used by the part of the dirstate API dedicated to
+            adjusting the dirstate after a update/merge.
+
+            note: calling this might result to no entry existing at all if the
+            dirstate map does not see any point at having one for this file
+            anymore.
+            """
+            if merged and (clean_p1 or clean_p2):
+                msg = (
+                    b'`merged` argument incompatible with `clean_p1`/`clean_p2`'
+                )
+                raise error.ProgrammingError(msg)
+            # copy information are now outdated
+            # (maybe new information should be in directly passed to this function)
+            self.copymap.pop(filename, None)
+
+            if not (p1_tracked or p2_tracked or wc_tracked):
+                self.dropfile(filename)
+            elif merged:
+                # XXX might be merged and removed ?
+                entry = self.get(filename)
+                if entry is not None and entry.tracked:
+                    # XXX mostly replicate dirstate.other parent.  We should get
+                    # the higher layer to pass us more reliable data where `merged`
+                    # actually mean merged. Dropping the else clause will show
+                    # failure in `test-graft.t`
+                    self.addfile(filename, merged=True)
+                else:
+                    self.addfile(filename, from_p2=True)
+            elif not (p1_tracked or p2_tracked) and wc_tracked:
+                self.addfile(
+                    filename, added=True, possibly_dirty=possibly_dirty
+                )
+            elif (p1_tracked or p2_tracked) and not wc_tracked:
+                # XXX might be merged and removed ?
+                self[filename] = DirstateItem(b'r', 0, 0, 0)
+                self.nonnormalset.add(filename)
+            elif clean_p2 and wc_tracked:
+                if p1_tracked or self.get(filename) is not None:
+                    # XXX the `self.get` call is catching some case in
+                    # `test-merge-remove.t` where the file is tracked in p1, the
+                    # p1_tracked argument is False.
+                    #
+                    # In addition, this seems to be a case where the file is marked
+                    # as merged without actually being the result of a merge
+                    # action. So thing are not ideal here.
+                    self.addfile(filename, merged=True)
+                else:
+                    self.addfile(filename, from_p2=True)
+            elif not p1_tracked and p2_tracked and wc_tracked:
+                self.addfile(
+                    filename, from_p2=True, possibly_dirty=possibly_dirty
+                )
+            elif possibly_dirty:
+                self.addfile(filename, possibly_dirty=possibly_dirty)
+            elif wc_tracked:
+                # this is a "normal" file
+                if parentfiledata is None:
+                    msg = b'failed to pass parentfiledata for a normal file: %s'
+                    msg %= filename
+                    raise error.ProgrammingError(msg)
+                mode, size, mtime = parentfiledata
+                self.addfile(filename, mode=mode, size=size, mtime=mtime)
+                self.nonnormalset.discard(filename)
+            else:
+                assert False, 'unreachable'
+
         def removefile(self, *args, **kwargs):
             return self._rustmap.removefile(*args, **kwargs)
 
@@ -683,3 +841,7 @@ 
             for name, _pseudo_entry in self.directories():
                 f[normcase(name)] = name
             return f
+
+        def __setitem__(self, key, value):
+            assert isinstance(value, DirstateItem)
+            self._rustmap.set_v1(key, value)
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -564,30 +564,46 @@ 
         if merged and (clean_p1 or clean_p2):
             msg = b'`merged` argument incompatible with `clean_p1`/`clean_p2`'
             raise error.ProgrammingError(msg)
-        if not (p1_tracked or p2_tracked or wc_tracked):
-            self._drop(filename)
-        elif merged:
-            assert wc_tracked
-            assert self.in_merge  # we are never in the "normallookup" case
-            self.otherparent(filename)
-        elif not (p1_tracked or p2_tracked) and wc_tracked:
-            self._addpath(filename, added=True, possibly_dirty=possibly_dirty)
-            self._map.copymap.pop(filename, None)
-        elif (p1_tracked or p2_tracked) and not wc_tracked:
-            self._remove(filename)
-        elif clean_p2 and wc_tracked:
-            assert p2_tracked
-            self.otherparent(filename)
-        elif not p1_tracked and p2_tracked and wc_tracked:
-            self._addpath(filename, from_p2=True, possibly_dirty=possibly_dirty)
-            self._map.copymap.pop(filename, None)
-        elif possibly_dirty:
-            self._addpath(filename, possibly_dirty=possibly_dirty)
-        elif wc_tracked:
-            self.normal(filename, parentfiledata=parentfiledata)
-        # XXX We need something for file that are dirty after an update
-        else:
-            assert False, 'unreachable'
+
+        # note: I do not think we need to double check name clash here since we
+        # are in a update/merge case that should already have taken care of
+        # this. The test agrees
+
+        self._dirty = True
+        self._updatedfiles.add(filename)
+
+        need_parent_file_data = (
+            not (possibly_dirty or clean_p2 or merged)
+            and wc_tracked
+            and p1_tracked
+        )
+
+        # this mean we are doing call for file we do not really care about the
+        # data (eg: added or removed), however this should be a minor overhead
+        # compared to the overall update process calling this.
+        if need_parent_file_data:
+            if parentfiledata is None:
+                parentfiledata = self._get_filedata(filename)
+            mtime = parentfiledata[2]
+
+            if mtime > self._lastnormaltime:
+                # Remember the most recent modification timeslot for
+                # status(), to make sure we won't miss future
+                # size-preserving file content modifications that happen
+                # within the same timeslot.
+                self._lastnormaltime = mtime
+
+        self._map.reset_state(
+            filename,
+            wc_tracked,
+            p1_tracked,
+            p2_tracked=p2_tracked,
+            merged=merged,
+            clean_p1=clean_p1,
+            clean_p2=clean_p2,
+            possibly_dirty=possibly_dirty,
+            parentfiledata=parentfiledata,
+        )
 
     def _addpath(
         self,