merged_tree: don't add empty subtree to parent, remove canceling terms prior to resolving file-level conflict

CHANGELOG.md

@@ -37,6 +37,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed bugs
 
+* Fixed another file conflict resolution issue where `jj status` would disagree
+  with the actual file content.
+  [#2654](https://github.com/martinvonz/jj/issues/2654)
+
 
 ## [0.11.0] - 2023-11-01
 

lib/src/merged_tree.rs

@@ -537,16 +537,18 @@ fn merge_tree_values(
     if let Some(trees) = values.to_tree_merge(store, path)? {
         // If all sides are trees or missing, merge the trees recursively, treating
         // missing trees as empty.
+        let empty_tree_id = store.empty_tree_id();
         let merged_tree = merge_trees(&trees)?;
-        if merged_tree.as_resolved().map(|tree| tree.id()) == Some(store.empty_tree_id()) {
-            Ok(Merge::absent())
-        } else {
-            Ok(merged_tree.map(|tree| Some(TreeValue::Tree(tree.id().clone()))))
-        }
+        Ok(merged_tree
+            .map(|tree| (tree.id() != empty_tree_id).then(|| TreeValue::Tree(tree.id().clone()))))
     } else {
         // Try to resolve file conflicts by merging the file contents. Treats missing
-        // files as empty.
-        if let Some(resolved) = try_resolve_file_conflict(store, path, &values)? {
+        // files as empty. The values may contain trees canceling each other (notably
+        // padded absent trees), so we need to simplify them first.
+        let simplified = values.clone().simplify();
+        // No fast path for simplified.is_resolved(). If it could be resolved, it would
+        // have been caught by values.resolve_trivial() above.
+        if let Some(resolved) = try_resolve_file_conflict(store, path, &simplified)? {
             Ok(Merge::normal(resolved))
         } else {
             // Failed to merge the files, or the paths are not files

lib/src/tree.rs

@@ -396,6 +396,10 @@ fn merge_tree_value(
     })
 }
 
+/// Resolves file-level conflict by merging content hunks.
+///
+/// The input `conflict` is supposed to be simplified. It shouldn't contain
+/// non-file values that cancel each other.
 pub fn try_resolve_file_conflict(
     store: &Store,
     filename: &RepoPath,
@@ -430,7 +434,9 @@ pub fn try_resolve_file_conflict(
         }));
     }
 
-    // Simplify the conflict for two reasons:
+    // While the input conflict should be simplified by caller, it might contain
+    // terms which only differ in executable bits. Simplify the conflict further
+    // for two reasons:
     // 1. Avoid reading unchanged file contents
     // 2. The simplified conflict can sometimes be resolved when the unsimplfied one
     //    cannot

lib/tests/test_merged_tree.rs

@@ -490,6 +490,24 @@ fn test_resolve_with_conflict() {
     )
 }
 
+#[test]
+fn test_resolve_with_conflict_containing_empty_subtree() {
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // Since "dir" in side2 is absent, the root tree should be empty as well.
+    // If it were added to the root tree, side2.id() would differ.
+    let conflict_path = RepoPath::from_internal_string("dir/file_conflict");
+    let base1 = create_single_tree(repo, &[(conflict_path, "base1")]);
+    let side1 = create_single_tree(repo, &[(conflict_path, "side1")]);
+    let side2 = create_single_tree(repo, &[]);
+
+    let original_tree = Merge::from_removes_adds(vec![base1], vec![side1, side2]);
+    let tree = MergedTree::new(original_tree.clone());
+    let resolved_tree = tree.resolve().unwrap();
+    assert_eq!(resolved_tree, original_tree);
+}
+
 #[test]
 fn test_conflict_iterator() {
     let test_repo = TestRepo::init();
@@ -1437,3 +1455,34 @@ fn test_merge_simplify_file_conflict() {
         MergeResult::Conflict(_)
     ));
 }
+
+/// Like `test_merge_simplify_file_conflict()`, but some of the conflicts are
+/// absent.
+#[test]
+fn test_merge_simplify_file_conflict_with_absent() {
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // conflict_path doesn't exist in parent and child2_left, and these two
+    // trees can't be canceled out at the root level. Still the file merge
+    // should succeed by eliminating absent entries.
+    let child2_path = RepoPath::from_internal_string("file_child2");
+    let conflict_path = RepoPath::from_internal_string("dir/file_conflict");
+    let child1 = create_single_tree(repo, &[(conflict_path, "1\n0\n")]);
+    let parent = create_single_tree(repo, &[]);
+    let child2_left = create_single_tree(repo, &[(child2_path, "")]);
+    let child2_base = create_single_tree(repo, &[(child2_path, ""), (conflict_path, "0\n")]);
+    let child2_right = create_single_tree(repo, &[(child2_path, ""), (conflict_path, "0\n2\n")]);
+    let child1_merged = MergedTree::resolved(child1);
+    let parent_merged = MergedTree::resolved(parent);
+    let child2_merged = MergedTree::new(Merge::from_removes_adds(
+        vec![child2_base],
+        vec![child2_left, child2_right],
+    ));
+
+    let expected = create_single_tree(repo, &[(child2_path, ""), (conflict_path, "1\n0\n2\n")]);
+    let expected_merged = MergedTree::resolved(expected);
+
+    let merged = child1_merged.merge(&parent_merged, &child2_merged).unwrap();
+    assert_eq!(merged, expected_merged);
+}