( )⚙ D10138 rhg: Don’t make repository path absolute too early

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10138

rhg: Don’t make repository path absolute too early
ClosedPublic

Authored by SimonSapin on Mar 9 2021, 4:40 AM.

Details

Reviewers
marmoute
Alphare
Group Reviewers
hg-reviewers
Commits
rHG97ac588b6d9e: rhg: Don’t make repository path absolute too early
Summary

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

Diff Detail

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

Event Timeline

SimonSapin created this revision.Mar 9 2021, 4:40 AM
baymax updated this revision to Diff 26197.Mar 9 2021, 8:40 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

marmoute accepted this revision.Mar 9 2021, 1:20 PM
marmoute requested changes to this revision.
This revision now requires changes to proceed.Mar 9 2021, 1:22 PM
Alphare added a subscriber: Alphare.Mar 10 2021, 3:59 AM

@marmoute why did you ask for changes on this diff?

rust/rhg/src/commands/root.rs
19

FYI: https://www.reddit.com/r/rust/comments/jqx8hf/normpath_more_reliable_path_manipulation/

I'm not sure if we want to do anything about it for now other than maybe adding a comment and a maybe wrapper function for all canonicalize.

SimonSapin added inline comments.Mar 11 2021, 4:51 AM
rust/rhg/src/commands/root.rs
19

https://docs.rs/normpath/0.2.0/normpath/trait.PathExt.html#tymethod.normalize says this method is the same as canonicalize on Unix. I think we can revisit if we run into the Windows-specific issues this crate aims to solve.

Alphare accepted this revision.Mar 11 2021, 5:18 AM
marmoute accepted this revision.Mar 12 2021, 11:52 AM
This revision now requires review to proceed.Mar 12 2021, 11:52 AM
SimonSapin edited the summary of this revision. (Show Details)Mar 12 2021, 5:07 PM
SimonSapin updated this revision to Diff 26298.
baymax updated this revision to Diff 26344.Mar 15 2021, 9:10 AM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

