( )⚙ D11484 rust: Align DirstateEntry internals with Python/C DirstateItem

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11484

rust: Align DirstateEntry internals with Python/C DirstateItem
ClosedPublic

Authored by SimonSapin on Sep 22 2021, 4:55 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG008959fcbfb2: rust: Align DirstateEntry internals with Python/C DirstateItem
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Sep 22 2021, 4:55 PM
Alphare accepted this revision.Sep 23 2021, 11:07 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/Cargo.lock
3

That's my bad, I didn't check when amending the other changeset. We'll let it live inside this changeset since it doesn't matter other than being annoying.

This revision is now accepted and ready to land.Sep 23 2021, 11:07 AM
Alphare added a comment.Sep 24 2021, 5:52 AM

Note that every commit description has a capital letter after the topic Align should be align, or check-code will complain.

Revision Contents
Changeset List

Diff 30411

rust/Cargo.lock

# This file is automatically @generated by Cargo. # This file is automatically @generated by Cargo.
# It is not intended for manual editing. # It is not intended for manual editing.
AlphareUnsubmitted
Not Done

That's my bad, I didn't check when amending the other changeset. We'll let it live inside this changeset since it doesn't matter other than being annoying.

Alphare: That's my bad, I didn't check when amending the other changeset. We'll let it live inside this…
[[package]] [[package]]
name = "adler" name = "adler"
version = "0.2.3" version = "0.2.3"
source = "registry+https://github.com/rust-lang/crates.io-index" source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "ee2a4ec343196209d6594e19543ae87a39f96d5534d7174822a3ad825dd6ed7e" checksum = "ee2a4ec343196209d6594e19543ae87a39f96d5534d7174822a3ad825dd6ed7e"
[[package]] [[package]]
name = "aho-corasick" name = "aho-corasick"
dependencies = [ dependencies = [
"libc", "libc",
] ]
[[package]] [[package]]
name = "hg-core" name = "hg-core"
version = "0.1.0" version = "0.1.0"
dependencies = [ dependencies = [
"bitflags",
"byteorder", "byteorder",
"bytes-cast", "bytes-cast",
"clap", "clap",
"crossbeam-channel 0.4.4", "crossbeam-channel 0.4.4",
"derive_more", "derive_more",
"flate2", "flate2",
"format-bytes", "format-bytes",
"home", "home",

rust/hg-core/Cargo.toml

[package] [package]
name = "hg-core" name = "hg-core"
version = "0.1.0" version = "0.1.0"
authors = ["Georges Racinet <gracinet@anybox.fr>"] authors = ["Georges Racinet <gracinet@anybox.fr>"]
description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)" description = "Mercurial pure Rust core library, with no assumption on Python bindings (FFI)"
edition = "2018" edition = "2018"
[lib] [lib]
name = "hg" name = "hg"
[dependencies] [dependencies]
bitflags = "1.2"
bytes-cast = "0.2" bytes-cast = "0.2"
byteorder = "1.3.4" byteorder = "1.3.4"
derive_more = "0.99" derive_more = "0.99"
home = "0.5" home = "0.5"
im-rc = "15.0.*" im-rc = "15.0.*"
itertools = "0.9" itertools = "0.9"
lazy_static = "1.4.0" lazy_static = "1.4.0"
rand = "0.7.3" rand = "0.7.3"

rust/hg-core/src/dirstate/dirstate_map.rs

( (
HgPathBuf::from_bytes(fname.as_ref()), HgPathBuf::from_bytes(fname.as_ref()),
DirstateEntry::from_v1_data(*state, *mode, *size, *mtime), DirstateEntry::from_v1_data(*state, *mode, *size, *mtime),
) )
}) })
.collect(); .collect();
let mut non_normal = [ 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() .iter()
.map(|x| HgPathBuf::from_bytes(x.as_ref())) .map(|x| HgPathBuf::from_bytes(x.as_ref()))
.collect(); .collect();
let mut other_parent = HashSet::new(); let mut other_parent = HashSet::new();
other_parent.insert(HgPathBuf::from_bytes(b"f4")); other_parent.insert(HgPathBuf::from_bytes(b"f4"));
let entries = map.get_non_normal_other_parent_entries(); let entries = map.get_non_normal_other_parent_entries();
assert_eq!( assert_eq!(
(&mut non_normal, &mut other_parent), (&mut non_normal, &mut other_parent),
(entries.0, entries.1) (entries.0, entries.1)
); );
} }
} }

rust/hg-core/src/dirstate/entry.rs

