Patchwork D11461: rust: Make the fields of DirstateEntry private

login
register
mail settings
Submitter phabricator
Date Sept. 20, 2021, 10:20 p.m.
Message ID <differential-rev-PHID-DREV-5t3rcy4zsvzb374c72w2-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49780/
State Superseded
Headers show

Comments

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

REVISION SUMMARY
  This is a first step toward making its internal structure equivalent to
  Python’s DirstateItem.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  rust/hg-core/src/dirstate/dirs_multiset.rs
  rust/hg-core/src/dirstate/dirstate_map.rs
  rust/hg-core/src/dirstate/entry.rs
  rust/hg-core/src/dirstate/parsers.rs
  rust/hg-core/src/dirstate/status.rs
  rust/hg-core/src/dirstate_tree/dirstate_map.rs
  rust/hg-core/src/dirstate_tree/on_disk.rs
  rust/hg-core/src/dirstate_tree/status.rs
  rust/hg-core/src/operations/list_tracked_files.rs
  rust/hg-cpython/src/dirstate.rs
  rust/hg-cpython/src/dirstate/dirstate_map.rs

CHANGE DETAILS




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

Patch

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
@@ -135,12 +135,12 @@ 
         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)?,
-        };
+        let entry = DirstateEntry::from_v1_data(
+            state.try_into().expect("state is always valid"),
+            item.getattr(py, "mode")?.extract(py)?,
+            item.getattr(py, "size")?.extract(py)?,
+            item.getattr(py, "mtime")?.extract(py)?,
+        );
         self.inner(py).borrow_mut().set_v1(filename, entry);
         Ok(py.None())
     }
@@ -176,13 +176,7 @@ 
         } else {
             mtime.extract(py)?
         };
-        let entry = DirstateEntry {
-            // XXX Arbitrary default value since the value is determined later
-            state: EntryState::Normal,
-            mode: mode,
-            size: size,
-            mtime: mtime,
-        };
+        let entry = DirstateEntry::new_for_add_file(mode, size, mtime);
         let added = added.extract::<PyBool>(py)?.is_true();
         let merged = merged.extract::<PyBool>(py)?.is_true();
         let from_p2 = from_p2.extract::<PyBool>(py)?.is_true();
