( )⚙ D11230 rhg: Propagate permission errors when finding a repository

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11230

rhg: Propagate permission errors when finding a repository
ClosedPublic

Authored by SimonSapin on Jul 29 2021, 6:22 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGcf5f8da2244c: rhg: Propagate permission errors when finding a repository
Summary

The Rust standard library has a Path::is_dir method that returns false
for any I/O error (such as a permission error),
not just "No such file or directory".

Instead add an is_dir function that returns false for non-directories
and for "No such file or directory" errors, but propagates other I/O errors.

Diff Detail

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

Event Timeline

SimonSapin created this revision.Jul 29 2021, 6:22 AM
SimonSapin added a comment.Jul 29 2021, 10:57 AM

This should not be merged as-is: some tests fail because this function returns an error if an ancestor of the given path is not a directory. We’d want to handle that case like "not found" errors instead, but https://doc.rust-lang.org/nightly/std/io/enum.ErrorKind.html#variant.NotADirectory is not stable yet (and does not exist at all in 1.41)

baymax updated this revision to Diff 29738.Jul 29 2021, 2:34 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠

SimonSapin added a comment.Jul 29 2021, 2:36 PM

I’ve changed this to only propagate permission errors specifically, for now. This seems to be the one that’s somewhat likely to happen, from possible errors listed in man 2 stat.

