diff --git a/Cargo.lock b/Cargo.lock index a15525ddde..9458e1fd8d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1680,6 +1680,7 @@ dependencies = [ "criterion", "crossterm", "dirs", + "dunce", "esl01-renderdag", "futures 0.3.30", "git2", diff --git a/Cargo.toml b/Cargo.toml index 4041e65b7b..eb73a1eee0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ criterion = "0.5.1" crossterm = { version = "0.27", default-features = false } digest = "0.10.7" dirs = "5.0.1" +dunce = "1.0.4" either = "1.13.0" esl01-renderdag = "0.3.0" futures = "0.3.30" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index fd1085b8b9..4204887da5 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -61,6 +61,7 @@ config = { workspace = true } criterion = { workspace = true, optional = true } crossterm = { workspace = true } dirs = { workspace = true } +dunce = { workspace = true } esl01-renderdag = { workspace = true } futures = { workspace = true } git2 = { workspace = true } diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index d33142d5da..af025144bd 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2138,6 +2138,9 @@ pub fn get_new_config_file_path( } pub fn run_ui_editor(settings: &UserSettings, edit_path: &PathBuf) -> Result<(), CommandError> { + // Work around UNC paths not being well supported on Windows (no-op for non-Windows): + // https://github.com/martinvonz/jj/issues/3986 + let edit_path = dunce::simplified(edit_path); let editor: CommandNameAndArgs = settings .config() .get("ui.editor") diff --git a/cli/tests/test_commit_command.rs b/cli/tests/test_commit_command.rs index 5d706de574..d29b598169 100644 --- a/cli/tests/test_commit_command.rs +++ b/cli/tests/test_commit_command.rs @@ -73,6 +73,28 @@ fn test_commit_with_editor() { "###); } +#[test] +#[cfg(windows)] +fn test_commit_with_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + let edit_script = test_env.set_up_fake_editor(); + + std::fs::write(edit_script, ["dumppath path"].join("\0")).unwrap(); + test_env.jj_cmd_ok(&workspace_path, &["commit"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} + #[test] fn test_commit_interactive() { let mut test_env = TestEnvironment::default(); diff --git a/cli/tests/test_config_command.rs b/cli/tests/test_config_command.rs index 75b5bb200a..ec2d64eb59 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -626,7 +626,7 @@ fn test_config_edit_user() { assert_eq!( std::fs::read_to_string(test_env.env_root().join("path")).unwrap(), - test_env.config_path().to_str().unwrap() + dunce::simplified(test_env.config_path()).to_str().unwrap() ); } @@ -642,7 +642,9 @@ fn test_config_edit_repo() { assert_eq!( std::fs::read_to_string(test_env.env_root().join("path")).unwrap(), - repo_path.join(".jj/repo/config.toml").to_str().unwrap() + dunce::simplified(&repo_path.join(".jj/repo/config.toml")) + .to_str() + .unwrap() ); } diff --git a/cli/tests/test_describe_command.rs b/cli/tests/test_describe_command.rs index fc88ad4b5b..c0a80e5956 100644 --- a/cli/tests/test_describe_command.rs +++ b/cli/tests/test_describe_command.rs @@ -323,3 +323,25 @@ fn test_describe_author() { ~ "###); } + +#[test] +#[cfg(windows)] +fn test_describe_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + let edit_script = test_env.set_up_fake_editor(); + + std::fs::write(edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&workspace_path, &["describe"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} diff --git a/cli/tests/test_move_command.rs b/cli/tests/test_move_command.rs index 41a38be861..8b69c0c3e4 100644 --- a/cli/tests/test_move_command.rs +++ b/cli/tests/test_move_command.rs @@ -432,6 +432,36 @@ fn test_move_description() { "###); } +#[test] +#[cfg(windows)] +fn test_move_description_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + let edit_script = test_env.set_up_fake_editor(); + std::fs::write(repo_path.join("file1"), "a\n").unwrap(); + std::fs::write(repo_path.join("file2"), "a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file1"), "b\n").unwrap(); + std::fs::write(repo_path.join("file2"), "b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-m", "destination"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "source"]); + + std::fs::write(&edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["move", "--to", "@-"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} + fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String { test_env.jj_cmd_success( repo_path, diff --git a/cli/tests/test_sparse_command.rs b/cli/tests/test_sparse_command.rs index b01e791342..03edf2d5f6 100644 --- a/cli/tests/test_sparse_command.rs +++ b/cli/tests/test_sparse_command.rs @@ -174,3 +174,24 @@ fn test_sparse_manage_patterns() { file3 "###); } + +#[test] +fn test_sparse_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + let edit_script = test_env.set_up_fake_editor(); + + std::fs::write(&edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["sparse", "edit"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 83b244daa8..0adb78c219 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -538,3 +538,28 @@ fn test_split_empty() { Hint: Use `jj new` if you want to create another empty commit. "###); } + +#[test] +#[cfg(windows)] +fn test_split_message_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + std::fs::write(repo_path.join("file1"), "foo").unwrap(); + std::fs::write(repo_path.join("file2"), "foo").unwrap(); + + let edit_script = test_env.set_up_fake_editor(); + std::fs::write(edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["split", "file2"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index b3f8d1ed41..d1a0445473 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -1030,6 +1030,36 @@ fn test_squash_description() { "###); } +#[test] +#[cfg(windows)] +fn test_squash_description_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + let edit_script = test_env.set_up_fake_editor(); + std::fs::write(repo_path.join("file1"), "a\n").unwrap(); + std::fs::write(repo_path.join("file2"), "a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file1"), "b\n").unwrap(); + std::fs::write(repo_path.join("file2"), "b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-m", "destination"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "source"]); + + std::fs::write(&edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["squash"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} + #[test] fn test_squash_empty() { let mut test_env = TestEnvironment::default(); diff --git a/cli/tests/test_unsquash_command.rs b/cli/tests/test_unsquash_command.rs index dfdce7a43c..fe4cb28d7f 100644 --- a/cli/tests/test_unsquash_command.rs +++ b/cli/tests/test_unsquash_command.rs @@ -313,6 +313,36 @@ fn test_unsquash_description() { "###); } +#[test] +#[cfg(windows)] +fn test_unsquash_description_editor_avoids_unc() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + + let edit_script = test_env.set_up_fake_editor(); + std::fs::write(repo_path.join("file1"), "a\n").unwrap(); + std::fs::write(repo_path.join("file2"), "a\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write(repo_path.join("file1"), "b\n").unwrap(); + std::fs::write(repo_path.join("file2"), "b\n").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-m", "destination"]); + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "source"]); + + std::fs::write(&edit_script, "dumppath path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["unsquash"]); + + let editor_path = std::fs::read_to_string(test_env.env_root().join("path")).unwrap(); + // While `assert!(!editor_path.starts_with("//?/"))` could work here in most cases, + // it fails when it is not safe to strip the prefix, such as paths over 260 chars. + assert_eq!( + editor_path, + dunce::simplified(std::path::Path::new(&editor_path)) + .to_str() + .unwrap() + ); +} + fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String { test_env.jj_cmd_success( repo_path,