This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7609

rust-dirs: handle forgotten `Result`s
ClosedPublic

Authored by Alphare on Dec 12 2019, 9:59 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGbc7d8f45c3b6: rust-dirs: handle forgotten `Result`s
Summary

In 1fe2e574616e I introduced a temporary bugfix to align Rust code with a new
behavior from C/Python and forgot about a few Results (cargo's compiler cache
does not re-emit warnings on cached modules). This fixes it.

For the record, I am still unsure that this behavior change is a good idea.

Note: I was already quite unhappy with the setters and getters for the
DirstateMap and, indirectly, Dirs, and this only further reinforces my
feelings. I hope we can one day fix that situation at the type level; Georges
Racinet and I were just talking about devising a POC for using the builder
pattern in the context of FFI with Python, we'll see what comes out of it.

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.Dec 12 2019, 9:59 AM
Alphare updated this revision to Diff 18633.Dec 12 2019, 10:09 AM
Alphare updated this revision to Diff 18686.Dec 13 2019, 4:52 AM
Alphare added a commit: rHGbc7d8f45c3b6: rust-dirs: handle forgotten `Result`s.Dec 13 2019, 12:58 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGbc7d8f45c3b6: rust-dirs: handle forgotten `Result`s (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 18687

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

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.
pub fn from_dirstate( pub fn from_dirstate(
dirstate: &FastHashMap<HgPathBuf, DirstateEntry>, dirstate: &FastHashMap<HgPathBuf, DirstateEntry>,
skip_state: Option<EntryState>, skip_state: Option<EntryState>,
) -> Self { ) -> Result<Self, DirstateMapError> {
let mut multiset = DirsMultiset { let mut multiset = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
for (filename, DirstateEntry { state, .. }) in dirstate { 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)?;
} }
} }
multiset Ok(multiset)
} }
/// Initializes the multiset from a manifest. /// Initializes the multiset from a manifest.
pub fn from_manifest(manifest: &[impl AsRef<HgPath>]) -> Self { pub fn from_manifest(
manifest: &[impl AsRef<HgPath>],
) -> Result<Self, DirstateMapError> {
let mut multiset = DirsMultiset { let mut multiset = DirsMultiset {
inner: FastHashMap::default(), inner: FastHashMap::default(),
}; };
for filename in manifest { for filename in manifest {
multiset.add_path(filename.as_ref()); multiset.add_path(filename.as_ref())?;
} }
multiset Ok(multiset)
} }
/// Increases the count of deepest directory contained in the path. /// Increases the count of deepest directory contained in the path.
/// ///
/// If the directory is not yet in the map, adds its parents. /// If the directory is not yet in the map, adds its parents.
pub fn add_path( pub fn add_path(
&mut self, &mut self,
path: impl AsRef<HgPath>, path: impl AsRef<HgPath>,
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
#[test] #[test]
fn test_delete_path_path_not_found() { fn test_delete_path_path_not_found() {
let manifest: Vec<HgPathBuf> = vec![]; let manifest: Vec<HgPathBuf> = vec![];
let mut map = DirsMultiset::from_manifest(&manifest); let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
let path = HgPathBuf::from_bytes(b"doesnotexist/"); let path = HgPathBuf::from_bytes(b"doesnotexist/");
assert_eq!( assert_eq!(
Err(DirstateMapError::PathNotFound(path.to_owned())), Err(DirstateMapError::PathNotFound(path.to_owned())),
map.delete_path(&path) map.delete_path(&path)
); );
} }
#[test] #[test]
fn test_delete_path_empty_path() { fn test_delete_path_empty_path() {
let mut map = DirsMultiset::from_manifest(&vec![HgPathBuf::new()]); let mut map =
DirsMultiset::from_manifest(&vec![HgPathBuf::new()]).unwrap();
let path = HgPath::new(b""); let path = HgPath::new(b"");
assert_eq!(Ok(()), map.delete_path(path)); assert_eq!(Ok(()), map.delete_path(path));
assert_eq!( assert_eq!(
Err(DirstateMapError::PathNotFound(path.to_owned())), Err(DirstateMapError::PathNotFound(path.to_owned())),
map.delete_path(path) map.delete_path(path)
); );
} }
))), ))),
map.delete_path(HgPath::new(b"a/c/")) map.delete_path(HgPath::new(b"a/c/"))
); );
} }
#[test] #[test]
fn test_add_path_empty_path() { fn test_add_path_empty_path() {
let manifest: Vec<HgPathBuf> = vec![]; let manifest: Vec<HgPathBuf> = vec![];
let mut map = DirsMultiset::from_manifest(&manifest); let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
let path = HgPath::new(b""); let path = HgPath::new(b"");
map.add_path(path); map.add_path(path);
assert_eq!(1, map.len()); assert_eq!(1, map.len());
} }
#[test] #[test]
fn test_add_path_successful() { fn test_add_path_successful() {
let manifest: Vec<HgPathBuf> = vec![]; let manifest: Vec<HgPathBuf> = vec![];
let mut map = DirsMultiset::from_manifest(&manifest); let mut map = DirsMultiset::from_manifest(&manifest).unwrap();
map.add_path(HgPath::new(b"a/")); map.add_path(HgPath::new(b"a/"));
assert_eq!(1, *map.inner.get(HgPath::new(b"a")).unwrap()); assert_eq!(1, *map.inner.get(HgPath::new(b"a")).unwrap());
assert_eq!(1, *map.inner.get(HgPath::new(b"")).unwrap()); assert_eq!(1, *map.inner.get(HgPath::new(b"")).unwrap());
assert_eq!(2, map.len()); assert_eq!(2, map.len());
// Non directory should be ignored // Non directory should be ignored
map.add_path(HgPath::new(b"a")); map.add_path(HgPath::new(b"a"));
.collect(), .collect(),
}; };
assert_eq!(map, expected); assert_eq!(map, expected);
} }
#[test] #[test]
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); 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 = DirsMultiset::from_dirstate(&FastHashMap::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); 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 = ["a/", "b/", "a/c", "a/d/"]
.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", 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_dirstate(&input_map, None); 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() {
// "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), ("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 = let new =
DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal)); DirsMultiset::from_dirstate(&input_map, Some(EntryState::Normal))
.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

