From 6c1aeff7a9b535dd2425ecd7949ba4cc728e636f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 8 Feb 2024 10:16:42 -0800 Subject: [PATCH] working copy: materialize symlinks on Windows as regular files I was a bit surprised to learn (or be reminded?) that checking out symlinks on Windows leads to a panic. This patch fixes the crash by materializing symlinks from the repo as regular files. It also updates the snapshotting code so we preserve the symlink-ness of a path. The user can update the symlink in the repo by updating the regular file in the working copy. This seems to match Git's behavior on Windows when symlinks are disabled. --- CHANGELOG.md | 3 ++ lib/src/local_working_copy.rs | 58 ++++++++++++++++++++-------- lib/src/working_copy.rs | 2 - lib/tests/test_local_working_copy.rs | 6 +-- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e9f5bd4d1..5eee4f705c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ 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 + working copy (instead of resulting in a crash). + ## [0.14.0] - 2024-02-07 ### Deprecations diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 4cfbad7818..35949ad221 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -118,6 +118,7 @@ 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 @@ -681,18 +682,32 @@ impl TreeState { path: &RepoPath, disk_path: &Path, ) -> Result { - let target = disk_path.read_link().map_err(|err| SnapshotError::Other { - message: format!("Failed to read symlink {}", disk_path.display()), - err: err.into(), - })?; - let str_target = - target - .to_str() - .ok_or_else(|| SnapshotError::InvalidUtf8SymlinkTarget { + #[cfg(unix)] + { + let target = disk_path.read_link().map_err(|err| SnapshotError::Other { + message: format!("Failed to read symlink {}", disk_path.display()), + err: err.into(), + })?; + let str_target = + target + .to_str() + .ok_or_else(|| SnapshotError::InvalidUtf8SymlinkTarget { + path: disk_path.to_path_buf(), + })?; + Ok(self.store.write_symlink(path, str_target)?) + } + #[cfg(windows)] + { + let target = fs::read(disk_path).map_err(|err| SnapshotError::Other { + message: format!("Failed to read file {}", disk_path.display()), + err: err.into(), + })?; + let string_target = + String::from_utf8(target).map_err(|_| SnapshotError::InvalidUtf8SymlinkTarget { path: disk_path.to_path_buf(), - target: target.clone(), })?; - Ok(self.store.write_symlink(path, str_target)?) + Ok(self.store.write_symlink(path, &string_target)?) + } } fn reset_watchman(&mut self) { @@ -1066,8 +1081,18 @@ impl TreeState { if clean { Ok(None) } else { - let new_file_type = new_file_state.file_type.clone(); let current_tree_values = current_tree.path_value(repo_path); + let new_file_type = if cfg!(windows) { + 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(_))) + { + new_file_type = FileType::Symlink; + } + new_file_type + } else { + new_file_state.file_type.clone() + }; let new_tree_values = match new_file_type { FileType::Normal { executable } => self.write_path_to_store( repo_path, @@ -1081,7 +1106,6 @@ impl TreeState { } FileType::GitSubmodule => panic!("git submodule cannot be written to store"), }; - if new_tree_values != current_tree_values { Ok(Some(new_tree_values)) } else { @@ -1184,7 +1208,7 @@ impl TreeState { fn write_symlink(&self, disk_path: &Path, target: String) -> Result { #[cfg(windows)] { - println!("ignoring symlink at {}", disk_path.display()); + self.write_file(disk_path, &mut target.as_bytes(), false) } #[cfg(unix)] { @@ -1197,11 +1221,11 @@ impl TreeState { ), 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 metadata = disk_path - .symlink_metadata() - .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_symlink(&metadata)) } fn write_conflict( diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 2eb12384ed..bb56b5016e 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -147,8 +147,6 @@ pub enum SnapshotError { /// The path of the symlink that has a target that's not valid UTF-8. /// This path itself is valid UTF-8. path: PathBuf, - /// The symlink target with invalid UTF-8. - target: PathBuf, }, /// Reading or writing from the commit backend failed. #[error("Internal backend error")] diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index ce74d56fc8..ea3746eed3 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -771,11 +771,7 @@ fn test_gitignores_ignored_directory_already_tracked() { testutils::write_executable_file(&mut tree_builder, path, contents); } Kind::Symlink => { - if cfg!(unix) { - testutils::write_symlink(&mut tree_builder, path, contents); - } else { - testutils::write_normal_file(&mut tree_builder, path, contents); - } + testutils::write_symlink(&mut tree_builder, path, contents); } } }