Add more of our own tests to trigger the remaining code

This also allows for deviations in case the Git result is worse.

Author: Byron

Cargo.lock

@@ -338,15 +338,20 @@ Check out the [performance discussion][gix-diff-performance] as well.
 
 ### gix-merge
 
-* [x] three-way merge analysis of **blobs** with choice of how to resolve conflicts
+* [x] three-way content-merge analysis of **blobs** with choice of how to resolve conflicts
+    - [x] respect git attributes and drivers.
     - [ ] choose how to resolve conflicts on the data-structure
+    - [ ] more efficient handling of paths with `merge=binary` attributes (do not load them into memory)
     - [x] produce a new blob based on data-structure containing possible resolutions
         - [x] `merge` style
         - [x] `diff3` style
         - [x] `zdiff` style
     - [ ] various newlines-related options during the merge (see https://git-scm.com/docs/git-merge#Documentation/git-merge.txt-ignore-space-change).
     - [ ] a way to control inter-hunk merging based on proximity (maybe via `gix-diff` feature which could use the same)
-* [ ] diff-heuristics match Git perfectly
+* [x] **tree**-diff-heuristics match Git for its test-cases
+    - [ ] a way to generate an index with stages 
+        - *currently the data it provides won't generate index entries, and possibly can't be used for it yet*
+    - [ ] submodule merges (*right now they count as conflicts if they differ*)
 * [x] API documentation
     * [ ] Examples
 

crate-status.md

@@ -43,31 +43,25 @@ pub struct Outcome<'a> {
     pub failed_on_first_unresolved_conflict: bool,
 }
 
+/// Determine what should be considered an unresolved conflict.
+///
+/// Note that no matter which variant, [conflicts](Conflict) with [resolution failure](`ResolutionFailure`)
+/// will always be unresolved.
+///
+/// However, those whose resolution was
+#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
+pub enum UnresolvedConflict {
+    /// Only consider content merges with conflict markers as unresolved.
+    ConflictMarkers,
+    /// Whenever there was any rename, or conflict markers, it is unresolved.
+    Renames,
+}
+
 impl Outcome<'_> {
     /// Return `true` if there is any conflict that would still need to be resolved as they would yield undesirable trees.
-    /// Note that this interpretation of conflicts and their resolution, see
-    /// [`has_unresolved_conflicts_strict`](Self::has_unresolved_conflicts_strict).
-    pub fn has_unresolved_conflicts(&self) -> bool {
-        self.conflicts.iter().any(|c| {
-            c.resolution.is_err()
-                || c.content_merge().map_or(false, |info| {
-                    matches!(info.resolution, crate::blob::Resolution::Conflict)
-                })
-        })
-    }
-
-    /// Return `true` only if there was any (even resolved) conflict in the tree structure, or if there are still conflict markers.
-    pub fn has_unresolved_conflicts_strict(&self) -> bool {
-        self.conflicts.iter().any(|c| match &c.resolution {
-            Ok(success) => match success {
-                Resolution::OursAddedTheirsAddedTypeMismatch { .. }
-                | Resolution::OursModifiedTheirsRenamedAndChangedThenRename { .. } => true,
-                Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
-                    matches!(merged_blob.resolution, crate::blob::Resolution::Conflict)
-                }
-            },
-            Err(_failure) => true,
-        })
+    /// This is based on `how` to determine what should be considered unresolved.
+    pub fn has_unresolved_conflicts(&self, how: UnresolvedConflict) -> bool {
+        function::is_unresolved(&self.conflicts, how)
     }
 }
 
