Patchwork D11484: rust: Align DirstateEntry internals with Python/C DirstateItem

login
register
mail settings
Submitter phabricator
Date Sept. 22, 2021, 8:56 p.m.
Message ID <differential-rev-PHID-DREV-tkt2qo6qktym6g7idki7-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49800/
State Superseded
Headers show

Comments

phabricator - Sept. 22, 2021, 8:56 p.m.
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  This propagate to this Rust struct the similar change that was made recently
  to the Python classe and C struct. Namely, instead of storing a four-valued
  `state` field we now store seven (bit-packed) booleans that give lower-level
  information.
  
  Additionally, the marker values -1 and -2 for mtime and size should not
  be used internally anymore. They are replaced by some combinations of booleans
  
  For now, all uses of of `DirstateEntry` still use the compatibility APIs
  with `state` and marker values. Later the Rust API for DirstateMap
  will be increasingly updated to the new style.
  
  Also change the expected result of the test_non_normal_other_parent_entries
  unit test. Only a `DirstateEntry` with `size == -2 && mtime != -1` is
  affected, but this case never occurs outside of unit tests.
  `size == -2` was the marker value for "from other parent" entries,
  where no meaningful mtime is stored.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/Cargo.lock
  rust/hg-core/Cargo.toml
  rust/hg-core/src/dirstate/dirstate_map.rs
  rust/hg-core/src/dirstate/entry.rs

CHANGE DETAILS




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

Patch

diff --git a/rust/hg-core/src/dirstate/entry.rs b/rust/hg-core/src/dirstate/entry.rs
--- a/rust/hg-core/src/dirstate/entry.rs
+++ b/rust/hg-core/src/dirstate/entry.rs
@@ -1,4 +1,5 @@ 
 use crate::errors::HgError;
+use bitflags::bitflags;
 use std::convert::TryFrom;
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -14,12 +15,25 @@ 
 /// comes first.
 #[derive(Debug, PartialEq, Copy, Clone)]
 pub struct DirstateEntry {
-    state: EntryState,
+    flags: Flags,
     mode: i32,
     size: i32,
     mtime: i32,
 }
 
+bitflags! {
+    struct Flags: u8 {
+        const WDIR_TRACKED = 1 << 0;
+        const P1_TRACKED = 1 << 1;
+        const P2_TRACKED = 1 << 2;
+        const POSSIBLY_DIRTY = 1 << 3;
+        const MERGED = 1 << 4;
+        const CLEAN_P1 = 1 << 5;
+        const CLEAN_P2 = 1 << 6;
+        const ENTRYLESS_TREE_NODE = 1 << 7;
+    }
+}
+
 pub const V1_RANGEMASK: i32 = 0x7FFFFFFF;
 
 pub const MTIME_UNSET: i32 = -1;
@@ -39,11 +53,89 @@ 
         size: i32,
         mtime: i32,
     ) -> Self {
+        match state {
+            EntryState::Normal => {
+                if size == SIZE_FROM_OTHER_PARENT {
+                    Self::new_from_p2()
+                } else if size == SIZE_NON_NORMAL {
+                    Self::new_possibly_dirty()
+                } else if mtime == MTIME_UNSET {
+                    Self {
+                        flags: Flags::WDIR_TRACKED
+                            | Flags::P1_TRACKED
+                            | Flags::POSSIBLY_DIRTY,
+                        mode,
+                        size,
+                        mtime: 0,
+                    }
+                } else {
+                    Self {
+                        flags: Flags::WDIR_TRACKED | Flags::P1_TRACKED,
+                        mode,
+                        size,
+                        mtime,
+                    }
+                }
+            }
+            EntryState::Added => Self::new_added(),
+            EntryState::Removed => Self {
+                flags: if size == SIZE_NON_NORMAL {
+                    Flags::P1_TRACKED // might not be true because of rename ?
+                    | Flags::P2_TRACKED // might not be true because of rename ?
+                    | Flags::MERGED
+                } else if size == SIZE_FROM_OTHER_PARENT {
+                    // We don’t know if P1_TRACKED should be set (file history)
+                    Flags::P2_TRACKED | Flags::CLEAN_P2
+                } else {
+                    Flags::P1_TRACKED
+                },
+                mode: 0,
+                size: 0,
+                mtime: 0,
+            },
+            EntryState::Merged => Self::new_merged(),
+        }
+    }
+
+    fn new_from_p2() -> Self {
         Self {
-            state,
-            mode,
-            size,
-            mtime,
+            // might be missing P1_TRACKED
+            flags: Flags::WDIR_TRACKED | Flags::P2_TRACKED | Flags::CLEAN_P2,
+            mode: 0,
+            size: SIZE_FROM_OTHER_PARENT,
+            mtime: MTIME_UNSET,
+        }
+    }
+
+    fn new_possibly_dirty() -> Self {
+        Self {
+            flags: Flags::WDIR_TRACKED
+                | Flags::P1_TRACKED
+                | Flags::POSSIBLY_DIRTY,
+            mode: 0,
+            size: SIZE_NON_NORMAL,
+            mtime: MTIME_UNSET,
+        }
+    }
+
+    fn new_added() -> Self {
+        Self {
+            flags: Flags::WDIR_TRACKED,
+            mode: 0,
+            size: SIZE_NON_NORMAL,
+            mtime: MTIME_UNSET,
+        }
+    }
+
+    fn new_merged() -> Self {
+        Self {
+            flags: Flags::WDIR_TRACKED
+                | Flags::P1_TRACKED // might not be true because of rename ?
+                | Flags::P2_TRACKED // might not be true because of rename ?
+                | Flags::MERGED,
+            mode: 0,
+            size: SIZE_NON_NORMAL,
+            mtime: MTIME_UNSET,
         }
     }
 
