( )⚙ D7921 rust-dirs-multiset: improve temporary error message

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7921

rust-dirs-multiset: improve temporary error message
ClosedPublic

Authored by Alphare on Jan 17 2020, 5:41 AM.

Details

Reviewers
kevincox
pulkit
Group Reviewers
hg-reviewers
Commits
rHG934a79697c36: rust-dirs-multiset: improve temporary error message
rHG5a07db9a31fd: rust-dirs-multiset: improve temporary error message
rHGa5c7d6cdaa2e: rust-dirs-multiset: improve temporary error message
Summary

While we wait on a future patch that could verify that the paths passed to
DirsMultiset have been audited, we still need to handle this error.
This patch makes it easier to bubble up and makes the error clearer.

Also, this patch introduces the subslice_index function that could be useful
for other - albeit niche - purposes.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Jan 17 2020, 5:41 AM
kevincox requested changes to this revision.Jan 24 2020, 4:59 AM
kevincox added inline comments.
rust/hg-core/src/dirstate/dirs_multiset.rs
86

It appears to me that find_dirs always returns prefixes. Why is this offset required?

This revision now requires changes to proceed.Jan 24 2020, 4:59 AM
Alphare marked an inline comment as done.Jan 24 2020, 5:16 AM
martinvonz added a subscriber: martinvonz.Jan 24 2020, 11:47 AM

It seems this one doesn't apply cleanly

pulkit accepted this revision.Feb 7 2020, 4:10 PM
Alphare added a commit: rHGa5c7d6cdaa2e: rust-dirs-multiset: improve temporary error message.Feb 7 2020, 4:38 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGa5c7d6cdaa2e: rust-dirs-multiset: improve temporary error message (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 19400

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::utils::hg_path::HgPathError;
use crate::utils::subslice_offset;
use crate::{ use crate::{
dirstate::EntryState, dirstate::EntryState,
utils::{ utils::{
files, files,
hg_path::{HgPath, HgPathBuf}, hg_path::{HgPath, HgPathBuf},
}, },
DirstateEntry, DirstateMapError, FastHashMap, DirstateEntry, DirstateMapError, FastHashMap,
}; };
pub fn add_path( pub fn add_path(
&mut self, &mut self,
path: impl AsRef<HgPath>, path: impl AsRef<HgPath>,
) -> Result<(), DirstateMapError> { ) -> Result<(), DirstateMapError> {
for subpath in files::find_dirs(path.as_ref()) { for subpath in files::find_dirs(path.as_ref()) {
if subpath.as_bytes().last() == Some(&b'/') { if subpath.as_bytes().last() == Some(&b'/') {
// TODO Remove this once PathAuditor is certified // TODO Remove this once PathAuditor is certified
// as the only entrypoint for path data // as the only entrypoint for path data
return Err(DirstateMapError::ConsecutiveSlashes); let offset = subslice_offset(
path.as_ref().as_bytes(),
subpath.as_bytes(),
);
kevincoxUnsubmitted
Done

It appears to me that find_dirs always returns prefixes. Why is this offset required?

kevincox: It appears to me that `find_dirs` always returns prefixes. Why is this offset required?
let second_slash_index = offset.unwrap() + subpath.len() - 1;
return Err(DirstateMapError::InvalidPath(
HgPathError::ConsecutiveSlashes {
bytes: path.as_ref().as_bytes().to_owned(),
second_slash_index,
},
));
} }
if let Some(val) = self.inner.get_mut(subpath) { if let Some(val) = self.inner.get_mut(subpath) {
*val += 1; *val += 1;
break; break;
} }
self.inner.insert(subpath.to_owned(), 1); self.inner.insert(subpath.to_owned(), 1);
} }
Ok(()) Ok(())

rust/hg-core/src/lib.rs

fn from(e: std::io::Error) -> Self { fn from(e: std::io::Error) -> Self {
DirstatePackError::CorruptedEntry(e.to_string()) DirstatePackError::CorruptedEntry(e.to_string())
} }
} }
#[derive(Debug, PartialEq)] #[derive(Debug, PartialEq)]
pub enum DirstateMapError { pub enum DirstateMapError {
PathNotFound(HgPathBuf), PathNotFound(HgPathBuf),
EmptyPath, EmptyPath,
ConsecutiveSlashes, InvalidPath(HgPathError),
} }
impl ToString for DirstateMapError { impl ToString for DirstateMapError {
fn to_string(&self) -> String { fn to_string(&self) -> String {
use crate::DirstateMapError::*;
match self { match self {
PathNotFound(_) => "expected a value, found none".to_string(), DirstateMapError::PathNotFound(_) => {
EmptyPath => "Overflow in dirstate.".to_string(), "expected a value, found none".to_string()
ConsecutiveSlashes => {
"found invalid consecutive slashes in path".to_string()
} }
DirstateMapError::EmptyPath => "Overflow in dirstate.".to_string(),
DirstateMapError::InvalidPath(e) => e.to_string(),
} }
} }
} }
pub enum DirstateError { pub enum DirstateError {
Parse(DirstateParseError), Parse(DirstateParseError),
Pack(DirstatePackError), Pack(DirstatePackError),
Map(DirstateMapError), Map(DirstateMapError),

rust/hg-core/src/utils.rs

} }
for i in 0..=buf.len() - from.len() { for i in 0..=buf.len() - from.len() {
if buf[i..].starts_with(from) { if buf[i..].starts_with(from) {
buf[i..(i + from.len())].clone_from_slice(to); buf[i..(i + from.len())].clone_from_slice(to);
} }
} }
} }
/// Find the offset of the subslice relative to the original collection
///
/// This function panics for zero-sized types.
/// # Examples:
///
/// ```
/// use crate::hg::utils::subslice_offset;
/// let mut line = b"Subslice me!".to_vec();
/// assert_eq!(subslice_offset(&line, &line[8..]), Some(8));
///
/// assert_eq!(subslice_offset(&line, b"hahaha"), None);
///
/// // Empty array
/// let v: [u8; 0] = [];
/// assert_eq!(subslice_offset(&v, &v), Some(0));
/// assert_eq!(subslice_offset(&v, b"hehe"), None);
/// ```
pub fn subslice_offset<T: Sized>(outer: &[T], inner: &[T]) -> Option<usize> {
let pointee_size = std::mem::size_of::<T>();
assert!(0 < pointee_size && pointee_size <= isize::max_value() as usize);
let outer_start = outer.as_ptr() as usize;
let inner = inner.as_ptr() as usize;
if inner < outer_start || inner > outer_start.wrapping_add(outer.len()) {
None
} else {
Some(inner.wrapping_sub(outer_start))
}
}
pub trait SliceExt { pub trait SliceExt {
fn trim_end(&self) -> &Self; fn trim_end(&self) -> &Self;
fn trim_start(&self) -> &Self; fn trim_start(&self) -> &Self;
fn trim(&self) -> &Self; fn trim(&self) -> &Self;
fn drop_prefix(&self, needle: &Self) -> Option<&Self>; fn drop_prefix(&self, needle: &Self) -> Option<&Self>;
} }
fn is_not_whitespace(c: &u8) -> bool { fn is_not_whitespace(c: &u8) -> bool {