feat!: replace tree::Options::allow_lossy_resolution with *::tree_conflicts.

That way it's possible to steer how to resolve tree-related conflicts while
making it possible to detect that a conflict happened.

Author: Byron

gix-merge/src/blob/builtin_driver/text/mod.rs

@@ -57,8 +57,11 @@ pub enum ConflictStyle {
 /// That way it becomes clearer where the content of conflicts are originating from.
 #[derive(Default, Copy, Clone, Debug, Eq, PartialEq)]
 pub struct Labels<'a> {
+    /// The label for the common *ancestor*.
     pub ancestor: Option<&'a BStr>,
+    /// The label for the *current* (or *our*) side.
     pub current: Option<&'a BStr>,
+    /// The label for the *other* (or *their*) side.
     pub other: Option<&'a BStr>,
 }
 

gix-merge/src/commit/mod.rs

@@ -55,5 +55,6 @@ pub struct Outcome<'a> {
 
 pub(super) mod function;
 
+///
 pub mod virtual_merge_base;
 pub use virtual_merge_base::function::virtual_merge_base;

gix-merge/src/commit/virtual_merge_base.rs

@@ -30,7 +30,7 @@ pub enum Error {
 pub(super) mod function {
     use super::Error;
     use crate::blob::builtin_driver;
-    use crate::tree::TreatAsUnresolved;
+    use crate::tree::{treat_as_unresolved, TreatAsUnresolved};
     use gix_object::FindExt;
 
     /// Create a single virtual merge-base by merging `first_commit`, `second_commit` and `others` into one.
@@ -55,7 +55,7 @@ pub(super) mod function {
         let mut merged_commit_id = first_commit;
         others.push(second_commit);
 
-        options.allow_lossy_resolution = true;
+        options.tree_conflicts = Some(crate::tree::ResolveWith::Ancestor);
         options.blob_merge.is_virtual_ancestor = true;
         options.blob_merge.text.conflict = builtin_driver::text::Conflict::ResolveWithOurs;
         let favor_ancestor = Some(builtin_driver::binary::ResolveWith::Ancestor);
@@ -86,10 +86,10 @@ pub(super) mod function {
                 },
             )?;
             // This shouldn't happen, but if for some buggy reason it does, we rather bail.
-            if out
-                .tree_merge
-                .has_unresolved_conflicts(TreatAsUnresolved::ConflictMarkers)
-            {
+            if out.tree_merge.has_unresolved_conflicts(TreatAsUnresolved {
+                content_merge: treat_as_unresolved::ContentMerge::Markers,
+                tree_merge: treat_as_unresolved::TreeMerge::Undecidable,
+            }) {
                 return Err(Error::VirtualMergeBaseConflict.into());
             }
             let merged_tree_id = out

gix-merge/src/lib.rs

@@ -4,6 +4,7 @@
 //! * [tree-merges](mod@tree) look at trees and merge them structurally, triggering blob-merges as needed.
 //! * [commit-merges](mod@commit) are like tree merges, but compute or create the merge-base on the fly.
 #![deny(rust_2018_idioms)]
+#![deny(missing_docs)]
 #![forbid(unsafe_code)]
 
 ///

gix-merge/src/tree/function.rs

@@ -71,7 +71,8 @@ where
     let _span = gix_trace::coarse!("gix_merge::tree", ?base_tree, ?our_tree, ?their_tree, ?labels);
     let (mut base_buf, mut side_buf) = (Vec::new(), Vec::new());
     let ancestor_tree = objects.find_tree(base_tree, &mut base_buf)?;
-    let allow_resolution_failure = !options.allow_lossy_resolution;
+    // TODO: properly handle 'Ours' as well.
+    let allow_resolution_failure = !matches!(options.tree_conflicts, Some(crate::tree::ResolveWith::Ancestor));
 
     let mut editor = tree::Editor::new(ancestor_tree.to_owned(), objects, base_tree.kind());
     let ancestor_tree = gix_object::TreeRefIter::from_bytes(&base_buf);

gix-merge/src/tree/mod.rs

@@ -39,40 +39,47 @@ pub struct Outcome<'a> {
     /// Use [`has_unresolved_conflicts()`](Outcome::has_unresolved_conflicts()) to see if any action is needed
     /// before using [`tree`](Outcome::tree).
     pub conflicts: Vec<Conflict>,
-    /// `true` if `conflicts` contains only a single *unresolved* conflict in the last slot, but possibly more resolved ones.
+    /// `true` if `conflicts` contains only a single [*unresolved* conflict](ResolutionFailure) in the last slot, but
+    /// possibly more [resolved ones](Resolution) before that.
     /// This also makes this outcome a very partial merge that cannot be completed.
     /// Only set if [`fail_on_conflict`](Options::fail_on_conflict) is `true`.
     pub failed_on_first_unresolved_conflict: bool,
 }
 
 /// Determine what should be considered an unresolved conflict.
+#[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub struct TreatAsUnresolved {
+    /// Determine which content merges should be considered unresolved.
+    pub content_merge: treat_as_unresolved::ContentMerge,
+    /// Determine which tree merges should be considered unresolved.
+    pub tree_merge: treat_as_unresolved::TreeMerge,
+}
+
 ///
-/// Note that no matter which variant, [conflicts](Conflict) with
-/// [resolution failure](`ResolutionFailure`) will always be unresolved.
-///
-/// Also, when one side was modified but the other side renamed it, this will not
-/// be considered a conflict, even if a non-conflicting merge happened.
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
-pub enum TreatAsUnresolved {
-    /// Only consider content merges with conflict markers as unresolved.
-    ///
-    /// Auto-resolved tree conflicts will *not* be considered unresolved.
-    ConflictMarkers,
-    /// Consider content merges with conflict markers as unresolved, and content
-    /// merges where conflicts where auto-resolved in any way, like choosing
-    /// *ours*, *theirs*  or by their *union*.
-    ///
-    /// Auto-resolved tree conflicts will *not* be considered unresolved.
-    ConflictMarkersAndAutoResolved,
-    /// Whenever there were conflicting renames, or conflict markers, it is unresolved.
-    /// Note that auto-resolved content merges will *not* be considered unresolved.
-    ///
-    /// Also note that files that were changed in one and renamed in another will
-    /// be moved into place, which will be considered resolved.
-    Renames,
-    /// Similar to [`Self::Renames`], but auto-resolved content-merges will
-    /// also be considered unresolved.
-    RenamesAndAutoResolvedContent,
+pub mod treat_as_unresolved {
+    /// Which kind of content merges should be considered unresolved?
+    #[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
+    pub enum ContentMerge {
+        /// Content merges that still show conflict markers.
+        #[default]
+        Markers,
+        /// Content merges who would have conflicted if it wasn't for a
+        /// [resolution strategy](crate::blob::builtin_driver::text::Conflict::ResolveWithOurs).
+        ForcedResolution,
+    }
+
+    /// Which kind of tree merges should be considered unresolved?
+    #[derive(Default, Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
+    pub enum TreeMerge {
+        /// All failed renames.
+        Undecidable,
+        /// All failed renames, and the ones where a tree item was renamed to avoid a clash.
+        #[default]
+        EvasiveRenames,
+        /// All of `EvasiveRenames`, and tree merges that would have conflicted but which were resolved
+        /// with a [resolution strategy](super::ResolveWith).
+        ForcedResolution,
+    }
 }
 
 impl Outcome<'_> {
@@ -174,35 +181,36 @@ 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 {
-            TreatAsUnresolved::ConflictMarkers | TreatAsUnresolved::Renames => {
-                matches!(info.resolution, blob::Resolution::Conflict)
-            }
-            TreatAsUnresolved::RenamesAndAutoResolvedContent | TreatAsUnresolved::ConflictMarkersAndAutoResolved => {
+        let content_merge_matches = |info: &ContentMerge| match how.content_merge {
+            treat_as_unresolved::ContentMerge::Markers => matches!(info.resolution, blob::Resolution::Conflict),
+            treat_as_unresolved::ContentMerge::ForcedResolution => {
                 matches!(
                     info.resolution,
                     blob::Resolution::Conflict | blob::Resolution::CompleteWithAutoResolvedConflict
                 )
             }
         };
-        match how {
-            TreatAsUnresolved::ConflictMarkers | TreatAsUnresolved::ConflictMarkersAndAutoResolved => {
+        match how.tree_merge {
+            treat_as_unresolved::TreeMerge::Undecidable => {
                 self.resolution.is_err() || self.content_merge().map_or(false, |info| content_merge_matches(&info))
             }
-            TreatAsUnresolved::Renames | TreatAsUnresolved::RenamesAndAutoResolvedContent => match &self.resolution {
-                Ok(success) => match success {
-                    Resolution::SourceLocationAffectedByRename { .. } => false,
-                    Resolution::OursModifiedTheirsRenamedAndChangedThenRename {
-                        merged_blob,
-                        final_location,
-                        ..
-                    } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_matches),
-                    Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
-                        content_merge_matches(merged_blob)
-                    }
-                },
-                Err(_failure) => true,
-            },
+            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::OursModifiedTheirsRenamedAndChangedThenRename {
+                            merged_blob,
+                            final_location,
+                            ..
+                        } => final_location.is_some() || merged_blob.as_ref().map_or(false, content_merge_matches),
+                        Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
+                            content_merge_matches(merged_blob)
+                        }
+                    },
+                    Err(_failure) => true,
+                }
+            }
         }
     }
 
