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

Author: Byron

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

gix-merge/src/tree/function.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,
@@ -175,13 +175,19 @@ impl ConflictMapping {
             ConflictMapping::Swapped => ConflictMapping::Original,
         }
     }
+    fn to_global(self, global: ConflictMapping) -> ConflictMapping {
+        match global {
+            ConflictMapping::Original => self,
+            ConflictMapping::Swapped => self.swapped(),
+        }
+    }
 }
 
 impl Conflict {
     /// Return `true` if this instance is considered unresolved based on the criterion specified by `how`.
     pub fn is_unresolved(&self, how: TreatAsUnresolved) -> bool {
         use crate::blob;
-        let content_merge_matches = |info: &ContentMerge| match how.content_merge {
+        let content_merge_unresolved = |info: &ContentMerge| match how.content_merge {
             treat_as_unresolved::ContentMerge::Markers => matches!(info.resolution, blob::Resolution::Conflict),
             treat_as_unresolved::ContentMerge::ForcedResolution => {
                 matches!(
@@ -192,20 +198,28 @@ impl Conflict {
         };
         match how.tree_merge {
             treat_as_unresolved::TreeMerge::Undecidable => {
-                self.resolution.is_err() || self.content_merge().map_or(false, |info| content_merge_matches(&info))
+                self.resolution.is_err()
+                    || self
+                        .content_merge()
+                        .map_or(false, |info| content_merge_unresolved(&info))
             }
             treat_as_unresolved::TreeMerge::EvasiveRenames | treat_as_unresolved::TreeMerge::ForcedResolution => {
                 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_unresolved(&merged_blob))
+                        }
                         Resolution::OursModifiedTheirsRenamedAndChangedThenRename {
                             merged_blob,
                             final_location,
                             ..
-                        } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_matches),
+                        } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_unresolved),
                         Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
-                            content_merge_matches(merged_blob)
+                            content_merge_unresolved(merged_blob)
                         }
                     },
                     Err(_failure) => true,
@@ -249,6 +263,7 @@ impl Conflict {
             match failure {
                 ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob,
                 ResolutionFailure::Unknown
+                | ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed { .. }
                 | ResolutionFailure::OursModifiedTheirsDeleted
                 | ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch
                 | ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed {
@@ -321,6 +336,17 @@ pub enum ResolutionFailure {
         /// The path at which `ours` can be found in the tree - it's in the same directory that it was in before.
         renamed_unique_path_to_modified_blob: BString,
     },
+    /// *ours* is a directory, but *theirs* is a non-directory (i.e. file), which wants to be in its place, even though
+    /// *ours* has a modification in that subtree.
+    /// Rename *theirs* to retain that modification.
+    ///
+    /// Important: there is no actual modification on *ours* side, so *ours* is filled in with *theirs* as the data structure
+    /// cannot represent this case.
+    // TODO: Can we have a better data-structure? This would be for a rewrite though.
+    OursDirectoryTheirsNonDirectoryTheirsRenamed {
+        /// The non-conflicting path of *their* non-tree entry.
+        renamed_unique_path_of_theirs: BString,
+    },
     /// *ours* was added (or renamed into place) with a different mode than theirs, e.g. blob and symlink, and we kept
     /// the symlink in its original location, renaming the other side to `their_unique_location`.
     OursAddedTheirsAddedTypeMismatch {
@@ -376,8 +402,10 @@ pub struct Options {
     /// If `None`, tree irreconcilable tree conflicts will result in [resolution failures](ResolutionFailure).
     /// Otherwise, one can choose a side. Note that it's still possible to determine that auto-resolution happened
     /// despite this choice, which allows carry forward the conflicting information, possibly for later resolution.
-    /// If `Some(…)`, irreconcilable conflicts are reconciled by making a choice. This mlso means that [`Conflict::entries()`]
-    /// won't be set as the conflict was officially resolved.
+    /// If `Some(…)`, irreconcilable conflicts are reconciled by making a choice.
+    /// Note that [`Conflict::entries()`] will still be set, to not degenerate information, even though they then represent
+    /// the entries what would fit the index if no forced resolution was performed.
+    /// It's up to the caller to handle that information mindfully.
     pub tree_conflicts: Option<ResolveWith>,
 }
 
@@ -386,7 +414,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,
 }
 
@@ -439,6 +470,9 @@ pub mod apply_index_entries {
                         }
                     },
                     Err(failure) => match failure {
+                        ResolutionFailure::OursDirectoryTheirsNonDirectoryTheirsRenamed {
+                            renamed_unique_path_of_theirs,
+                        } => (Some(renamed_unique_path_of_theirs.as_bstr()), conflict.ours.location()),
                         ResolutionFailure::OursRenamedTheirsRenamedDifferently { .. } => {
                             (Some(conflict.theirs.location()), conflict.ours.location())
                         }

gix-merge/src/tree/mod.rs

@@ -23,7 +23,7 @@ use std::collections::HashMap;
 /// over a directory rewrite in *our* tree. If so, rewrite it so that we get the path
 /// it would have had if it had been renamed along with *our* directory.
 pub fn possibly_rewritten_location(
-    check_tree: &mut TreeNodes,
+    check_tree: &TreeNodes,
     their_location: &BStr,
     our_changes: &ChangeListRef,
 ) -> Option<BString> {
@@ -60,7 +60,7 @@ pub fn rewrite_location_with_renamed_directory(their_location: &BStr, passed_cha
 pub fn unique_path_in_tree(
     file_path: &BStr,
     editor: &tree::Editor<'_>,
-    tree: &mut TreeNodes,
+    tree: &TreeNodes,
     side_name: &BStr,
 ) -> Result<BString, Error> {
     let mut buf = file_path.to_owned();
@@ -400,11 +400,11 @@ impl TreeNodes {
                     Some(index) => {
                         cursor = &mut self.0[index];
                         if is_last && !cursor.is_leaf_node() {
-                            assert_eq!(
-                                cursor.change_idx, None,
-                                "BUG: each node should only see a single change when tracking initially: {path} {change_idx}"
-                            );
-                            cursor.change_idx = Some(change_idx);
+                            // NOTE: we might encounter the same path multiple times in rare conditions.
+                            //       At least we avoid overwriting existing intermediate changes, for good measure.
+                            if cursor.change_idx.is_none() {
+                                cursor.change_idx = Some(change_idx);
+                            }
                         }
                     }
                 }
@@ -414,12 +414,12 @@ impl TreeNodes {
 
     /// Search the tree with `our` changes for `theirs` by [`source_location()`](Change::source_location())).
     /// If there is an entry but both are the same, or if there is no entry, return `None`.
-    pub fn check_conflict(&mut self, theirs_location: &BStr) -> Option<PossibleConflict> {
+    pub fn check_conflict(&self, theirs_location: &BStr) -> Option<PossibleConflict> {
         if self.0.len() == 1 {
             return None;
         }
         let components = to_components(theirs_location);
-        let mut cursor = &mut self.0[0];
+        let mut cursor = &self.0[0];
         let mut cursor_idx = 0;
         let mut intermediate_change = None;
         for component in components {
@@ -436,7 +436,7 @@ impl TreeNodes {
                     } else {
                         // a change somewhere else, i.e. `a/c` and we know `a/b` only.
                         intermediate_change.and_then(|(change, cursor_idx)| {
-                            let cursor = &mut self.0[cursor_idx];
+                            let cursor = &self.0[cursor_idx];
                             // If this is a destination location of a rename, then the `their_location`
                             // is already at the right spot, and we can just ignore it.
                             if matches!(cursor.location, ChangeLocation::CurrentLocation) {
@@ -450,7 +450,7 @@ impl TreeNodes {
                 }
                 Some(child_idx) => {
                     cursor_idx = child_idx;
-                    cursor = &mut self.0[cursor_idx];
+                    cursor = &self.0[cursor_idx];
                 }
             }
         }
@@ -496,15 +496,15 @@ impl TreeNodes {
         let mut cursor = &mut self.0[0];
         while let Some(component) = components.next() {
             match cursor.children.get(component).copied() {
-                None => assert!(!must_exist, "didn't find '{location}' for removal"),
+                None => debug_assert!(!must_exist, "didn't find '{location}' for removal"),
                 Some(existing_idx) => {
                     let is_last = components.peek().is_none();
                     if is_last {
                         cursor.children.remove(component);
                         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 {
@@ -578,10 +578,7 @@ impl Conflict {
             ours: ours.clone(),
             theirs: theirs.clone(),
             entries,
-            map: match outer_map {
-                ConflictMapping::Original => map,
-                ConflictMapping::Swapped => map.swapped(),
-            },
+            map: map.to_global(outer_map),
         }
     }