rhg: Don’t make repository path absolute too early

Some error messages want to include a relative path,
which affects the output of some tests.

Differential Revision: https://phab.mercurial-scm.org/D10138

rust/hg-core/src/errors.rs

@@ -1,196 +1,201 b''
 use crate::config::ConfigValueParseError;
 use std::fmt;
 
 /// Common error cases that can happen in many different APIs
 #[derive(Debug, derive_more::From)]
 pub enum HgError {
     IoError {
         error: std::io::Error,
         context: IoErrorContext,
     },
 
     /// A file under `.hg/` normally only written by Mercurial is not in the
     /// expected format. This indicates a bug in Mercurial, filesystem
     /// corruption, or hardware failure.
     ///
     /// The given string is a short explanation for users, not intended to be
     /// machine-readable.
     CorruptedRepository(String),
 
     /// The respository or requested operation involves a feature not
     /// supported by the Rust implementation. Falling back to the Python
     /// implementation may or may not work.
     ///
     /// The given string is a short explanation for users, not intended to be
     /// machine-readable.
     UnsupportedFeature(String),
 
     /// Operation cannot proceed for some other reason.
     ///
     /// The given string is a short explanation for users, not intended to be
     /// machine-readable.
     Abort(String),
 
     /// A configuration value is not in the expected syntax.
     ///
     /// These errors can happen in many places in the code because values are
     /// parsed lazily as the file-level parser does not know the expected type
     /// and syntax of each value.
     #[from]
     ConfigValueParseError(ConfigValueParseError),
 }
 
 /// Details about where an I/O error happened
 #[derive(Debug)]
 pub enum IoErrorContext {
     ReadingFile(std::path::PathBuf),
     WritingFile(std::path::PathBuf),
     RemovingFile(std::path::PathBuf),
     RenamingFile {
         from: std::path::PathBuf,
         to: std::path::PathBuf,
     },
+    /// `std::fs::canonicalize`
+    CanonicalizingPath(std::path::PathBuf),
     /// `std::env::current_dir`
     CurrentDir,
     /// `std::env::current_exe`
     CurrentExe,
 }
 
 impl HgError {
     pub fn corrupted(explanation: impl Into<String>) -> Self {
         // TODO: capture a backtrace here and keep it in the error value
         // to aid debugging?
         // https://doc.rust-lang.org/std/backtrace/struct.Backtrace.html
         HgError::CorruptedRepository(explanation.into())
     }
 
     pub fn unsupported(explanation: impl Into<String>) -> Self {
         HgError::UnsupportedFeature(explanation.into())
     }
     pub fn abort(explanation: impl Into<String>) -> Self {
         HgError::Abort(explanation.into())
     }
 }
 
 // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
 impl fmt::Display for HgError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             HgError::Abort(explanation) => write!(f, "{}", explanation),
             HgError::IoError { error, context } => {
                 write!(f, "abort: {}: {}", context, error)
             }
             HgError::CorruptedRepository(explanation) => {
                 write!(f, "abort: {}", explanation)
             }
             HgError::UnsupportedFeature(explanation) => {
                 write!(f, "unsupported feature: {}", explanation)
             }
             HgError::ConfigValueParseError(ConfigValueParseError {
                 origin: _,
                 line: _,
                 section,
                 item,
                 value,
                 expected_type,
             }) => {
                 // TODO: add origin and line number information, here and in
                 // corresponding python code
                 write!(
                     f,
                     "config error: {}.{} is not a {} ('{}')",
                     String::from_utf8_lossy(section),
                     String::from_utf8_lossy(item),
                     expected_type,
                     String::from_utf8_lossy(value)
                 )
             }
         }
     }
 }
 
 // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
 impl fmt::Display for IoErrorContext {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             IoErrorContext::ReadingFile(path) => {
                 write!(f, "when reading {}", path.display())
             }
             IoErrorContext::WritingFile(path) => {
                 write!(f, "when writing {}", path.display())
             }
             IoErrorContext::RemovingFile(path) => {
                 write!(f, "when removing {}", path.display())
             }
             IoErrorContext::RenamingFile { from, to } => write!(
                 f,
                 "when renaming {} to {}",
                 from.display(),
                 to.display()
             ),
+            IoErrorContext::CanonicalizingPath(path) => {
+                write!(f, "when canonicalizing {}", path.display())
+            }
             IoErrorContext::CurrentDir => {
                 write!(f, "error getting current working directory")
             }
             IoErrorContext::CurrentExe => {
                 write!(f, "error getting current executable")
             }
         }
     }
 }
 
 pub trait IoResultExt<T> {
     /// Annotate a possible I/O error as related to a reading a file at the
     /// given path.
     ///
     /// This allows printing something like “File not found when reading
     /// example.txt” instead of just “File not found”.
     ///
     /// Converts a `Result` with `std::io::Error` into one with `HgError`.
     fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError>;
 
     fn with_context(
         self,
         context: impl FnOnce() -> IoErrorContext,
     ) -> Result<T, HgError>;
 }
 
 impl<T> IoResultExt<T> for std::io::Result<T> {
     fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError> {
         self.with_context(|| IoErrorContext::ReadingFile(path.to_owned()))
     }
 
     fn with_context(
         self,
         context: impl FnOnce() -> IoErrorContext,
     ) -> Result<T, HgError> {
         self.map_err(|error| HgError::IoError {
             error,
             context: context(),
         })
     }
 }
 
 pub trait HgResultExt<T> {
     /// Handle missing files separately from other I/O error cases.
     ///
     /// Wraps the `Ok` type in an `Option`:
     ///
     /// * `Ok(x)` becomes `Ok(Some(x))`
     /// * An I/O "not found" error becomes `Ok(None)`
     /// * Other errors are unchanged
     fn io_not_found_as_none(self) -> Result<Option<T>, HgError>;
 }
 
 impl<T> HgResultExt<T> for Result<T, HgError> {
     fn io_not_found_as_none(self) -> Result<Option<T>, HgError> {
         match self {
             Ok(x) => Ok(Some(x)),
             Err(HgError::IoError { error, .. })
                 if error.kind() == std::io::ErrorKind::NotFound =>
             {
                 Ok(None)
             }
             Err(other_error) => Err(other_error),
         }
     }
 }

