From a8119654409c208ff9649f68d84888dce2ed6c30 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 2 Dec 2023 13:44:30 +0900 Subject: [PATCH] merged_tree: do not propagate conflicting empty tree value to parent Otherwise an empty subtree would be added to the parent tree. If the stored tree contained an empty subtree, simplify() wouldn't work against new "absent" subtree representation. I don't know if there's a such code path, but I believe it's very rare to encounter the problem. #2654 --- lib/src/merged_tree.rs | 8 +++----- lib/tests/test_merged_tree.rs | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index d03b0f8848..1d2c30184b 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -537,12 +537,10 @@ 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. diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index d6e9f2aff0..fe5c619a37 100644 --- a/lib/tests/test_merged_tree.rs +++ b/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();