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

@@ -22,6 +22,8 @@ pub enum Error {
     BlobMerge(#[from] crate::blob::platform::merge::Error),
     #[error("Failed to write merged blob content as blob to the object database")]
     WriteBlobToOdb(Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("The merge was performed, but the binary merge result couldn't be selected as it wasn't found")]
+    MergeResourceNotFound,
 }
 
 /// The outcome produced by [`tree()`](crate::tree()).
@@ -43,31 +45,23 @@ 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.
+#[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;

gix-merge/src/tree/function.rs

@@ -1,3 +1,10 @@
+//! ## About `debug_assert!()
+//!
+//! The idea is to have code that won't panic in production. Thus, if in production that assertion would fail,
+//! we will rather let the code run and hope it will either be correct enough or fail in more graceful ways later.
+//!
+//! Once such a case becomes a bug and is reproduced in testing, the debug-assertion will kick in and hopefully
+//! contribute to finding a fix faster.
 use crate::blob::ResourceKind;
 use crate::tree::{Conflict, ConflictMapping, Error, Options, Resolution, ResolutionFailure};
 use bstr::ByteSlice;
@@ -6,27 +13,23 @@ use gix_diff::tree_with_rewrites::{Change, ChangeRef};
 use gix_hash::ObjectId;
 use gix_object::tree;
 use gix_object::tree::EntryMode;
-use std::borrow::Cow;
 use std::collections::HashMap;
 
 /// Assuming that `their_location` is the destination of *their* rewrite, check if *it* passes
 /// 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<'a>(
+pub fn possibly_rewritten_location(
     check_tree: &mut TreeNodes,
-    their_location: &'a BStr,
+    their_location: &BStr,
     our_changes: &ChangeListRef,
-) -> Cow<'a, BStr> {
-    check_tree
-        .check_conflict(their_location)
-        .and_then(|pc| match pc {
-            PossibleConflict::PassedRewrittenDirectory { change_idx } => {
-                let passed_change = &our_changes[change_idx];
-                rewrite_location_with_renamed_directory(their_location, &passed_change.inner).map(Cow::Owned)
-            }
-            _ => None,
-        })
-        .unwrap_or(Cow::Borrowed(their_location))
+) -> Option<BString> {
+    check_tree.check_conflict(their_location).and_then(|pc| match pc {
+        PossibleConflict::PassedRewrittenDirectory { change_idx } => {
+            let passed_change = &our_changes[change_idx];
+            rewrite_location_with_renamed_directory(their_location, &passed_change.inner)
+        }
+        _ => None,
+    })
 }
 
 pub fn rewrite_location_with_renamed_directory(their_location: &BStr, passed_change: &Change) -> Option<BString> {
@@ -140,9 +143,11 @@ where
     };
     let prep = blob_merge.prepare_merge(objects, with_extra_markers(options, extra_markers))?;
     let (pick, resolution) = prep.merge(buf, labels, &options.blob_merge_command_ctx)?;
-    // TODO: properly handle binary/other buffers. This API is troublesome, fix it
-    let merged_content = prep.buffer_by_pick(pick).unwrap_or(buf);
-    let merged_blob_id = write_blob_to_odb(merged_content).map_err(|err| Error::WriteBlobToOdb(err.into()))?;
+
+    let merged_blob_id = prep
+        .id_by_pick(pick, buf, write_blob_to_odb)
+        .map_err(|err| Error::WriteBlobToOdb(err.into()))?
+        .ok_or(Error::MergeResourceNotFound)?;
     Ok((merged_blob_id, resolution))
 }
 
@@ -161,10 +166,17 @@ pub struct TrackedChange {
     pub inner: Change,
     /// If `true`, this change counts as written to the tree using a [`tree::Editor`].
     pub was_written: bool,
-    /// If `true`, this change must be placed into the tree before handling it.
-    /// This makes sure that new changes aren't visible too early, which would mean the algorihtm
+    /// If `Some(ours_idx_to_ignore)`, this change must be placed into the tree before handling it.
+    /// This makes sure that new changes aren't visible too early, which would mean the algorithm
     /// knows things too early which can be misleading.
-    pub needs_tree_insertion: bool,
+    /// The `ours_idx_to_ignore` assures that the same rewrite won't be used as matching side, which
+    /// would lead to strange effects. Only set if it's a rewrite though.
+    pub needs_tree_insertion: Option<Option<usize>>,
+    /// A new `(location, change_idx)` pair for the change that can happen if the location is touching a rewrite in a parent
+    /// directory, but otherwise doesn't have a match. This means we shall redo the operation but with
+    /// the changed path.
+    /// The second tuple entry `change_idx` is the change-idx we passed over, which refers to the other side that interfered.
+    pub rewritten_location: Option<(BString, usize)>,
 }
 
 pub type ChangeList = Vec<TrackedChange>;
