D11771: rhg: Propogate manifest parse errors instead of panicking
SimonSapin
phabricator at mercurial-scm.org
Tue Nov 23 19:11:56 UTC 2021
SimonSapin created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.
REVISION SUMMARY
The Rust parser for the manifest file format is an iterator. Now every item
from that iterator is a `Result`, which makes error handling required
in multiple new places.
This makes better recovery on errors possible, compare to a run time panic.
REPOSITORY
rHG Mercurial
BRANCH
default
REVISION DETAIL
https://phab.mercurial-scm.org/D11771
AFFECTED FILES
rust/hg-core/src/operations/cat.rs
rust/hg-core/src/operations/list_tracked_files.rs
rust/hg-core/src/revlog/manifest.rs
rust/rhg/src/commands/files.rs
rust/rhg/src/commands/status.rs
rust/rhg/src/utils/path_utils.rs
CHANGE DETAILS
diff --git a/rust/rhg/src/utils/path_utils.rs b/rust/rhg/src/utils/path_utils.rs
--- a/rust/rhg/src/utils/path_utils.rs
+++ b/rust/rhg/src/utils/path_utils.rs
@@ -5,6 +5,7 @@
use crate::error::CommandError;
use crate::ui::UiError;
+use hg::errors::HgError;
use hg::repo::Repo;
use hg::utils::current_dir;
use hg::utils::files::{get_bytes_from_path, relativize_path};
@@ -14,7 +15,7 @@
pub fn relativize_paths(
repo: &Repo,
- paths: impl IntoIterator<Item = impl AsRef<HgPath>>,
+ paths: impl IntoIterator<Item = Result<impl AsRef<HgPath>, HgError>>,
mut callback: impl FnMut(Cow<[u8]>) -> Result<(), UiError>,
) -> Result<(), CommandError> {
let cwd = current_dir()?;
@@ -38,10 +39,10 @@
for file in paths {
if outside_repo {
- let file = repo_root_hgpath.join(file.as_ref());
+ let file = repo_root_hgpath.join(file?.as_ref());
callback(relativize_path(&file, &cwd_hgpath))?;
} else {
- callback(relativize_path(file.as_ref(), &cwd_hgpath))?;
+ callback(relativize_path(file?.as_ref(), &cwd_hgpath))?;
}
}
Ok(())
diff --git a/rust/rhg/src/commands/status.rs b/rust/rhg/src/commands/status.rs
--- a/rust/rhg/src/commands/status.rs
+++ b/rust/rhg/src/commands/status.rs
@@ -279,7 +279,7 @@
if relative && !ui.plain() {
relativize_paths(
repo,
- paths,
+ paths.iter().map(Ok),
|path: Cow<[u8]>| -> Result<(), UiError> {
ui.write_stdout(
&[status_prefix, b" ", path.as_ref(), b"\n"].concat(),
diff --git a/rust/rhg/src/commands/files.rs b/rust/rhg/src/commands/files.rs
--- a/rust/rhg/src/commands/files.rs
+++ b/rust/rhg/src/commands/files.rs
@@ -3,6 +3,7 @@
use crate::ui::UiError;
use crate::utils::path_utils::relativize_paths;
use clap::Arg;
+use hg::errors::HgError;
use hg::operations::list_rev_tracked_files;
use hg::operations::Dirstate;
use hg::repo::Repo;
@@ -45,14 +46,14 @@
} else {
let distate = Dirstate::new(repo)?;
let files = distate.tracked_files()?;
- display_files(invocation.ui, repo, files)
+ display_files(invocation.ui, repo, files.into_iter().map(Ok))
}
}
fn display_files<'a>(
ui: &Ui,
repo: &Repo,
- files: impl IntoIterator<Item = &'a HgPath>,
+ files: impl IntoIterator<Item = Result<&'a HgPath, HgError>>,
) -> Result<(), CommandError> {
let mut stdout = ui.stdout_buffer();
let mut any = false;
diff --git a/rust/hg-core/src/revlog/manifest.rs b/rust/hg-core/src/revlog/manifest.rs
--- a/rust/hg-core/src/revlog/manifest.rs
+++ b/rust/hg-core/src/revlog/manifest.rs
@@ -63,26 +63,28 @@
}
/// Return an iterator over the files of the entry.
- pub fn files(&self) -> impl Iterator<Item = &HgPath> {
+ pub fn files(&self) -> impl Iterator<Item = Result<&HgPath, HgError>> {
self.lines().filter(|line| !line.is_empty()).map(|line| {
- let pos = line
- .iter()
- .position(|x| x == &b'\0')
- .expect("manifest line should contain \\0");
- HgPath::new(&line[..pos])
+ let pos =
+ line.iter().position(|x| x == &b'\0').ok_or_else(|| {
+ HgError::corrupted("manifest line should contain \\0")
+ })?;
+ Ok(HgPath::new(&line[..pos]))
})
}
/// Return an iterator over the files of the entry.
- pub fn files_with_nodes(&self) -> impl Iterator<Item = (&HgPath, &[u8])> {
+ pub fn files_with_nodes(
+ &self,
+ ) -> impl Iterator<Item = Result<(&HgPath, &[u8]), HgError>> {
self.lines().filter(|line| !line.is_empty()).map(|line| {
- let pos = line
- .iter()
- .position(|x| x == &b'\0')
- .expect("manifest line should contain \\0");
+ let pos =
+ line.iter().position(|x| x == &b'\0').ok_or_else(|| {
+ HgError::corrupted("manifest line should contain \\0")
+ })?;
let hash_start = pos + 1;
let hash_end = hash_start + 40;
- (HgPath::new(&line[..pos]), &line[hash_start..hash_end])
+ Ok((HgPath::new(&line[..pos]), &line[hash_start..hash_end]))
})
}
@@ -91,7 +93,8 @@
// TODO: use binary search instead of linear scan. This may involve
// building (and caching) an index of the byte indicex of each manifest
// line.
- for (manifest_path, node) in self.files_with_nodes() {
+ for entry in self.files_with_nodes() {
+ let (manifest_path, node) = entry?;
if manifest_path == path {
return Ok(Some(Node::from_hex_for_repo(node)?));
}
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
@@ -76,7 +76,7 @@
pub struct FilesForRev(Manifest);
impl FilesForRev {
- pub fn iter(&self) -> impl Iterator<Item = &HgPath> {
+ pub fn iter(&self) -> impl Iterator<Item = Result<&HgPath, HgError>> {
self.0.files()
}
}
diff --git a/rust/hg-core/src/operations/cat.rs b/rust/hg-core/src/operations/cat.rs
--- a/rust/hg-core/src/operations/cat.rs
+++ b/rust/hg-core/src/operations/cat.rs
@@ -11,6 +11,7 @@
use crate::utils::hg_path::HgPath;
+use crate::errors::HgError;
use itertools::put_back;
use itertools::PutBack;
use std::cmp::Ordering;
@@ -28,21 +29,24 @@
}
// Find an item in an iterator over a sorted collection.
-fn find_item<'a, 'b, 'c, D, I: Iterator<Item = (&'a HgPath, D)>>(
+fn find_item<'a, D, I: Iterator<Item = Result<(&'a HgPath, D), HgError>>>(
i: &mut PutBack<I>,
- needle: &'b HgPath,
-) -> Option<D> {
+ needle: &HgPath,
+) -> Result<Option<D>, HgError> {
loop {
match i.next() {
- None => return None,
- Some(val) => match needle.as_bytes().cmp(val.0.as_bytes()) {
- Ordering::Less => {
- i.put_back(val);
- return None;
+ None => return Ok(None),
+ Some(result) => {
+ let (path, value) = result?;
+ match needle.as_bytes().cmp(path.as_bytes()) {
+ Ordering::Less => {
+ i.put_back(Ok((path, value)));
+ return Ok(None);
+ }
+ Ordering::Greater => continue,
+ Ordering::Equal => return Ok(Some(value)),
}
- Ordering::Greater => continue,
- Ordering::Equal => return Some(val.1),
- },
+ }
}
}
}
@@ -51,23 +55,23 @@
'manifest,
'query,
Data,
- Manifest: Iterator<Item = (&'manifest HgPath, Data)>,
+ Manifest: Iterator<Item = Result<(&'manifest HgPath, Data), HgError>>,
Query: Iterator<Item = &'query HgPath>,
>(
manifest: Manifest,
query: Query,
-) -> (Vec<(&'query HgPath, Data)>, Vec<&'query HgPath>) {
+) -> Result<(Vec<(&'query HgPath, Data)>, Vec<&'query HgPath>), HgError> {
let mut manifest = put_back(manifest);
let mut res = vec![];
let mut missing = vec![];
for file in query {
- match find_item(&mut manifest, file) {
+ match find_item(&mut manifest, file)? {
None => missing.push(file),
Some(item) => res.push((file, item)),
}
}
- return (res, missing);
+ return Ok((res, missing));
}
/// Output the given revision of files
@@ -94,7 +98,7 @@
let (found, missing) = find_files_in_manifest(
manifest.files_with_nodes(),
files.into_iter().map(|f| f.as_ref()),
- );
+ )?;
for (file_path, node_bytes) in found {
found_any = true;
To: SimonSapin, #hg-reviewers
Cc: mercurial-patches, mercurial-devel
More information about the Mercurial-devel
mailing list