From eeaa12247e37b30c68afeed62e6c65e80af0d8cb Mon Sep 17 00:00:00 2001 From: mlcui Date: Sun, 30 Jun 2024 20:02:21 +1000 Subject: [PATCH] windows: avoid UNC paths in `run_ui_editor` See #3986 for more details. This is a no-op for non-Windows. --- Cargo.lock | 1 + Cargo.toml | 1 + cli/Cargo.toml | 1 + cli/src/cli_util.rs | 5 ++++- cli/tests/test_commit_command.rs | 20 ++++++++++++++++++++ cli/tests/test_config_command.rs | 7 +++++-- cli/tests/test_describe_command.rs | 21 +++++++++++++++++++++ cli/tests/test_move_command.rs | 29 +++++++++++++++++++++++++++++ cli/tests/test_sparse_command.rs | 20 ++++++++++++++++++++ cli/tests/test_split_command.rs | 24 ++++++++++++++++++++++++ cli/tests/test_squash_command.rs | 29 +++++++++++++++++++++++++++++ cli/tests/test_unsquash_command.rs | 29 +++++++++++++++++++++++++++++ 12 files changed, 184 insertions(+), 3 deletions(-) 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..b8fdb89b8f 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2137,7 +2137,10 @@ pub fn get_new_config_file_path( Ok(edit_path) } -pub fn run_ui_editor(settings: &UserSettings, edit_path: &PathBuf) -> Result<(), CommandError> { +pub fn run_ui_editor(settings: &UserSettings, edit_path: &Path) -> 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..6a8c4981a3 100644 --- a/cli/tests/test_commit_command.rs +++ b/cli/tests/test_commit_command.rs @@ -73,6 +73,26 @@ fn test_commit_with_editor() { "###); } +#[test] +fn test_commit_with_editor_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&workspace_path, &["commit"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} + #[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 a6fc3cbfdf..ca1c07adbd 100644 --- a/cli/tests/test_config_command.rs +++ b/cli/tests/test_config_command.rs @@ -628,7 +628,7 @@ fn test_config_edit_user() { let edited_path = PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); - assert_eq!(&edited_path, test_env.config_path()); + assert_eq!(edited_path, dunce::simplified(test_env.config_path())); } #[test] @@ -643,7 +643,10 @@ fn test_config_edit_repo() { let edited_path = PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); - assert_eq!(edited_path, repo_path.join(".jj/repo/config.toml")); + assert_eq!( + edited_path, + dunce::simplified(&repo_path.join(".jj/repo/config.toml")) + ); } #[test] diff --git a/cli/tests/test_describe_command.rs b/cli/tests/test_describe_command.rs index fc88ad4b5b..9510751215 100644 --- a/cli/tests/test_describe_command.rs +++ b/cli/tests/test_describe_command.rs @@ -323,3 +323,24 @@ fn test_describe_author() { ~ "###); } + +#[test] +#[cfg(windows)] +fn test_describe_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&workspace_path, &["describe"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} diff --git a/cli/tests/test_move_command.rs b/cli/tests/test_move_command.rs index 41a38be861..e1ce5aa014 100644 --- a/cli/tests/test_move_command.rs +++ b/cli/tests/test_move_command.rs @@ -432,6 +432,35 @@ fn test_move_description() { "###); } +#[test] +#[cfg(windows)] +fn test_move_description_editor_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["move", "--to", "@-"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} + 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..1c697563d8 100644 --- a/cli/tests/test_sparse_command.rs +++ b/cli/tests/test_sparse_command.rs @@ -174,3 +174,23 @@ fn test_sparse_manage_patterns() { file3 "###); } + +#[test] +fn test_sparse_editor_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["sparse", "edit"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 83b244daa8..127fd8a9ff 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -538,3 +538,27 @@ 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() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["split", "file2"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs index b3f8d1ed41..8f3799fbfb 100644 --- a/cli/tests/test_squash_command.rs +++ b/cli/tests/test_squash_command.rs @@ -1030,6 +1030,35 @@ fn test_squash_description() { "###); } +#[test] +#[cfg(windows)] +fn test_squash_description_editor_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["squash"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} + #[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..55b910d4f8 100644 --- a/cli/tests/test_unsquash_command.rs +++ b/cli/tests/test_unsquash_command.rs @@ -313,6 +313,35 @@ fn test_unsquash_description() { "###); } +#[test] +#[cfg(windows)] +fn test_unsquash_description_editor_avoids_unc() { + use std::path::PathBuf; + + 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, "dump-path path").unwrap(); + test_env.jj_cmd_ok(&repo_path, &["unsquash"]); + + let edited_path = + PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap()); + // While `assert!(!edited_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!(edited_path, dunce::simplified(Path::new(&edited_path))); +} + fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String { test_env.jj_cmd_success( repo_path,