From 762c8984c6135d581ecc2db2e0ae2a74eae08239 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 17 Apr 2022 20:51:24 -0700 Subject: [PATCH] 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 | 5 ++++ lib/src/tree.rs | 36 ++++++++++++++++++--------- lib/tests/test_merge_trees.rs | 46 +++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f544b8fa..120b9578e 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/lib/src/tree.rs b/lib/src/tree.rs index d4e5ea2fb..75e133311 100644 --- a/lib/src/tree.rs +++ b/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, 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)) diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index e043ca12f..74b334d52 100644 --- a/lib/tests/test_merge_trees.rs +++ b/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) {