From c256ad8a0aa43a1acb8a5e4c69ed2baf4f8976d3 Mon Sep 17 00:00:00 2001
From: mlcui <mlcui@google.com>
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 | 20 ++++++++++++++++++++
cli/tests/test_move_command.rs | 28 +++++++++++++++++++++++++++-
cli/tests/test_sparse_command.rs | 20 ++++++++++++++++++++
cli/tests/test_split_command.rs | 23 ++++++++++++++++++++++-
cli/tests/test_squash_command.rs | 28 +++++++++++++++++++++++++++-
cli/tests/test_unsquash_command.rs | 28 +++++++++++++++++++++++++++-
12 files changed, 174 insertions(+), 8 deletions(-)
diff --git a/Cargo.lock b/Cargo.lock
index 27d9037f50..4e051678c5 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1859,6 +1859,7 @@ dependencies = [
"criterion",
"crossterm",
"dirs",
+ "dunce",
"esl01-renderdag",
"futures 0.3.30",
"git2",
diff --git a/Cargo.toml b/Cargo.toml
index 52e9d1ab35..286d6d7e5d 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 0c0cd1dddb..2a95167606 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 d1d00aa028..ffe797cd09 100644
--- a/cli/tests/test_commit_command.rs
+++ b/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();
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..b9606933ed 100644
--- a/cli/tests/test_describe_command.rs
+++ b/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));
+}
diff --git a/cli/tests/test_move_command.rs b/cli/tests/test_move_command.rs
index 41a38be861..0b5d6dbbfa 100644
--- a/cli/tests/test_move_command.rs
+++ b/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,
diff --git a/cli/tests/test_sparse_command.rs b/cli/tests/test_sparse_command.rs
index b01e791342..1d64a0483e 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(&edited_path));
+}
diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs
index 83b244daa8..86906a05b3 100644
--- a/cli/tests/test_split_command.rs
+++ b/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));
+}
diff --git a/cli/tests/test_squash_command.rs b/cli/tests/test_squash_command.rs
index b1d3ad7900..058113bd5d 100644
--- a/cli/tests/test_squash_command.rs
+++ b/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;
@@ -1079,6 +1079,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();
diff --git a/cli/tests/test_unsquash_command.rs b/cli/tests/test_unsquash_command.rs
index dfdce7a43c..2e638965eb 100644
--- a/cli/tests/test_unsquash_command.rs
+++ b/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,