diff --git a/CHANGELOG.md b/CHANGELOG.md index e73d6966af..f94d5805df 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 1d2c30184b..aa30177f2a 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -543,8 +543,12 @@ fn merge_tree_values( .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 diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 1ca25fed90..4217abce91 100644 --- a/lib/src/tree.rs +++ b/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 diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index fe5c619a37..a82b8ed748 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -1455,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); +}