rust/hg-core/src/repo.rs

@@ -1,268 +1,265 b''
 use crate::config::{Config, ConfigError, ConfigParseError};
 use crate::errors::{HgError, IoErrorContext, IoResultExt};
 use crate::requirements;
 use crate::utils::files::get_path_from_bytes;
-use crate::utils::{current_dir, SliceExt};
+use crate::utils::SliceExt;
 use memmap::{Mmap, MmapOptions};
 use std::collections::HashSet;
 use std::path::{Path, PathBuf};
 
 /// A repository on disk
 pub struct Repo {
     working_directory: PathBuf,
     dot_hg: PathBuf,
     store: PathBuf,
     requirements: HashSet<String>,
     config: Config,
 }
 
 #[derive(Debug, derive_more::From)]
 pub enum RepoError {
     NotFound {
         at: PathBuf,
     },
     #[from]
     ConfigParseError(ConfigParseError),
     #[from]
     Other(HgError),
 }
 
 impl From<ConfigError> for RepoError {
     fn from(error: ConfigError) -> Self {
         match error {
             ConfigError::Parse(error) => error.into(),
             ConfigError::Other(error) => error.into(),
         }
     }
 }
 
 /// Filesystem access abstraction for the contents of a given "base" diretory
 #[derive(Clone, Copy)]
 pub struct Vfs<'a> {
     pub(crate) base: &'a Path,
 }
 
 impl Repo {
     /// Find a repository, either at the given path (which must contain a `.hg`
     /// sub-directory) or by searching the current directory and its
     /// ancestors.
     ///
     /// 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
     /// sub-commands get from Clap for the `-R` / `--repository` CLI argument.
     /// Having two methods would just move that `if` to almost all callers.
     pub fn find(
         config: &Config,
         explicit_path: Option<&Path>,
     ) -> Result<Self, RepoError> {
         if let Some(root) = explicit_path {
-            // Having an absolute path isn’t necessary here but can help code
-            // elsewhere
-            let absolute_root = current_dir()?.join(root);
-            if absolute_root.join(".hg").is_dir() {
-                Self::new_at_path(absolute_root, config)
-            } else if absolute_root.is_file() {
+            if root.join(".hg").is_dir() {
+                Self::new_at_path(root.to_owned(), config)
+            } else if root.is_file() {
                 Err(HgError::unsupported("bundle repository").into())
             } else {
                 Err(RepoError::NotFound {
                     at: root.to_owned(),
                 })
             }
         } else {
             let current_directory = crate::utils::current_dir()?;
             // ancestors() is inclusive: it first yields `current_directory`
             // as-is.
             for ancestor in current_directory.ancestors() {
                 if ancestor.join(".hg").is_dir() {
                     return Self::new_at_path(ancestor.to_owned(), config);
                 }
             }
             Err(RepoError::NotFound {
                 at: current_directory,
             })
         }
     }
 
     /// To be called after checking that `.hg` is a sub-directory
     fn new_at_path(
         working_directory: PathBuf,
         config: &Config,
     ) -> Result<Self, RepoError> {
         let dot_hg = working_directory.join(".hg");
 
         let mut repo_config_files = Vec::new();
         repo_config_files.push(dot_hg.join("hgrc"));
         repo_config_files.push(dot_hg.join("hgrc-not-shared"));
 
         let hg_vfs = Vfs { base: &dot_hg };
         let mut reqs = requirements::load_if_exists(hg_vfs)?;
         let relative =
             reqs.contains(requirements::RELATIVE_SHARED_REQUIREMENT);
         let shared =
             reqs.contains(requirements::SHARED_REQUIREMENT) || relative;
 
         // From `mercurial/localrepo.py`:
         //
         // if .hg/requires contains the sharesafe requirement, it means
         // there exists a `.hg/store/requires` too and we should read it
         // NOTE: presence of SHARESAFE_REQUIREMENT imply that store requirement
         // is present. We never write SHARESAFE_REQUIREMENT for a repo if store
         // is not present, refer checkrequirementscompat() for that
         //
         // However, if SHARESAFE_REQUIREMENT is not present, it means that the
         // repository was shared the old way. We check the share source
         // .hg/requires for SHARESAFE_REQUIREMENT to detect whether the
         // current repository needs to be reshared
         let share_safe = reqs.contains(requirements::SHARESAFE_REQUIREMENT);
 
         let store_path;
         if !shared {
             store_path = dot_hg.join("store");
         } else {
             let bytes = hg_vfs.read("sharedpath")?;
             let mut shared_path =
                 get_path_from_bytes(bytes.trim_end_newlines()).to_owned();
             if relative {
                 shared_path = dot_hg.join(shared_path)
             }
             if !shared_path.is_dir() {
                 return Err(HgError::corrupted(format!(
                     ".hg/sharedpath points to nonexistent directory {}",
                     shared_path.display()
                 ))
                 .into());
             }
 
             store_path = shared_path.join("store");
 
             let source_is_share_safe =
                 requirements::load(Vfs { base: &shared_path })?
                     .contains(requirements::SHARESAFE_REQUIREMENT);
 
             if share_safe && !source_is_share_safe {
                 return Err(match config
                     .get(b"share", b"safe-mismatch.source-not-safe")
                 {
                     Some(b"abort") | None => HgError::abort(
                         "abort: share source does not support share-safe requirement\n\
                         (see `hg help config.format.use-share-safe` for more information)",
                     ),
                     _ => HgError::unsupported("share-safe downgrade"),
                 }
                 .into());
             } else if source_is_share_safe && !share_safe {
                 return Err(
                     match config.get(b"share", b"safe-mismatch.source-safe") {
                         Some(b"abort") | None => HgError::abort(
                             "abort: version mismatch: source uses share-safe \
                             functionality while the current share does not\n\
                             (see `hg help config.format.use-share-safe` for more information)",
                         ),
                         _ => HgError::unsupported("share-safe upgrade"),
                     }
                     .into(),
                 );
             }
 
             if share_safe {
                 repo_config_files.insert(0, shared_path.join("hgrc"))
             }
         }
         if share_safe {
             reqs.extend(requirements::load(Vfs { base: &store_path })?);
         }
 
         let repo_config = config.combine_with_repo(&repo_config_files)?;
 
         let repo = Self {
             requirements: reqs,
             working_directory,
             store: store_path,
             dot_hg,
             config: repo_config,
         };
 
         requirements::check(&repo)?;
 
         Ok(repo)
     }
 
     pub fn working_directory_path(&self) -> &Path {
         &self.working_directory
     }
 
     pub fn requirements(&self) -> &HashSet<String> {
         &self.requirements
     }
 
     pub fn config(&self) -> &Config {
         &self.config
     }
 
     /// For accessing repository files (in `.hg`), except for the store
     /// (`.hg/store`).
     pub fn hg_vfs(&self) -> Vfs<'_> {
         Vfs { base: &self.dot_hg }
     }
 
     /// For accessing repository store files (in `.hg/store`)
     pub fn store_vfs(&self) -> Vfs<'_> {
         Vfs { base: &self.store }
     }
 
     /// For accessing the working copy
 
     // The undescore prefix silences the "never used" warning. Remove before
     // using.
     pub fn _working_directory_vfs(&self) -> Vfs<'_> {
         Vfs {
             base: &self.working_directory,
         }
     }
 
     pub fn dirstate_parents(
         &self,
     ) -> Result<crate::dirstate::DirstateParents, HgError> {
         let dirstate = self.hg_vfs().mmap_open("dirstate")?;
         let parents =
             crate::dirstate::parsers::parse_dirstate_parents(&dirstate)?;
         Ok(parents.clone())
     }
 }
 
 impl Vfs<'_> {
     pub fn join(&self, relative_path: impl AsRef<Path>) -> PathBuf {
         self.base.join(relative_path)
     }
 
     pub fn read(
         &self,
         relative_path: impl AsRef<Path>,
     ) -> Result<Vec<u8>, HgError> {
         let path = self.join(relative_path);
         std::fs::read(&path).when_reading_file(&path)
     }
 
     pub fn mmap_open(
         &self,
         relative_path: impl AsRef<Path>,
     ) -> Result<Mmap, HgError> {
         let path = self.base.join(relative_path);
         let file = std::fs::File::open(&path).when_reading_file(&path)?;
         // TODO: what are the safety requirements here?
         let mmap = unsafe { MmapOptions::new().map(&file) }
             .when_reading_file(&path)?;
         Ok(mmap)
     }
 
     pub fn rename(
         &self,
         relative_from: impl AsRef<Path>,
         relative_to: impl AsRef<Path>,
     ) -> Result<(), HgError> {
         let from = self.join(relative_from);
         let to = self.join(relative_to);
         std::fs::rename(&from, &to)
             .with_context(|| IoErrorContext::RenamingFile { from, to })
     }
 }

