Skip to content

Commit

Permalink
merged_tree: do not propagate conflicting empty tree value to parent
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yuja committed Dec 2, 2023
1 parent 48d586c commit a811965
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 5 deletions.
8 changes: 3 additions & 5 deletions lib/src/merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
18 changes: 18 additions & 0 deletions lib/tests/test_merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit a811965

Please sign in to comment.