This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10767

rhg: look for repository in ancestors also instead of cwd only
ClosedPublic

Authored by pulkit on May 24 2021, 6:59 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG88119fffecc8: rhg: look for repository in ancestors also instead of cwd only
Summary

Last patch introduced config reading at startup to parse value of --repository
flag. However, that patch only tried to check for current repository at current
working directory and not it's ancestors. This patch fixes that.

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

pulkit created this revision.May 24 2021, 6:59 AM
SimonSapin added a subscriber: SimonSapin.May 24 2021, 1:49 PM
SimonSapin added inline comments.
rust/hg-core/src/repo.rs
46–48

This doc should also mention ancestors. Something like "Find the nearest repository root, if any, in the current directory or its ancestors"

The try_ prefix can be dropped from the name since fallibility is already apparent from the return type and there’s no equivalent infallibly (panicky) API. Compare with std::cell::RefCell which has both borrow and try_borrow, and std::fs::File whose open method is not named try_open.

85–86

Please use if let or match instead of is_ok + unwrap

pulkit updated this revision to Diff 28196.May 24 2021, 6:31 PM
pulkit updated this revision to Diff 28202.May 25 2021, 6:01 PM
baymax updated this revision to Diff 28207.May 26 2021, 4:48 PM

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

baymax updated this revision to Diff 28505.Jun 7 2021, 7:15 AM

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

SimonSapin added inline comments.Jun 7 2021, 10:59 AM
rust/hg-core/src/repo.rs
85–86

Sorry I didn’t realize this earlier: this entire match can be replaced with the ? operator:

let root = Self::find_repo_root()?
Self::new_at_path(root, config)
pulkit updated this revision to Diff 28514.Jun 9 2021, 3:51 AM
Alphare accepted this revision.Jun 9 2021, 5:14 AM
This revision is now accepted and ready to land.Jun 9 2021, 5:14 AM

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/repo.rs (29 lines)
Mrust/rhg/src/main.rs (5 lines)

Diff 28516

rust/hg-core/src/repo.rs

/// Filesystem access abstraction for the contents of a given "base" diretory /// Filesystem access abstraction for the contents of a given "base" diretory
#[derive(Clone, Copy)] #[derive(Clone, Copy)]
pub struct Vfs<'a> { pub struct Vfs<'a> {
pub(crate) base: &'a Path, pub(crate) base: &'a Path,
} }
impl Repo { impl Repo {
/// tries to find nearest repository root in current working directory or
/// its ancestors
pub fn find_repo_root() -> Result<PathBuf, RepoError> {
SimonSapinUnsubmitted
Not Done

This doc should also mention ancestors. Something like "Find the nearest repository root, if any, in the current directory or its ancestors"

The try_ prefix can be dropped from the name since fallibility is already apparent from the return type and there’s no equivalent infallibly (panicky) API. Compare with std::cell::RefCell which has both borrow and try_borrow, and std::fs::File whose open method is not named try_open.

SimonSapin: This doc should also mention ancestors. Something like "Find the nearest repository root, if…
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 Ok(ancestor.to_path_buf());
}
}
return Err(RepoError::NotFound {
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 root.join(".hg").is_dir() {
Self::new_at_path(root.to_owned(), config) Self::new_at_path(root.to_owned(), config)
} else if root.is_file() { } else if 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 root = Self::find_repo_root()?;
// ancestors() is inclusive: it first yields `current_directory` Self::new_at_path(root, config)
SimonSapinUnsubmitted
Not Done

Please use if let or match instead of is_ok + unwrap

SimonSapin: Please use `if let` or `match` instead of `is_ok` + `unwrap`
SimonSapinUnsubmitted
Not Done

Sorry I didn’t realize this earlier: this entire match can be replaced with the ? operator:

let root = Self::find_repo_root()?
Self::new_at_path(root, config)
SimonSapin: Sorry I didn’t realize this earlier: this entire `match` can be replaced with the `?` operator…
// 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 /// To be called after checking that `.hg` is a sub-directory
fn new_at_path( fn new_at_path(
working_directory: PathBuf, working_directory: PathBuf,
config: &Config, config: &Config,
) -> Result<Self, RepoError> { ) -> Result<Self, RepoError> {

rust/rhg/src/main.rs

} }
let repo_arg = early_args.repo.unwrap_or(Vec::new()); let repo_arg = early_args.repo.unwrap_or(Vec::new());
let repo_path: Option<PathBuf> = { let repo_path: Option<PathBuf> = {
if repo_arg.is_empty() { if repo_arg.is_empty() {
None None
} else { } else {
let local_config = { let local_config = {
if std::env::var_os("HGRCSKIPREPO").is_none() { if std::env::var_os("HGRCSKIPREPO").is_none() {
let current_dir = hg::utils::current_dir(); // TODO: handle errors from find_repo_root
// TODO: handle errors from current_dir if let Ok(current_dir_path) = Repo::find_repo_root() {
if let Ok(current_dir_path) = current_dir {
let config_files = vec![ let config_files = vec![
ConfigSource::AbsPath( ConfigSource::AbsPath(
current_dir_path.join(".hg/hgrc"), current_dir_path.join(".hg/hgrc"),
), ),
ConfigSource::AbsPath( ConfigSource::AbsPath(
current_dir_path.join(".hg/hgrc-not-shared"), current_dir_path.join(".hg/hgrc-not-shared"),
), ),
]; ];