From cfc53ce7481457fb114df7664fe6c1e2a30e8851 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 | 4 ++- lib/src/local_working_copy.rs | 25 ++++++++++++----- lib/src/merge.rs | 10 +++++++ lib/tests/test_local_working_copy.rs | 40 +++++++++++++++++++++++++++- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e432f64bdc..b1f88874f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj tag list` now prints commit summary along with the tag name. ### Fixed bugs - +* Files with conflicts are now checked out as executable if all sides of the + conflict are executable. + ## [0.17.0] - 2024-05-01 ### Breaking changes diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 1392166dd8..996f9b0c1e 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1151,10 +1151,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.into_resolved().unwrap_or(Some(false)).unwrap_or_default() + } else { + false + } }; Ok(Merge::normal(TreeValue::File { id: file_id.unwrap(), @@ -1223,6 +1228,7 @@ impl TreeState { &self, disk_path: &Path, conflict_data: Vec, + executable: bool, ) -> Result { let mut file = OpenOptions::new() .write(true) @@ -1238,12 +1244,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))] @@ -1392,8 +1397,16 @@ 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() { + match merge.into_resolved() { + Ok(resolved) => resolved.unwrap_or_default(), + Err(_conflict) => false, + } + } 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 46330b901f..06493e9b3a 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -513,6 +513,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(None), + Some(TreeValue::File { id: _, executable }) => Some(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 e676ecf480..ea2f1be90b 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();