This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9133

rust: start plugging the dirstate tree behind a feature gate
ClosedPublic

Authored by Alphare on Sep 30 2020, 1:36 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG496537c9c1b4: rust: start plugging the dirstate tree behind a feature gate
Summary

The previous patch added the dirstate-tree feature gate to enable the two
dirstate implementations to co-habit while the tree-based one gets better.

This patch copies over the code that differs, be it because the algorithm
changed or because the borrowing rules are different.

Indeed, DirstateTree is not observationally equivalent to the std HashMap in
the APIs we use: it does not have the Entry API (yet?) and its iterator
returns owned values instead of references. This last point is because the
implementation needs to be changed to a more clever and efficient solution.

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

Alphare created this revision.Sep 30 2020, 1:36 PM
Alphare added a commit: rHG496537c9c1b4: rust: start plugging the dirstate tree behind a feature gate.Oct 1 2020, 12:57 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG496537c9c1b4: rust: start plugging the dirstate tree behind a feature gate (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.Oct 1 2020, 1:28 AM
martinvonz added inline comments.
rust/hg-core/src/dirstate/dirs_multiset.rs
60

I think I asked you a long time ago if we still need this ugly argument. Do you know if we do? I.e, I was hoping we could remove it from the Python and C code as well by filtering those entries out before we call this.

66–73

Two alternatives that avoid the repeated multiset.add_path() (it's just one short line, so it's not a big deal, of course). I don't know if they result in the same binary code.

match skip_state {
    Some(skip) if skip == state => {
        // we were told to skip this files in this state
    },
    _ => {
        multiset.add_path(filename)?;
    }
}
if Some(*state) != skip_state {
    multiset.add_path(filename)?;
}
rust/hg-core/src/dirstate/dirstate_map.rs
303–308

Did you consider making these conditions functions on the DirstateEntry struct?

rust/hg-core/src/dirstate/parsers.rs
183

Can we remove the vector by using iter_mut() on line 188?

191–208

Same here, would it make sense to write this something like entry.is_potential_race(now) (feel free to pick a better name)?

rust/hg-core/src/dirstate/status.rs
733

nit: ref no longer needed with recent rust versions? but maybe we need to be compatible with older versions?

Revision Contents
Changeset List

Diff 22963

rust/hg-core/src/dirstate.rs

pub size: i32, pub size: i32,
} }
/// 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;
#[cfg(not(feature = "dirstate-tree"))]
pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>; pub type StateMap = FastHashMap<HgPathBuf, DirstateEntry>;
#[cfg(not(feature = "dirstate-tree"))]
pub type StateMapIter<'a> = hash_map::Iter<'a, HgPathBuf, DirstateEntry>; pub type StateMapIter<'a> = hash_map::Iter<'a, HgPathBuf, DirstateEntry>;
#[cfg(feature = "dirstate-tree")]
pub type StateMap = dirstate_tree::tree::Tree;
#[cfg(feature = "dirstate-tree")]
pub type StateMapIter<'a> = dirstate_tree::iter::Iter<'a>;
pub type CopyMap = FastHashMap<HgPathBuf, HgPathBuf>; pub type CopyMap = FastHashMap<HgPathBuf, HgPathBuf>;
pub type CopyMapIter<'a> = hash_map::Iter<'a, HgPathBuf, HgPathBuf>; pub type CopyMapIter<'a> = hash_map::Iter<'a, HgPathBuf, HgPathBuf>;
#[derive(Copy, Clone, Debug, Eq, PartialEq)] #[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub enum EntryState { pub enum EntryState {
Normal, Normal,
Added, Added,
Removed, Removed,

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