@@ -52,28 +144,57 @@ 
     /// `size` is expected to be zero, `SIZE_NON_NORMAL`, or
     /// `SIZE_FROM_OTHER_PARENT`
     pub fn new_removed(size: i32) -> Self {
-        Self {
-            state: EntryState::Removed,
-            mode: 0,
-            size,
-            mtime: 0,
-        }
+        Self::from_v1_data(EntryState::Removed, 0, size, 0)
     }
 
     /// TODO: refactor `DirstateMap::add_file` to not take a `DirstateEntry`
     /// parameter and remove this constructor
     pub fn new_for_add_file(mode: i32, size: i32, mtime: i32) -> Self {
-        Self {
-            // XXX Arbitrary default value since the value is determined later
-            state: EntryState::Normal,
-            mode,
-            size,
-            mtime,
-        }
+        // XXX Arbitrary default value since the value is determined later
+        let state = EntryState::Normal;
+        Self::from_v1_data(state, mode, size, mtime)
+    }
+
+    fn tracked_in_any_parent(&self) -> bool {
+        self.flags.intersects(Flags::P1_TRACKED | Flags::P2_TRACKED)
+    }
+
+    fn removed(&self) -> bool {
+        self.tracked_in_any_parent()
+            && !self.flags.contains(Flags::WDIR_TRACKED)
+    }
+
+    fn merged_removed(&self) -> bool {
+        self.removed() && self.flags.contains(Flags::MERGED)
+    }
+
+    fn from_p2_removed(&self) -> bool {
+        self.removed() && self.flags.contains(Flags::CLEAN_P2)
+    }
+
+    fn merged(&self) -> bool {
+        self.flags.contains(Flags::WDIR_TRACKED | Flags::MERGED)
+    }
+
+    fn added(&self) -> bool {
+        self.flags.contains(Flags::WDIR_TRACKED)
+            && !self.tracked_in_any_parent()
+    }
+
+    fn from_p2(&self) -> bool {
+        self.flags.contains(Flags::WDIR_TRACKED | Flags::CLEAN_P2)
     }
 
     pub fn state(&self) -> EntryState {
-        self.state
+        if self.removed() {
+            EntryState::Removed
+        } else if self.merged() {
+            EntryState::Merged
+        } else if self.added() {
+            EntryState::Added
+        } else {
+            EntryState::Normal
+        }
     }
 
     pub fn mode(&self) -> i32 {
@@ -81,11 +202,39 @@ 
     }
 
     pub fn size(&self) -> i32 {
-        self.size
+        if self.merged_removed() {
+            SIZE_NON_NORMAL
+        } else if self.from_p2_removed() {
+            SIZE_FROM_OTHER_PARENT
+        } else if self.removed() {
+            0
+        } else if self.merged() {
+            SIZE_FROM_OTHER_PARENT
+        } else if self.added() {
+            SIZE_NON_NORMAL
+        } else if self.from_p2() {
+            SIZE_FROM_OTHER_PARENT
+        } else if self.flags.contains(Flags::POSSIBLY_DIRTY) {
+            self.size // TODO: SIZE_NON_NORMAL ?
+        } else {
+            self.size
+        }
     }
 
     pub fn mtime(&self) -> i32 {
-        self.mtime
+        if self.removed() {
+            0
+        } else if self.flags.contains(Flags::POSSIBLY_DIRTY) {
+            MTIME_UNSET
+        } else if self.merged() {
+            MTIME_UNSET
+        } else if self.added() {
+            MTIME_UNSET
+        } else if self.from_p2() {
+            MTIME_UNSET
+        } else {
+            self.mtime
+        }
     }
 
     /// Returns `(state, mode, size, mtime)` for the puprose of serialization