@@ -237,13 +245,8 @@ impl Conflict {
 
     /// Return information about the content merge if it was performed.
     pub fn content_merge(&self) -> Option<ContentMerge> {
-        match &self.resolution {
-            Ok(success) => match success {
-                Resolution::SourceLocationAffectedByRename { .. } => None,
-                Resolution::OursModifiedTheirsRenamedAndChangedThenRename { merged_blob, .. } => *merged_blob,
-                Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => Some(*merged_blob),
-            },
-            Err(failure) => match failure {
+        fn failure_merged_blob(failure: &ResolutionFailure) -> Option<ContentMerge> {
+            match failure {
                 ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob,
                 ResolutionFailure::Unknown
                 | ResolutionFailure::OursModifiedTheirsDeleted
@@ -253,7 +256,16 @@ impl Conflict {
                 }
                 | ResolutionFailure::OursAddedTheirsAddedTypeMismatch { .. }
                 | ResolutionFailure::OursDeletedTheirsRenamed => None,
+            }
+        }
+        match &self.resolution {
+            Ok(success) => match success {
+                Resolution::Forced(failure) => failure_merged_blob(failure),
+                Resolution::SourceLocationAffectedByRename { .. } => None,
+                Resolution::OursModifiedTheirsRenamedAndChangedThenRename { merged_blob, .. } => *merged_blob,
+                Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => Some(*merged_blob),
             },
+            Err(failure) => failure_merged_blob(failure),
         }
     }
 }
@@ -291,6 +303,9 @@ pub enum Resolution {
         /// The outcome of the content merge.
         merged_blob: ContentMerge,
     },
+    /// This is a resolution failure was forcefully turned into a usable resolution, i.e. [making a choice](ResolveWith)
+    /// is turned into a valid resolution.
+    Forced(ResolutionFailure),
 }
 
 /// Describes of a conflict involving *our* change and *their* failed to be resolved.
@@ -358,13 +373,26 @@ pub struct Options {
     /// If `None`, when symlinks clash *ours* will be chosen and a conflict will occur.
     /// Otherwise, the same logic applies as for the merge of binary resources.
     pub symlink_conflicts: Option<crate::blob::builtin_driver::binary::ResolveWith>,
-    /// If `true`, instead of issuing a conflict with [`ResolutionFailure`], do nothing and keep the base/ancestor
-    /// version. This is useful when one wants to avoid any kind of merge-conflict to have *some*, *lossy* resolution.
-    pub allow_lossy_resolution: bool,
+    /// 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.
+    pub tree_conflicts: Option<ResolveWith>,
+}
+
+/// Decide how to resolve tree-related conflicts, but only those that have [no way of being correct](ResolutionFailure).
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
+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
+    Ours,
 }
 
 pub(super) mod function;
 mod utils;