if old_state != EntryState::Unknown && old_state != EntryState::Removed if old_state != EntryState::Unknown && old_state != EntryState::Removed
{ {
if let Some(ref mut dirs) = self.dirs { if let Some(ref mut dirs) = self.dirs {
dirs.delete_path(filename)?; dirs.delete_path(filename)?;
} }
} }
if old_state == EntryState::Unknown { if old_state == EntryState::Unknown {
if let Some(ref mut all_dirs) = self.all_dirs { if let Some(ref mut all_dirs) = self.all_dirs {
all_dirs.add_path(filename); all_dirs.add_path(filename)?;
} }
} }
if let Some(ref mut file_fold_map) = self.file_fold_map { if let Some(ref mut file_fold_map) = self.file_fold_map {
file_fold_map.remove(&normalize_case(filename)); file_fold_map.remove(&normalize_case(filename));
} }
self.state_map.insert( self.state_map.insert(
filename.to_owned(), filename.to_owned(),
(non_normal, other_parent) (non_normal, 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) { 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 =
Some(DirsMultiset::from_dirstate(&self.state_map, None)); Some(DirsMultiset::from_dirstate(&self.state_map, None)?);
} }
Ok(())
} }
pub fn set_dirs(&mut self) { pub fn set_dirs(&mut self) -> Result<(), DirstateMapError> {
if self.dirs.is_none() { if self.dirs.is_none() {
self.dirs = Some(DirsMultiset::from_dirstate( self.dirs = Some(DirsMultiset::from_dirstate(
&self.state_map, &self.state_map,
Some(EntryState::Removed), Some(EntryState::Removed),
)); )?);
} }
Ok(())
} }
pub fn has_tracked_dir(&mut self, directory: &HgPath) -> bool { pub fn has_tracked_dir(
self.set_dirs(); &mut self,
self.dirs.as_ref().unwrap().contains(directory) directory: &HgPath,
) -> Result<bool, DirstateMapError> {
self.set_dirs()?;
Ok(self.dirs.as_ref().unwrap().contains(directory))
} }
pub fn has_dir(&mut self, directory: &HgPath) -> bool { pub fn has_dir(
self.set_all_dirs(); &mut self,
self.all_dirs.as_ref().unwrap().contains(directory) directory: &HgPath,
) -> Result<bool, DirstateMapError> {
self.set_all_dirs()?;
Ok(self.all_dirs.as_ref().unwrap().contains(directory))
} }
pub fn parents( pub fn parents(
&mut self, &mut self,
file_contents: &[u8], file_contents: &[u8],
) -> Result<&DirstateParents, DirstateError> { ) -> Result<&DirstateParents, DirstateError> {
if let Some(ref parents) = self.parents { if let Some(ref parents) = self.parents {
return Ok(parents); return Ok(parents);
use super::*; use super::*;
#[test] #[test]
fn test_dirs_multiset() { fn test_dirs_multiset() {
let mut map = DirstateMap::new(); let mut map = DirstateMap::new();
assert!(map.dirs.is_none()); assert!(map.dirs.is_none());
assert!(map.all_dirs.is_none()); assert!(map.all_dirs.is_none());
assert_eq!(false, map.has_dir(HgPath::new(b"nope"))); assert_eq!(map.has_dir(HgPath::new(b"nope")).unwrap(), false);
assert!(map.all_dirs.is_some()); assert!(map.all_dirs.is_some());
assert!(map.dirs.is_none()); assert!(map.dirs.is_none());
assert_eq!(false, map.has_tracked_dir(HgPath::new(b"nope"))); assert_eq!(map.has_tracked_dir(HgPath::new(b"nope")).unwrap(), false);
assert!(map.dirs.is_some()); assert!(map.dirs.is_some());
} }
#[test] #[test]
fn test_add_file() { fn test_add_file() {
let mut map = DirstateMap::new(); let mut map = DirstateMap::new();
assert_eq!(0, map.len()); assert_eq!(0, map.len());

rust/hg-core/src/matchers.rs

// matchers.rs // matchers.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.
//! Structs and types for matching files and directories. //! Structs and types for matching files and directories.
use crate::utils::hg_path::{HgPath, HgPathBuf}; use crate::utils::hg_path::HgPath;
use std::collections::HashSet; use std::collections::HashSet;
pub enum VisitChildrenSet<'a> { pub enum VisitChildrenSet<'a> {
/// Don't visit anything /// Don't visit anything
Empty, Empty,
/// Only visit this directory /// Only visit this directory
This, This,
/// Visit this directory and these subdirectories /// Visit this directory and these subdirectories

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

.map_err(|e: DirstateParseError| { .map_err(|e: DirstateParseError| {
PyErr::new::<exc::ValueError, _>(py, e.to_string()) PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?, })?,
); );
} }
let inner = if let Ok(map) = map.cast_as::<PyDict>(py) { let inner = if let Ok(map) = map.cast_as::<PyDict>(py) {
let dirstate = extract_dirstate(py, &map)?; let dirstate = extract_dirstate(py, &map)?;
DirsMultiset::from_dirstate(&dirstate, skip_state) DirsMultiset::from_dirstate(&dirstate, skip_state)
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?
} else { } else {
let map: Result<Vec<HgPathBuf>, PyErr> = map let map: Result<Vec<HgPathBuf>, PyErr> = map
.iter(py)? .iter(py)?
.map(|o| { .map(|o| {
Ok(HgPathBuf::from_bytes( Ok(HgPathBuf::from_bytes(
o?.extract::<PyBytes>(py)?.data(py), o?.extract::<PyBytes>(py)?.data(py),
)) ))
}) })
.collect(); .collect();
DirsMultiset::from_manifest(&map?) DirsMultiset::from_manifest(&map?)
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?
}; };
Self::create_instance( Self::create_instance(
py, py,
PySharedRefCell::new(inner), PySharedRefCell::new(inner),
) )
} }

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

py.eval("set(non_normal), set(other_parent)", None, Some(&locals)) py.eval("set(non_normal), set(other_parent)", None, Some(&locals))
} }
def hastrackeddir(&self, d: PyObject) -> PyResult<PyBool> { def hastrackeddir(&self, d: PyObject) -> PyResult<PyBool> {
let d = d.extract::<PyBytes>(py)?; let d = d.extract::<PyBytes>(py)?;
Ok(self.inner_shared(py).borrow_mut()? Ok(self.inner_shared(py).borrow_mut()?
.has_tracked_dir(HgPath::new(d.data(py))) .has_tracked_dir(HgPath::new(d.data(py)))
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?
.to_py_object(py)) .to_py_object(py))
} }
def hasdir(&self, d: PyObject) -> PyResult<PyBool> { def hasdir(&self, d: PyObject) -> PyResult<PyBool> {
let d = d.extract::<PyBytes>(py)?; let d = d.extract::<PyBytes>(py)?;
Ok(self.inner_shared(py).borrow_mut()? Ok(self.inner_shared(py).borrow_mut()?
.has_dir(HgPath::new(d.data(py))) .has_dir(HgPath::new(d.data(py)))
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?
.to_py_object(py)) .to_py_object(py))
} }
def parents(&self, st: PyObject) -> PyResult<PyTuple> { def parents(&self, st: PyObject) -> PyResult<PyTuple> {
self.inner_shared(py).borrow_mut()? self.inner_shared(py).borrow_mut()?
.parents(st.extract::<PyBytes>(py)?.data(py)) .parents(st.extract::<PyBytes>(py)?.data(py))
.and_then(|d| { .and_then(|d| {
Ok((PyBytes::new(py, &d.p1), PyBytes::new(py, &d.p2)) Ok((PyBytes::new(py, &d.p1), PyBytes::new(py, &d.p2))
DirstateMapKeysIterator::from_inner( DirstateMapKeysIterator::from_inner(
py, py,
unsafe { leaked_ref.map(py, |o| o.iter()) }, unsafe { leaked_ref.map(py, |o| o.iter()) },
) )
} }
def getdirs(&self) -> PyResult<Dirs> { def getdirs(&self) -> PyResult<Dirs> {
// TODO don't copy, share the reference // TODO don't copy, share the reference
self.inner_shared(py).borrow_mut()?.set_dirs(); self.inner_shared(py).borrow_mut()?.set_dirs()
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?;
Dirs::from_inner( Dirs::from_inner(
py, py,
DirsMultiset::from_dirstate( DirsMultiset::from_dirstate(
&self.inner_shared(py).borrow(), &self.inner_shared(py).borrow(),
Some(EntryState::Removed), Some(EntryState::Removed),
), )
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?,
) )
} }
def getalldirs(&self) -> PyResult<Dirs> { def getalldirs(&self) -> PyResult<Dirs> {
// TODO don't copy, share the reference // TODO don't copy, share the reference
self.inner_shared(py).borrow_mut()?.set_all_dirs(); self.inner_shared(py).borrow_mut()?.set_all_dirs()
.map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?;
Dirs::from_inner( Dirs::from_inner(
py, py,
DirsMultiset::from_dirstate( DirsMultiset::from_dirstate(
&self.inner_shared(py).borrow(), &self.inner_shared(py).borrow(),
None, None,
), ).map_err(|e| {
PyErr::new::<exc::ValueError, _>(py, e.to_string())
})?,
) )
} }
// TODO all copymap* methods, see docstring above // TODO all copymap* methods, see docstring above
def copymapcopy(&self) -> PyResult<PyDict> { def copymapcopy(&self) -> PyResult<PyDict> {
let dict = PyDict::new(py); let dict = PyDict::new(py);
for (key, value) in self.inner_shared(py).borrow().copy_map.iter() { for (key, value) in self.inner_shared(py).borrow().copy_map.iter() {
dict.set_item( dict.set_item(