This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8609

hg-core: add FindRoot operation to find repository root path
ClosedPublic

Authored by acezar on Jun 5 2020, 10:31 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG5965efb609b6: hg-core: add FindRoot operation to find repository root path

Diff Detail

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

Event Timeline

acezar created this revision.Jun 5 2020, 10:31 AM
marmoute added a subscriber: marmoute.Jun 9 2020, 5:14 AM

Pathc looks overall good to me. I have a question regarding the test about some potential tempfile leackage that I woudl like clarified.

rust/hg-core/src/operations/find_root.rs
22

(small nits: s/current/current/)

70

small nits: s/anscestors/ancestors/

91

Do we have to do anything to make sure the temporary directory is deleted at the end of the run ? or are we leaking them ?

Alphare accepted this revision.Jun 15 2020, 5:31 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/operations/find_root.rs
11

nit: s/exists/exist

91

https://docs.rs/tempfile/3.1.0/tempfile/struct.TempDir.html#resource-leaking

This is what tempfile does, whether that's acceptable is arguable.

acezar updated this revision to Diff 21644.Jun 16 2020, 12:20 PM
acezar added a commit: rHG5965efb609b6: hg-core: add FindRoot operation to find repository root path.Jun 16 2020, 5:42 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG5965efb609b6: hg-core: add FindRoot operation to find repository root path (authored by acezar). · Explain Why
This revision was automatically updated to reflect the committed changes.
acezar updated this revision to Diff 21654.Jun 17 2020, 4:25 AM

Revision Contents
Changeset List

Diff 21654

rust/hg-core/src/operations/find_root.rs

  • This file was added.
use super::Operation;
use std::fmt;
use std::path::{Path, PathBuf};
/// Kind of error encoutered by FindRoot
#[derive(Debug)]
pub enum FindRootErrorKind {
/// Root of the repository has not been found
/// Contains the current directory used by FindRoot
RootNotFound(PathBuf),
/// The current directory does not exists or permissions are insufficient
AlphareUnsubmitted
Not Done

nit: s/exists/exist

Alphare: nit: s/exists/exist
/// to get access to it
GetCurrentDirError(std::io::Error),
}
/// A FindRoot error
#[derive(Debug)]
pub struct FindRootError {
/// Kind of error encoutered by FindRoot
pub kind: FindRootErrorKind,
}
marmouteUnsubmitted
Not Done

(small nits: s/current/current/)

marmoute: (small nits: s/current/current/)
impl std::error::Error for FindRootError {}
impl fmt::Display for FindRootError {
fn fmt(&self, _f: &mut fmt::Formatter<'_>) -> fmt::Result {
unimplemented!()
}
}
/// Find the root of the repository
/// by searching for a .hg directory in the current directory and its
/// ancestors
pub struct FindRoot<'a> {
current_dir: Option<&'a Path>,
}
impl<'a> FindRoot<'a> {
pub fn new() -> Self {
Self { current_dir: None }
}
pub fn new_from_path(current_dir: &'a Path) -> Self {
Self {
current_dir: Some(current_dir),
}
}
}
impl<'a> Operation<PathBuf> for FindRoot<'a> {
type Error = FindRootError;
fn run(&self) -> Result<PathBuf, Self::Error> {
let current_dir = match self.current_dir {
None => std::env::current_dir().or_else(|e| {
Err(FindRootError {
kind: FindRootErrorKind::GetCurrentDirError(e),
})
})?,
Some(path) => path.into(),
};
if current_dir.join(".hg").exists() {
return Ok(current_dir.into());
}
let mut ancestors = current_dir.ancestors();
while let Some(parent) = ancestors.next() {
if parent.join(".hg").exists() {
return Ok(parent.into());
}
marmouteUnsubmitted
Not Done

small nits: s/anscestors/ancestors/

marmoute: small nits: s/anscestors/ancestors/
}
Err(FindRootError {
kind: FindRootErrorKind::RootNotFound(current_dir.to_path_buf()),
})
}
}
#[cfg(test)]
mod tests {
use super::*;
use std::fs;
use tempfile;
#[test]
fn dot_hg_not_found() {
let tmp_dir = tempfile::TempDir::new().unwrap();
let path = tmp_dir.path();
let err = FindRoot::new_from_path(&path).run().unwrap_err();
// TODO do something better
marmouteUnsubmitted
Not Done

Do we have to do anything to make sure the temporary directory is deleted at the end of the run ? or are we leaking them ?

marmoute: Do we have to do anything to make sure the temporary directory is deleted at the end of the run…
AlphareUnsubmitted
Not Done

https://docs.rs/tempfile/3.1.0/tempfile/struct.TempDir.html#resource-leaking

This is what tempfile does, whether that's acceptable is arguable.

Alphare: https://docs.rs/tempfile/3.1.0/tempfile/struct.TempDir.html#resource-leaking This is what…
assert!(match err {
FindRootError { kind } => match kind {
FindRootErrorKind::RootNotFound(p) => p == path.to_path_buf(),
_ => false,
},
})
}
#[test]
fn dot_hg_in_current_path() {
let tmp_dir = tempfile::TempDir::new().unwrap();
let root = tmp_dir.path();
fs::create_dir_all(root.join(".hg")).unwrap();
let result = FindRoot::new_from_path(&root).run().unwrap();
assert_eq!(result, root)
}
#[test]
fn dot_hg_in_parent() {
let tmp_dir = tempfile::TempDir::new().unwrap();
let root = tmp_dir.path();
fs::create_dir_all(root.join(".hg")).unwrap();
let result =
FindRoot::new_from_path(&root.join("some/nested/directory"))
.run()
.unwrap();
assert_eq!(result, root)
}
} /* tests */

rust/hg-core/src/operations/mod.rs

mod find_root;
pub use find_root::{FindRoot, FindRootError, FindRootErrorKind};
/// An interface for high-level hg operations. /// An interface for high-level hg operations.
/// ///
/// A distinction is made between operation and commands. /// A distinction is made between operation and commands.
/// An operation is what can be done whereas a command is what is exposed by /// An operation is what can be done whereas a command is what is exposed by
/// the cli. A single command can use several operations to achieve its goal. /// the cli. A single command can use several operations to achieve its goal.
pub trait Operation<T> { pub trait Operation<T> {
type Error; type Error;
fn run(&self) -> Result<T, Self::Error>; fn run(&self) -> Result<T, Self::Error>;
} }