use crate::errors::HgError; use crate::errors::HgError;
use bitflags::bitflags;
use std::convert::TryFrom; use std::convert::TryFrom;
#[derive(Copy, Clone, Debug, Eq, PartialEq)] #[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum EntryState { pub enum EntryState {
Normal, Normal,
Added, Added,
Removed, Removed,
Merged, Merged,
} }
/// The C implementation uses all signed types. This will be an issue /// The C implementation uses all signed types. This will be an issue
/// either when 4GB+ source files are commonplace or in 2038, whichever /// either when 4GB+ source files are commonplace or in 2038, whichever
/// comes first. /// comes first.
#[derive(Debug, PartialEq, Copy, Clone)] #[derive(Debug, PartialEq, Copy, Clone)]
pub struct DirstateEntry { pub struct DirstateEntry {
state: EntryState, flags: Flags,
mode: i32, mode: i32,
size: i32, size: i32,
mtime: 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 V1_RANGEMASK: i32 = 0x7FFFFFFF;
pub const MTIME_UNSET: i32 = -1; pub const MTIME_UNSET: i32 = -1;
/// A `DirstateEntry` with a size of `-2` means that it was merged from the /// A `DirstateEntry` with a size of `-2` means that it was merged from the
/// other parent. This allows revert to pick the right status back during a /// other parent. This allows revert to pick the right status back during a
/// merge. /// merge.
pub const SIZE_FROM_OTHER_PARENT: i32 = -2; pub const SIZE_FROM_OTHER_PARENT: i32 = -2;
/// A special value used for internal representation of special case in /// A special value used for internal representation of special case in
/// dirstate v1 format. /// dirstate v1 format.
pub const SIZE_NON_NORMAL: i32 = -1; pub const SIZE_NON_NORMAL: i32 = -1;
impl DirstateEntry { impl DirstateEntry {
pub fn from_v1_data( pub fn from_v1_data(
state: EntryState, state: EntryState,
mode: i32, mode: i32,
size: i32, size: i32,
mtime: i32, mtime: i32,
) -> Self { ) -> 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 { Self {
state, flags: Flags::WDIR_TRACKED
| Flags::P1_TRACKED
| Flags::POSSIBLY_DIRTY,
mode,
size,
mtime: 0,
}
} else {
Self {
flags: Flags::WDIR_TRACKED | Flags::P1_TRACKED,
mode, mode,
size, size,
mtime, 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 {
// 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,
}
}
/// Creates a new entry in "removed" state. /// Creates a new entry in "removed" state.
/// ///
/// `size` is expected to be zero, `SIZE_NON_NORMAL`, or /// `size` is expected to be zero, `SIZE_NON_NORMAL`, or
/// `SIZE_FROM_OTHER_PARENT` /// `SIZE_FROM_OTHER_PARENT`
pub fn new_removed(size: i32) -> Self { pub fn new_removed(size: i32) -> Self {
Self { Self::from_v1_data(EntryState::Removed, 0, size, 0)
state: EntryState::Removed,
mode: 0,
size,
mtime: 0,
}
} }
/// TODO: refactor `DirstateMap::add_file` to not take a `DirstateEntry` /// TODO: refactor `DirstateMap::add_file` to not take a `DirstateEntry`
/// parameter and remove this constructor /// parameter and remove this constructor
pub fn new_for_add_file(mode: i32, size: i32, mtime: i32) -> Self { pub fn new_for_add_file(mode: i32, size: i32, mtime: i32) -> Self {
Self {
// XXX Arbitrary default value since the value is determined later // XXX Arbitrary default value since the value is determined later
state: EntryState::Normal, let state = EntryState::Normal;
mode, Self::from_v1_data(state, mode, size, mtime)
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 { 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 { pub fn mode(&self) -> i32 {
self.mode self.mode
} }
pub fn size(&self) -> i32 { pub fn size(&self) -> i32 {
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 self.size
} }
}
pub fn mtime(&self) -> i32 { pub fn mtime(&self) -> i32 {
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 self.mtime
} }
}
/// Returns `(state, mode, size, mtime)` for the puprose of serialization /// Returns `(state, mode, size, mtime)` for the puprose of serialization
/// in the dirstate-v1 format. /// in the dirstate-v1 format.
/// ///
/// This includes marker values such as `mtime == -1`. In the future we may /// 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 /// want to not represent these cases that way in memory, but serialization
/// will need to keep the same format. /// will need to keep the same format.
pub fn v1_data(&self) -> (u8, i32, i32, i32) { 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 { 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 { 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 // TODO: other platforms
#[cfg(unix)] #[cfg(unix)]
pub fn mode_changed( pub fn mode_changed(
&self, &self,
filesystem_metadata: &std::fs::Metadata, filesystem_metadata: &std::fs::Metadata,
) -> bool { ) -> bool {
use std::os::unix::fs::MetadataExt; use std::os::unix::fs::MetadataExt;
const EXEC_BIT_MASK: u32 = 0o100; 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; let fs_exec_bit = filesystem_metadata.mode() & EXEC_BIT_MASK;
dirstate_exec_bit != fs_exec_bit dirstate_exec_bit != fs_exec_bit
} }
/// Returns a `(state, mode, size, mtime)` tuple as for /// Returns a `(state, mode, size, mtime)` tuple as for
/// `DirstateMapMethods::debug_iter`. /// `DirstateMapMethods::debug_iter`.
pub fn debug_tuple(&self) -> (u8, i32, i32, i32) { 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 { 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 { pub fn clear_ambiguous_mtime(&mut self, now: i32) -> bool {
let ambiguous = self.mtime_is_ambiguous(now); let ambiguous = self.mtime_is_ambiguous(now);
if ambiguous { if ambiguous {
// The file was last modified "simultaneously" with the current // The file was last modified "simultaneously" with the current
// write to dirstate (i.e. within the same second for file- // write to dirstate (i.e. within the same second for file-
// systems with a granularity of 1 sec). This commonly happens // systems with a granularity of 1 sec). This commonly happens