diff --git a/CHANGELOG.md b/CHANGELOG.md index f167ad4958b..1f866fc5d8a 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. + ## [0.24.0] - 2024-12-04 ### Release highlights diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index b3c28077410..71f309899c8 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,53 @@ 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" + ); + } +} diff --git a/cli/tests/test_git_clone.rs b/cli/tests/test_git_clone.rs index e8bc09106a6..d0ebdedca6e 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 eec18a1befe..3a868329d96 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 = {