Merge pull request #1651 from GitoxideLabs/merge

tree-merge follow-up

Author: Byron

Diff for: README.md

@@ -46,7 +46,7 @@ What follows is a high-level list of features and those which are planned:
 * [x] blob-diff
 * [ ] merge
     - [x] blobs
-    - [ ] trees
+    - [x] trees
     - [ ] commits
 * [ ] rebase
 * [ ] commit

Diff for: gitoxide-core/Cargo.toml

@@ -49,7 +49,7 @@ serde = ["gix/serde", "dep:serde_json", "dep:serde", "bytesize/serde"]
 
 [dependencies]
 # deselect everything else (like "performance") as this should be controllable by the parent application.
-gix = { version = "^0.67.0", path = "../gix", default-features = false, features = ["blob-merge", "blob-diff", "revision", "mailmap", "excludes", "attributes", "worktree-mutation", "credentials", "interrupt", "status", "dirwalk"] }
+gix = { version = "^0.67.0", path = "../gix", default-features = false, features = ["merge", "blob-diff", "revision", "mailmap", "excludes", "attributes", "worktree-mutation", "credentials", "interrupt", "status", "dirwalk"] }
 gix-pack-for-configuration-only = { package = "gix-pack", version = "^0.54.0", path = "../gix-pack", default-features = false, features = ["pack-cache-lru-dynamic", "pack-cache-lru-static", "generate", "streaming-input"] }
 gix-transport-configuration-only = { package = "gix-transport", version = "^0.43.0", path = "../gix-transport", default-features = false }
 gix-archive-for-configuration-only = { package = "gix-archive", version = "^0.16.0", path = "../gix-archive", optional = true, features = ["tar", "tar_gz"] }

Diff for: gitoxide-core/src/repository/merge/tree.rs

@@ -2,7 +2,7 @@ use crate::OutputFormat;
 
 pub struct Options {
     pub format: OutputFormat,
-    pub resolve_content_merge: Option<gix::merge::blob::builtin_driver::text::Conflict>,
+    pub file_favor: Option<gix::merge::tree::FileFavor>,
     pub in_memory: bool,
 }
 
