From d91e355674136c26de25f2fb045deb1747b96956 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 21 Dec 2024 12:01:28 +0900 Subject: [PATCH] cli: git clone: convert local Git remote path to slash-separated path Since source.contains(':') can't be used for non-local path detection on Windows, we now use gix::url for parsing. It might be stricter, but I assume it would be more reliable. Closes #4188 --- CHANGELOG.md | 3 ++ cli/src/commands/git/clone.rs | 81 +++++++++++++++++++++++++++++------ cli/tests/common/mod.rs | 24 ++++++----- cli/tests/test_git_clone.rs | 18 ++++++++ cli/tests/test_git_push.rs | 2 - 5 files changed, 103 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd2b5acda0..c545a1c85c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * On Windows, workspace paths (printed by `jj root`) no longer use UNC-style `\\?\` paths unless necessary. +* On Windows, `jj git clone` now converts local Git remote path to + slash-separated path. + * `jj resolve` no longer removes the executable bit on resolved files when using an external merge tool. diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index b3c2807741..743fd5e844 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -15,6 +15,7 @@ use std::fs; use std::io; use std::io::Write; +use std::mem; use std::num::NonZeroU32; use std::path::Path; @@ -64,21 +65,20 @@ pub struct GitCloneArgs { depth: Option, } -fn absolute_git_source(cwd: &Path, source: &str) -> String { +fn absolute_git_source(cwd: &Path, source: &str) -> Result { // Git appears to turn URL-like source to absolute path if local git directory // exits, and fails because '$PWD/https' is unsupported protocol. Since it would - // be tedious to copy the exact git (or libgit2) behavior, we simply assume a - // source containing ':' is a URL, SSH remote, or absolute path with Windows - // drive letter. - if !source.contains(':') && Path::new(source).exists() { - // It's less likely that cwd isn't utf-8, so just fall back to original source. - cwd.join(source) - .into_os_string() - .into_string() - .unwrap_or_else(|_| source.to_owned()) - } else { - source.to_owned() + // be tedious to copy the exact git (or libgit2) behavior, we simply let gix + // parse the input as URL, rcp-like, or local path. + let mut url = gix::url::parse(source.as_ref()).map_err(cli_error)?; + url.canonicalize(cwd).map_err(user_error)?; + // As of gix 0.68.0, the canonicalized path uses platform-native directory + // separator, which isn't compatible with libgit2 on Windows. + if url.scheme == gix::url::Scheme::File { + url.path = gix::path::to_unix_separators_on_windows(mem::take(&mut url.path)).into_owned(); } + // It's less likely that cwd isn't utf-8, so just fall back to original source. + Ok(String::from_utf8(url.to_bstring().into()).unwrap_or_else(|_| source.to_owned())) } fn clone_destination_for_source(source: &str) -> Option<&str> { @@ -106,7 +106,7 @@ pub fn cmd_git_clone( if command.global_args().at_operation.is_some() { return Err(cli_error("--at-op is not respected")); } - let source = absolute_git_source(command.cwd(), &args.source); + let source = absolute_git_source(command.cwd(), &args.source)?; let wc_path_str = args .destination .as_deref() @@ -239,3 +239,58 @@ fn do_git_clone( fetch_tx.finish(ui, "fetch from git remote into empty repo")?; Ok((workspace_command, stats)) } + +#[cfg(test)] +mod tests { + use std::path::MAIN_SEPARATOR; + + use super::*; + + #[test] + fn test_absolute_git_source() { + // gix::Url::canonicalize() works even if the path doesn't exist. + // However, we need to ensure that no symlinks exist at the test paths. + let temp_dir = testutils::new_temp_dir(); + let cwd = dunce::canonicalize(temp_dir.path()).unwrap(); + let cwd_slash = cwd.to_str().unwrap().replace(MAIN_SEPARATOR, "/"); + + // Local path + assert_eq!( + absolute_git_source(&cwd, "foo").unwrap(), + format!("{cwd_slash}/foo") + ); + assert_eq!( + absolute_git_source(&cwd, r"foo\bar").unwrap(), + if cfg!(windows) { + format!("{cwd_slash}/foo/bar") + } else { + format!(r"{cwd_slash}/foo\bar") + } + ); + assert_eq!( + absolute_git_source(&cwd.join("bar"), &format!("{cwd_slash}/foo")).unwrap(), + format!("{cwd_slash}/foo") + ); + + // rcp-like + assert_eq!( + absolute_git_source(&cwd, "git@example.org:foo/bar.git").unwrap(), + "git@example.org:foo/bar.git" + ); + // URL + assert_eq!( + absolute_git_source(&cwd, "https://example.org/foo.git").unwrap(), + "https://example.org/foo.git" + ); + // Custom scheme isn't an error + assert_eq!( + absolute_git_source(&cwd, "custom://example.org/foo.git").unwrap(), + "custom://example.org/foo.git" + ); + // Password shouldn't be redacted (gix::Url::to_string() would do) + assert_eq!( + absolute_git_source(&cwd, "https://user:pass@example.org/").unwrap(), + "https://user:pass@example.org/" + ); + } +} diff --git a/cli/tests/common/mod.rs b/cli/tests/common/mod.rs index f64c4db2d4..e1a883934c 100644 --- a/cli/tests/common/mod.rs +++ b/cli/tests/common/mod.rs @@ -321,16 +321,20 @@ impl TestEnvironment { pub fn normalize_output(&self, text: &str) -> String { let text = text.replace("jj.exe", "jj"); - let regex = Regex::new(&format!( - r"{}(\S+)", - regex::escape(&self.env_root.display().to_string()) - )) - .unwrap(); - regex - .replace_all(&text, |caps: &Captures| { - format!("$TEST_ENV{}", caps[1].replace('\\', "/")) - }) - .to_string() + let env_root = self.env_root.display().to_string(); + // Platform-native $TEST_ENV + let regex = Regex::new(&format!(r"{}(\S+)", regex::escape(&env_root))).unwrap(); + let text = regex.replace_all(&text, |caps: &Captures| { + format!("$TEST_ENV{}", caps[1].replace('\\', "/")) + }); + // Slash-separated $TEST_ENV + let text = if cfg!(windows) { + let regex = Regex::new(®ex::escape(&env_root.replace('\\', "/"))).unwrap(); + regex.replace_all(&text, regex::NoExpand("$TEST_ENV")) + } else { + text + }; + text.into_owned() } /// Used before mutating operations to create more predictable commit ids diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index e8bc09106a..d0ebdedca6 100644 --- a/cli/tests/test_git_clone.rs +++ b/cli/tests/test_git_clone.rs @@ -161,6 +161,24 @@ fn test_git_clone() { "###); } +#[test] +fn test_git_clone_bad_source() { + let test_env = TestEnvironment::default(); + + let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["git", "clone", "", "dest"]); + insta::assert_snapshot!(stderr, @r#"Error: local path "" does not specify a path to a repository"#); + + // Invalid port number + let stderr = test_env.jj_cmd_cli_error( + test_env.env_root(), + &["git", "clone", "https://example.net:bad-port/bar", "dest"], + ); + insta::assert_snapshot!(stderr, @r#" + Error: URL "https://example.net:bad-port/bar" can not be parsed as valid URL + Caused by: invalid port number + "#); +} + #[test] fn test_git_clone_colocate() { let test_env = TestEnvironment::default(); diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index eec18a1bef..3a868329d9 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -1386,8 +1386,6 @@ fn test_git_push_moved_sideways_untracked() { } #[test] -// TODO: This test fails with libgit2 v1.8.1 on Windows. -#[cfg(not(target_os = "windows"))] fn test_git_push_to_remote_named_git() { let (test_env, workspace_root) = set_up(); let git_repo = {