rust/rhg/src/commands/cat.rs

@@ -1,67 +1,69 b''
 use crate::error::CommandError;
 use clap::Arg;
 use hg::operations::cat;
 use hg::utils::hg_path::HgPathBuf;
 use micro_timer::timed;
 use std::convert::TryFrom;
 
 pub const HELP_TEXT: &str = "
 Output the current or given revision of files
 ";
 
 pub fn args() -> clap::App<'static, 'static> {
     clap::SubCommand::with_name("cat")
         .arg(
             Arg::with_name("rev")
                 .help("search the repository as it is in REV")
                 .short("-r")
                 .long("--revision")
                 .value_name("REV")
                 .takes_value(true),
         )
         .arg(
             clap::Arg::with_name("files")
                 .required(true)
                 .multiple(true)
                 .empty_values(false)
                 .value_name("FILE")
                 .help("Activity to start: activity@category"),
         )
         .about(HELP_TEXT)
 }
 
 #[timed]
 pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
     let rev = invocation.subcommand_args.value_of("rev");
     let file_args = match invocation.subcommand_args.values_of("files") {
         Some(files) => files.collect(),
         None => vec![],
     };
 
     let repo = invocation.repo?;
     let cwd = hg::utils::current_dir()?;
+    let working_directory = repo.working_directory_path();
+    let working_directory = cwd.join(working_directory); // Make it absolute
 
     let mut files = vec![];
     for file in file_args.iter() {
         // TODO: actually normalize `..` path segments etc?
         let normalized = cwd.join(&file);
         let stripped = normalized
-            .strip_prefix(&repo.working_directory_path())
+            .strip_prefix(&working_directory)
             // TODO: error message for path arguments outside of the repo
             .map_err(|_| CommandError::abort(""))?;
         let hg_file = HgPathBuf::try_from(stripped.to_path_buf())
             .map_err(|e| CommandError::abort(e.to_string()))?;
         files.push(hg_file);
     }
 
     match rev {
         Some(rev) => {
             let data = cat(&repo, rev, &files).map_err(|e| (e, rev))?;
             invocation.ui.write_stdout(&data)?;
             Ok(())
         }
         None => Err(CommandError::unsupported(
             "`rhg cat` without `--rev` / `-r`",
         )),
     }
 }

