Add support for multiple merge-bases (squash with prior one)

Byron · Byron · commit f95bbfe5f49e · 2024-10-30T21:36:02.000+01:00
diff --git a/gix-merge/src/commit.rs b/gix-merge/src/commit.rs
@@ -30,9 +30,17 @@ pub struct Options {
     pub use_first_merge_base: bool,
 }
 
+/// The result of [`commit()`](crate::commit()).
+#[derive(Clone)]
+pub struct Outcome<'a> {
+    /// The outcome of the actual tree-merge.
+    pub tree_merge: crate::tree::Outcome<'a>,
+}
+
 pub(super) mod function {
     use crate::commit::{Error, Options};
     use gix_object::FindExt;
+    use std::borrow::Cow;
 
     /// Like [`tree()`](crate::tree()), but it takes only two commits, `our_commit` and `their_commit` to automatically
     /// compute the merge-bases among them.
@@ -47,45 +55,47 @@ pub(super) mod function {
     /// The `graph` is used to find the merge-base between `our_commit` and `their_commit`, and can also act as cache
     /// to speed up subsequent merge-base queries.
     ///
+    /// Use `abbreviate_hash(id)` to shorten the given `id` according to standard git shortening rules. It's used in case
+    /// the ancestor-label isn't explicitly set so that the merge base label becomes the shortened `id`.
+    ///
     /// ### Performance
     ///
     /// Note that `objects` *should* have an object cache to greatly accelerate tree-retrieval.
     #[allow(clippy::too_many_arguments)]
     pub fn commit<'objects, E>(
         our_commit: gix_hash::ObjectId,
         their_commit: gix_hash::ObjectId,
-        mut labels: crate::blob::builtin_driver::text::Labels<'_>,
+        labels: crate::blob::builtin_driver::text::Labels<'_>,
         graph: &mut gix_revwalk::Graph<'_, '_, gix_revwalk::graph::Commit<gix_revision::merge_base::Flags>>,
         diff_resource_cache: &mut gix_diff::blob::Platform,
         blob_merge: &mut crate::blob::Platform,
         objects: &'objects impl gix_object::FindObjectOrHeader,
         write_blob_to_odb: impl FnMut(&[u8]) -> Result<gix_hash::ObjectId, E>,
+        abbreviate_hash: impl FnOnce(&gix_hash::oid) -> String,
         options: Options,
     ) -> Result<crate::tree::Outcome<'objects>, Error>
     where
         E: Into<Box<dyn std::error::Error + Send + Sync + 'static>>,
     {
         let merge_bases_commit_ids = gix_revision::merge_base(our_commit, &[their_commit], graph)?;
         let mut state = gix_diff::tree::State::default();
-        let (merge_base_tree_id, ancestor_name) = match merge_bases_commit_ids {
-            Some(base_commit) if base_commit.len() == 1 => {
-                (objects.find_commit(&base_commit[0], &mut state.buf1)?.tree(), None)
-            }
+        let (merge_base_tree_id, ancestor_name): (_, Cow<'_, str>) = match merge_bases_commit_ids {
+            Some(base_commit) if base_commit.len() == 1 => (
+                objects.find_commit(&base_commit[0], &mut state.buf1)?.tree(),
+                abbreviate_hash(&base_commit[0]).into(),
+            ),
             Some(base_commits) => {
                 let virtual_base_tree = if options.use_first_merge_base {
                     let first = *base_commits.first().expect("TODO: merge multiple bases into one");
                     objects.find_commit(&first, &mut state.buf1)?.tree()
                 } else {
                     todo!("merge multiple merge bases")
                 };
-                (virtual_base_tree, Some("merged common ancestors".into()))
+                (virtual_base_tree, "merged common ancestors".into())
             }
             None => {
                 if options.allow_missing_merge_base {
-                    (
-                        gix_hash::ObjectId::empty_tree(our_commit.kind()),
-                        Some("empty tree".into()),
-                    )
+                    (gix_hash::ObjectId::empty_tree(our_commit.kind()), "empty tree".into())
                 } else {
                     return Err(Error::NoMergeBase {
                         our_commit_id: our_commit,
@@ -94,8 +104,10 @@ pub(super) mod function {
                 }
             }
         };
+
+        let mut labels = labels; // TODO(borrowchk): this re-assignment shouldn't be needed.
         if labels.ancestor.is_none() {
-            labels.ancestor = ancestor_name;
+            labels.ancestor = Some(ancestor_name.as_ref().into());
         }
 
         let our_tree_id = objects.find_commit(&our_commit, &mut state.buf1)?.tree();
diff --git a/gix-merge/src/tree/utils.rs b/gix-merge/src/tree/utils.rs
@@ -132,11 +132,11 @@ where
     let labels = if our_location == their_location {
         labels
     } else {
-        ancestor = previous_location.as_bstr();
+        ancestor = labels.ancestor.map(|side| combined(side, previous_location));
         current = labels.current.map(|side| combined(side, our_location));
         other = labels.other.map(|side| combined(side, their_location));
         crate::blob::builtin_driver::text::Labels {
-            ancestor: Some(ancestor),
+            ancestor: ancestor.as_ref().map(|n| n.as_bstr()),
             current: current.as_ref().map(|n| n.as_bstr()),
             other: other.as_ref().map(|n| n.as_bstr()),
         }
diff --git a/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar b/gix-merge/tests/fixtures/generated-archives/tree-baseline.tar
diff --git a/gix-merge/tests/fixtures/tree-baseline.sh b/gix-merge/tests/fixtures/tree-baseline.sh
@@ -43,18 +43,25 @@ function baseline () (
   local our_commit_id
   local their_commit_id
 
+  local conflict_style="merge"
+   if [[ "$output_name" == *-merge ]]; then
+       conflict_style="merge"
+   elif [[ "$output_name" == *-diff3 ]]; then
+       conflict_style="diff3"
+   fi
+
   our_commit_id="$(git rev-parse "$our_committish")"
   their_commit_id="$(git rev-parse "$their_committish")"
   local maybe_expected_tree="$(git rev-parse expected^{tree})"
 
   local merge_info="${output_name}.merge-info"
-  git merge-tree -z --write-tree --allow-unrelated-histories "$our_committish" "$their_committish" > "$merge_info" || :
-  echo "$dir" "$our_commit_id" "$our_committish" "$their_commit_id" "$their_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases
+  git -c merge.conflictStyle=$conflict_style merge-tree -z --write-tree --allow-unrelated-histories "$our_committish" "$their_committish" > "$merge_info" || :
+  echo "$dir" "$conflict_style" "$our_commit_id" "$our_committish" "$their_commit_id" "$their_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases
 
   if [[ "$one_side" != "no-reverse" ]]; then
     local merge_info="${output_name}-reversed.merge-info"
-    git merge-tree -z --write-tree --allow-unrelated-histories "$their_committish" "$our_committish" > "$merge_info" || :
-    echo "$dir" "$their_commit_id" "$their_committish" "$our_commit_id" "$our_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases
+    git -c merge.conflictStyle=$conflict_style merge-tree -z --write-tree --allow-unrelated-histories "$their_committish" "$our_committish" > "$merge_info" || :
+    echo "$dir" "$conflict_style" "$their_commit_id" "$their_committish" "$our_commit_id" "$our_committish" "$merge_info" "$maybe_expected_tree" "$opt_deviation_message" >> ../baseline.cases
   fi
 )
 
@@ -677,38 +684,64 @@ git init big-file-merge
   git commit -am "a normal but conflicting file change"
 )
 
+git init no-merge-base
+(cd no-merge-base
+  git checkout -b A
+  echo "A" >content && git add . && git commit -m "content A"
+
+  git checkout --orphan B
+  echo "B" >content && git add . && git commit -m "content B"
+
+  git checkout -b expectation
+)
+
+
 baseline simple side-1-3-without-conflict side1 side3
+baseline simple side-1-3-without-conflict-diff3 side1 side3
 baseline simple side-1-2-various-conflicts side1 side2
+baseline simple side-1-2-various-conflicts-diff3 side1 side2
 baseline simple single-content-conflict side1 side4
+baseline simple single-content-conflict-diff3 side1 side4
 baseline simple tweak1-side2 tweak1 side2
+#baseline simple tweak1-side2-diff3 tweak1 side2 # TODO: fix failure
 baseline simple side-1-unrelated side1 unrelated
+baseline simple side-1-unrelated-diff3 side1 unrelated
 baseline rename-delete A-B A B
 baseline rename-delete A-similar A A
 baseline rename-delete B-similar B B
 baseline rename-add A-B A B
+baseline rename-add A-B-diff3 A B
 baseline rename-add-symlink A-B A B
+baseline rename-add-symlink A-B-diff3 A B
 baseline rename-rename-plus-content A-B A B
+baseline rename-rename-plus-content A-B-diff3 A B
 baseline rename-add-delete A-B A B
 baseline rename-rename-delete-delete A-B A B
 baseline super-1 A-B A B
+#baseline super-1 A-B-diff3 A B # TODO: fix failure
 baseline super-2 A-B A B
+baseline super-2 A-B-diff3 A B
 
 baseline rename-within-rename A-B-deviates A B "Git doesn't detect the rename-nesting so there is duplication - we achieve the optimal result"
-baseline rename-within-rename-2 A-B-deviates A B "TBD"
+baseline rename-within-rename-2 A-B-deviates A B "TBD: Right, something is different documentation was forgotten :/"
 baseline conflicting-rename A-B A B
 baseline conflicting-rename-2 A-B A B
 baseline conflicting-rename-complex A-B A B "Git has different rename tracking which is why a-renamed/w disappears - it's still close enough"
 
 baseline same-rename-different-mode A-B A B "Git works for the A/B case, but for B/A it forgets to set the executable bit"
+baseline same-rename-different-mode A-B-diff3 A B "Git works for the A/B case, but for B/A it forgets to set the executable bit"
 baseline renamed-symlink-with-conflict A-B A B
 baseline added-file-changed-content-and-mode A-B A B "We improve on executable bit handling, but loose on diff quality as we are definitely missing some tweaks"
 
 baseline type-change-and-renamed A-B A B
 baseline change-and-delete A-B A B
 baseline submodule-both-modify A-B A B "We can't handle submodules yet and just mark them as conflicting. This is planned to be improved."
 baseline both-modify-union-attr A-B A B
+baseline both-modify-union-attr A-B-diff3 A B
 baseline both-modify-binary A-B A B
 baseline both-modify-binary A-B A B
 baseline both-modify-file-with-binary-attr A-B A B
 baseline big-file-merge A-B A B "Git actually ignores core.bigFileThreshold during merging and tries a normal merge (or binary one) anyway. We don't ignore it and treat big files like binary files" \
                                 no-reverse
+baseline no-merge-base A-B A B
+baseline no-merge-base A-B-diff3 A B
diff --git a/gix-merge/tests/merge/tree/baseline.rs b/gix-merge/tests/merge/tree/baseline.rs
@@ -1,5 +1,6 @@
 use bstr::{BStr, ByteSlice};
 use gix_hash::ObjectId;
+use gix_merge::blob::builtin_driver::text::ConflictStyle;
 use gix_object::tree::EntryMode;
 use gix_object::FindExt;
 use std::path::{Path, PathBuf};
@@ -87,6 +88,7 @@ pub struct MergeInfo {
 
 pub struct Expectation {
     pub root: PathBuf,
+    pub conflict_style: gix_merge::blob::builtin_driver::text::ConflictStyle,
     pub odb: gix_odb::memory::Proxy<gix_odb::Handle>,
     pub our_commit_id: gix_hash::ObjectId,
     pub our_side_name: String,
@@ -127,6 +129,7 @@ impl Iterator for Expectations<'_> {
         let mut tokens = line.split(' ');
         let (
             Some(subdir),
+            Some(conflict_style),
             Some(our_commit_id),
             Some(our_side_name),
             Some(their_commit_id),
@@ -141,6 +144,7 @@ impl Iterator for Expectations<'_> {
             tokens.next(),
             tokens.next(),
             tokens.next(),
+            tokens.next(),
         )
         else {
             unreachable!("invalid line: {line:?}")
@@ -156,13 +160,19 @@ impl Iterator for Expectations<'_> {
         });
 
         let subdir_path = self.root.join(subdir);
+        let conflict_style = match conflict_style {
+            "merge" => ConflictStyle::Merge,
+            "diff3" => ConflictStyle::Diff3,
+            unknown => unreachable!("Unknown conflict style: '{unknown}'"),
+        };
         let odb = gix_odb::at(subdir_path.join(".git/objects")).expect("object dir exists");
         let objects = gix_odb::memory::Proxy::new(odb, gix_hash::Kind::Sha1);
         let our_commit_id = gix_hash::ObjectId::from_hex(our_commit_id.as_bytes()).unwrap();
         let their_commit_id = gix_hash::ObjectId::from_hex(their_commit_id.as_bytes()).unwrap();
         let merge_info = parse_merge_info(std::fs::read_to_string(subdir_path.join(merge_info_filename)).unwrap());
         Some(Expectation {
             root: subdir_path,
+            conflict_style,
             odb: objects,
             our_commit_id,
             our_side_name: our_side_name.to_owned(),
diff --git a/gix-merge/tests/merge/tree/mod.rs b/gix-merge/tests/merge/tree/mod.rs
@@ -24,6 +24,7 @@ fn run_baseline() -> crate::Result {
     let new_test = None;
     for baseline::Expectation {
         root,
+        conflict_style,
         odb,
         our_commit_id,
         our_side_name,
@@ -40,7 +41,14 @@ fn run_baseline() -> crate::Result {
         let large_file_threshold_bytes = 80;
         let mut blob_merge = new_blob_merge_platform(&root, large_file_threshold_bytes);
         let mut diff_resource_cache = new_diff_resource_cache(&root);
-        let options = basic_merge_options();
+        let mut options = basic_merge_options();
+        options.tree_merge.blob_merge.text.conflict = gix_merge::blob::builtin_driver::text::Conflict::Keep {
+            style: conflict_style,
+            marker_size: gix_merge::blob::builtin_driver::text::Conflict::DEFAULT_MARKER_SIZE
+                .try_into()
+                .expect("non-zero"),
+        };
+
         dbg!(&case_name);
         let mut actual = gix_merge::commit(
             our_commit_id,
@@ -55,6 +63,7 @@ fn run_baseline() -> crate::Result {
             &mut blob_merge,
             &odb,
             |content| odb.write_buf(gix_object::Kind::Blob, content),
+            |id| id.to_hex_with_len(7).to_string(),
             options,
         )?;
 
@@ -71,15 +80,16 @@ fn run_baseline() -> crate::Result {
                 expected_tree_id,
             }) => {
                 // Sometimes only the reversed part of a specific test is different.
-                if case_name != "same-rename-different-mode-A-B" {
+                if case_name != "same-rename-different-mode-A-B" && case_name != "same-rename-different-mode-A-B-diff3"
+                {
                     assert_ne!(
                         actual_id, merge_info.merged_tree,
-                        "Git caught up - adjust expectation {message}"
+                        "{case_name}: Git caught up - adjust expectation {message}"
                     );
                 } else {
                     assert_eq!(
                         actual_id, merge_info.merged_tree,
-                        "Git should match here, it just doesn't match in one of two cases"
+                        "{case_name}: Git should match here, it just doesn't match in one of two cases"
                     );
                 }
                 eprintln!("{}", baseline::visualize_tree(&actual_id, &odb, None));
@@ -96,7 +106,7 @@ fn run_baseline() -> crate::Result {
     }
 
     assert_eq!(
-        actual_cases, 61,
+        actual_cases, 85,
         "BUG: update this number, and don't forget to remove a filter in the end"
     );
 