// dirs_multiset.rs // dirs_multiset.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! A multiset of directory names. //! A multiset of directory names.
//! //!
//! Used to counts the references to directories in a manifest or dirstate. //! Used to counts the references to directories in a manifest or dirstate.
use crate::{ use crate::{
dirstate::EntryState, dirstate::EntryState,
utils::{ utils::{
files, files,
hg_path::{HgPath, HgPathBuf, HgPathError}, hg_path::{HgPath, HgPathBuf, HgPathError},
}, },
DirstateEntry, DirstateMapError, FastHashMap, DirstateEntry, DirstateMapError, FastHashMap, StateMap,
}; };
use std::collections::{hash_map, hash_map::Entry, HashMap, HashSet}; use std::collections::{hash_map, hash_map::Entry, HashMap, HashSet};
// could be encapsulated if we care API stability more seriously // could be encapsulated if we care API stability more seriously
pub type DirsMultisetIter<'a> = hash_map::Keys<'a, HgPathBuf, u32>; pub type DirsMultisetIter<'a> = hash_map::Keys<'a, HgPathBuf, u32>;
#[derive(PartialEq, Debug)] #[derive(PartialEq, Debug)]
pub struct DirsMultiset { pub struct DirsMultiset {
inner: FastHashMap<HgPathBuf, u32>, inner: FastHashMap<HgPathBuf, u32>,
} }
impl DirsMultiset { impl DirsMultiset {
/// Initializes the multiset from a dirstate. /// Initializes the multiset from a dirstate.
/// ///
/// If `skip_state` is provided, skips dirstate entries with equal state. /// If `skip_state` is provided, skips dirstate entries with equal state.
#[cfg(not(feature = "dirstate-tree"))]
pub fn from_dirstate( pub fn from_dirstate(
dirstate: &FastHashMap<HgPathBuf, DirstateEntry>, dirstate: &StateMap,
skip_state: Option<EntryState>, skip_state: Option<EntryState>,
) -> Result<Self, DirstateMapError> { ) -> Result<Self, DirstateMapError> {
let mut multiset = DirsMultiset { let mut multiset = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
for (filename, DirstateEntry { state, .. }) in dirstate.iter() {
for (filename, DirstateEntry { state, .. }) in dirstate {
// This `if` is optimized out of the loop // This `if` is optimized out of the loop
if let Some(skip) = skip_state { if let Some(skip) = skip_state {
if skip != *state { if skip != *state {
multiset.add_path(filename)?; multiset.add_path(filename)?;
} }
} else { } else {
multiset.add_path(filename)?; multiset.add_path(filename)?;
} }
} }
Ok(multiset) Ok(multiset)
} }
/// Initializes the multiset from a dirstate.
///
/// If `skip_state` is provided, skips dirstate entries with equal state.
#[cfg(feature = "dirstate-tree")]
pub fn from_dirstate(
dirstate: &StateMap,
skip_state: Option<EntryState>,
martinvonzUnsubmitted
Not Done

I think I asked you a long time ago if we still need this ugly argument. Do you know if we do? I.e, I was hoping we could remove it from the Python and C code as well by filtering those entries out before we call this.

martinvonz: I think I asked you a long time ago if we still need this ugly argument. Do you know if we do?
) -> Result<Self, DirstateMapError> {
let mut multiset = DirsMultiset {
inner: FastHashMap::default(),
};
for (filename, DirstateEntry { state, .. }) in dirstate.iter() {
// This `if` is optimized out of the loop
if let Some(skip) = skip_state {
if skip != state {
multiset.add_path(filename)?;
}
} else {
multiset.add_path(filename)?;
}
martinvonzUnsubmitted
Not Done

Two alternatives that avoid the repeated multiset.add_path() (it's just one short line, so it's not a big deal, of course). I don't know if they result in the same binary code.

match skip_state {
    Some(skip) if skip == state => {
        // we were told to skip this files in this state
    },
    _ => {
        multiset.add_path(filename)?;
    }
}
if Some(*state) != skip_state {
    multiset.add_path(filename)?;
}
martinvonz: Two alternatives that avoid the repeated `multiset.add_path()` (it's just one short line, so…
}
Ok(multiset)
}
/// Initializes the multiset from a manifest. /// Initializes the multiset from a manifest.
pub fn from_manifest( pub fn from_manifest(
manifest: &[impl AsRef<HgPath>], manifest: &[impl AsRef<HgPath>],
) -> Result<Self, DirstateMapError> { ) -> Result<Self, DirstateMapError> {
let mut multiset = DirsMultiset { let mut multiset = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
fn test_dirsmultiset_new_empty() { fn test_dirsmultiset_new_empty() {
let manifest: Vec<HgPathBuf> = vec![]; let manifest: Vec<HgPathBuf> = vec![];
let new = DirsMultiset::from_manifest(&manifest).unwrap(); let new = DirsMultiset::from_manifest(&manifest).unwrap();
let expected = DirsMultiset { let expected = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
assert_eq!(expected, new); assert_eq!(expected, new);
let new = DirsMultiset::from_dirstate(&FastHashMap::default(), None) let new =
.unwrap(); DirsMultiset::from_dirstate(&StateMap::default(), None).unwrap();
let expected = DirsMultiset { let expected = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
assert_eq!(expected, new); assert_eq!(expected, new);
} }
#[test] #[test]
fn test_dirsmultiset_new_no_skip() { fn test_dirsmultiset_new_no_skip() {
let input_vec: Vec<HgPathBuf> = ["a/", "b/", "a/c", "a/d/"] let input_vec: Vec<HgPathBuf> = ["a/", "b/", "a/c", "a/d/"]
.iter() .iter()
.map(|e| HgPathBuf::from_bytes(e.as_bytes())) .map(|e| HgPathBuf::from_bytes(e.as_bytes()))
.collect(); .collect();
let expected_inner = [("", 2), ("a", 3), ("b", 1), ("a/d", 1)] let expected_inner = [("", 2), ("a", 3), ("b", 1), ("a/d", 1)]
.iter() .iter()
.map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v)) .map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v))
.collect(); .collect();
let new = DirsMultiset::from_manifest(&input_vec).unwrap(); let new = DirsMultiset::from_manifest(&input_vec).unwrap();
let expected = DirsMultiset { let expected = DirsMultiset {
inner: expected_inner, inner: expected_inner,
}; };
assert_eq!(expected, new); assert_eq!(expected, new);
let input_map = ["a/", "b/", "a/c", "a/d/"] let input_map = ["b/x", "a/c", "a/d/x"]
.iter() .iter()
.map(|f| { .map(|f| {
( (
HgPathBuf::from_bytes(f.as_bytes()), HgPathBuf::from_bytes(f.as_bytes()),
DirstateEntry { DirstateEntry {
state: EntryState::Normal, state: EntryState::Normal,
mode: 0, mode: 0,
mtime: 0, mtime: 0,
size: 0, size: 0,
}, },
) )
}) })
.collect(); .collect();
let expected_inner = [("", 2), ("a", 3), ("b", 1), ("a/d", 1)] let expected_inner = [("", 2), ("a", 2), ("b", 1), ("a/d", 1)]
.iter() .iter()
.map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v)) .map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v))
.collect(); .collect();
let new = DirsMultiset::from_dirstate(&input_map, None).unwrap(); let new = DirsMultiset::from_dirstate(&input_map, None).unwrap();
let expected = DirsMultiset { let expected = DirsMultiset {
inner: expected_inner, inner: expected_inner,
}; };
assert_eq!(expected, new); assert_eq!(expected, new);
} }
#[test] #[test]
fn test_dirsmultiset_new_skip() { fn test_dirsmultiset_new_skip() {
let input_map = [ let input_map = [
("a/", EntryState::Normal), ("a/", EntryState::Normal),
("a/b/", EntryState::Normal), ("a/b", EntryState::Normal),
("a/c", EntryState::Removed), ("a/c", EntryState::Removed),
("a/d/", EntryState::Merged), ("a/d", EntryState::Merged),
] ]
.iter() .iter()
.map(|(f, state)| { .map(|(f, state)| {
( (
HgPathBuf::from_bytes(f.as_bytes()), HgPathBuf::from_bytes(f.as_bytes()),
DirstateEntry { DirstateEntry {
state: *state, state: *state,
mode: 0, mode: 0,
mtime: 0, mtime: 0,
size: 0, size: 0,
}, },
) )
}) })
.collect(); .collect();
// "a" incremented with "a/c" and "a/d/" // "a" incremented with "a/c" and "a/d/"
let expected_inner = [("", 1), ("a", 2), ("a/d", 1)] let expected_inner = [("", 1), ("a", 2)]
.iter() .iter()
.map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v)) .map(|(k, v)| (HgPathBuf::from_bytes(k.as_bytes()), *v))
.collect(); .collect();
let new = let new =
DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal)) DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal))
.unwrap(); .unwrap();
let expected = DirsMultiset { let expected = DirsMultiset {
inner: expected_inner, inner: expected_inner,
}; };
assert_eq!(expected, new); assert_eq!(expected, new);
} }
} }

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

