local_working_copy: when all sides of a conflict are executable, mate…

…rialise the conflicted file as executable

Fixes martinvonz#3579 and adds a testcase for an executable conflict treevalue.

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

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.resolve_trivial().copied().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<u8>,
+        executable: bool,
     ) -> Result<FileState, CheckoutError> {
         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,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));

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<Merge<bool>> {
+        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<Option<FileId>>) -> Self {

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();