implement support for resolving irreconcilable tree conflicts with 'ours' or 'ancestor'

Byron · Byron · commit bfa4aecbe12a · 2024-11-28T21:38:02.000+01:00
diff --git a/crate-status.md b/crate-status.md
@@ -352,8 +352,11 @@ Check out the [performance discussion][gix-diff-performance] as well.
     - [ ] a way to control inter-hunk merging based on proximity (maybe via `gix-diff` feature which could use the same)
 * [x] **tree**-diff-heuristics match Git for its test-cases
     - [x] a way to generate an index with stages, mostly conforming with Git.
+    - [ ] resolve to be *ours* or the *ancestors* version of the tree.
     - [ ] submodule merges (*right now they count as conflicts if they differ*)
     - [ ] assure sparse indices are handled correctly during application - right now we refuse.
+    - [ ] rewrite so that the whole logic can be proven to be correct - it's too insane now and probably has way 
+          more possible states than are tested, despite best attempts.
 * [x] **commits** - with handling of multiple merge bases by recursive merge-base merge
 * [x] API documentation
     * [ ] Examples
diff --git a/gix-merge/src/tree/function.rs b/gix-merge/src/tree/function.rs
diff --git a/gix-merge/src/tree/mod.rs b/gix-merge/src/tree/mod.rs
@@ -157,7 +157,7 @@ enum ConflictIndexEntryPathHint {
 }
 
 /// A utility to help define which side is what in the [`Conflict`] type.
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
 enum ConflictMapping {
     /// The sides are as described in the field documentation, i.e. `ours` is `ours`.
     Original,
@@ -198,7 +198,12 @@ impl Conflict {
                 match &self.resolution {
                     Ok(success) => match success {
                         Resolution::SourceLocationAffectedByRename { .. } => false,
-                        Resolution::Forced(_) => how.tree_merge == treat_as_unresolved::TreeMerge::ForcedResolution,
+                        Resolution::Forced(_) => {
+                            how.tree_merge == treat_as_unresolved::TreeMerge::ForcedResolution
+                                || self
+                                    .content_merge()
+                                    .map_or(false, |merged_blob| content_merge_matches(&merged_blob))
+                        }
                         Resolution::OursModifiedTheirsRenamedAndChangedThenRename {
                             merged_blob,
                             final_location,
@@ -386,7 +391,10 @@ pub struct Options {
 pub enum ResolveWith {
     /// On irreconcilable conflict, choose neither *our* nor *their* state, but keep the common *ancestor* state instead.
     Ancestor,
-    /// On irreconcilable conflict, choose *our* side
+    /// On irreconcilable conflict, choose *our* side.
+    ///
+    /// Note that in order to get something equivalent to *theirs*, put *theirs* into the side of *ours*,
+    /// swapping the sides essentially.
     Ours,
 }
 
diff --git a/gix-merge/src/tree/utils.rs b/gix-merge/src/tree/utils.rs
@@ -504,7 +504,7 @@ impl TreeNodes {
                         cursor = &mut self.0[existing_idx];
                         debug_assert!(
                             cursor.is_leaf_node(),
-                            "BUG: we should really only try to remove leaf nodes"
+                            "BUG: we should really only try to remove leaf nodes: {cursor:?}"
                         );
                         cursor.change_idx = None;
                     } else {
diff --git a/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar b/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar
diff --git a/gix-merge/tests/fixtures/tree-baseline.sh b/gix-merge/tests/fixtures/tree-baseline.sh
@@ -36,6 +36,15 @@ function make_conflict_index() {
   cp .git/index .git/"${identifier}".index
 }
 
+function make_resolve_tree() {
+  local resolve=${1:?Their 'ancestor' or 'ours'}
+  local our_side=${2:-}
+  local their_side=${3:-}
+
+  local filename="resolve-${our_side}-${their_side}-with-${resolve}"
+  git write-tree > ".git/${filename}.tree"
+}
+
 function baseline () (
   local dir=${1:?the directory to enter}
   local output_name=${2:?the basename of the output of the merge}
@@ -1074,3 +1083,122 @@ baseline rename-and-modification A-B A B
 baseline symlink-modification A-B A B
 baseline symlink-addition A-B A B
 baseline type-change-to-symlink A-B A B
+
+##
+## Only once the tree-merges were performed can we refer to their objects
+## when making tree-conflict resolution expectations. It's important
+## to get these right.
+##
+(cd simple
+  rm .git/index
+  # 'whatever' is tree-conflict, 'greeting' is content conflict with markers
+  git update-index --index-info <<EOF
+100644 9dc97bdc2426e68423360e3e5299280b2cf6b8ff 0	greeting
+100644 09c277aa66897c58157f57a374eacc63a407dcab 0	numbers
+100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 0	whatever
+EOF
+  make_resolve_tree ours side1 side2
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 1a2664a9924754c698e323f756f9f87f3f2fb337 0	greeting
+100644 09c277aa66897c58157f57a374eacc63a407dcab 0	numbers
+100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0	whatever/empty
+EOF
+  make_resolve_tree ours side2 side1
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 9dc97bdc2426e68423360e3e5299280b2cf6b8ff 0	greeting
+100644 09c277aa66897c58157f57a374eacc63a407dcab 0	numbers
+100644 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 0	whatever
+EOF
+  make_resolve_tree ancestor side1 side2
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 1a2664a9924754c698e323f756f9f87f3f2fb337 0	greeting
+100644 09c277aa66897c58157f57a374eacc63a407dcab 0	numbers
+100644 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 0	whatever
+EOF
+  make_resolve_tree ancestor side2 side1
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 a4ae6e4709228b5da6001cb9d1cfa7736851e2a6 0	greeting
+100644 257cc5642cb1a054f08cc83f2d943e56fd3ebe99 0	whatever
+100644 542802a799ded74fa01c47ba2f8925e284a369e2 0	Αυτά μου φαίνονται κινέζικα
+EOF
+  make_resolve_tree ancestor tweak1 side2
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 a4ae6e4709228b5da6001cb9d1cfa7736851e2a6 0	greeting
+100644 5716ca5987cbf97d6bb54920bea6adde242d87e6 0	whatever
+100644 542802a799ded74fa01c47ba2f8925e284a369e2 0	Αυτά μου φαίνονται κινέζικα
+EOF
+  make_resolve_tree ours tweak1 side2
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 blob ea28dcd7f627a2a7bbd09daa679c452180617c9f	greeting
+100644 blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99	whatever
+100644 blob f801a62deed900f8a80ff35e3339474ad6352a93	Αυτά μου φαίνονται κινέζικα
+EOF
+  make_resolve_tree ancestor side2 tweak1
+
+  rm .git/index
+  git update-index --index-info <<EOF
+100644 blob ea28dcd7f627a2a7bbd09daa679c452180617c9f	greeting
+100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	whatever/empty
+100644 blob f801a62deed900f8a80ff35e3339474ad6352a93	Αυτά μου φαίνονται κινέζικα
+EOF
+  make_resolve_tree ours side2 tweak1
+)
+
+(cd rename-add-symlink
+  rm .git/index
+  # the symlink of 'bar' from A
+  git update-index --index-info <<EOF
+120000 blob 19102815663d23f8b75a47e7a01965dcdc96468c	bar
+EOF
+  make_resolve_tree ours A B
+
+  rm .git/index
+  # the merged form of 'bar' from B, not replaced by symlink
+  git update-index --index-info <<EOF
+100644 blob b414108e81e5091fe0974a1858b4d0d22b107f70	bar
+EOF
+  make_resolve_tree ours B A
+
+  rm .git/index
+  # foo is renamed to bar, type clash means neither A nor B can be added - empty tree
+  # It is not able to 'get foo back', it can't track that currently.
+  make_resolve_tree ancestor A B
+  make_resolve_tree ancestor B A
+)
+
+(cd rename-rename-plus-content
+  rm .git/index
+  # both sides rename 'foo' into something else.
+  git update-index --index-info <<EOF
+100644 blob 8a1218a1024a212bb3db30becd860315f9f3ac52	foo
+EOF
+  make_resolve_tree ancestor A B
+  make_resolve_tree ancestor B A
+
+  rm .git/index
+  # 'bar' is the name in 'A', and there is a merge with the content from 'B' (conflicting)
+  git update-index --index-info <<EOF
+100644 blob b3b7e96453e640c018ef2d2e984cd0a3641a7d93	bar
+EOF
+  make_resolve_tree ours A B
+
+  rm .git/index
+  # 'baz' is the name in 'B', and there is a merge with the content from 'A' (conflicting)
+  git update-index --index-info <<EOF
+100644 blob b3b7e96453e640c018ef2d2e984cd0a3641a7d93	baz
+EOF
+  make_resolve_tree ours B A
+)
+
diff --git a/gix-merge/tests/merge/tree/baseline.rs b/gix-merge/tests/merge/tree/baseline.rs
@@ -48,6 +48,7 @@ pub enum ConflictKind {
 
 /// More loosely structured information about the `Conflict`.
 #[derive(Debug)]
+#[allow(dead_code)] // used only for debugging
 pub struct ConflictInfo {
     /// All the paths involved in the informational message
     pub paths: Vec<String>,
@@ -371,6 +372,22 @@ pub fn show_diff_and_fail(
     );
 }
 
+pub fn show_diff_trees_and_fail(
+    case_name: &str,
+    actual_id: ObjectId,
+    actual: &gix_merge::tree::Outcome<'_>,
+    expected_tree_id: gix_hash::ObjectId,
+    additional_information: &str,
+    odb: &gix_odb::memory::Proxy<gix_odb::Handle>,
+) {
+    pretty_assertions::assert_str_eq!(
+        visualize_tree(&actual_id, odb, None).to_string(),
+        visualize_tree(&expected_tree_id, odb, None).to_string(),
+        "{case_name}: merged tree mismatch\n{:#?}\n{additional_information}\n{case_name}",
+        actual.conflicts,
+    );
+}
+
 pub(crate) fn apply_git_index_entries(conflicts: &[Conflict], state: &mut gix_index::State) {
     let len = state.entries().len();
     for Conflict { ours, theirs, ancestor } in conflicts {
diff --git a/gix-merge/tests/merge/tree/mod.rs b/gix-merge/tests/merge/tree/mod.rs
@@ -21,8 +21,9 @@ fn run_baseline() -> crate::Result {
     let root = gix_testtools::scripted_fixture_read_only("tree-baseline.sh")?;
     let cases = std::fs::read_to_string(root.join("baseline.cases"))?;
     let mut actual_cases = 0;
-    // let new_test = Some("rename-add-symlink-A-B");
-    let new_test = None;
+    let mut skipped_tree_resolve_cases = 0;
+    let new_test = Some("rename-rename-plus-content");
+    // let new_test = None;
     for baseline::Expectation {
         root,
         conflict_style,
@@ -63,7 +64,7 @@ fn run_baseline() -> crate::Result {
             &mut blob_merge,
             &odb,
             &mut |id| id.to_hex_with_len(7).to_string(),
-            options,
+            options.clone(),
         )?
         .tree_merge;
 
@@ -138,12 +139,63 @@ fn run_baseline() -> crate::Result {
             actual.has_unresolved_conflicts(conflicts_like_in_git),
             "{case_name}: If there is any kind of conflict, the index should have been changed"
         );
+
+        // The content-merge mode is not relevant for the upcoming tree-conflict resolution.
+        if case_name.contains("diff3") {
+            continue;
+        }
+
+        for tree_resolution in [
+            gix_merge::tree::ResolveWith::Ancestor,
+            gix_merge::tree::ResolveWith::Ours,
+        ] {
+            let resolution_name = match tree_resolution {
+                gix_merge::tree::ResolveWith::Ancestor => "ancestor",
+                gix_merge::tree::ResolveWith::Ours => "ours",
+            };
+            let basename = format!("resolve-{our_side_name}-{their_side_name}-with-{resolution_name}");
+            let tree_path = root.join(".git").join(format!("{basename}.tree"));
+            if !tree_path.exists() {
+                skipped_tree_resolve_cases += 1;
+                continue;
+            };
+            let expected_tree_id = gix_hash::ObjectId::from_hex(std::fs::read_to_string(tree_path)?.trim().as_bytes())?;
+            options.tree_merge.tree_conflicts = Some(tree_resolution);
+            dbg!(&case_name, &basename);
+            let mut actual = gix_merge::commit(
+                our_commit_id,
+                their_commit_id,
+                gix_merge::blob::builtin_driver::text::Labels {
+                    ancestor: None,
+                    current: Some(our_side_name.as_str().into()),
+                    other: Some(their_side_name.as_str().into()),
+                },
+                &mut graph,
+                &mut diff_resource_cache,
+                &mut blob_merge,
+                &odb,
+                &mut |id| id.to_hex_with_len(7).to_string(),
+                options.clone(),
+            )?
+            .tree_merge;
+
+            let actual_id = actual.tree.write(|tree| odb.write(tree))?;
+            if actual_id != expected_tree_id {
+                baseline::show_diff_trees_and_fail(&case_name, actual_id, &actual, expected_tree_id, &basename, &odb);
+            }
+        }
     }
 
     assert_eq!(
         actual_cases, 109,
         "BUG: update this number, and don't forget to remove a filter in the end"
     );
+    assert_eq!(
+        skipped_tree_resolve_cases,
+        166 - 4 * 7,
+        "this is done when no case is skipped, and we don't want to accidentally skip them.\
+        Some don't actually have conflicts"
+    );
 
     Ok(())
 }
@@ -215,6 +267,4 @@ fn new_blob_merge_platform(
     )
 }
 
-// TODO: make sure everything is read eventually, even if only to improve debug messages in case of failure.
-#[allow(dead_code)]
 mod baseline;