pack_dirstate, parse_dirstate, pack_dirstate, parse_dirstate,
utils::{ utils::{
files::normalize_case, files::normalize_case,
hg_path::{HgPath, HgPathBuf}, hg_path::{HgPath, HgPathBuf},
}, },
CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateMapError, CopyMap, DirsMultiset, DirstateEntry, DirstateError, DirstateMapError,
DirstateParents, DirstateParseError, FastHashMap, StateMap, DirstateParents, DirstateParseError, FastHashMap, StateMap,
}; };
use core::borrow::Borrow;
use micro_timer::timed; use micro_timer::timed;
use std::collections::HashSet; use std::collections::HashSet;
use std::convert::TryInto; use std::convert::TryInto;
use std::iter::FromIterator; use std::iter::FromIterator;
use std::ops::Deref; use std::ops::Deref;
use std::time::Duration; use std::time::Duration;
pub type FileFoldMap = FastHashMap<HgPathBuf, HgPathBuf>; pub type FileFoldMap = FastHashMap<HgPathBuf, HgPathBuf>;
} }
impl DirstateMap { impl DirstateMap {
pub fn new() -> Self { pub fn new() -> Self {
Self::default() Self::default()
} }
pub fn clear(&mut self) { pub fn clear(&mut self) {
self.state_map.clear(); self.state_map = StateMap::default();
self.copy_map.clear(); self.copy_map.clear();
self.file_fold_map = None; self.file_fold_map = None;
self.non_normal_set = None; self.non_normal_set = None;
self.other_parent_set = None; self.other_parent_set = None;
self.set_parents(&DirstateParents { self.set_parents(&DirstateParents {
p1: NULL_NODE_ID, p1: NULL_NODE_ID,
p2: NULL_NODE_ID, p2: NULL_NODE_ID,
}) })
pub fn clear_ambiguous_times( pub fn clear_ambiguous_times(
&mut self, &mut self,
filenames: Vec<HgPathBuf>, filenames: Vec<HgPathBuf>,
now: i32, now: i32,
) { ) {
for filename in filenames { for filename in filenames {
let mut changed = false; let mut changed = false;
self.state_map if let Some(entry) = self.state_map.get_mut(&filename) {
.entry(filename.to_owned()) if entry.state == EntryState::Normal && entry.mtime == now {
.and_modify(|entry| {
if entry.state == EntryState::Normal && entry.mtime == now
{
changed = true; changed = true;
*entry = DirstateEntry { *entry = DirstateEntry {
mtime: MTIME_UNSET, mtime: MTIME_UNSET,
..*entry ..*entry
}; };
} }
}); }
if changed { if changed {
self.get_non_normal_other_parent_entries() self.get_non_normal_other_parent_entries()
.0 .0
.insert(filename.to_owned()); .insert(filename.to_owned());
} }
} }
} }
&self, &self,
) -> (&HashSet<HgPathBuf>, &HashSet<HgPathBuf>) { ) -> (&HashSet<HgPathBuf>, &HashSet<HgPathBuf>) {
( (
self.non_normal_set.as_ref().unwrap(), self.non_normal_set.as_ref().unwrap(),
self.other_parent_set.as_ref().unwrap(), self.other_parent_set.as_ref().unwrap(),
) )
} }
#[cfg(not(feature = "dirstate-tree"))]
pub fn set_non_normal_other_parent_entries(&mut self, force: bool) { pub fn set_non_normal_other_parent_entries(&mut self, force: bool) {
if !force if !force
&& self.non_normal_set.is_some() && self.non_normal_set.is_some()
&& self.other_parent_set.is_some() && self.other_parent_set.is_some()
{ {
return; return;
} }
let mut non_normal = HashSet::new(); let mut non_normal = HashSet::new();
if *state == EntryState::Normal && *size == SIZE_FROM_OTHER_PARENT if *state == EntryState::Normal && *size == SIZE_FROM_OTHER_PARENT
{ {
other_parent.insert(filename.to_owned()); other_parent.insert(filename.to_owned());
} }
} }
self.non_normal_set = Some(non_normal); self.non_normal_set = Some(non_normal);
self.other_parent_set = Some(other_parent); self.other_parent_set = Some(other_parent);
} }
#[cfg(feature = "dirstate-tree")]
pub fn set_non_normal_other_parent_entries(&mut self, force: bool) {
if !force
&& self.non_normal_set.is_some()
&& self.other_parent_set.is_some()
{
return;
}
let mut non_normal = HashSet::new();
let mut other_parent = HashSet::new();
for (
filename,
DirstateEntry {
state, size, mtime, ..
},
) in self.state_map.iter()
{
if state != EntryState::Normal || mtime == MTIME_UNSET {
non_normal.insert(filename.to_owned());
}
if state == EntryState::Normal && size == SIZE_FROM_OTHER_PARENT {
other_parent.insert(filename.to_owned());
}
martinvonzUnsubmitted
Not Done

Did you consider making these conditions functions on the DirstateEntry struct?

martinvonz: Did you consider making these conditions functions on the `DirstateEntry` struct?
}
self.non_normal_set = Some(non_normal);
self.other_parent_set = Some(other_parent);
}
/// Both of these setters and their uses appear to be the simplest way to /// Both of these setters and their uses appear to be the simplest way to
/// emulate a Python lazy property, but it is ugly and unidiomatic. /// emulate a Python lazy property, but it is ugly and unidiomatic.
/// TODO One day, rewriting this struct using the typestate might be a /// TODO One day, rewriting this struct using the typestate might be a
/// good idea. /// good idea.
pub fn set_all_dirs(&mut self) -> Result<(), DirstateMapError> { pub fn set_all_dirs(&mut self) -> Result<(), DirstateMapError> {
if self.all_dirs.is_none() { if self.all_dirs.is_none() {
self.all_dirs = self.all_dirs =
let packed = let packed =
pack_dirstate(&mut self.state_map, &self.copy_map, parents, now)?; pack_dirstate(&mut self.state_map, &self.copy_map, parents, now)?;
self.dirty_parents = false; self.dirty_parents = false;
self.set_non_normal_other_parent_entries(true); self.set_non_normal_other_parent_entries(true);
Ok(packed) Ok(packed)
} }
#[cfg(not(feature = "dirstate-tree"))]
pub fn build_file_fold_map(&mut self) -> &FileFoldMap { pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map { if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map; return file_fold_map;
} }
let mut new_file_fold_map = FileFoldMap::default(); let mut new_file_fold_map = FileFoldMap::default();
for (filename, DirstateEntry { state, .. }) in self.state_map.borrow()
{ for (filename, DirstateEntry { state, .. }) in self.state_map.iter() {
if *state == EntryState::Removed { if *state == EntryState::Removed {
new_file_fold_map new_file_fold_map
.insert(normalize_case(filename), filename.to_owned()); .insert(normalize_case(&filename), filename.to_owned());
}
}
self.file_fold_map = Some(new_file_fold_map);
self.file_fold_map.as_ref().unwrap()
}
#[cfg(feature = "dirstate-tree")]
pub fn build_file_fold_map(&mut self) -> &FileFoldMap {
if let Some(ref file_fold_map) = self.file_fold_map {
return file_fold_map;
}
let mut new_file_fold_map = FileFoldMap::default();
for (filename, DirstateEntry { state, .. }) in self.state_map.iter() {
if state == EntryState::Removed {
new_file_fold_map
.insert(normalize_case(&filename), filename.to_owned());
} }
} }
self.file_fold_map = Some(new_file_fold_map); self.file_fold_map = Some(new_file_fold_map);
self.file_fold_map.as_ref().unwrap() self.file_fold_map.as_ref().unwrap()
} }
} }
#[cfg(test)] #[cfg(test)]

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