@@ -12,8 +12,6 @@ pub(super) mod function {
     use anyhow::{anyhow, bail, Context};
     use gix::bstr::BString;
     use gix::bstr::ByteSlice;
-    use gix::merge::blob::builtin_driver::binary;
-    use gix::merge::blob::builtin_driver::text::Conflict;
     use gix::merge::tree::UnresolvedConflict;
     use gix::prelude::Write;
 
@@ -29,7 +27,7 @@ pub(super) mod function {
         theirs: BString,
         Options {
             format,
-            resolve_content_merge,
+            file_favor,
             in_memory,
         }: Options,
     ) -> anyhow::Result<()> {
@@ -44,17 +42,7 @@ pub(super) mod function {
         let (ours_ref, ours_id) = refname_and_tree(&repo, ours)?;
         let (theirs_ref, theirs_id) = refname_and_tree(&repo, theirs)?;
 
-        let mut options = repo.tree_merge_options()?;
-        if let Some(resolve) = resolve_content_merge {
-            options.blob_merge.text.conflict = resolve;
-            options.blob_merge.resolve_binary_with = match resolve {
-                Conflict::Keep { .. } => None,
-                Conflict::ResolveWithOurs => Some(binary::ResolveWith::Ours),
-                Conflict::ResolveWithTheirs => Some(binary::ResolveWith::Theirs),
-                Conflict::ResolveWithUnion => None,
-            };
-        }
-
+        let options = repo.tree_merge_options()?.with_file_favor(file_favor);
         let base_id_str = base_id.to_string();
         let ours_id_str = ours_id.to_string();
         let theirs_id_str = theirs_id.to_string();
@@ -72,12 +60,15 @@ pub(super) mod function {
                 .map_or(theirs_id_str.as_str().into(), |n| n.as_bstr())
                 .into(),
         };
-        let mut res = repo.merge_trees(base_id, ours_id, theirs_id, labels, options)?;
+        let res = repo.merge_trees(base_id, ours_id, theirs_id, labels, options)?;
+        let has_conflicts = res.conflicts.is_empty();
+        let has_unresolved_conflicts = res.has_unresolved_conflicts(UnresolvedConflict::Renames);
         {
             let _span = gix::trace::detail!("Writing merged tree");
             let mut written = 0;
             let tree_id = res
                 .tree
+                .detach()
                 .write(|tree| {
                     written += 1;
                     repo.write(tree)
@@ -86,10 +77,10 @@ pub(super) mod function {
             writeln!(out, "{tree_id} (wrote {written} trees)")?;
         }
 
-        if !res.conflicts.is_empty() {
+        if !has_conflicts {
             writeln!(err, "{} possibly resolved conflicts", res.conflicts.len())?;
         }
-        if res.has_unresolved_conflicts(UnresolvedConflict::Renames) {
+        if has_unresolved_conflicts {
             bail!("Tree conflicted")
         }
         Ok(())

Diff for: gix-merge/src/lib.rs

@@ -1,3 +1,8 @@
+//! Provide facilities to merge *blobs*, *trees* and *commits*.
+//!
+//! * [blob-merges](blob) look at file content.
+//! * [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)]
 #![forbid(unsafe_code)]
 

Diff for: gix-merge/src/tree/function.rs

@@ -3,9 +3,7 @@ use crate::tree::utils::{
     to_components, track, unique_path_in_tree, ChangeList, ChangeListRef, PossibleConflict, TrackedChange, TreeNodes,
 };
 use crate::tree::ConflictMapping::{Original, Swapped};
-use crate::tree::{
-    Conflict, ConflictMapping, ContentMerge, Error, Options, Outcome, Resolution, ResolutionFailure, UnresolvedConflict,
-};
+use crate::tree::{Conflict, ConflictMapping, ContentMerge, Error, Options, Outcome, Resolution, ResolutionFailure};
 use bstr::{BString, ByteSlice};
 use gix_diff::tree::recorder::Location;
 use gix_diff::tree_with_rewrites::Change;
@@ -129,7 +127,7 @@ where
     let mut failed_on_first_conflict = false;
     let mut should_fail_on_conflict = |conflict: Conflict| -> bool {
         if let Some(how) = options.fail_on_conflict {
-            if conflict.resolution.is_err() || is_unresolved(std::slice::from_ref(&conflict), how) {
+            if conflict.resolution.is_err() || conflict.is_unresolved(how) {
                 failed_on_first_conflict = true;
             }
         }
@@ -1002,27 +1000,6 @@ where
     })
 }
 
-pub(super) fn is_unresolved(conflicts: &[Conflict], how: UnresolvedConflict) -> bool {
-    match how {
-        UnresolvedConflict::ConflictMarkers => conflicts.iter().any(|c| {
-            c.resolution.is_err()
-                || c.content_merge().map_or(false, |info| {
-                    matches!(info.resolution, crate::blob::Resolution::Conflict)
-                })
-        }),
-        UnresolvedConflict::Renames => conflicts.iter().any(|c| match &c.resolution {
-            Ok(success) => match success {
-                Resolution::SourceLocationAffectedByRename { .. }
-                | Resolution::OursModifiedTheirsRenamedAndChangedThenRename { .. } => true,
-                Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
-                    matches!(merged_blob.resolution, crate::blob::Resolution::Conflict)
-                }
-            },
-            Err(_failure) => true,
-        }),
-    }
-}
-
 fn involves_submodule(a: &EntryMode, b: &EntryMode) -> bool {
     a.is_commit() || b.is_commit()
 }

Diff for: gix-merge/src/tree/mod.rs

@@ -29,7 +29,7 @@ pub enum Error {
 /// The outcome produced by [`tree()`](crate::tree()).
 #[derive(Clone)]
 pub struct Outcome<'a> {
-    /// The ready-made (but unwritten) which is the *base* tree, including all non-conflicting changes, and the changes that had
+    /// The ready-made (but unwritten) *base* tree, including all non-conflicting changes, and the changes that had
     /// conflicts which could be resolved automatically.
     ///
     /// This means, if all of their changes were conflicting, this will be equivalent to the *base* tree.
@@ -61,7 +61,7 @@ impl Outcome<'_> {
     /// Return `true` if there is any conflict that would still need to be resolved as they would yield undesirable trees.
     /// 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)
+        self.conflicts.iter().any(|c| c.is_unresolved(how))
     }
 }
 
@@ -70,7 +70,7 @@ impl Outcome<'_> {
 #[derive(Debug, Clone)]
 pub struct Conflict {
     /// A record on how the conflict resolution succeeded with `Ok(_)` or failed with `Err(_)`.
-    /// In case of `Err(_)`, no write
+    /// Note that in case of `Err(_)`, edits may still have been made to the tree to aid resolution.
     /// On failure, one can examine `ours` and `theirs` to potentially find a custom solution.
     /// Note that the descriptions of resolutions or resolution failures may be swapped compared
     /// to the actual changes. This is due to changes like `modification|deletion` being treated the
@@ -81,13 +81,17 @@ pub struct Conflict {
     pub ours: Change,
     /// The change representing *their* side.
     pub theirs: Change,
+    /// Determine how to interpret the `ours` and `theirs` fields. This is used to implement [`Self::changes_in_resolution()`]
+    /// and [`Self::into_parts_by_resolution()`].
     map: ConflictMapping,
 }
 
-/// A utility to help define which side is what.
+/// A utility to help define which side is what in the [`Conflict`] type.
 #[derive(Debug, Clone, Copy)]
 enum ConflictMapping {
+    /// The sides are as described in the field documentation, i.e. `ours` is `ours`.
     Original,
+    /// The sides are the opposite of the field documentation. i.e. `ours` is `theirs` and `theirs` is `ours`.
     Swapped,
 }
 
@@ -104,6 +108,28 @@ impl ConflictMapping {
 }
 
 impl Conflict {
+    /// Return `true` if this instance is considered unresolved based on the criterion specified by `how`.
+    pub fn is_unresolved(&self, how: UnresolvedConflict) -> bool {
+        match how {
+            UnresolvedConflict::ConflictMarkers => {
+                self.resolution.is_err()
+                    || self.content_merge().map_or(false, |info| {
+                        matches!(info.resolution, crate::blob::Resolution::Conflict)
+                    })
+            }
+            UnresolvedConflict::Renames => match &self.resolution {
+                Ok(success) => match success {
+                    Resolution::SourceLocationAffectedByRename { .. }
+                    | Resolution::OursModifiedTheirsRenamedAndChangedThenRename { .. } => true,
+                    Resolution::OursModifiedTheirsModifiedThenBlobContentMerge { merged_blob } => {
+                        matches!(merged_blob.resolution, crate::blob::Resolution::Conflict)
+                    }
+                },
+                Err(_failure) => true,
+            },
+        }
+    }
+
     /// Returns the changes of fields `ours` and `theirs` so they match their description in the
     /// [`Resolution`] or [`ResolutionFailure`] respectively.
     /// Without this, the sides may appear swapped as `ours|theirs` is treated the same as `theirs/ours`
@@ -236,11 +262,8 @@ pub struct Options {
     /// The context to use when invoking merge-drivers.
     pub blob_merge_command_ctx: gix_command::Context,
     /// 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.
+    /// This is useful to see if there is any conflict, without performing the whole operation, something
+    /// that can be very relevant during merges that would cause a lot of blob-diffs.
     pub fail_on_conflict: Option<UnresolvedConflict>,
     /// This value also affects the size of merge-conflict markers, to allow differentiating
     /// merge conflicts on each level, for any value greater than 0, with values `N` causing `N*2`

Diff for: gix/Cargo.toml

@@ -65,11 +65,11 @@ extras = [
     "interrupt",
     "status",
     "dirwalk",
-    "blob-merge"
 ]
 
 ## A collection of features that need a larger MSRV, and thus are disabled by default.
-need-more-recent-msrv = ["tree-editor"]
+## * `blob-merge` should be in extras, but needs `tree-editor` for convenience.
+need-more-recent-msrv = ["merge", "tree-editor"]
 
 ## Various progress-related features that improve the look of progress message units.
 comfort = [
@@ -139,7 +139,7 @@ revparse-regex = ["regex", "revision"]
 blob-diff = ["gix-diff/blob", "attributes"]
 
 ## Add functions to specifically merge files, using the standard three-way merge that git offers.
-blob-merge = ["blob-diff", "dep:gix-merge", "attributes"]
+merge = ["tree-editor", "blob-diff", "dep:gix-merge", "attributes"]
 
 ## Make it possible to turn a tree into a stream of bytes, which can be decoded to entries and turned into various other formats.
 worktree-stream = ["gix-worktree-stream", "attributes"]
@@ -399,7 +399,7 @@ document-features = { version = "0.2.0", optional = true }
 
 [dev-dependencies]
 # For additional features that aren't enabled by default due to MSRV
-gix = { path = ".", default-features = false, features = ["tree-editor"] }
+gix = { path = ".", default-features = false, features = ["need-more-recent-msrv"] }
 pretty_assertions = "1.4.0"
 gix-testtools = { path = "../tests/tools" }
 is_ci = "1.1.1"

Diff for: gix/src/config/cache/access.rs

@@ -100,7 +100,7 @@ impl Cache {
         Ok(out)
     }
 
-    #[cfg(feature = "blob-merge")]
+    #[cfg(feature = "merge")]
     pub(crate) fn merge_drivers(&self) -> Result<Vec<gix_merge::blob::Driver>, config::merge::drivers::Error> {
         let mut out = Vec::<gix_merge::blob::Driver>::new();
         for section in self
@@ -136,7 +136,7 @@ impl Cache {
         Ok(out)
     }
 
-    #[cfg(feature = "blob-merge")]
+    #[cfg(feature = "merge")]
     pub(crate) fn merge_pipeline_options(
         &self,
     ) -> Result<gix_merge::blob::pipeline::Options, config::merge::pipeline_options::Error> {

Diff for: gix/src/config/tree/sections/merge.rs

@@ -15,7 +15,7 @@ impl Merge {
         "The limit is actually squared, so 1000 stands for up to 1 million diffs if fuzzy rename tracking is enabled",
     );
     /// The `merge.renames` key.
-    #[cfg(feature = "blob-merge")]
+    #[cfg(feature = "merge")]
     pub const RENAMES: super::diff::Renames = super::diff::Renames::new_renames("renames", &config::Tree::MERGE);
     /// The `merge.renormalize` key
     pub const RENORMALIZE: keys::Boolean = keys::Boolean::new_boolean("renormalize", &Tree::MERGE);
@@ -31,7 +31,7 @@ impl Merge {
     pub const DRIVER_RECURSIVE: keys::String = keys::String::new_string("recursive", &config::Tree::MERGE)
         .with_subsection_requirement(Some(SubSectionRequirement::Parameter("driver")));
     /// The `merge.conflictStyle` key.
-    #[cfg(feature = "blob-merge")]
+    #[cfg(feature = "merge")]
     pub const CONFLICT_STYLE: ConflictStyle =
         ConflictStyle::new_with_validate("conflictStyle", &config::Tree::MERGE, validate::ConflictStyle);
 }
@@ -44,24 +44,24 @@ impl Section for Merge {
     fn keys(&self) -> &[&dyn Key] {
         &[
             &Self::RENAME_LIMIT,
-            #[cfg(feature = "blob-merge")]
+            #[cfg(feature = "merge")]
             &Self::RENAMES,
             &Self::RENORMALIZE,
             &Self::DEFAULT,
             &Self::DRIVER_NAME,
             &Self::DRIVER_COMMAND,
             &Self::DRIVER_RECURSIVE,
-            #[cfg(feature = "blob-merge")]
+            #[cfg(feature = "merge")]
             &Self::CONFLICT_STYLE,
         ]
     }
 }
 
 /// The `merge.conflictStyle` key.
-#[cfg(feature = "blob-merge")]
+#[cfg(feature = "merge")]
 pub type ConflictStyle = keys::Any<validate::ConflictStyle>;
 
-#[cfg(feature = "blob-merge")]
+#[cfg(feature = "merge")]
 mod conflict_style {
     use crate::{bstr::BStr, config, config::tree::sections::merge::ConflictStyle};
     use gix_merge::blob::builtin_driver::text;
@@ -87,7 +87,7 @@ mod conflict_style {
     }
 }
 
-#[cfg(feature = "blob-merge")]
+#[cfg(feature = "merge")]
 mod validate {
     use crate::{
         bstr::BStr,

Diff for: gix/src/lib.rs

@@ -120,8 +120,6 @@ pub use gix_ignore as ignore;
 #[cfg(feature = "index")]
 pub use gix_index as index;
 pub use gix_lock as lock;
-#[cfg(feature = "blob-merge")]
-pub use gix_merge as merge;
 #[cfg(feature = "credentials")]
 pub use gix_negotiate as negotiate;
 pub use gix_object as objs;
@@ -204,6 +202,10 @@ pub mod push;
 ///
 pub mod diff;
 
+///
+#[cfg(feature = "merge")]
+pub mod merge;
+
 /// See [`ThreadSafeRepository::discover()`], but returns a [`Repository`] instead.
 ///
 /// # Note