rust/rhg/src/commands/files.rs

@@ -1,68 +1,71 b''
 use crate::error::CommandError;
 use crate::ui::Ui;
 use clap::Arg;
 use hg::operations::list_rev_tracked_files;
 use hg::operations::Dirstate;
 use hg::repo::Repo;
+use hg::utils::current_dir;
 use hg::utils::files::{get_bytes_from_path, relativize_path};
 use hg::utils::hg_path::{HgPath, HgPathBuf};
 
 pub const HELP_TEXT: &str = "
 List tracked files.
 
 Returns 0 on success.
 ";
 
 pub fn args() -> clap::App<'static, 'static> {
     clap::SubCommand::with_name("files")
         .arg(
             Arg::with_name("rev")
                 .help("search the repository as it is in REV")
                 .short("-r")
                 .long("--revision")
                 .value_name("REV")
                 .takes_value(true),
         )
         .about(HELP_TEXT)
 }
 
 pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
     let relative = invocation.config.get(b"ui", b"relative-paths");
     if relative.is_some() {
         return Err(CommandError::unsupported(
             "non-default ui.relative-paths",
         ));
     }
 
     let rev = invocation.subcommand_args.value_of("rev");
 
     let repo = invocation.repo?;
     if let Some(rev) = rev {
         let files = list_rev_tracked_files(repo, rev).map_err(|e| (e, rev))?;
         display_files(invocation.ui, repo, files.iter())
     } else {
         let distate = Dirstate::new(repo)?;
         let files = distate.tracked_files()?;
         display_files(invocation.ui, repo, files)
     }
 }
 
 fn display_files<'a>(
     ui: &Ui,
     repo: &Repo,
     files: impl IntoIterator<Item = &'a HgPath>,
 ) -> Result<(), CommandError> {
     let cwd = HgPathBuf::from(get_bytes_from_path(hg::utils::current_dir()?));
+    let working_directory = repo.working_directory_path();
+    let working_directory = current_dir()?.join(working_directory); // Make it absolute
     let working_directory =
-        HgPathBuf::from(get_bytes_from_path(repo.working_directory_path()));
+        HgPathBuf::from(get_bytes_from_path(working_directory));
 
     let mut stdout = ui.stdout_buffer();
 
     for file in files {
         let file = working_directory.join(file);
         stdout.write_all(relativize_path(&file, &cwd).as_ref())?;
         stdout.write_all(b"\n")?;
     }
     stdout.flush()?;
     Ok(())
 }

rust/rhg/src/commands/root.rs

@@ -1,22 +1,28 b''
 use crate::error::CommandError;
 use format_bytes::format_bytes;
+use hg::errors::{IoErrorContext, IoResultExt};
 use hg::utils::files::get_bytes_from_path;
 
 pub const HELP_TEXT: &str = "
 Print the root directory of the current repository.
 
 Returns 0 on success.
 ";
 
 pub fn args() -> clap::App<'static, 'static> {
     clap::SubCommand::with_name("root").about(HELP_TEXT)
 }
 
 pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
     let repo = invocation.repo?;
-    let bytes = get_bytes_from_path(repo.working_directory_path());
+    let working_directory = repo.working_directory_path();
+    let working_directory = std::fs::canonicalize(working_directory)
+        .with_context(|| {
+            IoErrorContext::CanonicalizingPath(working_directory.to_owned())
+        })?;
+    let bytes = get_bytes_from_path(&working_directory);
     invocation
         .ui
         .write_stdout(&format_bytes!(b"{}\n", bytes.as_slice()))?;
     Ok(())
 }