@@ -135,14 +129,19 @@ impl Conflict {
     pub fn content_merge(&self) -> Option<ContentMerge> {
         match &self.resolution {
             Ok(success) => match success {
-                Resolution::OursAddedTheirsAddedTypeMismatch { .. } => None,
+                Resolution::SourceLocationAffectedByRename { .. } => None,
                 Resolution::OursModifiedTheirsRenamedAndChangedThenRename { merged_blob, .. } => *merged_blob,
                 Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => Some(*merged_blob),
             },
             Err(failure) => match failure {
-                ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed {
-                    renamed_path_to_modified_blob: _,
+                ResolutionFailure::OursRenamedTheirsRenamedDifferently { merged_blob } => *merged_blob,
+                ResolutionFailure::Unknown
+                | ResolutionFailure::OursModifiedTheirsDeleted
+                | ResolutionFailure::OursModifiedTheirsRenamedTypeMismatch
+                | ResolutionFailure::OursModifiedTheirsDirectoryThenOursRenamed {
+                    renamed_unique_path_to_modified_blob: _,
                 }
+                | ResolutionFailure::OursAddedTheirsAddedTypeMismatch { .. }
                 | ResolutionFailure::OursDeletedTheirsRenamed => None,
             },
         }
@@ -154,11 +153,20 @@ impl Conflict {
 /// Note that all resolutions are side-agnostic, so *ours* could also have been *theirs* and vice versa.
 #[derive(Debug, Clone)]
 pub enum Resolution {
+    /// *ours* had a renamed directory and *theirs* made a change in the now renamed directory.
+    /// We moved that change into its location.
+    SourceLocationAffectedByRename {
+        /// The repository-relative path to the location that the change ended up in after
+        /// being affected by a renamed directory.
+        final_location: BString,
+    },
     /// *ours* was a modified blob and *theirs* renamed that blob.
     /// We moved the changed blob from *ours* to its new location, and merged it successfully.
     /// If this is a `copy`, the source of the copy was set to be the changed blob as well so both match.
     OursModifiedTheirsRenamedAndChangedThenRename {
-        /// If not `None`, the content of the involved blob had to be merged.
+        /// If one side added the executable bit, we always add it in the merged result.
+        merged_mode: Option<gix_object::tree::EntryMode>,
+        /// If `Some(…)`, the content of the involved blob had to be merged.
         merged_blob: Option<ContentMerge>,
         /// The repository relative path to the location the blob finally ended up in.
         /// It's `Some()` only if *they* rewrote the blob into a directory which *we* renamed on *our* side.
@@ -172,11 +180,38 @@ pub enum Resolution {
         /// The outcome of the content merge.
         merged_blob: ContentMerge,
     },
+}
+
+/// Describes of a conflict involving *our* change and *their* failed to be resolved.
+#[derive(Debug, Clone)]
+pub enum ResolutionFailure {
+    /// *ours* was renamed, but *theirs* was renamed differently. Both versions will be present in the tree,
+    OursRenamedTheirsRenamedDifferently {
+        /// If `Some(…)`, the content of the involved blob had to be merged.
+        merged_blob: Option<ContentMerge>,
+    },
+    /// *ours* was modified, but *theirs* was turned into a directory, so *ours* was renamed to a non-conflicting path.
+    OursModifiedTheirsDirectoryThenOursRenamed {
+        /// 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* 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 {
-        /// The location at which *their* state was placed to resolve the name and type clash.
-        their_final_location: BString,
+        /// The location at which *their* state was placed to resolve the name and type clash, named to indicate
+        /// where the entry is coming from.
+        their_unique_location: BString,
     },
+    /// *ours* was modified, and they renamed the same file, but there is also a non-mergable type-change.
+    /// Here we keep both versions of the file.
+    OursModifiedTheirsRenamedTypeMismatch,
+    /// *ours* was deleted, but *theirs* was renamed.
+    OursDeletedTheirsRenamed,
+    /// *ours* was modified and *theirs* was deleted. We keep the modified one and ignore the deletion.
+    OursModifiedTheirsDeleted,
+    /// *ours* and *theirs* are in an untested state so it can't be handled yet, and is considered a conflict
+    /// without adding our *or* their side to the resulting tree.
+    Unknown,
 }
 
 /// Information about a blob content merge for use in a [`Resolution`].
@@ -190,18 +225,6 @@ pub struct ContentMerge {
     pub resolution: crate::blob::Resolution,
 }
 
-/// Describes of a conflict involving *our* change and *their* failed to be resolved.
-#[derive(Debug, Clone)]
-pub enum ResolutionFailure {
-    /// *ours* was modified, but *theirs* was turned into a directory, so *ours* was renamed to a non-conflicting path.
-    OursModifiedTheirsDirectoryThenOursRenamed {
-        /// The path at which `ours` can be found in the tree - it's in the same directory that it was in before.
-        renamed_path_to_modified_blob: BString,
-    },
-    /// *ours* was deleted, but *theirs* was renamed.
-    OursDeletedTheirsRenamed,
-}
-
 /// A way to configure [`tree()`](crate::tree()).
 #[derive(Default, Debug, Clone)]
 pub struct Options {
@@ -211,14 +234,19 @@ pub struct Options {
     pub blob_merge: crate::blob::platform::merge::Options,
     /// The context to use when invoking merge-drivers.
     pub blob_merge_command_ctx: gix_command::Context,
-    /// If `true`, the first conflict will cause the entire
-    pub fail_on_conflict: bool,
+    /// If `Some(what-is-unresolved)`, the first unresolved conflict will cause the entire merge to stop.
+    /// This is useful to see if there is any conflict, without performing the whole operation.
+    // TODO: Maybe remove this if the cost of figuring out conflicts is so low - after all, the data structures
+    //       and initial diff is the expensive thing right now, which are always done upfront.
+    //       However, this could change once we know do everything during the traversal, which probably doesn't work
+    //       without caching stuff and is too complicated to actually do.
+    pub fail_on_conflict: Option<UnresolvedConflict>,
     /// If greater than 0, each level indicates another merge-of-merge. This can be greater than
     /// 0 when merging merge-bases, which are merged like a pyramid.
     /// This value also affects the size of merge-conflict markers, to allow differentiating
     /// merge conflicts on each level.
     pub call_depth: u8,
-    // TODO(Perf) add a flag to allow parallelizing the tree-diff itself.
+    // TODO(Perf) add a flag to allow parallelizing the tree-diff itself once there was some profiling.
 }
 
 pub(super) mod function;