merge-tools builtin: update mode of files with unchanged contents

If an empty file or a binary file has its file mode (specifically the
executable bit) changed but its contents remain the same, scm-record
reports its contents as `Unchanged`. By not doing anything for unchanged
_contents_, a potentially selected file mode change was lost.

For text file, scm_record 0.8.0 currently always reports
`SelectedContents::Text`, even if the file hasn't changed at all (or
only its executable bit has). I've created arxanas/scm-record#104
upstream to track this bug. This unexpected behavior caused file
mode-only changes to be handled correctly but it unnecessarily rewrote
the file object, where writing the tree would have sufficed. Thus,
fixing the upstream issue prior to this commit would exacerbate the
issue by also affecting text files.

This commit now addresses file mode changes by updating the `TreeValue`
without rewriting the files or generating new file IDs. It is sufficient
to do so only if a file mode change has been selected, reusing the same
`file_mode_change_selected` condition introduced by the previous commit.

Author: jgreitemann

cli/src/merge_tools/builtin.rs

@@ -388,6 +388,7 @@ fn apply_diff_builtin(
         &mut tree_builder,
         changed_files,
         files,
+        |path| left_tree.path_value(path),
         |path| right_tree.path_value(path),
         |path, contents, executable| {
             let old_value = left_tree.path_value(path)?;
@@ -422,6 +423,7 @@ fn apply_changes(
     tree_builder: &mut MergedTreeBuilder,
     changed_files: Vec<RepoPathBuf>,
     files: &[scm_record::File],
+    select_left: impl Fn(&RepoPath) -> BackendResult<MergedTreeValue>,
     select_right: impl Fn(&RepoPath) -> BackendResult<MergedTreeValue>,
     write_file: impl Fn(&RepoPath, &[u8], bool) -> BackendResult<MergedTreeValue>,
 ) -> BackendResult<()> {
@@ -449,7 +451,7 @@ fn apply_changes(
             _unselected,
         ) = file.get_selected_contents();
 
-        if file_mode == scm_record::FileMode::Absent {
+        if file_mode == mode::ABSENT {
             // The file is not present in the selected changes.
             // Either a file mode change was selected to delete an existing file, so we
             // should remove it from the tree,
@@ -462,9 +464,16 @@ fn apply_changes(
             continue;
         }
 
+        let executable = file_mode == mode::EXECUTABLE;
         match contents {
             scm_record::SelectedContents::Unchanged => {
-                // Do nothing.
+                if file_mode_change_selected {
+                    // File contents haven't changed, but file mode needs to be updated on the tree.
+                    let value = override_file_executable_bit(select_left(&path)?, executable);
+                    tree_builder.set_or_remove(path, value);
+                } else {
+                    // Neither file mode, nor contents changed => Do nothing.
+                }
             }
             scm_record::SelectedContents::Binary {
                 old_description: _,
@@ -474,7 +483,6 @@ fn apply_changes(
                 tree_builder.set_or_remove(path, value);
             }
             scm_record::SelectedContents::Text { contents } => {
-                let executable = file_mode == mode::EXECUTABLE;
                 let value = write_file(&path, contents.as_bytes(), executable)?;
                 tree_builder.set_or_remove(path, value);
             }
@@ -483,6 +491,19 @@ fn apply_changes(
     Ok(())
 }
 
+fn override_file_executable_bit(
+    mut merged_tree_value: MergedTreeValue,
+    new_executable_bit: bool,
+) -> MergedTreeValue {
+    for tree_value in merged_tree_value.iter_mut().flatten() {
+        let TreeValue::File { executable, .. } = tree_value else {
+            panic!("incompatible update: expected a TreeValue::File, got {tree_value:?}");
+        };
+        *executable = new_executable_bit;
+    }
+    merged_tree_value
+}
+
 pub fn edit_diff_builtin(
     left_tree: &MergedTree,
     right_tree: &MergedTree,
@@ -648,8 +669,11 @@ pub fn edit_merge_builtin(
             .map(|file| file.repo_path.clone())
             .collect_vec(),
         &state.files,
-        // TODO: It doesn't make sense to select new value from the source tree.
-        // Perhaps, "their" tree value should be extracted from a conflict?
+        |path| tree.path_value(path),
+        // FIXME: It doesn't make sense to select a new value from the source tree.
+        // Presently, `select_right` is never actually called, since it is used to select binary
+        // sections, but `make_merge_file` does not produce `Binary` sections for conflicted files.
+        // This needs to be revisited when the UI becomes capable of representing binary conflicts.
         |path| tree.path_value(path),
         |path, contents, executable| {
             let id = store.write_file(path, &mut &contents[..]).block_on()?;
@@ -952,6 +976,207 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_edit_diff_builtin_empty_file_mode_change() {
+        let test_repo = TestRepo::init();
+        let store = test_repo.repo.store();
+
+        let empty_file_path = repo_path("empty_file");
+        let left_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder.file(empty_file_path, vec![]).executable(false);
+        });
+        let right_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder.file(empty_file_path, vec![]).executable(true);
+        });
+
+        let (changed_files, files) = make_diff(store, &left_tree, &right_tree);
+        insta::assert_debug_snapshot!(changed_files, @r#"
+        [
+            "empty_file",
+        ]
+        "#);
+        insta::assert_debug_snapshot!(files, @r#"
+        [
+            File {
+                old_path: None,
+                path: "empty_file",
+                file_mode: Unix(
+                    33188,
+                ),
+                sections: [
+                    FileMode {
+                        is_checked: false,
+                        mode: Unix(
+                            33261,
+                        ),
+                    },
+                ],
+            },
+        ]
+        "#);
+        let no_changes_tree_id = apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap();
+        assert_eq!(
+            left_tree.id(),
+            no_changes_tree.id(),
+            "no-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &left_tree.id()),
+            dump_tree(store, &no_changes_tree.id()),
+        );
+
+        let mut files = files;
+        for file in &mut files {
+            file.toggle_all();
+        }
+        let all_changes_tree_id =
+            apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap();
+        assert_eq!(
+            right_tree.id(),
+            all_changes_tree.id(),
+            "all-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &right_tree.id()),
+            dump_tree(store, &all_changes_tree.id()),
+        );
+    }
+
+    #[test]
+    fn test_edit_diff_builtin_text_file_mode_change() {
+        let test_repo = TestRepo::init();
+        let store = test_repo.repo.store();
+
+        let text_file_path = repo_path("text_file");
+        let left_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder.file(text_file_path, "text").executable(false);
+        });
+        let right_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder.file(text_file_path, "text").executable(true);
+        });
+
+        let (changed_files, files) = make_diff(store, &left_tree, &right_tree);
+        insta::assert_debug_snapshot!(changed_files, @r#"
+        [
+            "text_file",
+        ]
+        "#);
+        insta::assert_debug_snapshot!(files, @r#"
+        [
+            File {
+                old_path: None,
+                path: "text_file",
+                file_mode: Unix(
+                    33188,
+                ),
+                sections: [
+                    FileMode {
+                        is_checked: false,
+                        mode: Unix(
+                            33261,
+                        ),
+                    },
+                    Unchanged {
+                        lines: [
+                            "text",
+                        ],
+                    },
+                ],
+            },
+        ]
+        "#);
+        let no_changes_tree_id = apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap();
+        assert_eq!(
+            left_tree.id(),
+            no_changes_tree.id(),
+            "no-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &left_tree.id()),
+            dump_tree(store, &no_changes_tree.id()),
+        );
+
+        let mut files = files;
+        for file in &mut files {
+            file.toggle_all();
+        }
+        let all_changes_tree_id =
+            apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap();
+        assert_eq!(
+            right_tree.id(),
+            all_changes_tree.id(),
+            "all-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &right_tree.id()),
+            dump_tree(store, &all_changes_tree.id()),
+        );
+    }
+
+    #[test]
+    fn test_edit_diff_builtin_binary_file_mode_change() {
+        let test_repo = TestRepo::init();
+        let store = test_repo.repo.store();
+
+        let binary_file_path = repo_path("binary_file");
+        let left_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder
+                .file(binary_file_path, vec![0xff, 0x00])
+                .executable(false);
+        });
+        let right_tree = testutils::create_tree_with(&test_repo.repo, |builder| {
+            builder
+                .file(binary_file_path, vec![0xff, 0x00])
+                .executable(true);
+        });
+
+        let (changed_files, files) = make_diff(store, &left_tree, &right_tree);
+        insta::assert_debug_snapshot!(changed_files, @r#"
+        [
+            "binary_file",
+        ]
+        "#);
+        insta::assert_debug_snapshot!(files, @r#"
+        [
+            File {
+                old_path: None,
+                path: "binary_file",
+                file_mode: Unix(
+                    33188,
+                ),
+                sections: [
+                    FileMode {
+                        is_checked: false,
+                        mode: Unix(
+                            33261,
+                        ),
+                    },
+                ],
+            },
+        ]
+        "#);
+        let no_changes_tree_id = apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let no_changes_tree = store.get_root_tree(&no_changes_tree_id).unwrap();
+        assert_eq!(
+            left_tree.id(),
+            no_changes_tree.id(),
+            "no-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &left_tree.id()),
+            dump_tree(store, &no_changes_tree.id()),
+        );
+
+        let mut files = files;
+        for file in &mut files {
+            file.toggle_all();
+        }
+        let all_changes_tree_id =
+            apply_diff(store, &left_tree, &right_tree, &changed_files, &files);
+        let all_changes_tree = store.get_root_tree(&all_changes_tree_id).unwrap();
+        assert_eq!(
+            right_tree.id(),
+            all_changes_tree.id(),
+            "all-changes tree was different:\nexpected tree:\n{}\nactual tree:\n{}",
+            dump_tree(store, &right_tree.id()),
+            dump_tree(store, &all_changes_tree.id()),
+        );
+    }
+
     #[test]
     fn test_edit_diff_builtin_delete_file() {
         let test_repo = TestRepo::init();