| hg-reviewers |
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.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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)
✅ refresh by Heptapod after a successful CI run (🐙 💚)
⚠ This patch is intended for stable ⚠
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.
| Path | Packages | |||
|---|---|---|---|---|
| M | rust/hg-core/src/errors.rs (5 lines) | |||
| M | rust/hg-core/src/repo.rs (35 lines) |
| /// 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()) | ||||
| 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())) | |||||
| } | |||||