Patchwork D10953: dirstate: move most of the `remove` logic with dirstatemap `removefile`

login
register
mail settings
Submitter phabricator
Date July 4, 2021, 9:53 p.m.
Message ID <differential-rev-PHID-DREV-azd4dzga6yjavq7twr55-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49272/
State Superseded
Headers show

Comments

phabricator - July 4, 2021, 9:53 p.m.
marmoute created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  This code deal with special logic to preserving "merged" and "from_p2" information when removing a file. These are implementation details that are more suitable for the dirstatemap layer. Since the dirstatemap layer alreaday have most of the information necessary to do so, the move is easy.
  
  This move helps us to encapsulate more implementation details within the dirstatemap and its entry. Easing the use of a different storage for dirstate v2.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

CHANGE DETAILS




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

Patch

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
@@ -33,10 +33,9 @@ 
     fn remove_file(
         &mut self,
         filename: &HgPath,
-        old_state: EntryState,
-        size: i32,
+        in_merge: bool,
     ) -> Result<(), DirstateError> {
-        self.get_mut().remove_file(filename, old_state, size)
+        self.get_mut().remove_file(filename, in_merge)
     }
 
     fn drop_file(
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
@@ -137,18 +137,12 @@ 
     def removefile(
         &self,
         f: PyObject,
-        oldstate: PyObject,
-        size: PyObject
+        in_merge: PyObject
     ) -> PyResult<PyObject> {
         self.inner(py).borrow_mut()
             .remove_file(
                 HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
-                oldstate.extract::<PyBytes>(py)?.data(py)[0]
-                    .try_into()
-                    .map_err(|e: HgError| {
-                        PyErr::new::<exc::ValueError, _>(py, e.to_string())
-                    })?,
-                size.extract(py)?,
+                in_merge.extract::<PyBool>(py)?.is_true(),
             )
             .or_else(|_| {
                 Err(PyErr::new::<exc::OSError, _>(
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
@@ -61,8 +61,7 @@ 
     fn remove_file(
         &mut self,
         filename: &HgPath,
-        old_state: EntryState,
-        size: i32,
+        in_merge: bool,
     ) -> Result<(), DirstateError>;
 
     /// Drop information about this file from the map if any, and return
@@ -295,10 +294,9 @@ 
     fn remove_file(
         &mut self,
         filename: &HgPath,
-        old_state: EntryState,
-        size: i32,
+        in_merge: bool,
     ) -> Result<(), DirstateError> {
-        self.remove_file(filename, old_state, size)
+        self.remove_file(filename, in_merge)
     }
 
     fn drop_file(
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
@@ -11,6 +11,8 @@ 
 use crate::dirstate::parsers::packed_entry_size;
 use crate::dirstate::parsers::parse_dirstate_entries;
 use crate::dirstate::parsers::Timestamp;
+use crate::dirstate::SIZE_FROM_OTHER_PARENT;
+use crate::dirstate::SIZE_NON_NORMAL;
 use crate::matchers::Matcher;
 use crate::utils::hg_path::{HgPath, HgPathBuf};
 use crate::CopyMapIter;
@@ -726,9 +728,34 @@ 
     fn remove_file(
         &mut self,
         filename: &HgPath,
-        old_state: EntryState,
-        size: i32,
+        in_merge: bool,
     ) -> Result<(), DirstateError> {
+        let old_entry_opt = self.get(filename)?;
+        let old_state = match old_entry_opt {
+            Some(e) => e.state,
+            None => EntryState::Unknown,
+        };
+        let mut size = 0;
+        if in_merge {
+            // XXX we should not be able to have 'm' state and 'FROM_P2' if not
+            // during a merge. So I (marmoute) am not sure we need the
+            // conditionnal at all. Adding double checking this with assert
+            // would be nice.
+            if let Some(old_entry) = old_entry_opt {
+                // backup the previous state
+                if old_entry.state == EntryState::Merged {
+                    size = SIZE_NON_NORMAL;
+                } else if old_entry.state == EntryState::Normal
+                    && old_entry.size == SIZE_FROM_OTHER_PARENT
+                {
+                    // other parent
+                    size = SIZE_FROM_OTHER_PARENT;
+                }
+            }
+        }
+        if size == 0 {
+            self.copy_map_remove(filename)?;
+        }
         let entry = DirstateEntry {
             state: EntryState::Removed,
             mode: 0,
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
@@ -8,6 +8,8 @@ 
 use crate::dirstate::parsers::Timestamp;
 use crate::{
     dirstate::EntryState,
+    dirstate::SIZE_FROM_OTHER_PARENT,
+    dirstate::SIZE_NON_NORMAL,
     pack_dirstate, parse_dirstate,
     utils::hg_path::{HgPath, HgPathBuf},
     CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateParents,
@@ -102,9 +104,34 @@ 
     pub fn remove_file(
         &mut self,
         filename: &HgPath,
-        old_state: EntryState,
-        size: i32,
+        in_merge: bool,
     ) -> Result<(), DirstateError> {
+        let old_entry_opt = self.get(filename);
+        let old_state = match old_entry_opt {
+            Some(e) => e.state,
+            None => EntryState::Unknown,
+        };
+        let mut size = 0;
+        if in_merge {
+            // XXX we should not be able to have 'm' state and 'FROM_P2' if not
+            // during a merge. So I (marmoute) am not sure we need the
+            // conditionnal at all. Adding double checking this with assert
+            // would be nice.
+            if let Some(old_entry) = old_entry_opt {
+                // backup the previous state
+                if old_entry.state == EntryState::Merged {
+                    size = SIZE_NON_NORMAL;
+                } else if old_entry.state == EntryState::Normal
+                    && old_entry.size == SIZE_FROM_OTHER_PARENT
+                {
+                    // other parent
+                    size = SIZE_FROM_OTHER_PARENT;
+                    self.get_non_normal_other_parent_entries()
+                        .1
+                        .insert(filename.to_owned());
+                }
+            }
+        }
         if old_state != EntryState::Unknown && old_state != EntryState::Removed
         {
             if let Some(ref mut dirs) = self.dirs {
@@ -116,6 +143,9 @@ 
                 all_dirs.add_path(filename)?;
             }
         }
+        if size == 0 {
+            self.copy_map.remove(filename);
+        }
 
         self.state_map.insert(
             filename.to_owned(),
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
@@ -83,6 +83,9 @@ 
 /// other parent. This allows revert to pick the right status back during a
 /// merge.
 pub const SIZE_FROM_OTHER_PARENT: i32 = -2;
+/// A special value used for internal representation of special case in
+/// dirstate v1 format.
+pub const SIZE_NON_NORMAL: i32 = -1;
 
 pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>;
 pub type StateMapIter<'a> = Box<
diff --git a/mercurial/dirstatemap.py b/mercurial/dirstatemap.py
--- a/mercurial/dirstatemap.py
+++ b/mercurial/dirstatemap.py
@@ -154,7 +154,7 @@ 
         if size == FROM_P2:
             self.otherparentset.add(f)
 
-    def removefile(self, f, oldstate, size):
+    def removefile(self, f, in_merge=False):
         """
         Mark a file as removed in the dirstate.
 
@@ -162,9 +162,26 @@ 
         the file's previous state.  In the future, we should refactor this
         to be more explicit about what that state is.
         """
-        if oldstate not in b"?r" and "_dirs" in self.__dict__:
+        entry = self.get(f)
+        size = 0
+        if in_merge:
+            # XXX we should not be able to have 'm' state and 'FROM_P2' if not
+            # during a merge. So I (marmoute) am not sure we need the
+            # conditionnal at all. Adding double checking this with assert
+            # would be nice.
+            if entry is not None:
+                # backup the previous state
+                if entry[0] == b'm':  # merge
+                    size = NONNORMAL
+                elif entry[0] == b'n' and entry[2] == FROM_P2:  # other parent
+                    size = FROM_P2
+                    self.otherparentset.add(f)
+        if size == 0:
+            self.copymap.pop(f, None)
+
+        if entry is not None and entry[0] != b'r' and "_dirs" in self.__dict__:
             self._dirs.delpath(f)
-        if oldstate == b"?" and "_alldirs" in self.__dict__:
+        if entry is None and "_alldirs" in self.__dict__:
             self._alldirs.addpath(f)
         if "filefoldmap" in self.__dict__:
             normed = util.normcase(f)
diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -559,21 +559,8 @@ 
     def remove(self, f):
         '''Mark a file removed.'''
         self._dirty = True
-        oldstate = self[f]
-        size = 0
-        if self.in_merge:
-            entry = self._map.get(f)
-            if entry is not None:
-                # backup the previous state
-                if entry[0] == b'm':  # merge
-                    size = NONNORMAL
-                elif entry[0] == b'n' and entry[2] == FROM_P2:  # other parent
-                    size = FROM_P2
-                    self._map.otherparentset.add(f)
         self._updatedfiles.add(f)
-        self._map.removefile(f, oldstate, size)
-        if size == 0:
-            self._map.copymap.pop(f, None)
+        self._map.removefile(f, in_merge=self.in_merge)
 
     def merge(self, f):
         '''Mark a file merged.'''