trees: when merging trees and one is missing, treat it as empty

When a directory is missing in one merge input (base or one side), we
would consider that a merge conflict. This patch changes that so we
instead merge trees by treating the missing tree as empty.

CHANGELOG.md

@@ -48,6 +48,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   but the contents was the same used to lead to a crash. That has now been
   fixed. 
 
+* If one side of a merge modified a directory and the other side deleted it, it
+  used to be considered a conflict. The same was true if both sides added a
+  directory with different files in. They are now merged as if the missing
+  directory had been empty.
+
 * `jj untrack` now requires at least one path (allowing no arguments was a UX
   bug).
 

lib/src/tree.rs

@@ -552,6 +552,19 @@ pub fn merge_trees(
     store.write_tree(dir, &new_tree)
 }
 
+/// Returns `Some(TreeId)` if this is a directory or missing. If it's missing,
+/// we treat it as an empty tree.
+fn maybe_tree_id<'id>(
+    value: Option<&'id TreeValue>,
+    empty_tree_id: &'id TreeId,
+) -> Option<&'id TreeId> {
+    match value {
+        Some(TreeValue::Tree(id)) => Some(id),
+        None => Some(empty_tree_id),
+        _ => None,
+    }
+}
+
 fn merge_tree_value(
     store: &Arc<Store>,
     dir: &RepoPath,
@@ -566,19 +579,18 @@ fn merge_tree_value(
     //   * leave other conflicts (e.g. file/dir conflicts, remove/modify conflicts)
     //     unresolved
 
-    Ok(match (maybe_base, maybe_side1, maybe_side2) {
-        (
-            Some(TreeValue::Tree(base)),
-            Some(TreeValue::Tree(side1)),
-            Some(TreeValue::Tree(side2)),
-        ) => {
+    let empty_tree_id = store.empty_tree_id();
+    let base_tree_id = maybe_tree_id(maybe_base, empty_tree_id);
+    let side1_tree_id = maybe_tree_id(maybe_side1, empty_tree_id);
+    let side2_tree_id = maybe_tree_id(maybe_side2, empty_tree_id);
+    Ok(match (base_tree_id, side1_tree_id, side2_tree_id) {
+        (Some(base_id), Some(side1_id), Some(side2_id)) => {
             let subdir = dir.join(basename);
-            let merged_tree_id = merge_trees(
-                &store.get_tree(&subdir, side1).unwrap(),
-                &store.get_tree(&subdir, base).unwrap(),
-                &store.get_tree(&subdir, side2).unwrap(),
-            )?;
-            if &merged_tree_id == store.empty_tree_id() {
+            let base_tree = store.get_tree(&subdir, base_id)?;
+            let side1_tree = store.get_tree(&subdir, side1_id)?;
+            let side2_tree = store.get_tree(&subdir, side2_id)?;
+            let merged_tree_id = merge_trees(&side1_tree, &base_tree, &side2_tree)?;
+            if merged_tree_id == *empty_tree_id {
                 None
             } else {
                 Some(TreeValue::Tree(merged_tree_id))

lib/tests/test_merge_trees.rs

@@ -297,6 +297,52 @@ fn test_subtree_becomes_empty(use_git: bool) {
     assert_eq!(merged_tree.id(), store.empty_tree_id());
 }
 
+#[test_case(false ; "local backend")]
+#[test_case(true ; "git backend")]
+fn test_subtree_one_missing(use_git: bool) {
+    // Tests that merging trees where one side is missing is resolved as if the
+    // missing side was empty.
+    let settings = testutils::user_settings();
+    let test_repo = testutils::init_repo(&settings, use_git);
+    let repo = &test_repo.repo;
+    let store = repo.store();
+
+    let write_tree = |paths: Vec<&str>| -> Tree {
+        let mut tree_builder = store.tree_builder(store.empty_tree_id().clone());
+        for path in paths {
+            testutils::write_normal_file(
+                &mut tree_builder,
+                &RepoPath::from_internal_string(path),
+                &format!("contents of {:?}", path),
+            );
+        }
+        let tree_id = tree_builder.write_tree();
+        store.get_tree(&RepoPath::root(), &tree_id).unwrap()
+    };
+
+    let tree1 = write_tree(vec![]);
+    let tree2 = write_tree(vec!["d1/f1"]);
+    let tree3 = write_tree(vec!["d1/f1", "d1/f2"]);
+
+    // The two sides add different trees
+    let merged_tree_id = tree::merge_trees(&tree2, &tree1, &tree3).unwrap();
+    let merged_tree = store.get_tree(&RepoPath::root(), &merged_tree_id).unwrap();
+    let expected_entries = write_tree(vec!["d1/f1", "d1/f2"]).entries().collect_vec();
+    assert_eq!(merged_tree.entries().collect_vec(), expected_entries);
+    // Same tree other way
+    let merged_tree_id = tree::merge_trees(&tree3, &tree1, &tree2).unwrap();
+    assert_eq!(merged_tree_id, *merged_tree.id());
+
+    // One side removes, the other side modifies
+    let merged_tree_id = tree::merge_trees(&tree1, &tree2, &tree3).unwrap();
+    let merged_tree = store.get_tree(&RepoPath::root(), &merged_tree_id).unwrap();
+    let expected_entries = write_tree(vec!["d1/f2"]).entries().collect_vec();
+    assert_eq!(merged_tree.entries().collect_vec(), expected_entries);
+    // Same tree other way
+    let merged_tree_id = tree::merge_trees(&tree3, &tree2, &tree1).unwrap();
+    assert_eq!(merged_tree_id, *merged_tree.id());
+}
+
 #[test_case(false ; "local backend")]
 #[test_case(true ; "git backend")]
 fn test_types(use_git: bool) {