From f725e8a40ca9d9edddacdc9e1c9280beec0398ab Mon Sep 17 00:00:00 2001 From: Thomas Castiglione Date: Sun, 4 Feb 2024 16:03:03 +0800 Subject: [PATCH] working_copy: implement symlinks on Windows with a helper function tests: enable symlink tests on windows, ignoring failures due to disabled developer mode docs: add note on Windows symlink support --- CHANGELOG.md | 6 ++- Cargo.lock | 11 ++++++ Cargo.toml | 3 +- docs/windows.md | 10 +++++ lib/Cargo.toml | 3 ++ lib/src/file_util.rs | 48 ++++++++++++++++++++++++ lib/src/local_working_copy.rs | 55 ++++++++++++---------------- lib/tests/test_local_working_copy.rs | 31 +++++++++------- 8 files changed, 119 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 988c7f55b9a..839dbc981ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -73,9 +73,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed bugs -* On Windows, symlinks in the repo are now materialized as regular files in the +* On Windows, symlinks in the repo are now are now supported when Developer Mode is enabled. + When symlink support is unavailable, they will be materialized as regular files in the working copy (instead of resulting in a crash). - + [#2](https://github.com/martinvonz/jj/issues/2) + * On Windows, the pager will now be the built-in instead of disabled. * Auto-rebase now preserves the shape of history even for merge commits where diff --git a/Cargo.lock b/Cargo.lock index 558ba932d6f..a2cc80bc857 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1734,6 +1734,7 @@ dependencies = [ "version_check", "watchman_client", "whoami", + "winreg", "zstd", ] @@ -3519,6 +3520,16 @@ dependencies = [ "memchr", ] +[[package]] +name = "winreg" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a277a57398d4bfa075df44f501a17cfdf8542d224f0d36095a2adc7aee4ef0a5" +dependencies = [ + "cfg-if", + "windows-sys 0.48.0", +] + [[package]] name = "yaml-rust" version = "0.4.5" diff --git a/Cargo.toml b/Cargo.toml index 66e244496a1..44f94ffbbb1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -60,7 +60,7 @@ insta = { version = "1.35.1", features = ["filters"] } itertools = "0.12.1" libc = { version = "0.2.153" } maplit = "1.0.2" -minus = { version = "5.5.0", features = [ "dynamic_output", "search" ] } +minus = { version = "5.5.0", features = ["dynamic_output", "search"] } num_cpus = "1.16.0" once_cell = "1.19.0" ouroboros = "0.18.0" @@ -109,6 +109,7 @@ unicode-width = "0.1.11" version_check = "0.9.4" watchman_client = { version = "0.8.0" } whoami = "1.4.1" +winreg = "0.52" zstd = "0.12.4" # put all inter-workspace libraries, i.e. those that use 'path = ...' here in diff --git a/docs/windows.md b/docs/windows.md index 5cc8759d703..7d5e6fc826c 100644 --- a/docs/windows.md +++ b/docs/windows.md @@ -62,3 +62,13 @@ Then only use the `~/my-repo` workspace from Linux. [issue-2040]: https://github.com/martinvonz/jj/issues/2040 [array-op]: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_arrays?view=powershell-7.4#the-array-sub-expression-operator + +## Symbolic link support + +`jj` supports symlinks on Windows only when they are enabled by the operating +system. This requires Windows 10 version 14972 or higher, as well as Developer +Mode. If those conditions are not satisfied, `jj` will materialize symlinks as +ordinary files. + +For colocated repositories, Git support must also be enabled using the +`git config` option `core.symlinks=true`. \ No newline at end of file diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 8b10d12e59e..2df34e7a3c1 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -65,6 +65,9 @@ zstd = { workspace = true } [target.'cfg(unix)'.dependencies] rustix = { workspace = true } +[target.'cfg(windows)'.dependencies] +winreg = { workspace = true } + [dev-dependencies] assert_matches = { workspace = true } criterion = { workspace = true } diff --git a/lib/src/file_util.rs b/lib/src/file_util.rs index d92b9f4e75d..985f673afa9 100644 --- a/lib/src/file_util.rs +++ b/lib/src/file_util.rs @@ -21,6 +21,8 @@ use std::{io, iter}; use tempfile::{NamedTempFile, PersistError}; use thiserror::Error; +pub use self::platform::*; + #[derive(Debug, Error)] #[error("Cannot access {path}")] pub struct PathError { @@ -146,6 +148,52 @@ pub fn persist_content_addressed_temp_file>( } } +#[cfg(unix)] +mod platform { + use std::io; + use std::os::unix::fs::symlink; + use std::path::Path; + + /// Symlinks are always available on UNIX + pub fn check_symlink_support() -> io::Result { + Ok(true) + } + + #[cfg(unix)] + pub fn try_symlink, Q: AsRef>(original: P, link: Q) -> io::Result<()> { + symlink(original, link) + } +} + +#[cfg(windows)] +mod platform { + use std::io; + use std::os::windows::fs::symlink_file; + use std::path::Path; + use winreg::{enums::HKEY_LOCAL_MACHINE, RegKey}; + + /// Symlinks may or may not be enabled on Windows. They require the Developer Mode + /// setting, which is stored in the registry key below. + pub fn check_symlink_support() -> io::Result { + let hklm = RegKey::predef(HKEY_LOCAL_MACHINE); + let sideloading = + hklm.open_subkey("SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\AppModelUnlock")?; + let developer_mode: u32 = sideloading.get_value("AllowDevelopmentWithoutDevLicense")?; + Ok(developer_mode == 1) + } + + #[cfg(windows)] + pub fn try_symlink, Q: AsRef>(original: P, link: Q) -> io::Result<()> { + // this will create a nonfunctional link for directories, but at the moment + // we don't have enough information in the tree to determine whether the + // symlink target is a file or a directory + // note: if developer mode is not enabled the error code will be 1314, + // ERROR_PRIVILEGE_NOT_HELD + + symlink_file(original, link) + } +} + #[cfg(test)] mod tests { use std::io::Write; diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 1ae8415b11b..e5ab6c87e95 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -21,8 +21,6 @@ use std::fs::{File, Metadata, OpenOptions}; use std::io::{Read, Write}; use std::ops::Range; #[cfg(unix)] -use std::os::unix::fs::symlink; -#[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::sync::mpsc::{channel, Sender}; @@ -46,6 +44,7 @@ use crate::backend::{ }; use crate::commit::Commit; use crate::conflicts::{self, materialize_tree_value, MaterializedTreeValue}; +use crate::file_util::{check_symlink_support, try_symlink}; #[cfg(feature = "watchman")] use crate::fsmonitor::watchman; use crate::fsmonitor::FsmonitorKind; @@ -118,7 +117,6 @@ impl FileState { } } - #[cfg(unix)] fn for_symlink(metadata: &Metadata) -> Self { // When using fscrypt, the reported size is not the content size. So if // we were to record the content size here (like we do for regular files), we @@ -294,6 +292,7 @@ pub struct TreeState { // Currently only path prefixes sparse_patterns: Vec, own_mtime: MillisSinceEpoch, + symlink_support: bool, /// The most recent clock value returned by Watchman. Will only be set if /// the repo is configured to use the Watchman filesystem monitor and @@ -549,6 +548,7 @@ impl TreeState { file_states: FileStatesMap::new(), sparse_patterns: vec![RepoPathBuf::root()], own_mtime: MillisSinceEpoch(0), + symlink_support: check_symlink_support().unwrap_or(false), watchman_clock: None, } } @@ -682,8 +682,7 @@ impl TreeState { path: &RepoPath, disk_path: &Path, ) -> Result { - #[cfg(unix)] - { + if self.symlink_support { let target = disk_path.read_link().map_err(|err| SnapshotError::Other { message: format!("Failed to read symlink {}", disk_path.display()), err: err.into(), @@ -695,9 +694,7 @@ impl TreeState { path: disk_path.to_path_buf(), })?; Ok(self.store.write_symlink(path, str_target)?) - } - #[cfg(windows)] - { + } else { let target = fs::read(disk_path).map_err(|err| SnapshotError::Other { message: format!("Failed to read file {}", disk_path.display()), err: err.into(), @@ -1082,7 +1079,7 @@ impl TreeState { Ok(None) } else { let current_tree_values = current_tree.path_value(repo_path); - let new_file_type = if cfg!(windows) { + let new_file_type = if !self.symlink_support { let mut new_file_type = new_file_state.file_type.clone(); if matches!(new_file_type, FileType::Normal { .. }) && matches!(current_tree_values.as_normal(), Some(TreeValue::Symlink(_))) @@ -1204,28 +1201,20 @@ impl TreeState { Ok(FileState::for_file(executable, size, &metadata)) } - #[cfg_attr(windows, allow(unused_variables))] fn write_symlink(&self, disk_path: &Path, target: String) -> Result { - #[cfg(windows)] - { - self.write_file(disk_path, &mut target.as_bytes(), false) - } - #[cfg(unix)] - { - let target = PathBuf::from(&target); - symlink(&target, disk_path).map_err(|err| CheckoutError::Other { - message: format!( - "Failed to create symlink from {} to {}", - disk_path.display(), - target.display() - ), - err: err.into(), - })?; - let metadata = disk_path - .symlink_metadata() - .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_symlink(&metadata)) - } + let target = PathBuf::from(&target); + try_symlink(&target, disk_path).map_err(|err| CheckoutError::Other { + message: format!( + "Failed to create symlink from {} to {}", + disk_path.display(), + target.display() + ), + err: err.into(), + })?; + let metadata = disk_path + .symlink_metadata() + .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; + Ok(FileState::for_symlink(&metadata)) } fn write_conflict( @@ -1388,7 +1377,11 @@ impl TreeState { .. } => self.write_file(&disk_path, &mut reader, executable)?, MaterializedTreeValue::Symlink { id: _, target } => { - self.write_symlink(&disk_path, target)? + if self.symlink_support { + self.write_symlink(&disk_path, target)? + } else { + self.write_file(&disk_path, &mut target.as_bytes(), false)? + } } MaterializedTreeValue::GitSubmodule(_) => { println!("ignoring git submodule at {path:?}"); diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index ecfe2bac3d8..1b058c920fb 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -21,6 +21,7 @@ use std::sync::Arc; use itertools::Itertools; use jj_lib::backend::{MergedTreeId, TreeId, TreeValue}; +use jj_lib::file_util::{check_symlink_support, try_symlink}; use jj_lib::fsmonitor::FsmonitorKind; use jj_lib::local_working_copy::LocalWorkingCopy; use jj_lib::merge::Merge; @@ -80,7 +81,6 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { // Executable, but same content as Normal, to test transition where only the bit changed ExecutableNormalContent, Conflict, - #[cfg_attr(windows, allow(dead_code))] Symlink, Tree, GitSubmodule, @@ -170,7 +170,6 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { Kind::Conflict, Kind::Tree, ]; - #[cfg(unix)] kinds.push(Kind::Symlink); if backend == TestRepoBackend::Git { kinds.push(Kind::GitSubmodule); @@ -244,10 +243,12 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { Kind::Symlink => { assert!(maybe_metadata.is_ok(), "{path:?} should exist"); let metadata = maybe_metadata.unwrap(); - assert!( - metadata.file_type().is_symlink(), - "{path:?} should be a symlink" - ); + if check_symlink_support().unwrap_or(false) { + assert!( + metadata.file_type().is_symlink(), + "{path:?} should be a symlink" + ); + } } Kind::Tree => { assert!(maybe_metadata.is_ok(), "{path:?} should exist"); @@ -803,13 +804,11 @@ fn test_gitignores_ignored_directory_already_tracked() { ) .unwrap(); std::fs::remove_file(deleted_executable_path.to_fs_path(&workspace_root)).unwrap(); - if cfg!(unix) { - let fs_path = modified_symlink_path.to_fs_path(&workspace_root); - std::fs::remove_file(&fs_path).unwrap(); - #[cfg(unix)] - std::os::unix::fs::symlink("modified", &fs_path).unwrap(); + let fs_path = modified_symlink_path.to_fs_path(&workspace_root); + std::fs::remove_file(&fs_path).unwrap(); + if check_symlink_support().unwrap_or(false) { + try_symlink("modified", &fs_path).unwrap(); } else { - let fs_path = modified_symlink_path.to_fs_path(&workspace_root); std::fs::write(fs_path, "modified").unwrap(); } std::fs::remove_file(deleted_symlink_path.to_fs_path(&workspace_root)).unwrap(); @@ -923,7 +922,6 @@ fn test_gitsubmodule() { ); } -#[cfg(unix)] #[test] fn test_existing_directory_symlink() { let settings = testutils::user_settings(); @@ -933,7 +931,12 @@ fn test_existing_directory_symlink() { // Creates a symlink in working directory, and a tree that will add a file under // the symlinked directory. - std::os::unix::fs::symlink("..", workspace_root.join("parent")).unwrap(); + if check_symlink_support().unwrap_or(false) { + try_symlink("..", workspace_root.join("parent")).unwrap(); + } else { + return; + } + let file_path = RepoPath::from_internal_string("parent/escaped"); let tree = create_tree(repo, &[(file_path, "contents")]); let commit = commit_with_tree(repo.store(), tree.id());