From 59d3a2c866784543765501502f458558ce2f39a4 Mon Sep 17 00:00:00 2001 From: Thomas Castiglione Date: Fri, 26 Apr 2024 12:42:20 +0800 Subject: [PATCH] local_working_copy: when all sides of a conflict are executable, materialise the conflicted file as executable Fixes #3579 and adds a testcase for an executable conflict treevalue. --- CHANGELOG.md | 3 +++ lib/src/local_working_copy.rs | 22 ++++++++++----- lib/src/merge.rs | 10 +++++++ lib/tests/test_local_working_copy.rs | 40 +++++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4e82d7a2..e0d2cbca4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,9 @@ to avoid letting the user edit the immutable one. * `jj config list` now properly escapes TOML keys (#1322). +* Files with conflicts are now checked out as executable if all sides of the + conflict are executable. + ## [0.17.1] - 2024-05-07 ### Fixed bugs diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index b0e4caecc..aac680bcf 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1152,10 +1152,15 @@ impl TreeState { .block_on()?; match new_file_ids.into_resolved() { Ok(file_id) => { + // On Windows, we preserve the executable bit from the merged trees. #[cfg(windows)] let executable = { let () = executable; // use the variable - false + if let Some(merge) = current_tree_values.to_executable_merge() { + merge.resolve_trivial().copied().unwrap_or_default() + } else { + false + } }; Ok(Merge::normal(TreeValue::File { id: file_id.unwrap(), @@ -1224,6 +1229,7 @@ impl TreeState { &self, disk_path: &Path, conflict_data: Vec, + executable: bool, ) -> Result { let mut file = OpenOptions::new() .write(true) @@ -1239,12 +1245,11 @@ impl TreeState { err: err.into(), })?; let size = conflict_data.len() as u64; - // TODO: Set the executable bit correctly (when possible) and preserve that on - // Windows like we do with the executable bit for regular files. + self.set_executable(disk_path, executable)?; let metadata = file .metadata() .map_err(|err| checkout_error_for_stat_error(err, disk_path))?; - Ok(FileState::for_file(false, size, &metadata)) + Ok(FileState::for_file(executable, size, &metadata)) } #[cfg_attr(windows, allow(unused_variables))] @@ -1393,8 +1398,13 @@ impl TreeState { MaterializedTreeValue::Tree(_) => { panic!("unexpected tree entry in diff at {path:?}"); } - MaterializedTreeValue::Conflict { id: _, contents } => { - self.write_conflict(&disk_path, contents)? + MaterializedTreeValue::Conflict { id, contents } => { + let executable = if let Some(merge) = id.to_executable_merge() { + merge.resolve_trivial().copied().unwrap_or_default() + } else { + false + }; + self.write_conflict(&disk_path, contents, executable)? } }; changed_file_states.push((path, file_state)); diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 7ee68535c..5ac6a765b 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -512,6 +512,16 @@ impl MergedTreeValue { }) } + /// If this merge contains only files or absent entries, returns a merge of + /// the files' executable bits. + pub fn to_executable_merge(&self) -> Option> { + self.maybe_map(|term| match term { + None => Some(false), + Some(TreeValue::File { id: _, executable }) => Some(*executable), + _ => None, + }) + } + /// Creates a new merge with the file ids from the given merge. In other /// words, only the executable bits from `self` will be preserved. pub fn with_new_file_ids(&self, file_ids: &Merge>) -> Self { diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index e676ecf48..ea2f1be90 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -87,6 +87,8 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { // Executable, but same content as Normal, to test transition where only the bit changed ExecutableNormalContent, Conflict, + // Same content as Executable, to test that transition preserves the executable bit + ConflictedExecutableContent, Symlink, Tree, GitSubmodule, @@ -110,7 +112,8 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { }) } Kind::Executable => { - let id = testutils::write_file(store, path, "executable file contents"); + let id: jj_lib::backend::FileId = + testutils::write_file(store, path, "executable file contents"); Merge::normal(TreeValue::File { id, executable: true, @@ -144,6 +147,29 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { ], ) } + Kind::ConflictedExecutableContent => { + let base_file_id = testutils::write_file(store, path, "executable file contents"); + let left_file_id = + testutils::write_file(store, path, "left executable file contents"); + let right_file_id = + testutils::write_file(store, path, "right executable file contents"); + Merge::from_removes_adds( + vec![Some(TreeValue::File { + id: base_file_id, + executable: true, + })], + vec![ + Some(TreeValue::File { + id: left_file_id, + executable: true, + }), + Some(TreeValue::File { + id: right_file_id, + executable: true, + }), + ], + ) + } Kind::Symlink => { let id = store.write_symlink(path, "target").unwrap(); Merge::normal(TreeValue::Symlink(id)) @@ -174,6 +200,7 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { Kind::Executable, Kind::ExecutableNormalContent, Kind::Conflict, + Kind::ConflictedExecutableContent, Kind::Tree, ]; kinds.push(Kind::Symlink); @@ -246,6 +273,17 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { "{path:?} should not be executable" ); } + Kind::ConflictedExecutableContent => { + assert!(maybe_metadata.is_ok(), "{path:?} should exist"); + let metadata = maybe_metadata.unwrap(); + assert!(metadata.is_file(), "{path:?} should be a file"); + #[cfg(unix)] + assert_ne!( + metadata.permissions().mode() & 0o111, + 0, + "{path:?} should be executable" + ); + } Kind::Symlink => { assert!(maybe_metadata.is_ok(), "{path:?} should exist"); let metadata = maybe_metadata.unwrap();