Merge pull request #13247 from gitbutlerapp/is-conflicted-refactor

Move conflicted commit detection from headers to commit message markers

Author: Byron

crates/but-core/src/commit/conflict.rs

@@ -555,8 +555,11 @@ impl<'repo> Commit<'repo> {
     }
 
     /// Return `true` if this commit contains a tree that is conflicted.
+    ///
+    /// Checks the commit message for conflict markers first (new style),
+    /// then falls back to the `gitbutler-conflicted` header (legacy).
     pub fn is_conflicted(&self) -> bool {
-        self.headers().is_some_and(|hdr| hdr.is_conflicted())
+        is_conflicted(self.inner.message.as_ref(), self.headers().as_ref())
     }
 
     /// If the commit is conflicted, then it returns the auto-resolution tree,
@@ -686,3 +689,9 @@ impl ConflictEntries {
         set.len()
     }
 }
+
+mod conflict;
+pub use conflict::{
+    add_conflict_markers, is_conflicted, message_is_conflicted,
+    rewrite_conflict_markers_on_message_change, strip_conflict_markers,
+};

crates/but-core/src/commit.rs crates/but-core/src/commit/mod.rscrates/but-core/src/commit.rs renamed to crates/but-core/src/commit/mod.rs

@@ -29,6 +29,9 @@ git remote add origin normal-remote
 git remote add nested/remote nested-remote
 git remote add nested/remote-b nested-remote-b
 
+# NOTE: `git fetch` automatically creates remote HEAD refs (e.g.
+# refs/remotes/origin/HEAD) since git 2.48. Tests relying on these
+# refs require at least that version.
 git fetch origin
 git fetch nested/remote
 git fetch nested/remote-b

crates/but-core/tests/fixtures/scenario/multiple-remotes-with-tracking-branches.sh

@@ -23,7 +23,7 @@ pub enum EmptyCommit {
 }
 
 pub(crate) mod function {
-    use std::{collections::HashSet, path::PathBuf};
+    use std::path::PathBuf;
 
     use anyhow::{Context as _, bail};
     use bstr::BString;
@@ -168,18 +168,16 @@ pub(crate) mod function {
         }
 
         let headers = to_rebase.headers();
-        let to_rebase_is_conflicted = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted());
         let mut new_commit = to_rebase.inner;
         new_commit.tree = resolved_tree_id.detach();
 
         // Assure the commit isn't thinking it's conflicted.
-        if to_rebase_is_conflicted {
-            if let Some(pos) = new_commit
-                .extra_headers()
-                .find_pos(HEADERS_CONFLICTED_FIELD)
-            {
-                new_commit.extra_headers.remove(pos);
-            }
+        new_commit.message = but_core::commit::strip_conflict_markers(new_commit.message.as_ref());
+        if let Some(pos) = new_commit
+            .extra_headers()
+            .find_pos(HEADERS_CONFLICTED_FIELD)
+        {
+            new_commit.extra_headers.remove(pos);
         } else if headers.is_none() {
             let headers = Headers::from_config(&repo.config_snapshot());
             new_commit
@@ -204,10 +202,6 @@ pub(crate) mod function {
         treat_as_unresolved: gix::merge::tree::TreatAsUnresolved,
     ) -> anyhow::Result<gix::Id<'repo>> {
         let repo = resolved_tree_id.repo;
-        // in case someone checks this out with vanilla Git, we should warn why it looks like this
-        let readme_content =
-            b"You have checked out a GitButler Conflicted commit. You probably didn't mean to do this.";
-        let readme_blob = repo.write_blob(readme_content)?;
 
         let conflicted_files =
             extract_conflicted_files(resolved_tree_id, cherry_pick, treat_as_unresolved)?;
@@ -242,15 +236,18 @@ pub(crate) mod function {
             resolved_tree_id,
         )?;
         tree.upsert(".conflict-files", EntryKind::Blob, conflicted_files_blob)?;
-        tree.upsert("CONFLICT-README.txt", EntryKind::Blob, readme_blob)?;
 
         let mut headers = to_rebase
             .headers()
             .unwrap_or_else(|| Headers::from_config(&repo.config_snapshot()));
-        headers.conflicted = conflicted_files.conflicted_header_field();
+        headers.conflicted = None;
         to_rebase.tree = tree.write().context("failed to write tree")?.detach();
         set_parent(&mut to_rebase, head.id.detach())?;
 
+        // Add conflict markers to the commit message
+        to_rebase.inner.message =
+            but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref());
+
         to_rebase.set_headers(&headers);
         Ok(crate::commit::create(
             repo,
@@ -341,22 +338,5 @@ pub(crate) mod function {
                 || !self.our_entries.is_empty()
                 || !self.their_entries.is_empty()
         }
-
-        fn total_entries(&self) -> usize {
-            let set = self
-                .ancestor_entries
-                .iter()
-                .chain(self.our_entries.iter())
-                .chain(self.their_entries.iter())
-                .collect::<HashSet<_>>();
-
-            set.len()
-        }
-
-        /// Return the `conflicted` header field value.
-        pub(crate) fn conflicted_header_field(&self) -> Option<u64> {
-            let entries = self.total_entries();
-            Some(if entries > 0 { entries as u64 } else { 1 })
-        }
     }
 }

crates/but-rebase/src/cherry_pick.rs

@@ -305,18 +305,16 @@ fn commit_from_unconflicted_tree<'repo>(
     let repo = to_rebase.id.repo;
 
     let headers = to_rebase.headers();
-    let to_rebase_is_conflicted = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted());
     let mut new_commit = to_rebase.inner;
     new_commit.tree = resolved_tree_id.detach();
 
     // Ensure the commit isn't thinking it's conflicted.