+///
 pub mod apply_index_entries {
 
     pub(super) mod function {
@@ -398,6 +426,7 @@ pub mod apply_index_entries {
             for conflict in conflicts.iter().filter(|c| c.is_unresolved(how)) {
                 let (renamed_path, current_path): (Option<&BStr>, &BStr) = match &conflict.resolution {
                     Ok(success) => match success {
+                        Resolution::Forced(_) => continue,
                         Resolution::SourceLocationAffectedByRename { final_location } => {
                             (Some(final_location.as_bstr()), final_location.as_bstr())
                         }

gix-merge/tests/fixtures/generated-archives/tree-baseline.tar

@@ -222,6 +222,26 @@ git init rename-add-symlink
   git commit -m "rename foo to bar"
 )
 
+git init rename-add-same-symlink
+(cd rename-add-same-symlink
+  touch target
+  ln -s target link
+  git add .
+  git commit -m "original"
+
+  git branch A
+  git branch B
+
+  git checkout A
+  git mv link link-new
+  git commit -m "rename link to link-new"
+
+  git checkout B
+  ln -s target link-new
+  git add link-new
+  git commit -m "create link-new"
+)
+
 git init rename-rename-plus-content
 (cd rename-rename-plus-content
   write_lines 1 2 3 4 5 >foo
@@ -993,7 +1013,7 @@ git init type-change-to-symlink
 
 
 
-# TODO: Git does not detect the conflict (one turns exe off, the other turns it on), and we do exactly the same.
+baseline rename-add-same-symlink A-B A B
 baseline rename-add-exe-bit-conflict A-B A B
 baseline remove-executable-mode A-B A B
 baseline simple side-1-3-without-conflict side1 side3

gix-merge/tests/fixtures/tree-baseline.sh

@@ -1,7 +1,7 @@
 use crate::tree::baseline::Deviation;
 use gix_diff::Rewrites;
 use gix_merge::commit::Options;
-use gix_merge::tree::TreatAsUnresolved;
+use gix_merge::tree::{treat_as_unresolved, TreatAsUnresolved};
 use gix_object::Write;
 use gix_worktree::stack::state::attributes;
 use std::path::Path;
@@ -119,7 +119,10 @@ fn run_baseline() -> crate::Result {
                 index
             }
         };
-        let conflicts_like_in_git = TreatAsUnresolved::Renames;
+        let conflicts_like_in_git = TreatAsUnresolved {
+            content_merge: treat_as_unresolved::ContentMerge::Markers,
+            tree_merge: treat_as_unresolved::TreeMerge::EvasiveRenames,
+        };
         let did_change = actual.index_changed_after_applying_conflicts(&mut actual_index, conflicts_like_in_git);
         actual_index.remove_entries(|_, _, e| e.flags.contains(gix_index::entry::Flags::REMOVE));
 
@@ -130,27 +133,15 @@ fn run_baseline() -> crate::Result {
             actual.conflicts,
             merge_info.conflicts
         );
-        // if case_name.starts_with("submodule-both-modify-A-B") {
-        if false {
-            assert!(
-                !did_change,
-                "{case_name}: We can't handle submodules, so there is no index change"
-            );
-            assert!(
-                actual.has_unresolved_conflicts(conflicts_like_in_git),
-                "{case_name}: submodules currently result in an unresolved (unknown) conflict"
-            );
-        } else {
-            assert_eq!(
-                did_change,
-                actual.has_unresolved_conflicts(conflicts_like_in_git),
-                "{case_name}: If there is any kind of conflict, the index should have been changed"
-            );
-        }
+        assert_eq!(
+            did_change,
+            actual.has_unresolved_conflicts(conflicts_like_in_git),
+            "{case_name}: If there is any kind of conflict, the index should have been changed"
+        );
     }
 
     assert_eq!(
-        actual_cases, 107,
+        actual_cases, 109,
         "BUG: update this number, and don't forget to remove a filter in the end"
     );
 
@@ -163,6 +154,7 @@ fn basic_merge_options() -> Options {
         use_first_merge_base: false,
         tree_merge: gix_merge::tree::Options {
             symlink_conflicts: None,
+            tree_conflicts: None,
             rewrites: Some(Rewrites {
                 copies: None,
                 percentage: Some(0.5),
@@ -172,7 +164,6 @@ fn basic_merge_options() -> Options {
             blob_merge_command_ctx: Default::default(),
             fail_on_conflict: None,
             marker_size_multiplier: 0,
-            allow_lossy_resolution: false,
         },
     }
 }