SimonSapin added a commit: rHGcf5f8da2244c: rhg: Propagate permission errors when finding a repository.Jul 30 2021, 11:12 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGcf5f8da2244c: rhg: Propagate permission errors when finding a repository (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/errors.rs (5 lines)
Mrust/hg-core/src/repo.rs (35 lines)

Diff 29738

rust/hg-core/src/errors.rs

/// and syntax of each value. /// and syntax of each value.
#[from] #[from]
ConfigValueParseError(ConfigValueParseError), ConfigValueParseError(ConfigValueParseError),
} }
/// Details about where an I/O error happened /// Details about where an I/O error happened
#[derive(Debug)] #[derive(Debug)]
pub enum IoErrorContext { pub enum IoErrorContext {
/// `std::fs::metadata`
ReadingMetadata(std::path::PathBuf),
ReadingFile(std::path::PathBuf), ReadingFile(std::path::PathBuf),
WritingFile(std::path::PathBuf), WritingFile(std::path::PathBuf),
RemovingFile(std::path::PathBuf), RemovingFile(std::path::PathBuf),
RenamingFile { RenamingFile {
from: std::path::PathBuf, from: std::path::PathBuf,
to: std::path::PathBuf, to: std::path::PathBuf,
}, },
/// `std::fs::canonicalize` /// `std::fs::canonicalize`
} }
} }
} }
// TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly? // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
impl fmt::Display for IoErrorContext { impl fmt::Display for IoErrorContext {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self { match self {
IoErrorContext::ReadingMetadata(path) => {
write!(f, "when reading metadata of {}", path.display())
}
IoErrorContext::ReadingFile(path) => { IoErrorContext::ReadingFile(path) => {
write!(f, "when reading {}", path.display()) write!(f, "when reading {}", path.display())
} }
IoErrorContext::WritingFile(path) => { IoErrorContext::WritingFile(path) => {
write!(f, "when writing {}", path.display()) write!(f, "when writing {}", path.display())
} }
IoErrorContext::RemovingFile(path) => { IoErrorContext::RemovingFile(path) => {
write!(f, "when removing {}", path.display()) write!(f, "when removing {}", path.display())

rust/hg-core/src/repo.rs

use crate::config::{Config, ConfigError, ConfigParseError}; use crate::config::{Config, ConfigError, ConfigParseError};
use crate::errors::{HgError, IoErrorContext, IoResultExt}; use crate::errors::{HgError, IoErrorContext, IoResultExt};
use crate::exit_codes; use crate::exit_codes;
use crate::requirements; use crate::requirements;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::SliceExt; use crate::utils::SliceExt;
use memmap::{Mmap, MmapOptions}; use memmap::{Mmap, MmapOptions};
use std::collections::HashSet; use std::collections::HashSet;
use std::io::ErrorKind;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
/// A repository on disk /// A repository on disk
pub struct Repo { pub struct Repo {
working_directory: PathBuf, working_directory: PathBuf,
dot_hg: PathBuf, dot_hg: PathBuf,
store: PathBuf, store: PathBuf,
requirements: HashSet<String>, requirements: HashSet<String>,
impl Repo { impl Repo {
/// tries to find nearest repository root in current working directory or /// tries to find nearest repository root in current working directory or
/// its ancestors /// its ancestors
pub fn find_repo_root() -> Result<PathBuf, RepoError> { pub fn find_repo_root() -> Result<PathBuf, RepoError> {
let current_directory = crate::utils::current_dir()?; let current_directory = crate::utils::current_dir()?;
// ancestors() is inclusive: it first yields `current_directory` // ancestors() is inclusive: it first yields `current_directory`
// as-is. // as-is.
for ancestor in current_directory.ancestors() { for ancestor in current_directory.ancestors() {
if ancestor.join(".hg").is_dir() { if is_dir(ancestor.join(".hg"))? {
return Ok(ancestor.to_path_buf()); return Ok(ancestor.to_path_buf());
} }
} }
return Err(RepoError::NotFound { return Err(RepoError::NotFound {
at: current_directory, at: current_directory,
}); });
} }
/// Find a repository, either at the given path (which must contain a `.hg` /// Find a repository, either at the given path (which must contain a `.hg`
/// sub-directory) or by searching the current directory and its /// sub-directory) or by searching the current directory and its
/// ancestors. /// ancestors.
/// ///
/// A method with two very different "modes" like this usually a code smell /// A method with two very different "modes" like this usually a code smell
/// to make two methods instead, but in this case an `Option` is what rhg /// to make two methods instead, but in this case an `Option` is what rhg
/// sub-commands get from Clap for the `-R` / `--repository` CLI argument. /// sub-commands get from Clap for the `-R` / `--repository` CLI argument.
/// Having two methods would just move that `if` to almost all callers. /// Having two methods would just move that `if` to almost all callers.
pub fn find( pub fn find(
config: &Config, config: &Config,
explicit_path: Option<PathBuf>, explicit_path: Option<PathBuf>,
) -> Result<Self, RepoError> { ) -> Result<Self, RepoError> {
if let Some(root) = explicit_path { if let Some(root) = explicit_path {
if root.join(".hg").is_dir() { if is_dir(root.join(".hg"))? {
Self::new_at_path(root.to_owned(), config) Self::new_at_path(root.to_owned(), config)
} else if root.is_file() { } else if is_file(&root)? {
Err(HgError::unsupported("bundle repository").into()) Err(HgError::unsupported("bundle repository").into())
} else { } else {
Err(RepoError::NotFound { Err(RepoError::NotFound {
at: root.to_owned(), at: root.to_owned(),
}) })
} }
} else { } else {
let root = Self::find_repo_root()?; let root = Self::find_repo_root()?;
store_path = dot_hg.join("store"); store_path = dot_hg.join("store");
} else { } else {
let bytes = hg_vfs.read("sharedpath")?; let bytes = hg_vfs.read("sharedpath")?;
let mut shared_path = let mut shared_path =
get_path_from_bytes(bytes.trim_end_newlines()).to_owned(); get_path_from_bytes(bytes.trim_end_newlines()).to_owned();
if relative { if relative {
shared_path = dot_hg.join(shared_path) shared_path = dot_hg.join(shared_path)
} }
if !shared_path.is_dir() { if !is_dir(&shared_path)? {
return Err(HgError::corrupted(format!( return Err(HgError::corrupted(format!(
".hg/sharedpath points to nonexistent directory {}", ".hg/sharedpath points to nonexistent directory {}",
shared_path.display() shared_path.display()
)) ))
.into()); .into());
} }
store_path = shared_path.join("store"); store_path = shared_path.join("store");
relative_to: impl AsRef<Path>, relative_to: impl AsRef<Path>,
) -> Result<(), HgError> { ) -> Result<(), HgError> {
let from = self.join(relative_from); let from = self.join(relative_from);
let to = self.join(relative_to); let to = self.join(relative_to);
std::fs::rename(&from, &to) std::fs::rename(&from, &to)
.with_context(|| IoErrorContext::RenamingFile { from, to }) .with_context(|| IoErrorContext::RenamingFile { from, to })
} }
} }
fn fs_metadata(
path: impl AsRef<Path>,
) -> Result<Option<std::fs::Metadata>, HgError> {
let path = path.as_ref();
match std::fs::metadata(path) {
Ok(meta) => Ok(Some(meta)),
Err(error) => match error.kind() {
// TODO: when we require a Rust version where `NotADirectory` is
// stable, invert this logic and return None for it and `NotFound`
// and propagate any other error.
ErrorKind::PermissionDenied => Err(error).with_context(|| {
IoErrorContext::ReadingMetadata(path.to_owned())
}),
_ => Ok(None),
},
}
}
fn is_dir(path: impl AsRef<Path>) -> Result<bool, HgError> {
Ok(fs_metadata(path)?.map_or(false, |meta| meta.is_dir()))
}
fn is_file(path: impl AsRef<Path>) -> Result<bool, HgError> {
Ok(fs_metadata(path)?.map_or(false, |meta| meta.is_file()))
}