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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 26421

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(())
} }