@@ -95,15 +244,16 @@ 
     /// want to not represent these cases that way in memory, but serialization
     /// will need to keep the same format.
     pub fn v1_data(&self) -> (u8, i32, i32, i32) {
-        (self.state.into(), self.mode, self.size, self.mtime)
+        (self.state().into(), self.mode(), self.size(), self.mtime())
     }
 
     pub fn is_non_normal(&self) -> bool {
-        self.state != EntryState::Normal || self.mtime == MTIME_UNSET
+        self.state() != EntryState::Normal || self.mtime() == MTIME_UNSET
     }
 
     pub fn is_from_other_parent(&self) -> bool {
-        self.state == EntryState::Normal && self.size == SIZE_FROM_OTHER_PARENT
+        self.state() == EntryState::Normal
+            && self.size() == SIZE_FROM_OTHER_PARENT
     }
 
     // TODO: other platforms
@@ -114,7 +264,7 @@ 
     ) -> bool {
         use std::os::unix::fs::MetadataExt;
         const EXEC_BIT_MASK: u32 = 0o100;
-        let dirstate_exec_bit = (self.mode as u32) & EXEC_BIT_MASK;
+        let dirstate_exec_bit = (self.mode() as u32) & EXEC_BIT_MASK;
         let fs_exec_bit = filesystem_metadata.mode() & EXEC_BIT_MASK;
         dirstate_exec_bit != fs_exec_bit
     }
@@ -122,11 +272,16 @@ 
     /// 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)
+        let state = if self.flags.contains(Flags::ENTRYLESS_TREE_NODE) {
+            b' '
+        } else {
+            self.state().into()
+        };
+        (state, self.mode(), self.size(), self.mtime())
     }
 
     pub fn mtime_is_ambiguous(&self, now: i32) -> bool {
-        self.state == EntryState::Normal && self.mtime == now
+        self.state() == EntryState::Normal && self.mtime() == now
     }
 
     pub fn clear_ambiguous_mtime(&mut self, now: i32) -> bool {
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
@@ -452,7 +452,8 @@ 
         .collect();
 
         let mut non_normal = [
-            b"f1", b"f2", b"f5", b"f6", b"f7", b"f8", b"f9", b"fa", b"fb",
+            b"f1", b"f2", b"f4", b"f5", b"f6", b"f7", b"f8", b"f9", b"fa",
+            b"fb",
         ]
         .iter()
         .map(|x| HgPathBuf::from_bytes(x.as_ref()))
diff --git a/rust/hg-core/Cargo.toml b/rust/hg-core/Cargo.toml
--- a/rust/hg-core/Cargo.toml
+++ b/rust/hg-core/Cargo.toml
@@ -9,6 +9,7 @@ 
 name = "hg"
 
 [dependencies]
+bitflags = "1.2"
 bytes-cast = "0.2"
 byteorder = "1.3.4"
 derive_more = "0.99"
diff --git a/rust/Cargo.lock b/rust/Cargo.lock
--- a/rust/Cargo.lock
+++ b/rust/Cargo.lock
@@ -1,6 +1,5 @@ 
 # This file is automatically @generated by Cargo.
 # It is not intended for manual editing.
-
 [[package]]
 name = "adler"
 version = "0.2.3"
@@ -375,6 +374,7 @@ 
 name = "hg-core"
 version = "0.1.0"
 dependencies = [
+ "bitflags",
  "byteorder",
  "bytes-cast",
  "clap",