@@ -179,6 +191,7 @@ pub fn track(change: ChangeRef<'_>, changes: &mut ChangeList) {
     if change.entry_mode().is_tree() && matches!(change, ChangeRef::Modification { .. }) {
         return;
     }
+    let is_tree = change.entry_mode().is_tree();
     changes.push(TrackedChange {
         inner: match change.into_owned() {
             Change::Rewrite {
@@ -196,22 +209,24 @@ pub fn track(change: ChangeRef<'_>, changes: &mut ChangeList) {
             },
             other => other,
         },
-        was_written: false,
-        needs_tree_insertion: false,
+        was_written: is_tree,
+        needs_tree_insertion: None,
+        rewritten_location: None,
     });
 }
 
 /// Unconditionally apply `change` to `editor`.
-pub fn apply_change(editor: &mut tree::Editor<'_>, change: &Change) -> Result<(), tree::editor::Error> {
+pub fn apply_change(
+    editor: &mut tree::Editor<'_>,
+    change: &Change,
+    alternative_location: Option<&BString>,
+) -> Result<(), tree::editor::Error> {
     use to_components_bstring_ref as to_components;
-    // TODO(performance): we could apply tree changes if they are renames-by-identity, and then save the
-    //                    work needed to set all the children recursively. This would require more elaborate
-    //                    tracking though.
     if change.entry_mode().is_tree() {
         return Ok(());
     }
 
-    match change {
+    let (location, mode, id) = match change {
         Change::Addition {
             location,
             entry_mode,
@@ -223,8 +238,11 @@ pub fn apply_change(editor: &mut tree::Editor<'_>, change: &Change) -> Result<()
             entry_mode,
             id,
             ..
-        } => editor.upsert(to_components(location), entry_mode.kind(), *id)?,
-        Change::Deletion { location, .. } => editor.remove(to_components(location))?,
+        } => (location, entry_mode, id),
+        Change::Deletion { location, .. } => {
+            editor.remove(to_components(alternative_location.unwrap_or(location)))?;
+            return Ok(());
+        }
         Change::Rewrite {
             source_location,
             entry_mode,
@@ -236,17 +254,21 @@ pub fn apply_change(editor: &mut tree::Editor<'_>, change: &Change) -> Result<()
             if !*copy {
                 editor.remove(to_components(source_location))?;
             }
-            editor.upsert(to_components(location), entry_mode.kind(), *id)?
+            (location, entry_mode, id)
         }
     };
+
+    editor.upsert(
+        to_components(alternative_location.unwrap_or(location)),
+        mode.kind(),
+        *id,
+    )?;
     Ok(())
 }
 
 /// A potential conflict that needs to be checked. It comes in several varieties and always happens
 /// if paths overlap in some way between *theirs* and *ours*.
 #[derive(Debug)]
-// TODO: remove this when all fields are used.
-#[allow(dead_code)]
 pub enum PossibleConflict {
     /// *our* changes have a tree here, but *they* place a non-tree or edit an existing item (that we removed).
     TreeToNonTree {
@@ -270,7 +292,7 @@ pub enum PossibleConflict {
 }
 
 impl PossibleConflict {
-    fn change_idx(&self) -> Option<usize> {
+    pub(super) fn change_idx(&self) -> Option<usize> {
         match self {
             PossibleConflict::TreeToNonTree { change_idx, .. } | PossibleConflict::NonTreeToTree { change_idx, .. } => {
                 *change_idx
@@ -483,7 +505,6 @@ impl TreeNodes {
             }
         }
 
-        // TODO: This overwrites nodes, possibly rewrites, is that OK or can there be issues later?
         debug_assert!(
             !matches!(new_change, Change::Rewrite { .. }),
             "BUG: we thought we wouldn't do that current.location is related?"