windows: avoid UNC paths in run_ui_editor

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"

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 }

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")

cli/tests/test_commit_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::common::TestEnvironment;
 
@@ -73,6 +73,24 @@ fn test_commit_with_editor() {
     "###);
 }
 
+#[test]
+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, "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(&edited_path));
+}
+
 #[test]
 fn test_commit_interactive() {
     let mut test_env = TestEnvironment::default();

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]

cli/tests/test_describe_command.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::path::PathBuf;
+
 use crate::common::{get_stderr_string, TestEnvironment};
 
 #[test]
@@ -323,3 +325,21 @@ fn test_describe_author() {
     ~
     "###);
 }
+
+#[test]
+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, "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(&edited_path));
+}

cli/tests/test_move_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::common::TestEnvironment;
 
@@ -432,6 +432,32 @@ fn test_move_description() {
     "###);
 }
 
+#[test]
+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, "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(&edited_path));
+}
+
 fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String {
     test_env.jj_cmd_success(
         repo_path,

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(&edited_path));
+}

cli/tests/test_split_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::common::TestEnvironment;
 
@@ -538,3 +538,24 @@ fn test_split_empty() {
     Hint: Use `jj new` if you want to create another empty commit.
     "###);
 }
+
+#[test]
+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, "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(&edited_path));
+}

cli/tests/test_squash_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::common::TestEnvironment;
 
@@ -1030,6 +1030,32 @@ fn test_squash_description() {
     "###);
 }
 
+#[test]
+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, "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(&edited_path));
+}
+
 #[test]
 fn test_squash_empty() {
     let mut test_env = TestEnvironment::default();

cli/tests/test_unsquash_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::common::TestEnvironment;
 
@@ -313,6 +313,32 @@ fn test_unsquash_description() {
     "###);
 }
 
+#[test]
+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, "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(&edited_path));
+}
+
 fn get_description(test_env: &TestEnvironment, repo_path: &Path, rev: &str) -> String {
     test_env.jj_cmd_success(
         repo_path,