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

@@ -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.
 

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<NonZeroU32>,
 }
 
-fn absolute_git_source(cwd: &Path, source: &str) -> String {
+fn absolute_git_source(cwd: &Path, source: &str) -> Result<String, CommandError> {
     // 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/"
+        );
+    }
+}

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(&regex::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

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();

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 = {