SimonSapin added a commit: rHG97ac588b6d9e: rhg: Don’t make repository path absolute too early.Mar 16 2021, 5:47 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG97ac588b6d9e: rhg: Don’t make repository path absolute too early (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 26298

rust/hg-core/src/errors.rs

pub enum IoErrorContext { pub enum IoErrorContext {
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`
CanonicalizingPath(std::path::PathBuf),
/// `std::env::current_dir` /// `std::env::current_dir`
CurrentDir, CurrentDir,
/// `std::env::current_exe` /// `std::env::current_exe`
CurrentExe, CurrentExe,
} }
impl HgError { impl HgError {
pub fn corrupted(explanation: impl Into<String>) -> Self { pub fn corrupted(explanation: impl Into<String>) -> Self {
write!(f, "when removing {}", path.display()) write!(f, "when removing {}", path.display())
} }
IoErrorContext::RenamingFile { from, to } => write!( IoErrorContext::RenamingFile { from, to } => write!(
f, f,
"when renaming {} to {}", "when renaming {} to {}",
from.display(), from.display(),
to.display() to.display()
), ),
IoErrorContext::CanonicalizingPath(path) => {
write!(f, "when canonicalizing {}", path.display())
}
IoErrorContext::CurrentDir => { IoErrorContext::CurrentDir => {
write!(f, "error getting current working directory") write!(f, "error getting current working directory")
} }
IoErrorContext::CurrentExe => { IoErrorContext::CurrentExe => {
write!(f, "error getting current executable") write!(f, "error getting current executable")
} }
} }
} }

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::requirements; use crate::requirements;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::{current_dir, SliceExt}; use crate::utils::SliceExt;
use memmap::{Mmap, MmapOptions}; use memmap::{Mmap, MmapOptions};
use std::collections::HashSet; use std::collections::HashSet;
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,
/// 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<&Path>, explicit_path: Option<&Path>,
) -> Result<Self, RepoError> { ) -> Result<Self, RepoError> {
if let Some(root) = explicit_path { if let Some(root) = explicit_path {
// Having an absolute path isn’t necessary here but can help code if root.join(".hg").is_dir() {
// elsewhere Self::new_at_path(root.to_owned(), config)
let absolute_root = current_dir()?.join(root); } else if root.is_file() {
if absolute_root.join(".hg").is_dir() {
Self::new_at_path(absolute_root, config)
} else if absolute_root.is_file() {
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 current_directory = crate::utils::current_dir()?; let current_directory = crate::utils::current_dir()?;

rust/rhg/src/commands/cat.rs

let rev = invocation.subcommand_args.value_of("rev"); let rev = invocation.subcommand_args.value_of("rev");
let file_args = match invocation.subcommand_args.values_of("files") { let file_args = match invocation.subcommand_args.values_of("files") {
Some(files) => files.collect(), Some(files) => files.collect(),
None => vec![], None => vec![],
}; };
let repo = invocation.repo?; let repo = invocation.repo?;
let cwd = hg::utils::current_dir()?; 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![]; let mut files = vec![];
for file in file_args.iter() { for file in file_args.iter() {
// TODO: actually normalize `..` path segments etc? // TODO: actually normalize `..` path segments etc?
let normalized = cwd.join(&file); let normalized = cwd.join(&file);
let stripped = normalized let stripped = normalized
.strip_prefix(&repo.working_directory_path()) .strip_prefix(&working_directory)
// TODO: error message for path arguments outside of the repo // TODO: error message for path arguments outside of the repo
.map_err(|_| CommandError::abort(""))?; .map_err(|_| CommandError::abort(""))?;
let hg_file = HgPathBuf::try_from(stripped.to_path_buf()) let hg_file = HgPathBuf::try_from(stripped.to_path_buf())
.map_err(|e| CommandError::abort(e.to_string()))?; .map_err(|e| CommandError::abort(e.to_string()))?;
files.push(hg_file); files.push(hg_file);
} }
match rev { match rev {

rust/rhg/src/commands/files.rs

use crate::error::CommandError; use crate::error::CommandError;
use crate::ui::Ui; use crate::ui::Ui;
use clap::Arg; use clap::Arg;
use hg::operations::list_rev_tracked_files; use hg::operations::list_rev_tracked_files;
use hg::operations::Dirstate; use hg::operations::Dirstate;
use hg::repo::Repo; use hg::repo::Repo;
use hg::utils::current_dir;
use hg::utils::files::{get_bytes_from_path, relativize_path}; use hg::utils::files::{get_bytes_from_path, relativize_path};
use hg::utils::hg_path::{HgPath, HgPathBuf}; use hg::utils::hg_path::{HgPath, HgPathBuf};
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
List tracked files. List tracked files.
Returns 0 on success. Returns 0 on success.
"; ";
} }
fn display_files<'a>( fn display_files<'a>(
ui: &Ui, ui: &Ui,
repo: &Repo, repo: &Repo,
files: impl IntoIterator<Item = &'a HgPath>, files: impl IntoIterator<Item = &'a HgPath>,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let cwd = HgPathBuf::from(get_bytes_from_path(hg::utils::current_dir()?)); 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 = 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(); let mut stdout = ui.stdout_buffer();
for file in files { for file in files {
let file = working_directory.join(file); let file = working_directory.join(file);
stdout.write_all(relativize_path(&file, &cwd).as_ref())?; stdout.write_all(relativize_path(&file, &cwd).as_ref())?;
stdout.write_all(b"\n")?; stdout.write_all(b"\n")?;
} }
stdout.flush()?; stdout.flush()?;
Ok(()) Ok(())
} }

rust/rhg/src/commands/root.rs

use crate::error::CommandError; use crate::error::CommandError;
use format_bytes::format_bytes; use format_bytes::format_bytes;
use hg::errors::{IoErrorContext, IoResultExt};
use hg::utils::files::get_bytes_from_path; use hg::utils::files::get_bytes_from_path;
pub const HELP_TEXT: &str = " pub const HELP_TEXT: &str = "
Print the root directory of the current repository. Print the root directory of the current repository.
Returns 0 on success. Returns 0 on success.
"; ";
pub fn args() -> clap::App<'static, 'static> { pub fn args() -> clap::App<'static, 'static> {
clap::SubCommand::with_name("root").about(HELP_TEXT) clap::SubCommand::with_name("root").about(HELP_TEXT)
} }
pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> { pub fn run(invocation: &crate::CliInvocation) -> Result<(), CommandError> {
let repo = invocation.repo?; 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)
AlphareUnsubmitted
Not Done

FYI: https://www.reddit.com/r/rust/comments/jqx8hf/normpath_more_reliable_path_manipulation/

I'm not sure if we want to do anything about it for now other than maybe adding a comment and a maybe wrapper function for all canonicalize.

Alphare: FYI: https://www.reddit.com/r/rust/comments/jqx8hf/normpath_more_reliable_path_manipulation/…
SimonSapinAuthorUnsubmitted
Done

https://docs.rs/normpath/0.2.0/normpath/trait.PathExt.html#tymethod.normalize says this method is the same as canonicalize on Unix. I think we can revisit if we run into the Windows-specific issues this crate aims to solve.

SimonSapin: https://docs.rs/normpath/0.2.0/normpath/trait.PathExt.html#tymethod.normalize says this method…
.with_context(|| {
IoErrorContext::CanonicalizingPath(working_directory.to_owned())
})?;
let bytes = get_bytes_from_path(&working_directory);
invocation invocation
.ui .ui
.write_stdout(&format_bytes!(b"{}\n", bytes.as_slice()))?; .write_stdout(&format_bytes!(b"{}\n", bytes.as_slice()))?;
Ok(()) Ok(())
} }