state, state,
mode, mode,
size, size,
mtime, mtime,
}, },
)); ));
curr_pos = curr_pos + MIN_ENTRY_SIZE + (path_len); curr_pos = curr_pos + MIN_ENTRY_SIZE + (path_len);
} }
Ok((parents, entries, copies)) Ok((parents, entries, copies))
} }
/// `now` is the duration in seconds since the Unix epoch /// `now` is the duration in seconds since the Unix epoch
#[cfg(not(feature = "dirstate-tree"))]
pub fn pack_dirstate( pub fn pack_dirstate(
state_map: &mut StateMap, state_map: &mut StateMap,
copy_map: &CopyMap, copy_map: &CopyMap,
parents: DirstateParents, parents: DirstateParents,
now: Duration, now: Duration,
) -> Result<Vec<u8>, DirstatePackError> { ) -> Result<Vec<u8>, DirstatePackError> {
// TODO move away from i32 before 2038. // TODO move away from i32 before 2038.
let now: i32 = now.as_secs().try_into().expect("time overflow"); let now: i32 = now.as_secs().try_into().expect("time overflow");
if packed.len() != expected_size { if packed.len() != expected_size {
return Err(DirstatePackError::BadSize(expected_size, packed.len())); return Err(DirstatePackError::BadSize(expected_size, packed.len()));
} }
state_map.extend(new_state_map); state_map.extend(new_state_map);
Ok(packed) Ok(packed)
} }
/// `now` is the duration in seconds since the Unix epoch
#[cfg(feature = "dirstate-tree")]
pub fn pack_dirstate(
state_map: &mut StateMap,
copy_map: &CopyMap,
parents: DirstateParents,
now: Duration,
) -> Result<Vec<u8>, DirstatePackError> {
// TODO move away from i32 before 2038.
let now: i32 = now.as_secs().try_into().expect("time overflow");
let expected_size: usize = state_map
.iter()
.map(|(filename, _)| {
let mut length = MIN_ENTRY_SIZE + filename.len();
if let Some(copy) = copy_map.get(&filename) {
length += copy.len() + 1;
}
length
})
.sum();
let expected_size = expected_size + PARENT_SIZE * 2;
let mut packed = Vec::with_capacity(expected_size);
let mut new_state_map = vec![];
martinvonzUnsubmitted
Not Done

Can we remove the vector by using iter_mut() on line 188?

martinvonz: Can we remove the vector by using `iter_mut()` on line 188?
packed.extend(&parents.p1);
packed.extend(&parents.p2);
for (filename, entry) in state_map.iter() {
let new_filename = filename.to_owned();
let mut new_mtime: i32 = entry.mtime;
if entry.state == EntryState::Normal && entry.mtime == now {
// The file was last modified "simultaneously" with the current
// write to dirstate (i.e. within the same second for file-
// systems with a granularity of 1 sec). This commonly happens
// for at least a couple of files on 'update'.
// The user could change the file without changing its size
// within the same second. Invalidate the file's mtime in
// dirstate, forcing future 'status' calls to compare the
// contents of the file if the size is the same. This prevents
// mistakenly treating such files as clean.
new_mtime = -1;
new_state_map.push((
filename.to_owned(),
DirstateEntry {
mtime: new_mtime,
..entry
},
));
martinvonzUnsubmitted
Not Done

Same here, would it make sense to write this something like entry.is_potential_race(now) (feel free to pick a better name)?

martinvonz: Same here, would it make sense to write this something like `entry.is_potential_race(now)`…
}
let mut new_filename = new_filename.into_vec();
if let Some(copy) = copy_map.get(&filename) {
new_filename.push(b'\0');
new_filename.extend(copy.bytes());
}
packed.write_u8(entry.state.into())?;
packed.write_i32::<BigEndian>(entry.mode)?;
packed.write_i32::<BigEndian>(entry.size)?;
packed.write_i32::<BigEndian>(new_mtime)?;
packed.write_i32::<BigEndian>(new_filename.len() as i32)?;
packed.extend(new_filename)
}
if packed.len() != expected_size {
return Err(DirstatePackError::BadSize(expected_size, packed.len()));
}
state_map.extend(new_state_map);
Ok(packed)
}
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use crate::{utils::hg_path::HgPathBuf, FastHashMap}; use crate::{utils::hg_path::HgPathBuf, FastHashMap};
use pretty_assertions::assert_eq;
#[test] #[test]
fn test_pack_dirstate_empty() { fn test_pack_dirstate_empty() {
let mut state_map: StateMap = FastHashMap::default(); let mut state_map = StateMap::default();
let copymap = FastHashMap::default(); let copymap = FastHashMap::default();
let parents = DirstateParents { let parents = DirstateParents {
p1: *b"12345678910111213141", p1: *b"12345678910111213141",
p2: *b"00000000000000000000", p2: *b"00000000000000000000",
}; };
let now = Duration::new(15000000, 0); let now = Duration::new(15000000, 0);
let expected = b"1234567891011121314100000000000000000000".to_vec(); let expected = b"1234567891011121314100000000000000000000".to_vec();

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

// status.rs // status.rs
// //
// Copyright 2019 Raphaël Gomès <rgomes@octobus.net> // Copyright 2019 Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
//! Rust implementation of dirstate.status (dirstate.py). //! Rust implementation of dirstate.status (dirstate.py).
//! It is currently missing a lot of functionality compared to the Python one //! It is currently missing a lot of functionality compared to the Python one
//! and will only be triggered in narrow cases. //! and will only be triggered in narrow cases.
#[cfg(feature = "dirstate-tree")]
use crate::dirstate::dirstate_tree::iter::StatusShortcut;
#[cfg(not(feature = "dirstate-tree"))]
use crate::utils::path_auditor::PathAuditor;
use crate::{ use crate::{
dirstate::SIZE_FROM_OTHER_PARENT, dirstate::SIZE_FROM_OTHER_PARENT,
filepatterns::PatternFileWarning, filepatterns::PatternFileWarning,
matchers::{get_ignore_function, Matcher, VisitChildrenSet}, matchers::{get_ignore_function, Matcher, VisitChildrenSet},
utils::{ utils::{
files::{find_dirs, HgMetadata}, files::{find_dirs, HgMetadata},
hg_path::{ hg_path::{
hg_path_to_path_buf, os_string_to_hg_path_buf, HgPath, HgPathBuf, hg_path_to_path_buf, os_string_to_hg_path_buf, HgPath, HgPathBuf,
HgPathError, HgPathError,
}, },
path_auditor::PathAuditor,
}, },
CopyMap, DirstateEntry, DirstateMap, EntryState, FastHashMap, CopyMap, DirstateEntry, DirstateMap, EntryState, FastHashMap,
PatternError, PatternError,
}; };
use lazy_static::lazy_static; use lazy_static::lazy_static;
use micro_timer::timed; use micro_timer::timed;
use rayon::prelude::*; use rayon::prelude::*;
use std::{ use std::{
traversed_sender.clone(), traversed_sender.clone(),
)?; )?;
} }
} }
Ok(()) Ok(())
}) })
} }
/// Add the files in the dirstate to the results.
///
/// This takes a mutable reference to the results to account for the
/// `extend` in timings
#[cfg(feature = "dirstate-tree")]
#[timed]
pub fn extend_from_dmap(&self, results: &mut Vec<DispatchedPath<'a>>) {
results.par_extend(
self.dmap
.fs_iter(self.root_dir.clone())
.par_bridge()
.filter(|(path, _)| self.matcher.matches(path))
.flat_map(move |(filename, shortcut)| {
let entry = match shortcut {
StatusShortcut::Entry(e) => e,
StatusShortcut::Dispatch(d) => {
return Ok((Cow::Owned(filename), d))
}
};
let filename_as_path = hg_path_to_path_buf(&filename)?;
let meta = self
.root_dir
.join(filename_as_path)
.symlink_metadata();
match meta {
Ok(ref m)
martinvonzUnsubmitted
Not Done

nit: ref no longer needed with recent rust versions? but maybe we need to be compatible with older versions?

martinvonz: nit: `ref` no longer needed with recent rust versions? but maybe we need to be compatible with…
if !(m.file_type().is_file()
|| m.file_type().is_symlink()) =>
{
Ok((
Cow::Owned(filename),
dispatch_missing(entry.state),
))
}
Ok(m) => {
let dispatch = dispatch_found(
&filename,
entry,
HgMetadata::from_metadata(m),
&self.dmap.copy_map,
self.options,
);
Ok((Cow::Owned(filename), dispatch))
}
Err(ref e)
if e.kind() == ErrorKind::NotFound
|| e.raw_os_error() == Some(20) =>
{
// Rust does not yet have an `ErrorKind` for
// `NotADirectory` (errno 20)
// It happens if the dirstate contains `foo/bar`
// and foo is not a
// directory
Ok((
Cow::Owned(filename),
dispatch_missing(entry.state),
))
}
Err(e) => Err(e),
}
}),
);
}
/// Add the files in the dirstate to the results.
///
/// This takes a mutable reference to the results to account for the
/// `extend` in timings
#[cfg(not(feature = "dirstate-tree"))]
#[timed]
pub fn extend_from_dmap(&self, results: &mut Vec<DispatchedPath<'a>>) {
results.par_extend(self.dmap.par_iter().flat_map(
move |(filename, entry)| {
let filename: &HgPath = filename;
let filename_as_path = hg_path_to_path_buf(filename)?;
let meta =
self.root_dir.join(filename_as_path).symlink_metadata();
match meta {
Ok(ref m)
if !(m.file_type().is_file()
|| m.file_type().is_symlink()) =>
{
Ok((
Cow::Borrowed(filename),
dispatch_missing(entry.state),
))
}
Ok(m) => Ok((
Cow::Borrowed(filename),
dispatch_found(
filename,
*entry,
HgMetadata::from_metadata(m),
&self.dmap.copy_map,
self.options,
),
)),
Err(ref e)
if e.kind() == ErrorKind::NotFound
|| e.raw_os_error() == Some(20) =>
{
// Rust does not yet have an `ErrorKind` for
// `NotADirectory` (errno 20)
// It happens if the dirstate contains `foo/bar`
// and foo is not a
// directory
Ok((
Cow::Borrowed(filename),
dispatch_missing(entry.state),
))
}
Err(e) => Err(e),
}
},
));
}
/// Checks all files that are in the dirstate but were not found during the /// Checks all files that are in the dirstate but were not found during the
/// working directory traversal. This means that the rest must /// working directory traversal. This means that the rest must
/// be either ignored, under a symlink or under a new nested repo. /// be either ignored, under a symlink or under a new nested repo.
/// ///
/// This takes a mutable reference to the results to account for the /// This takes a mutable reference to the results to account for the
/// `extend` in timings /// `extend` in timings
#[cfg(not(feature = "dirstate-tree"))]
#[timed] #[timed]
pub fn handle_unknowns( pub fn handle_unknowns(
&self, &self,
results: &mut Vec<DispatchedPath<'a>>, results: &mut Vec<DispatchedPath<'a>>,
) -> IoResult<()> { ) -> IoResult<()> {
let to_visit: Vec<(&HgPath, &DirstateEntry)> = let to_visit: Vec<(&HgPath, &DirstateEntry)> =
if results.is_empty() && self.matcher.matches_everything() { if results.is_empty() && self.matcher.matches_everything() {
self.dmap.iter().map(|(f, e)| (f.deref(), e)).collect() self.dmap.iter().map(|(f, e)| (f.deref(), e)).collect()
} }
}) })
.collect(); .collect();
results.par_extend(new_results?); results.par_extend(new_results?);
Ok(()) Ok(())
} }
/// Add the files in the dirstate to the results.
///
/// This takes a mutable reference to the results to account for the
/// `extend` in timings
#[timed]
pub fn extend_from_dmap(&self, results: &mut Vec<DispatchedPath<'a>>) {
results.par_extend(self.dmap.par_iter().flat_map(
move |(filename, entry)| {
let filename: &HgPath = filename;
let filename_as_path = hg_path_to_path_buf(filename)?;
let meta =
self.root_dir.join(filename_as_path).symlink_metadata();
match meta {
Ok(ref m)
if !(m.file_type().is_file()
|| m.file_type().is_symlink()) =>
{
Ok((
Cow::Borrowed(filename),
dispatch_missing(entry.state),
))
}
Ok(m) => Ok((
Cow::Borrowed(filename),
dispatch_found(
filename,
*entry,
HgMetadata::from_metadata(m),
&self.dmap.copy_map,
self.options,
),
)),
Err(ref e)
if e.kind() == ErrorKind::NotFound
|| e.raw_os_error() == Some(20) =>
{
// Rust does not yet have an `ErrorKind` for
// `NotADirectory` (errno 20)
// It happens if the dirstate contains `foo/bar`
// and foo is not a
// directory
Ok((
Cow::Borrowed(filename),
dispatch_missing(entry.state),
))
}
Err(e) => Err(e),
}
},
));
}
} }
#[timed] #[timed]
pub fn build_response<'a>( pub fn build_response<'a>(
results: impl IntoIterator<Item = DispatchedPath<'a>>, results: impl IntoIterator<Item = DispatchedPath<'a>>,
traversed: Vec<HgPathBuf>, traversed: Vec<HgPathBuf>,
) -> (Vec<HgPathCow<'a>>, DirstateStatus<'a>) { ) -> (Vec<HgPathCow<'a>>, DirstateStatus<'a>) {
let mut lookup = vec![]; let mut lookup = vec![];

rust/hg-core/src/operations/dirstate_status.rs

// dirstate_status.rs // dirstate_status.rs
// //
// Copyright 2019, Raphaël Gomès <rgomes@octobus.net> // Copyright 2019, Raphaël Gomès <rgomes@octobus.net>
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::dirstate::status::{build_response, Dispatch, HgPathCow, Status}; use crate::dirstate::status::{build_response, Dispatch, HgPathCow, Status};
use crate::matchers::Matcher; use crate::matchers::Matcher;
use crate::{DirstateStatus, StatusError}; use crate::{DirstateStatus, StatusError};
/// A tuple of the paths that need to be checked in the filelog because it's /// A tuple of the paths that need to be checked in the filelog because it's
/// ambiguous whether they've changed, and the rest of the already dispatched /// ambiguous whether they've changed, and the rest of the already dispatched
/// files. /// files.
pub type LookupAndStatus<'a> = (Vec<HgPathCow<'a>>, DirstateStatus<'a>); pub type LookupAndStatus<'a> = (Vec<HgPathCow<'a>>, DirstateStatus<'a>);
#[cfg(feature = "dirstate-tree")]
impl<'a, M: Matcher + Sync> Status<'a, M> {
pub(crate) fn run(&self) -> Result<LookupAndStatus<'a>, StatusError> {
let (traversed_sender, traversed_receiver) =
crossbeam::channel::unbounded();
// Step 1: check the files explicitly mentioned by the user
let (work, mut results) = self.walk_explicit(traversed_sender.clone());
// Step 2: Check files in the dirstate
if !self.matcher.is_exact() {
self.extend_from_dmap(&mut results);
}
// Step 3: Check the working directory if listing unknowns
if !work.is_empty() {
// Hashmaps are quite a bit slower to build than vecs, so only
// build it if needed.
let mut old_results = None;
// Step 2: recursively check the working directory for changes if
// needed
for (dir, dispatch) in work {
match dispatch {
Dispatch::Directory { was_file } => {
if was_file {
results.push((dir.to_owned(), Dispatch::Removed));
}
if self.options.list_ignored
|| self.options.list_unknown
&& !self.dir_ignore(&dir)
{
if old_results.is_none() {
old_results =
Some(results.iter().cloned().collect());
}
self.traverse(
&dir,
old_results
.as_ref()
.expect("old results should exist"),
&mut results,
traversed_sender.clone(),
)?;
}
}
_ => {
unreachable!("There can only be directories in `work`")
}
}
}
}
drop(traversed_sender);
let traversed = traversed_receiver.into_iter().collect();
Ok(build_response(results, traversed))
}
}
#[cfg(not(feature = "dirstate-tree"))]
impl<'a, M: Matcher + Sync> Status<'a, M> { impl<'a, M: Matcher + Sync> Status<'a, M> {
pub(crate) fn run(&self) -> Result<LookupAndStatus<'a>, StatusError> { pub(crate) fn run(&self) -> Result<LookupAndStatus<'a>, StatusError> {
let (traversed_sender, traversed_receiver) = let (traversed_sender, traversed_receiver) =
crossbeam::channel::unbounded(); crossbeam::channel::unbounded();
// Step 1: check the files explicitly mentioned by the user // Step 1: check the files explicitly mentioned by the user
let (work, mut results) = self.walk_explicit(traversed_sender.clone()); let (work, mut results) = self.walk_explicit(traversed_sender.clone());

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

HgPath::new(f.extract::<PyBytes>(py)?.data(py)), HgPath::new(f.extract::<PyBytes>(py)?.data(py)),
oldstate.extract::<PyBytes>(py)?.data(py)[0] oldstate.extract::<PyBytes>(py)?.data(py)[0]
.try_into() .try_into()
.map_err(|e: DirstateParseError| { .map_err(|e: DirstateParseError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string()) PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?, })?,
) )
.and_then(|b| Ok(b.to_py_object(py))) .and_then(|b| Ok(b.to_py_object(py)))
.or_else(|_| { .or_else(|e| {
Err(PyErr::new::<exc::OSError, _>( Err(PyErr::new::<exc::OSError, _>(
py, py,
"Dirstate error".to_string(), format!("Dirstate error: {}", e.to_string()),
)) ))
}) })
} }
def clearambiguoustimes( def clearambiguoustimes(
&self, &self,
files: PyObject, files: PyObject,
now: PyObject now: PyObject
impl DirstateMap { impl DirstateMap {
pub fn get_inner<'a>( pub fn get_inner<'a>(
&'a self, &'a self,
py: Python<'a>, py: Python<'a>,
) -> Ref<'a, RustDirstateMap> { ) -> Ref<'a, RustDirstateMap> {
self.inner(py).borrow() self.inner(py).borrow()
} }
#[cfg(not(feature = "dirstate-tree"))]
fn translate_key( fn translate_key(
py: Python, py: Python,
res: (&HgPathBuf, &DirstateEntry), res: (&HgPathBuf, &DirstateEntry),
) -> PyResult<Option<PyBytes>> { ) -> PyResult<Option<PyBytes>> {
Ok(Some(PyBytes::new(py, res.0.as_bytes()))) Ok(Some(PyBytes::new(py, res.0.as_bytes())))
} }
#[cfg(not(feature = "dirstate-tree"))]
fn translate_key_value( fn translate_key_value(
py: Python, py: Python,
res: (&HgPathBuf, &DirstateEntry), res: (&HgPathBuf, &DirstateEntry),
) -> PyResult<Option<(PyBytes, PyObject)>> { ) -> PyResult<Option<(PyBytes, PyObject)>> {
let (f, entry) = res; let (f, entry) = res;
Ok(Some(( Ok(Some((
PyBytes::new(py, f.as_bytes()), PyBytes::new(py, f.as_bytes()),
make_dirstate_tuple(py, entry)?, make_dirstate_tuple(py, &entry)?,
)))
}
#[cfg(feature = "dirstate-tree")]
fn translate_key(
py: Python,
res: (HgPathBuf, DirstateEntry),
) -> PyResult<Option<PyBytes>> {
Ok(Some(PyBytes::new(py, res.0.as_bytes())))
}
#[cfg(feature = "dirstate-tree")]
fn translate_key_value(
py: Python,
res: (HgPathBuf, DirstateEntry),
) -> PyResult<Option<(PyBytes, PyObject)>> {
let (f, entry) = res;
Ok(Some((
PyBytes::new(py, f.as_bytes()),
make_dirstate_tuple(py, &entry)?,
))) )))
} }
} }
py_shared_iterator!( py_shared_iterator!(
DirstateMapKeysIterator, DirstateMapKeysIterator,
UnsafePyLeaked<StateMapIter<'static>>, UnsafePyLeaked<StateMapIter<'static>>,
DirstateMap::translate_key, DirstateMap::translate_key,

rust/hg-cpython/src/dirstate/status.rs

.map(|f| { .map(|f| {
Ok(HgPathBuf::from_bytes( Ok(HgPathBuf::from_bytes(
f.extract::<PyBytes>(py)?.data(py), f.extract::<PyBytes>(py)?.data(py),
)) ))
}) })
.collect(); .collect();
let files = files?; let files = files?;
let matcher = FileMatcher::new(&files) let matcher = FileMatcher::new(files.as_ref())
.map_err(|e| PyErr::new::<ValueError, _>(py, e.to_string()))?; .map_err(|e| PyErr::new::<ValueError, _>(py, e.to_string()))?;
let ((lookup, status_res), warnings) = status( let ((lookup, status_res), warnings) = status(
&dmap, &dmap,
&matcher, &matcher,
root_dir.to_path_buf(), root_dir.to_path_buf(),
ignore_files, ignore_files,
StatusOptions { StatusOptions {
check_exec, check_exec,

rust/hg-cpython/src/parsers.rs

&copies?, &copies?,
DirstateParents { DirstateParents {
p1: p1.try_into().unwrap(), p1: p1.try_into().unwrap(),
p2: p2.try_into().unwrap(), p2: p2.try_into().unwrap(),
}, },
Duration::from_secs(now.as_object().extract::<u64>(py)?), Duration::from_secs(now.as_object().extract::<u64>(py)?),
) { ) {
Ok(packed) => { Ok(packed) => {
for (filename, entry) in &dirstate_map { for (filename, entry) in dirstate_map.iter() {
dmap.set_item( dmap.set_item(
py, py,
PyBytes::new(py, filename.as_bytes()), PyBytes::new(py, filename.as_bytes()),
make_dirstate_tuple(py, entry)?, make_dirstate_tuple(py, &entry)?,
)?; )?;
} }
Ok(PyBytes::new(py, &packed)) Ok(PyBytes::new(py, &packed))
} }
Err(error) => Err(PyErr::new::<exc::ValueError, _>( Err(error) => Err(PyErr::new::<exc::ValueError, _>(
py, py,
match error { match error {
DirstatePackError::CorruptedParent => { DirstatePackError::CorruptedParent => {