review

- more structured `but-core` addition
- idempotent `add_conflict_markers`
- simplify marker removal

Co-authored-by: GPT 5.4 <codex@openai.com>

Author: Byron

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

@@ -168,22 +168,16 @@ pub(crate) mod function {
         }
 
         let headers = to_rebase.headers();
-        let was_conflicted = to_rebase.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 was_conflicted {
-            // Strip the legacy header if present
-            if let Some(pos) = new_commit
-                .extra_headers()
-                .find_pos(HEADERS_CONFLICTED_FIELD)
-            {
-                new_commit.extra_headers.remove(pos);
-            }
-            // Strip conflict markers from the commit message
-            new_commit.message =
-                but_core::commit::strip_conflict_markers(new_commit.message.as_ref());
+        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
@@ -251,10 +245,8 @@ pub(crate) mod function {
         set_parent(&mut to_rebase, head.id.detach())?;
 
         // Add conflict markers to the commit message
-        if !but_core::commit::message_is_conflicted(to_rebase.inner.message.as_ref()) {
-            to_rebase.inner.message =
-                but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref());
-        }
+        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(

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

@@ -305,21 +305,16 @@ fn commit_from_unconflicted_tree<'repo>(
     let repo = to_rebase.id.repo;
 
     let headers = to_rebase.headers();
-    let was_conflicted = to_rebase.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 was_conflicted {
-        // Strip the legacy header if present
-        if let Some(pos) = new_commit
-            .extra_headers()
-            .find_pos(HEADERS_CONFLICTED_FIELD)
-        {
-            new_commit.extra_headers.remove(pos);
-        }
-        // Strip conflict markers from the commit message
-        new_commit.message = but_core::commit::strip_conflict_markers(new_commit.message.as_ref());
+    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
@@ -390,10 +385,8 @@ fn commit_from_conflicted_tree<'repo>(
     to_rebase.parents = parents.into();
 
     // Add conflict markers to the commit message
-    if !but_core::commit::message_is_conflicted(to_rebase.inner.message.as_ref()) {
-        to_rebase.inner.message =
-            but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref());
-    }
+    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(

crates/but-rebase/src/cherry_pick.rs

@@ -227,11 +227,7 @@ fn local_commits_gix(
         authors.insert(author.clone());
         authors.insert(committer);
         let is_conflicted = commit.is_conflicted();
-        let message = if is_conflicted {
-            but_core::commit::strip_conflict_markers(commit.message.as_ref())
-        } else {
-            commit.message.clone()
-        };
+        let message = but_core::commit::strip_conflict_markers(commit.message.as_ref());
         out.push(ui::Commit {
             id: info.id,
             parent_ids: commit.parents.iter().cloned().collect(),

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

@@ -69,11 +69,7 @@ impl From<but_core::Commit<'_>> for Commit {
         let gix::objs::Commit {
             message, author, ..
         } = value.inner;
-        let message = if has_conflicts {
-            but_core::commit::strip_conflict_markers(message.as_ref())
-        } else {
-            message
-        };
+        let message = but_core::commit::strip_conflict_markers(message.as_ref());
         Commit {
             id,
             tree_id,
@@ -103,13 +99,8 @@ impl Commit {
         graph_commit: &but_graph::Commit,
     ) -> Self {
         let hdr = but_core::commit::Headers::try_from_commit(&commit);
-        let has_conflicts = but_core::commit::message_is_conflicted(commit.message.as_ref())
-            || hdr.as_ref().is_some_and(|hdr| hdr.is_conflicted());
-        let message = if has_conflicts {
-            but_core::commit::strip_conflict_markers(commit.message.as_ref())
-        } else {
-            commit.message
-        };
+        let has_conflicts = but_core::commit::is_conflicted(commit.message.as_ref(), hdr.as_ref());
+        let message = but_core::commit::strip_conflict_markers(commit.message.as_ref());
         Commit {
             id: graph_commit.id,
             parent_ids: commit.parents.into_iter().collect(),

crates/but-workspace/src/branch_details.rs

@@ -118,19 +118,14 @@ impl TryFrom<gix::Commit<'_>> for Commit {
         let commit_id = commit.id;
         let commit = commit.decode()?;
         let headers = but_core::commit::Headers::try_from_commit_headers(|| commit.extra_headers());
-        let has_conflicts = but_core::commit::message_is_conflicted(commit.message)
-            || headers.as_ref().is_some_and(|hdr| hdr.is_conflicted());
+        let has_conflicts = but_core::commit::is_conflicted(commit.message, headers.as_ref());
         let change_id = headers
             .unwrap_or_default()
             .ensure_change_id(commit_id)
             .change_id
             .expect("change-id is ensured")
             .to_string();
-        let message = if has_conflicts {
-            but_core::commit::strip_conflict_markers(commit.message)
-        } else {
-            commit.message.to_owned()
-        };
+        let message = but_core::commit::strip_conflict_markers(commit.message);
         Ok(Commit {
             id: commit_id,
             parent_ids: commit.parents().collect(),
@@ -148,8 +143,8 @@ impl TryFrom<gix::Commit<'_>> for Commit {
 impl From<but_core::CommitOwned> for Commit {
     fn from(CommitOwned { id, inner }: CommitOwned) -> Self {
         let headers = commit::Headers::try_from_commit(&inner);
-        let has_conflicts = but_core::commit::message_is_conflicted(inner.message.as_ref())
-            || headers.as_ref().is_some_and(|hdr| hdr.is_conflicted());
+        let has_conflicts =
+            but_core::commit::is_conflicted(inner.message.as_ref(), headers.as_ref());
         let change_id = headers
             .unwrap_or_default()
             .ensure_change_id(id)
@@ -165,11 +160,7 @@ impl From<but_core::CommitOwned> for Commit {
             message,
             extra_headers: _,
         } = inner;
-        let message = if has_conflicts {
-            but_core::commit::strip_conflict_markers(message.as_ref())
-        } else {
-            message
-        };
+        let message = but_core::commit::strip_conflict_markers(message.as_ref());
         Commit {
             id,
             parent_ids: parents.into_iter().collect(),

crates/but-workspace/src/ref_info.rs

@@ -1,5 +1,4 @@
 use bstr::BStr;
-use but_core::commit::message_is_conflicted;
 use but_core::{ChangeId, commit::Headers};
 
 /// Extension trait for `gix::Commit`.
@@ -28,16 +27,11 @@ impl CommitExt for gix::Commit<'_> {
     }
 
     fn is_conflicted(&self) -> bool {
-        // Check commit message first (new style), fall back to header (legacy).
-        if let Ok(commit) = self.decode() {
-            if message_is_conflicted(commit.message) {
-                return true;
-            }
-            Headers::try_from_commit_headers(|| commit.extra_headers())
-                .is_some_and(|hdr| hdr.is_conflicted())
-        } else {
-            false
-        }
+        let Ok(commit) = self.decode() else {
+            return false;
+        };
+        let headers = Headers::try_from_commit_headers(|| commit.extra_headers());
+        but_core::commit::is_conflicted(commit.message, headers.as_ref())
     }
 }
 

crates/but-workspace/src/ui/mod.rs

@@ -290,10 +290,7 @@ pub(crate) fn save_and_return_to_workspace(ctx: &Context, perm: &mut RepoExclusi
                 (&headers).into()
             })
             .unwrap_or_default();
-        // Strip conflict markers only for genuinely conflicted commit messages.
-        if but_core::commit::message_is_conflicted(commit.message.as_ref()) {
-            commit.message = but_core::commit::strip_conflict_markers(commit.message.as_ref());
-        }
+        commit.message = but_core::commit::strip_conflict_markers(commit.message.as_ref());
         but_rebase::commit::create(
             repo,
             gix::objs::Commit {