Avoid fs::canonicalize (#539)

Previously, we used `fs::canonicalize` to ensure paths used in search
were absolute. This lead to bad behavior when the justfile was symbolic
link to a file in another directory. Additionally, on Windows, this
produced paths in extended length syntax, which, I believe, has
compatibility issues.

This diff replaces uses of `fs::canonicalize`  with a simpler algorithm
that roots path in the invocation directory (which will be a no-op if
said path is already absolute), uses `Path::components` to remove extra
`/` and `.`, and resolves instances of `..` without following symlinks, by
removing the `..` and the component that proceeds it.
This commit is contained in:
Casey Rodarmor 2019-11-19 03:51:44 -08:00 committed by GitHub
parent f8693d6fe0
commit c4e9857ebd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 78 additions and 28 deletions

View File

@ -32,7 +32,7 @@ pub(crate) use snafu::{ResultExt, Snafu};
pub(crate) use unicode_width::UnicodeWidthChar; pub(crate) use unicode_width::UnicodeWidthChar;
// modules // modules
pub(crate) use crate::{config_error, keyword, search_error, setting}; pub(crate) use crate::{config_error, keyword, setting};
// functions // functions
pub(crate) use crate::{default::default, empty::empty, load_dotenv::load_dotenv, output::output}; pub(crate) use crate::{default::default, empty::empty, load_dotenv::load_dotenv, output::output};

View File

@ -9,8 +9,6 @@ pub(crate) enum ConfigError {
message message
))] ))]
Internal { message: String }, Internal { message: String },
#[snafu(display("Could not canonicalize justfile path `{}`: {}", path.display(), source))]
JustfilePathCanonicalize { path: PathBuf, source: io::Error },
#[snafu(display("Failed to get current directory: {}", source))] #[snafu(display("Failed to get current directory: {}", source))]
CurrentDir { source: io::Error }, CurrentDir { source: io::Error },
#[snafu(display( #[snafu(display(

View File

@ -1,5 +1,7 @@
use crate::common::*; use crate::common::*;
use std::path::Component;
const FILENAME: &str = "justfile"; const FILENAME: &str = "justfile";
pub(crate) struct Search { pub(crate) struct Search {
@ -25,12 +27,7 @@ impl Search {
} }
SearchConfig::FromSearchDirectory { search_directory } => { SearchConfig::FromSearchDirectory { search_directory } => {
let search_directory = let search_directory = Self::clean(invocation_directory, search_directory);
search_directory
.canonicalize()
.context(search_error::Canonicalize {
path: search_directory,
})?;
let justfile = Self::justfile(&search_directory)?; let justfile = Self::justfile(&search_directory)?;
@ -43,7 +40,7 @@ impl Search {
} }
SearchConfig::WithJustfile { justfile } => { SearchConfig::WithJustfile { justfile } => {
let justfile: PathBuf = justfile.to_path_buf(); let justfile = Self::clean(invocation_directory, justfile);
let working_directory = Self::working_directory_from_justfile(&justfile)?; let working_directory = Self::working_directory_from_justfile(&justfile)?;
@ -57,8 +54,8 @@ impl Search {
justfile, justfile,
working_directory, working_directory,
} => Ok(Search { } => Ok(Search {
justfile: justfile.to_path_buf(), justfile: Self::clean(invocation_directory, justfile),
working_directory: working_directory.to_path_buf(), working_directory: Self::clean(invocation_directory, working_directory),
}), }),
} }
} }
@ -92,16 +89,30 @@ impl Search {
} }
} }
fn working_directory_from_justfile(justfile: &Path) -> SearchResult<PathBuf> { fn clean(invocation_directory: &Path, path: &Path) -> PathBuf {
let justfile_canonical = justfile let path = invocation_directory.join(path);
.canonicalize()
.context(search_error::Canonicalize { path: justfile })?;
let mut clean = Vec::new();
for component in path.components() {
if component == Component::ParentDir {
if let Some(Component::Normal(_)) = clean.last() {
clean.pop();
}
} else {
clean.push(component);
}
}
clean.into_iter().collect()
}
fn working_directory_from_justfile(justfile: &Path) -> SearchResult<PathBuf> {
Ok( Ok(
justfile_canonical justfile
.parent() .parent()
.ok_or_else(|| SearchError::JustfileHadNoParent { .ok_or_else(|| SearchError::JustfileHadNoParent {
path: justfile_canonical.clone(), path: justfile.to_path_buf(),
})? })?
.to_owned(), .to_owned(),
) )
@ -111,6 +122,7 @@ impl Search {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use test_utilities::tmptree;
#[test] #[test]
fn not_found() { fn not_found() {
@ -228,4 +240,51 @@ mod tests {
_ => panic!("No errors were expected"), _ => panic!("No errors were expected"),
} }
} }
#[test]
fn justfile_symlink_parent() {
let tmp = tmptree! {
src: "",
sub: {},
};
let src = tmp.path().join("src");
let sub = tmp.path().join("sub");
let justfile = sub.join("justfile");
#[cfg(unix)]
std::os::unix::fs::symlink(&src, &justfile).unwrap();
#[cfg(windows)]
std::os::windows::fs::symlink_file(&src, &justfile).unwrap();
let search_config = SearchConfig::FromInvocationDirectory;
let search = Search::search(&search_config, &sub).unwrap();
assert_eq!(search.justfile, justfile);
assert_eq!(search.working_directory, sub);
}
#[test]
fn clean() {
let cases = &[
("/", "foo", "/foo"),
("/bar", "/foo", "/foo"),
("//foo", "bar//baz", "/foo/bar/baz"),
("/", "..", "/"),
("/", "/..", "/"),
("/..", "", "/"),
("/../../../..", "../../../", "/"),
("/.", "./", "/"),
("/foo/../", "bar", "/bar"),
("/foo/bar", "..", "/foo"),
("/foo/bar/", "..", "/foo"),
];
for (prefix, suffix, want) in cases {
let have = Search::clean(Path::new(prefix), Path::new(suffix));
assert_eq!(have, Path::new(want));
}
}
} }

View File

@ -12,9 +12,7 @@ pub(crate) enum SearchError {
.map(|candidate| candidate.file_name().unwrap().to_string_lossy()) .map(|candidate| candidate.file_name().unwrap().to_string_lossy())
), ),
))] ))]
MultipleCandidates { MultipleCandidates { candidates: Vec<PathBuf> },
candidates: Vec<PathBuf>,
},
#[snafu(display( #[snafu(display(
"I/O error reading directory `{}`: {}", "I/O error reading directory `{}`: {}",
directory.display(), directory.display(),
@ -26,13 +24,8 @@ pub(crate) enum SearchError {
}, },
#[snafu(display("No justfile found"))] #[snafu(display("No justfile found"))]
NotFound, NotFound,
Canonicalize { #[snafu(display("Justfile path had no parent: {}", path.display()))]
path: PathBuf, JustfileHadNoParent { path: PathBuf },
source: io::Error,
},
JustfileHadNoParent {
path: PathBuf,
},
} }
impl Error for SearchError {} impl Error for SearchError {}