@@ -422,7 +416,7 @@ 
         let dict = PyDict::new(py);
         for item in self.inner(py).borrow_mut().iter() {
             let (path, entry) = item.map_err(|e| v2_error(py, e))?;
-            if entry.state != EntryState::Removed {
+            if entry.state() != EntryState::Removed {
                 let key = normalize_case(path);
                 let value = path;
                 dict.set_item(
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
@@ -48,17 +48,17 @@ 
     py: Python,
     entry: &DirstateEntry,
 ) -> PyResult<PyObject> {
-    let &DirstateEntry {
-        state,
-        mode,
-        size,
-        mtime,
-    } = entry;
     // Explicitly go through u8 first, then cast to platform-specific `c_char`
     // because Into<u8> has a specific implementation while `as c_char` would
     // just do a naive enum cast.
-    let state_code: u8 = state.into();
-    make_dirstate_item_raw(py, state_code, mode, size, mtime)
+    let state_code: u8 = entry.state().into();
+    make_dirstate_item_raw(
+        py,
+        state_code,
+        entry.mode(),
+        entry.size(),
+        entry.mtime(),
+    )
 }
 
 pub fn make_dirstate_item_raw(
diff --git a/rust/hg-core/src/operations/list_tracked_files.rs b/rust/hg-core/src/operations/list_tracked_files.rs
--- a/rust/hg-core/src/operations/list_tracked_files.rs
+++ b/rust/hg-core/src/operations/list_tracked_files.rs
@@ -51,7 +51,7 @@ 
                 let _parents = parse_dirstate_entries(
                     &self.content,
                     |path, entry, _copy_source| {
-                        if entry.state.is_tracked() {
+                        if entry.state().is_tracked() {
                             files.push(path)
                         }
                         Ok(())
diff --git a/rust/hg-core/src/dirstate_tree/status.rs b/rust/hg-core/src/dirstate_tree/status.rs
--- a/rust/hg-core/src/dirstate_tree/status.rs
+++ b/rust/hg-core/src/dirstate_tree/status.rs
@@ -506,11 +506,9 @@ 
         let hg_path = &dirstate_node.full_path_borrowed(self.dmap.on_disk)?;
         let mode_changed =
             || self.options.check_exec && entry.mode_changed(fs_metadata);
-        let size_changed = entry.size != truncate_u64(fs_metadata.len());
-        if entry.size >= 0
-            && size_changed
-            && fs_metadata.file_type().is_symlink()
-        {
+        let size = entry.size();
+        let size_changed = size != truncate_u64(fs_metadata.len());
+        if size >= 0 && size_changed && fs_metadata.file_type().is_symlink() {
             // issue6456: Size returned may be longer due to encryption
             // on EXT-4 fscrypt. TODO maybe only do it on EXT4?
             self.outcome
@@ -520,7 +518,7 @@ 
                 .push(hg_path.detach_from_tree())
         } else if dirstate_node.has_copy_source()
             || entry.is_from_other_parent()
-            || (entry.size >= 0 && (size_changed || mode_changed()))
+            || (size >= 0 && (size_changed || mode_changed()))
         {
             self.outcome
                 .lock()
@@ -529,7 +527,7 @@ 
                 .push(hg_path.detach_from_tree())
         } else {
             let mtime = mtime_seconds(fs_metadata);
-            if truncate_i64(mtime) != entry.mtime
+            if truncate_i64(mtime) != entry.mtime()
                 || mtime == self.options.last_normal_time
             {
                 self.outcome
diff --git a/rust/hg-core/src/dirstate_tree/on_disk.rs b/rust/hg-core/src/dirstate_tree/on_disk.rs
--- a/rust/hg-core/src/dirstate_tree/on_disk.rs
+++ b/rust/hg-core/src/dirstate_tree/on_disk.rs
@@ -403,12 +403,15 @@ 
     }
 
     fn entry_with_given_state(&self, state: EntryState) -> DirstateEntry {
-        DirstateEntry {
+        // For now, the on-disk representation of DirstateEntry in dirstate-v2
+        // format is equivalent to that of dirstate-v1. When that changes, add
+        // a new constructor.
+        DirstateEntry::from_v1_data(
             state,
-            mode: self.data.mode.get(),
-            mtime: self.data.mtime.get(),
-            size: self.data.size.get(),
-        }
+            self.data.mode.get(),
+            self.data.size.get(),
+            self.data.mtime.get(),
+        )
     }
 
     pub(super) fn entry(
@@ -640,11 +643,11 @@ 
                 NodeRef::InMemory(path, node) => {
                     let (state, data) = match &node.data {
                         dirstate_map::NodeData::Entry(entry) => (
-                            entry.state.into(),
+                            entry.state().into(),
                             Entry {
-                                mode: entry.mode.into(),
-                                mtime: entry.mtime.into(),
-                                size: entry.size.into(),
+                                mode: entry.mode().into(),
+                                mtime: entry.mtime().into(),
+                                size: entry.size().into(),
                             },
                         ),
                         dirstate_map::NodeData::CachedDirectory { mtime } => {
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
@@ -328,7 +328,7 @@ 
     ) -> Result<Option<EntryState>, DirstateV2ParseError> {
         match self {
             NodeRef::InMemory(_path, node) => {
-                Ok(node.data.as_entry().map(|entry| entry.state))
+                Ok(node.data.as_entry().map(|entry| entry.state()))
             }
             NodeRef::OnDisk(node) => node.state(),
         }
@@ -445,7 +445,7 @@ 
         let parents = parse_dirstate_entries(
             map.on_disk,
             |path, entry, copy_source| {
-                let tracked = entry.state.is_tracked();
+                let tracked = entry.state().is_tracked();
                 let node = Self::get_or_insert_node(
                     map.on_disk,
                     &mut map.unreachable_bytes,
@@ -598,7 +598,7 @@ 
     ) -> Result<(), DirstateV2ParseError> {
         let had_entry = old_state != EntryState::Unknown;
         let tracked_count_increment =
-            match (old_state.is_tracked(), new_entry.state.is_tracked()) {
+            match (old_state.is_tracked(), new_entry.state().is_tracked()) {
                 (false, true) => 1,
                 (true, false) => -1,
                 _ => 0,
@@ -776,36 +776,40 @@ 
         from_p2: bool,
         possibly_dirty: bool,
     ) -> Result<(), DirstateError> {
-        let mut entry = entry;
+        let state;
+        let size;
+        let mtime;
         if added {
             assert!(!possibly_dirty);
             assert!(!from_p2);
-            entry.state = EntryState::Added;
-            entry.size = SIZE_NON_NORMAL;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Added;
+            size = SIZE_NON_NORMAL;
+            mtime = MTIME_UNSET;
         } else if merged {
             assert!(!possibly_dirty);
             assert!(!from_p2);
-            entry.state = EntryState::Merged;
-            entry.size = SIZE_FROM_OTHER_PARENT;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Merged;
+            size = SIZE_FROM_OTHER_PARENT;
+            mtime = MTIME_UNSET;
         } else if from_p2 {
             assert!(!possibly_dirty);
-            entry.state = EntryState::Normal;
-            entry.size = SIZE_FROM_OTHER_PARENT;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Normal;
+            size = SIZE_FROM_OTHER_PARENT;
+            mtime = MTIME_UNSET;
         } else if possibly_dirty {
-            entry.state = EntryState::Normal;
-            entry.size = SIZE_NON_NORMAL;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Normal;
+            size = SIZE_NON_NORMAL;
+            mtime = MTIME_UNSET;
         } else {
-            entry.state = EntryState::Normal;
-            entry.size = entry.size & V1_RANGEMASK;
-            entry.mtime = entry.mtime & V1_RANGEMASK;
+            state = EntryState::Normal;
+            size = entry.size() & V1_RANGEMASK;
+            mtime = entry.mtime() & V1_RANGEMASK;
         }
+        let mode = entry.mode();
+        let entry = DirstateEntry::from_v1_data(state, mode, size, mtime);
 
         let old_state = match self.get(filename)? {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
 
@@ -819,7 +823,7 @@ 
     ) -> Result<(), DirstateError> {
         let old_entry_opt = self.get(filename)?;
         let old_state = match old_entry_opt {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
         let mut size = 0;
@@ -830,10 +834,10 @@ 
             // would be nice.
             if let Some(old_entry) = old_entry_opt {
                 // backup the previous state
-                if old_entry.state == EntryState::Merged {
+                if old_entry.state() == EntryState::Merged {
                     size = SIZE_NON_NORMAL;
-                } else if old_entry.state == EntryState::Normal
-                    && old_entry.size == SIZE_FROM_OTHER_PARENT
+                } else if old_entry.state() == EntryState::Normal
+                    && old_entry.size() == SIZE_FROM_OTHER_PARENT
                 {
                     // other parent
                     size = SIZE_FROM_OTHER_PARENT;
@@ -843,18 +847,13 @@ 
         if size == 0 {
             self.copy_map_remove(filename)?;
         }
-        let entry = DirstateEntry {
-            state: EntryState::Removed,
-            mode: 0,
-            size,
-            mtime: 0,
-        };
+        let entry = DirstateEntry::new_removed(size);
         Ok(self.add_or_remove_file(filename, old_state, entry)?)
     }
 
     fn drop_file(&mut self, filename: &HgPath) -> Result<bool, DirstateError> {
         let old_state = match self.get(filename)? {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
         struct Dropped {
@@ -921,7 +920,7 @@ 
                     was_tracked: node
                         .data
                         .as_entry()
-                        .map_or(false, |entry| entry.state.is_tracked()),
+                        .map_or(false, |entry| entry.state().is_tracked()),
                     had_entry,
                     had_copy_source: node.copy_source.take().is_some(),
                 };
diff --git a/rust/hg-core/src/dirstate/status.rs b/rust/hg-core/src/dirstate/status.rs
--- a/rust/hg-core/src/dirstate/status.rs
+++ b/rust/hg-core/src/dirstate/status.rs
@@ -157,22 +157,19 @@ 
     copy_map: &CopyMap,
     options: StatusOptions,
 ) -> Dispatch {
-    let DirstateEntry {
-        state,
-        mode,
-        mtime,
-        size,
-    } = entry;
+    match entry.state() {
+        EntryState::Normal => {
+            let mode = entry.mode();
+            let size = entry.size();
+            let mtime = entry.mtime();
 
-    let HgMetadata {
-        st_mode,
-        st_size,
-        st_mtime,
-        ..
-    } = metadata;
+            let HgMetadata {
+                st_mode,
+                st_size,
+                st_mtime,
+                ..
+            } = metadata;
 
-    match state {
-        EntryState::Normal => {
             let size_changed = mod_compare(size, st_size as i32);
             let mode_changed =
                 (mode ^ st_mode as i32) & 0o100 != 0o000 && options.check_exec;
@@ -473,7 +470,7 @@ 
                         if let Some(entry) = in_dmap {
                             return Some((
                                 Cow::Borrowed(normalized),
-                                dispatch_missing(entry.state),
+                                dispatch_missing(entry.state()),
                             ));
                         }
                     }
@@ -605,7 +602,10 @@ 
                 || self.matcher.matches(&filename)
             {
                 files_sender
-                    .send((filename.to_owned(), dispatch_missing(entry.state)))
+                    .send((
+                        filename.to_owned(),
+                        dispatch_missing(entry.state()),
+                    ))
                     .unwrap();
             }
         }
@@ -635,7 +635,7 @@ 
                     files_sender
                         .send((
                             directory.to_owned(),
-                            dispatch_missing(entry.state),
+                            dispatch_missing(entry.state()),
                         ))
                         .unwrap();
                 }
@@ -767,7 +767,7 @@ 
                         {
                             (
                                 Cow::Borrowed(filename),
-                                dispatch_missing(entry.state),
+                                dispatch_missing(entry.state()),
                             )
                         }
                         Ok(m) => (
@@ -791,7 +791,7 @@ 
                             // directory
                             (
                                 Cow::Borrowed(filename),
-                                dispatch_missing(entry.state),
+                                dispatch_missing(entry.state()),
                             )
                         }
                         Err(e) => {
@@ -863,7 +863,7 @@ 
                                 )
                             }
                             // File doesn't exist
-                            Err(_) => dispatch_missing(entry.state),
+                            Err(_) => dispatch_missing(entry.state()),
                         },
                     ))
                 } else {
@@ -871,7 +871,7 @@ 
                     // we, in this case, report as missing.
                     Some((
                         Cow::Owned(filename.to_owned()),
-                        dispatch_missing(entry.state),
+                        dispatch_missing(entry.state()),
                     ))
                 }
             },
diff --git a/rust/hg-core/src/dirstate/parsers.rs b/rust/hg-core/src/dirstate/parsers.rs
--- a/rust/hg-core/src/dirstate/parsers.rs
+++ b/rust/hg-core/src/dirstate/parsers.rs
@@ -50,7 +50,7 @@ 
 
 #[derive(BytesCast)]
 #[repr(C)]
-pub(super) struct RawEntry {
+struct RawEntry {
     state: u8,
     mode: unaligned::I32Be,
     size: unaligned::I32Be,
@@ -73,12 +73,12 @@ 
         let (raw_entry, rest) = RawEntry::from_bytes(contents)
             .map_err(|_| HgError::corrupted("Overflow in dirstate."))?;
 
-        let entry = DirstateEntry {
-            state: EntryState::try_from(raw_entry.state)?,
-            mode: raw_entry.mode.get(),
-            mtime: raw_entry.mtime.get(),
-            size: raw_entry.size.get(),
-        };
+        let entry = DirstateEntry::from_v1_data(
+            EntryState::try_from(raw_entry.state)?,
+            raw_entry.mode.get(),
+            raw_entry.size.get(),
+            raw_entry.mtime.get(),
+        );
         let (paths, rest) =
             u8::slice_from_bytes(rest, raw_entry.length.get() as usize)
                 .map_err(|_| HgError::corrupted("Overflow in dirstate."))?;
@@ -124,12 +124,13 @@ 
     packed: &mut Vec<u8>,
 ) {
     let length = packed_filename_and_copy_source_size(filename, copy_source);
+    let (state, mode, size, mtime) = entry.v1_data();
 
     // Unwrapping because `impl std::io::Write for Vec<u8>` never errors
-    packed.write_u8(entry.state.into()).unwrap();
-    packed.write_i32::<BigEndian>(entry.mode).unwrap();
-    packed.write_i32::<BigEndian>(entry.size).unwrap();
-    packed.write_i32::<BigEndian>(entry.mtime).unwrap();
+    packed.write_u8(state).unwrap();
+    packed.write_i32::<BigEndian>(mode).unwrap();
+    packed.write_i32::<BigEndian>(size).unwrap();
+    packed.write_i32::<BigEndian>(mtime).unwrap();
     packed.write_i32::<BigEndian>(length as i32).unwrap();
     packed.extend(filename.as_bytes());
     if let Some(source) = copy_source {
@@ -212,12 +213,12 @@ 
     fn test_pack_dirstate_one_entry() {
         let expected_state_map: StateMap = [(
             HgPathBuf::from_bytes(b"f1"),
-            DirstateEntry {
-                state: EntryState::Normal,
-                mode: 0o644,
-                size: 0,
-                mtime: 791231220,
-            },
+            DirstateEntry::from_v1_data(
+                EntryState::Normal,
+                0o644,
+                0,
+                791231220,
+            ),
         )]
         .iter()
         .cloned()
@@ -249,12 +250,12 @@ 
     fn test_pack_dirstate_one_entry_with_copy() {
         let expected_state_map: StateMap = [(
             HgPathBuf::from_bytes(b"f1"),
-            DirstateEntry {
-                state: EntryState::Normal,
-                mode: 0o644,
-                size: 0,
-                mtime: 791231220,
-            },
+            DirstateEntry::from_v1_data(
+                EntryState::Normal,
+                0o644,
+                0,
+                791231220,
+            ),
         )]
         .iter()
         .cloned()
@@ -290,12 +291,12 @@ 
     fn test_parse_pack_one_entry_with_copy() {
         let mut state_map: StateMap = [(
             HgPathBuf::from_bytes(b"f1"),
-            DirstateEntry {
-                state: EntryState::Normal,
-                mode: 0o644,
-                size: 0,
-                mtime: 791231220,
-            },
+            DirstateEntry::from_v1_data(
+                EntryState::Normal,
+                0o644,
+                0,
+                791231220,
+            ),
         )]
         .iter()
         .cloned()
@@ -336,39 +337,34 @@ 
         let mut state_map: StateMap = [
             (
                 HgPathBuf::from_bytes(b"f1"),
-                DirstateEntry {
-                    state: EntryState::Normal,
-                    mode: 0o644,
-                    size: 0,
-                    mtime: 791231220,
-                },
+                DirstateEntry::from_v1_data(
+                    EntryState::Normal,
+                    0o644,
+                    0,
+                    791231220,
+                ),
             ),
             (
                 HgPathBuf::from_bytes(b"f2"),
-                DirstateEntry {
-                    state: EntryState::Merged,
-                    mode: 0o777,
-                    size: 1000,
-                    mtime: 791231220,
-                },
+                DirstateEntry::from_v1_data(
+                    EntryState::Merged,
+                    0o777,
+                    1000,
+                    791231220,
+                ),
             ),
             (
                 HgPathBuf::from_bytes(b"f3"),
-                DirstateEntry {
-                    state: EntryState::Removed,
-                    mode: 0o644,
-                    size: 234553,
-                    mtime: 791231220,
-                },
+                DirstateEntry::from_v1_data(
+                    EntryState::Removed,
+                    0o644,
+                    234553,
+                    791231220,
+                ),
             ),
             (
                 HgPathBuf::from_bytes(b"f4\xF6"),
-                DirstateEntry {
-                    state: EntryState::Added,
-                    mode: 0o644,
-                    size: -1,
-                    mtime: -1,
-                },
+                DirstateEntry::from_v1_data(EntryState::Added, 0o644, -1, -1),
             ),
         ]
         .iter()
@@ -414,12 +410,12 @@ 
     fn test_parse_pack_one_entry_with_copy_and_time_conflict() {
         let mut state_map: StateMap = [(
             HgPathBuf::from_bytes(b"f1"),
-            DirstateEntry {
-                state: EntryState::Normal,
-                mode: 0o644,
-                size: 0,
-                mtime: 15000000,
-            },
+            DirstateEntry::from_v1_data(
+                EntryState::Normal,
+                0o644,
+                0,
+                15000000,
+            ),
         )]
         .iter()
         .cloned()
@@ -454,12 +450,12 @@ 
                 &parents,
                 [(
                     HgPathBuf::from_bytes(b"f1"),
-                    DirstateEntry {
-                        state: EntryState::Normal,
-                        mode: 0o644,
-                        size: 0,
-                        mtime: -1
-                    }
+                    DirstateEntry::from_v1_data(
+                        EntryState::Normal,
+                        0o644,
+                        0,
+                        -1
+                    )
                 )]
                 .iter()
                 .cloned()
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
@@ -15,10 +15,10 @@ 
 /// comes first.
 #[derive(Debug, PartialEq, Copy, Clone)]
 pub struct DirstateEntry {
-    pub state: EntryState,
-    pub mode: i32,
-    pub mtime: i32,
-    pub size: i32,
+    state: EntryState,
+    mode: i32,
+    size: i32,
+    mtime: i32,
 }
 
 pub const V1_RANGEMASK: i32 = 0x7FFFFFFF;
@@ -34,6 +34,71 @@ 
 pub const SIZE_NON_NORMAL: i32 = -1;
 
 impl DirstateEntry {
+    pub fn from_v1_data(
+        state: EntryState,
+        mode: i32,
+        size: i32,
+        mtime: i32,
+    ) -> Self {
+        Self {
+            state,
+            mode,
+            size,
+            mtime,
+        }
+    }
+
+    /// Creates a new entry in "removed" state.
+    ///
+    /// `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,
+        }
+    }
+
+    /// 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,
+        }
+    }
+
+    pub fn state(&self) -> EntryState {
+        self.state
+    }
+
+    pub fn mode(&self) -> i32 {
+        self.mode
+    }
+
+    pub fn size(&self) -> i32 {
+        self.size
+    }
+
+    pub fn mtime(&self) -> i32 {
+        self.mtime
+    }
+
+    /// Returns `(state, mode, size, mtime)` for the puprose of serialization
+    /// in the dirstate-v1 format.
+    ///
+    /// This includes marker values such as `mtime == -1`. In the future we may
+    /// 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)
+    }
+
     pub fn is_non_normal(&self) -> bool {
         self.state != EntryState::Normal || self.mtime == MTIME_UNSET
     }
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
@@ -79,36 +79,40 @@ 
         from_p2: bool,
         possibly_dirty: bool,
     ) -> Result<(), DirstateError> {
-        let mut entry = entry;
+        let state;
+        let size;
+        let mtime;
         if added {
-            assert!(!merged);
             assert!(!possibly_dirty);
             assert!(!from_p2);
-            entry.state = EntryState::Added;
-            entry.size = SIZE_NON_NORMAL;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Added;
+            size = SIZE_NON_NORMAL;
+            mtime = MTIME_UNSET;
         } else if merged {
             assert!(!possibly_dirty);
             assert!(!from_p2);
-            entry.state = EntryState::Merged;
-            entry.size = SIZE_FROM_OTHER_PARENT;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Merged;
+            size = SIZE_FROM_OTHER_PARENT;
+            mtime = MTIME_UNSET;
         } else if from_p2 {
             assert!(!possibly_dirty);
-            entry.state = EntryState::Normal;
-            entry.size = SIZE_FROM_OTHER_PARENT;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Normal;
+            size = SIZE_FROM_OTHER_PARENT;
+            mtime = MTIME_UNSET;
         } else if possibly_dirty {
-            entry.state = EntryState::Normal;
-            entry.size = SIZE_NON_NORMAL;
-            entry.mtime = MTIME_UNSET;
+            state = EntryState::Normal;
+            size = SIZE_NON_NORMAL;
+            mtime = MTIME_UNSET;
         } else {
-            entry.state = EntryState::Normal;
-            entry.size = entry.size & V1_RANGEMASK;
-            entry.mtime = entry.mtime & V1_RANGEMASK;
+            state = EntryState::Normal;
+            size = entry.size() & V1_RANGEMASK;
+            mtime = entry.mtime() & V1_RANGEMASK;
         }
+        let mode = entry.mode();
+        let entry = DirstateEntry::from_v1_data(state, mode, size, mtime);
+
         let old_state = match self.get(filename) {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
         if old_state == EntryState::Unknown || old_state == EntryState::Removed
@@ -150,7 +154,7 @@ 
     ) -> Result<(), DirstateError> {
         let old_entry_opt = self.get(filename);
         let old_state = match old_entry_opt {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
         let mut size = 0;
@@ -161,10 +165,10 @@ 
             // would be nice.
             if let Some(old_entry) = old_entry_opt {
                 // backup the previous state
-                if old_entry.state == EntryState::Merged {
+                if old_entry.state() == EntryState::Merged {
                     size = SIZE_NON_NORMAL;
-                } else if old_entry.state == EntryState::Normal
-                    && old_entry.size == SIZE_FROM_OTHER_PARENT
+                } else if old_entry.state() == EntryState::Normal
+                    && old_entry.size() == SIZE_FROM_OTHER_PARENT
                 {
                     // other parent
                     size = SIZE_FROM_OTHER_PARENT;
@@ -189,15 +193,8 @@ 
             self.copy_map.remove(filename);
         }
 
-        self.state_map.insert(
-            filename.to_owned(),
-            DirstateEntry {
-                state: EntryState::Removed,
-                mode: 0,
-                size,
-                mtime: 0,
-            },
-        );
+        self.state_map
+            .insert(filename.to_owned(), DirstateEntry::new_removed(size));
         self.get_non_normal_other_parent_entries()
             .0
             .insert(filename.to_owned());
@@ -211,7 +208,7 @@ 
         filename: &HgPath,
     ) -> Result<bool, DirstateError> {
         let old_state = match self.get(filename) {
-            Some(e) => e.state,
+            Some(e) => e.state(),
             None => EntryState::Unknown,
         };
         let exists = self.state_map.remove(filename).is_some();
@@ -428,12 +425,7 @@ 
 
         map.add_file(
             HgPath::new(b"meh"),
-            DirstateEntry {
-                state: EntryState::Normal,
-                mode: 1337,
-                mtime: 1337,
-                size: 1337,
-            },
+            DirstateEntry::from_v1_data(EntryState::Normal, 1337, 1337, 1337),
             false,
             false,
             false,
@@ -465,12 +457,7 @@ 
         .map(|(fname, (state, mode, size, mtime))| {
             (
                 HgPathBuf::from_bytes(fname.as_ref()),
-                DirstateEntry {
-                    state: *state,
-                    mode: *mode,
-                    size: *size,
-                    mtime: *mtime,
-                },
+                DirstateEntry::from_v1_data(*state, *mode, *size, *mtime),
             )
         })
         .collect();
diff --git a/rust/hg-core/src/dirstate/dirs_multiset.rs b/rust/hg-core/src/dirstate/dirs_multiset.rs
--- a/rust/hg-core/src/dirstate/dirs_multiset.rs
+++ b/rust/hg-core/src/dirstate/dirs_multiset.rs
@@ -49,7 +49,7 @@ 
             let filename = filename.as_ref();
             // This `if` is optimized out of the loop
             if only_tracked {
-                if entry.state != EntryState::Removed {
+                if entry.state() != EntryState::Removed {
                     multiset.add_path(filename)?;
                 }
             } else {
@@ -372,12 +372,7 @@ 
         let input_map = ["b/x", "a/c", "a/d/x"].iter().map(|f| {
             Ok((
                 HgPathBuf::from_bytes(f.as_bytes()),
-                DirstateEntry {
-                    state: EntryState::Normal,
-                    mode: 0,
-                    mtime: 0,
-                    size: 0,
-                },
+                DirstateEntry::from_v1_data(EntryState::Normal, 0, 0, 0),
             ))
         });
         let expected_inner = [("", 2), ("a", 2), ("b", 1), ("a/d", 1)]
@@ -404,12 +399,7 @@ 
         .map(|(f, state)| {
             Ok((
                 HgPathBuf::from_bytes(f.as_bytes()),
-                DirstateEntry {
-                    state: *state,
-                    mode: 0,
-                    mtime: 0,
-                    size: 0,
-                },
+                DirstateEntry::from_v1_data(*state, 0, 0, 0),
             ))
         });