-    if to_rebase_is_conflicted {
-        if let Some(pos) = new_commit
-            .extra_headers()
-            .find_pos(HEADERS_CONFLICTED_FIELD)
-        {
-            new_commit.extra_headers.remove(pos);
-        }
+    new_commit.message = but_core::commit::strip_conflict_markers(new_commit.message.as_ref());
+    if let Some(pos) = new_commit
+        .extra_headers()
+        .find_pos(HEADERS_CONFLICTED_FIELD)
+    {
+        new_commit.extra_headers.remove(pos);
     } else if headers.is_none() {
         let headers = Headers::from_config(&repo.config_snapshot());
         new_commit
@@ -347,10 +345,6 @@ fn commit_from_conflicted_tree<'repo>(
     sign_commit: SignCommit,
 ) -> anyhow::Result<gix::Id<'repo>> {
     let repo = resolved_tree_id.repo;
-    // in case someone checks this out with vanilla Git, we should warn why it looks like this
-    let readme_content =
-        b"You have checked out a GitButler Conflicted commit. You probably didn't mean to do this.";
-    let readme_blob = repo.write_blob(readme_content)?;
 
     let conflicted_files =
         extract_conflicted_files(resolved_tree_id, cherry_pick, treat_as_unresolved)?;
@@ -382,15 +376,18 @@ fn commit_from_conflicted_tree<'repo>(
         resolved_tree_id,
     )?;
     tree.upsert(".conflict-files", EntryKind::Blob, conflicted_files_blob)?;
-    tree.upsert("CONFLICT-README.txt", EntryKind::Blob, readme_blob)?;
 
     let mut headers = to_rebase
         .headers()
         .unwrap_or_else(|| Headers::from_config(&repo.config_snapshot()));
-    headers.conflicted = conflicted_files.conflicted_header_field();
+    headers.conflicted = None;
     to_rebase.tree = tree.write().context("failed to write tree")?.detach();
     to_rebase.parents = parents.into();
 
