diff --git a/Cargo.lock b/Cargo.lock index 0225521baf..d97083b1c1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1339,6 +1339,7 @@ dependencies = [ "git-sec", "git-testtools", "is_ci", + "serial_test", "tempfile", "thiserror", ] diff --git a/git-discover/Cargo.toml b/git-discover/Cargo.toml index fbba18c1e7..32196dfa13 100644 --- a/git-discover/Cargo.toml +++ b/git-discover/Cargo.toml @@ -22,6 +22,7 @@ thiserror = "1.0.26" [dev-dependencies] git-testtools = { path = "../tests/tools" } +serial_test = "0.9.0" is_ci = "1.1.1" [target.'cfg(target_os = "macos")'.dev-dependencies] diff --git a/git-discover/src/upwards/mod.rs b/git-discover/src/upwards/mod.rs index f70ec81801..b80098fc2c 100644 --- a/git-discover/src/upwards/mod.rs +++ b/git-discover/src/upwards/mod.rs @@ -141,6 +141,10 @@ pub(crate) mod function { cursor.pop(); } } + if cursor.parent().map_or(false, |p| p.as_os_str().is_empty()) { + cursor = cwd.to_path_buf(); + dir_made_absolute = true; + } if !cursor.pop() { if dir_made_absolute || matches!( @@ -151,16 +155,13 @@ pub(crate) mod function { break Err(Error::NoGitRepository { path: dir.into_owned() }); } else { dir_made_absolute = true; - cursor = if cursor.as_os_str().is_empty() { - cwd.clone().into_owned() - } else { - // TODO: realpath or absolutize? No test runs into this. - git_path::absolutize(&cursor, cwd.as_ref()) - .ok_or_else(|| Error::InvalidInput { - directory: cursor.clone(), - })? - .into_owned() - } + debug_assert!(!cursor.as_os_str().is_empty()); + // TODO: realpath or absolutize? No test runs into this. + cursor = git_path::absolutize(&cursor, cwd.as_ref()) + .ok_or_else(|| Error::InvalidInput { + directory: cursor.clone(), + })? + .into_owned(); } } } diff --git a/git-discover/src/upwards/util.rs b/git-discover/src/upwards/util.rs index 1d3dd5fd26..3c1574ceb1 100644 --- a/git-discover/src/upwards/util.rs +++ b/git-discover/src/upwards/util.rs @@ -34,18 +34,24 @@ pub(crate) fn shorten_path_with_cwd(cursor: PathBuf, cwd: &Path) -> PathBuf { /// Find the number of components parenting the `search_dir` before the first directory in `ceiling_dirs`. /// `search_dir` needs to be absolutized, and we absolutize every ceiling as well. pub(crate) fn find_ceiling_height(search_dir: &Path, ceiling_dirs: &[PathBuf], cwd: &Path) -> Option { + if ceiling_dirs.is_empty() { + return None; + } + + let search_realpath; + let search_dir = if search_dir.is_absolute() { + search_dir + } else { + search_realpath = git_path::realpath_opts(search_dir, cwd, git_path::realpath::MAX_SYMLINKS).ok()?; + search_realpath.as_path() + }; ceiling_dirs .iter() .filter_map(|ceiling_dir| { let mut ceiling_dir = git_path::absolutize(ceiling_dir, cwd)?; - match (search_dir.is_absolute(), ceiling_dir.is_absolute()) { - (true, false) => ceiling_dir = cwd.join(ceiling_dir.as_ref()).into(), - (false, true) => { - let stripped = ceiling_dir.as_ref().strip_prefix(cwd).ok()?.to_owned(); - ceiling_dir = stripped.into(); - } - (false, false) | (true, true) => {} - }; + if !ceiling_dir.is_absolute() { + ceiling_dir = git_path::absolutize(cwd.join(ceiling_dir.as_ref()), cwd)?; + } search_dir .strip_prefix(ceiling_dir.as_ref()) .ok() diff --git a/git-discover/tests/fixtures/make_basic_repo.sh b/git-discover/tests/fixtures/make_basic_repo.sh index 6b898833c8..7177aa3a79 100644 --- a/git-discover/tests/fixtures/make_basic_repo.sh +++ b/git-discover/tests/fixtures/make_basic_repo.sh @@ -10,6 +10,7 @@ git commit -q -m c1 echo hello >> this git commit -q -am c2 +mkdir subdir mkdir -p some/very/deeply/nested/subdir git clone --bare --shared . bare.git diff --git a/git-discover/tests/isolated.rs b/git-discover/tests/isolated.rs new file mode 100644 index 0000000000..6a98bd90c9 --- /dev/null +++ b/git-discover/tests/isolated.rs @@ -0,0 +1,70 @@ +use git_discover::upwards::Options; +use serial_test::serial; +use std::path::{Path, PathBuf}; + +#[test] +#[serial] +fn upwards_with_relative_directories_and_optional_ceiling() -> git_testtools::Result { + let repo = git_testtools::scripted_fixture_repo_read_only("make_basic_repo.sh")?; + + std::env::set_current_dir(repo.join("subdir"))?; + let cwd = std::env::current_dir()?; + + for (search_dir, ceiling_dir_component) in [ + (".", ".."), + (".", "./.."), + ("./.", "./.."), + (".", "./does-not-exist/../.."), + ] { + let ceiling_dir = cwd.join(ceiling_dir_component); + let (repo_path, _trust) = git_discover::upwards_opts( + search_dir, + Options { + ceiling_dirs: vec![ceiling_dir], + ..Default::default() + }, + ) + .expect("ceiling dir should allow us to discover the repo"); + assert_repo_is_current_workdir(repo_path, Path::new("..")); + + let (repo_path, _trust) = + git_discover::upwards_opts(search_dir, Default::default()).expect("without ceiling dir we see the same"); + assert_repo_is_current_workdir(repo_path, Path::new("..")); + + let (repo_path, _trust) = git_discover::upwards_opts( + search_dir, + Options { + ceiling_dirs: vec![PathBuf::from("..")], + ..Default::default() + }, + ) + .expect("purely relative ceiling dirs work as well"); + assert_repo_is_current_workdir(repo_path, Path::new("..")); + + let err = git_discover::upwards_opts( + search_dir, + Options { + ceiling_dirs: vec![PathBuf::from(".")], + ..Default::default() + }, + ) + .unwrap_err(); + + assert!( + matches!( + err, + git_discover::upwards::Error::NoGitRepositoryWithinCeiling { ceiling_height: 1, .. } + ), + "limiting the ceiling to the CWD cannot work as it's just an empty dir" + ); + } + + Ok(()) +} + +fn assert_repo_is_current_workdir(path: git_discover::repository::Path, work_dir: &Path) { + assert_eq!( + path.into_repository_and_work_tree_directories().1.expect("work dir"), + work_dir, + ); +} diff --git a/git-path/src/convert.rs b/git-path/src/convert.rs index 037192a9f5..81701438cf 100644 --- a/git-path/src/convert.rs +++ b/git-path/src/convert.rs @@ -220,6 +220,11 @@ pub fn to_windows_separators<'a>(path: impl Into>) -> Cow<'a, BStr /// Resolve relative components virtually without accessing the file system, e.g. turn `a/./b/c/.././..` into `a`, /// without keeping intermediate `..` and `/a/../b/..` becomes `/`. +/// +/// This is particularly useful when manipulating paths that are based on user input, and not resolving intermediate +/// symlinks keeps the path similar to what the user provided. If that's not desirable, use `[realpath()][crate::realpath()` +/// instead. +/// /// Note that we might access the `current_dir` if we run out of path components to pop off, which is expected to be absolute /// as typical return value of `std::env::current_dir()`. /// As a `current_dir` like `/c` can be exhausted by paths like `../../r`, `None` will be returned to indicate the inability diff --git a/git-path/tests/convert/absolutize.rs b/git-path/tests/convert/absolutize.rs index bf56951a8d..3e5a76ae4c 100644 --- a/git-path/tests/convert/absolutize.rs +++ b/git-path/tests/convert/absolutize.rs @@ -74,6 +74,7 @@ fn trailing_relative_components_are_resolved() { let cwd = std::env::current_dir().unwrap(); for (input, expected) in [ ("./a/b/./c/../d/..", "./a/b"), + ("a/./b/c/.././..", "a"), ("/a/b/c/.././../.", "/a"), ("./a/..", "."), ("a/..", "."),