+    // Add conflict markers to the commit message
+    to_rebase.inner.message =
+        but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref());
+
     to_rebase.set_headers(&headers);
     Ok(crate::commit::create(
         repo,

crates/but-rebase/src/graph_rebase/cherry_pick.rs

@@ -286,7 +286,11 @@ fn rebase(
                         None if commit.parents.is_empty() => {
                             let mut new_commit = commit;
                             if let Some(new_message) = new_message {
-                                new_commit.message = new_message;
+                                new_commit.message =
+                                    but_core::commit::rewrite_conflict_markers_on_message_change(
+                                        new_commit.message.as_ref(),
+                                        new_message,
+                                    );
                             }
                             cursor = Some(commit::create(
                                 repo,
@@ -325,7 +329,11 @@ fn rebase(
                 let mut new_commit = repo.find_commit(new_commit)?.decode()?.to_owned()?;
                 new_commit.parents = base_commit.parent_ids().map(|id| id.detach()).collect();
                 if let Some(new_message) = new_message {
-                    new_commit.message = new_message;
+                    new_commit.message =
+                        but_core::commit::rewrite_conflict_markers_on_message_change(
+                            new_commit.message.as_ref(),
+                            new_message,
+                        );
                 }
                 *cursor = commit::create(
                     repo,
@@ -371,7 +379,10 @@ fn reword_commit(
     new_message: BString,
 ) -> Result<gix::ObjectId> {
     let mut new_commit = repo.find_commit(oid)?.decode()?.to_owned()?;
-    new_commit.message = new_message;
+    new_commit.message = but_core::commit::rewrite_conflict_markers_on_message_change(
+        new_commit.message.as_ref(),
+        new_message,
+    );
     Ok(commit::create(
         repo,
         new_commit,

crates/but-rebase/src/lib.rs

@@ -69,7 +69,7 @@ fn basic_cherry_pick_cp_conflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(e9ee7b59aff786fc970c30f6965d1de1913c7ec4),
+        Sha1(9661555c611ff64e8c597f7f5a0575f211eb2e12),
     )
     ");
 
@@ -80,7 +80,7 @@ fn basic_cherry_pick_cp_conflicts() -> Result<()> {
     assert_eq!(&get_parents(&id.attach(&repo))?, &[onto]);
 
     insta::assert_snapshot!(visualize_tree(id.attach(&repo)), @r#"
-    0367fb7
+    1090f8a
     ├── .auto-resolution:aa3d213 
     │   ├── base-f:100644:7898192 "a\n"
     │   └── target-f:100644:eb5a316 "target\n"
@@ -95,7 +95,6 @@ fn basic_cherry_pick_cp_conflicts() -> Result<()> {
     │   ├── base-f:100644:7898192 "a\n"
     │   ├── clean-f:100644:8312630 "clean\n"
     │   └── target-f:100644:9b1719f "conflict\n"
-    ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this."
     ├── base-f:100644:7898192 "a\n"
     └── target-f:100644:eb5a316 "target\n"
     "#);
@@ -184,7 +183,7 @@ fn single_parent_to_multiple_parents_cp_conflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(0fcbe01202743fa55f1a1e07342ad26f2e7a0abe),
+        Sha1(c8779c405b92941176effe9eb7a72c0dd410c3fd),
     )
     ");
 
@@ -195,7 +194,7 @@ fn single_parent_to_multiple_parents_cp_conflicts() -> Result<()> {
     assert_eq!(&get_parents(&id.attach(&repo))?, &[onto, onto2]);
 
     insta::assert_snapshot!(visualize_tree(id.attach(&repo)), @r#"
-    1804f3d
+    4c6dc70
     ├── .auto-resolution:744efa9 
     │   ├── base-f:100644:7898192 "a\n"
     │   ├── target-2-f:100644:caac8f9 "target 2\n"
@@ -212,7 +211,6 @@ fn single_parent_to_multiple_parents_cp_conflicts() -> Result<()> {
     │   ├── base-f:100644:7898192 "a\n"
     │   ├── clean-f:100644:8312630 "clean\n"
     │   └── target-f:100644:9b1719f "conflict\n"
-    ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this."
     ├── base-f:100644:7898192 "a\n"
     ├── target-2-f:100644:caac8f9 "target 2\n"
     └── target-f:100644:eb5a316 "target\n"
@@ -311,7 +309,7 @@ fn multiple_parents_to_single_parent_cp_conflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(28fa7c91af8652f4e69c1e3184f92569a3468a34),
+        Sha1(2d4dcd916020924f2412642b842a791dfea74571),
     )
     ");
 
@@ -322,7 +320,7 @@ fn multiple_parents_to_single_parent_cp_conflicts() -> Result<()> {
     assert_eq!(&get_parents(&id.attach(&repo))?, &[onto]);
 
     insta::assert_snapshot!(visualize_tree(id.attach(&repo)), @r#"
-    91fe014
+    8c4acd1
     ├── .auto-resolution:aa3d213 
     │   ├── base-f:100644:7898192 "a\n"
     │   └── target-f:100644:eb5a316 "target\n"
@@ -339,7 +337,6 @@ fn multiple_parents_to_single_parent_cp_conflicts() -> Result<()> {
     │   ├── clean-2-f:100644:13e9394 "clean 2\n"
     │   ├── clean-f:100644:8312630 "clean\n"
     │   └── target-f:100644:9b1719f "conflict\n"
-    ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this."
     ├── base-f:100644:7898192 "a\n"
     └── target-f:100644:eb5a316 "target\n"
     "#);
@@ -441,7 +438,7 @@ fn multiple_parents_to_multiple_parents_cp_conflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(2e6cb06fe98780bb8c7a301a522edd98805d1499),
+        Sha1(bf66deb17cc4e84faa063a3253566004aabaf7fe),
     )
     ");
 
@@ -452,7 +449,7 @@ fn multiple_parents_to_multiple_parents_cp_conflicts() -> Result<()> {
     assert_eq!(&get_parents(&id.attach(&repo))?, &[onto, onto2]);
 
     insta::assert_snapshot!(visualize_tree(id.attach(&repo)), @r#"
-    0aeaf79
+    1620d95
     ├── .auto-resolution:744efa9 
     │   ├── base-f:100644:7898192 "a\n"
     │   ├── target-2-f:100644:caac8f9 "target 2\n"
@@ -471,7 +468,6 @@ fn multiple_parents_to_multiple_parents_cp_conflicts() -> Result<()> {
     │   ├── clean-2-f:100644:13e9394 "clean 2\n"
     │   ├── clean-f:100644:8312630 "clean\n"
     │   └── target-f:100644:9b1719f "conflict\n"
-    ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this."
     ├── base-f:100644:7898192 "a\n"
     ├── target-2-f:100644:caac8f9 "target 2\n"
     └── target-f:100644:eb5a316 "target\n"
@@ -688,7 +684,7 @@ fn no_parents_to_single_parent_cp_conflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(28f862257bff139659b763a2c873b8d3f0f780b0),
+        Sha1(2ee47b5cbe2705d5d81f4b4067ef4753d87b3b02),
     )
     ");
 
@@ -699,7 +695,7 @@ fn no_parents_to_single_parent_cp_conflicts() -> Result<()> {
     assert_eq!(&get_parents(&id.attach(&repo))?, &[onto]);
 
     insta::assert_snapshot!(visualize_tree(id.attach(&repo)), @r#"
-    1267a55
+    38edd44
     ├── .auto-resolution:aa3d213 
     │   ├── base-f:100644:7898192 "a\n"
     │   └── target-f:100644:eb5a316 "target\n"
@@ -711,7 +707,6 @@ fn no_parents_to_single_parent_cp_conflicts() -> Result<()> {
     ├── .conflict-side-1:144e5f5 
     │   ├── base-f:100644:7898192 "a\n"
     │   └── target-f:100644:9b1719f "conflict\n"
-    ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this."
     ├── base-f:100644:7898192 "a\n"
     └── target-f:100644:eb5a316 "target\n"
     "#);
@@ -738,7 +733,7 @@ fn cherry_pick_back_to_original_parents_unconflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     ConflictedCommit(
-        Sha1(2e6cb06fe98780bb8c7a301a522edd98805d1499),
+        Sha1(bf66deb17cc4e84faa063a3253566004aabaf7fe),
     )
     ");
 
@@ -758,7 +753,7 @@ fn cherry_pick_back_to_original_parents_unconflicts() -> Result<()> {
 
     insta::assert_debug_snapshot!(result, @"
     Commit(
-        Sha1(3d7dfa09a071658d3b84eb1ee195ea0ebfeb601f),
+        Sha1(2a307db18bf263e3a802ac71282c5d8016ea75a1),
     )
     ");