From 0d13d77bb352088d71c3aa50047511e42fa3155b Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Thu, 9 Apr 2026 17:36:12 +0200 Subject: [PATCH 1/3] Move conflicted commit detection from headers to commit message markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `gitbutler-conflicted` extra header is stripped when commits are rebased outside of GitButler, silently losing conflict state. Replace it with commit message markers that survive rebasing and are visible to developers in `git log`: - A `[conflict] ` prefix on the subject line for fast visual detection - A `GitButler-Conflict:` multi-line git trailer whose value explains the conflict tree layout Using a standard git trailer means the description is embedded as the trailer value with indented continuation lines, so `git interpret-trailers` and other standard tools can parse and manipulate it. The resulting commit message looks like: [conflict] GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved using the "ours" side. The commit tree contains additional directories: .conflict-side-0 — our tree .conflict-side-1 — their tree ... To manually resolve, check out this commit, remove the directories listed above, resolve the conflicts, and amend the commit. The trailer is appended after any existing trailers in the message, preserving Change-Id, Signed-off-by, and similar lines. When stripping conflict markers (on resolution, display, or commit matching), the prefix, trailer, and all continuation lines are removed to restore the original message. New commits only write message markers (no header). Reading checks the message first, then falls back to the header for backward compatibility with existing conflicted commits. The CONFLICT-README.txt tree blob is removed since the trailer now serves the same explanatory purpose with better visibility. The conflicted file count (`Option`) stored in the old header was only ever used as a boolean, so the new approach uses a simple presence check with no count. --- crates/but-core/src/commit.rs | 475 +++++++++++++++++- ...multiple-remotes-with-tracking-branches.sh | 3 + crates/but-rebase/src/cherry_pick.rs | 40 +- .../src/graph_rebase/cherry_pick.rs | 20 +- crates/but-rebase/src/lib.rs | 17 +- .../tests/rebase/graph_rebase/cherry_pick.rs | 29 +- .../graph_rebase/conflictable_restriction.rs | 21 +- crates/but-rebase/tests/rebase/main.rs | 41 +- crates/but-workspace/src/branch_details.rs | 10 +- crates/but-workspace/src/changeset.rs | 7 +- crates/but-workspace/src/legacy/stacks.rs | 10 +- crates/but-workspace/src/ref_info.rs | 32 +- crates/but-workspace/src/ui/mod.rs | 18 +- .../tests/workspace/commit/squash_commits.rs | 2 +- .../but-worktrees/tests/worktree/various.rs | 4 +- crates/but/tests/but/command/rub.rs | 2 +- .../tests/branch-actions/reorder.rs | 6 +- crates/gitbutler-commit/src/commit_ext.rs | 18 +- crates/gitbutler-edit-mode/src/lib.rs | 22 +- crates/gitbutler-repo/src/rebase.rs | 59 +-- 20 files changed, 687 insertions(+), 149 deletions(-) diff --git a/crates/but-core/src/commit.rs b/crates/but-core/src/commit.rs index 5a51003706f..09459c3ed44 100644 --- a/crates/but-core/src/commit.rs +++ b/crates/but-core/src/commit.rs @@ -555,8 +555,12 @@ 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()) + message_is_conflicted(self.inner.message.as_ref()) + || self.headers().is_some_and(|hdr| hdr.is_conflicted()) } /// If the commit is conflicted, then it returns the auto-resolution tree, @@ -686,3 +690,472 @@ impl ConflictEntries { set.len() } } + +/// The prefix prepended to the commit subject line to mark a conflicted commit. +pub const CONFLICT_MESSAGE_PREFIX: &str = "[conflict] "; + +/// The git trailer token used to identify GitButler-managed conflicted commits. +/// The description explaining the conflict is embedded as the multi-line trailer +/// value, with continuation lines indented by 3 spaces per git convention. +const CONFLICT_TRAILER_TOKEN: &str = "GitButler-Conflict"; + +/// The full multi-line git trailer appended to conflicted commit messages. +/// The description is the trailer value; continuation lines are indented with +/// 3 spaces so standard git trailer tools can parse and manipulate them. +const CONFLICT_TRAILER: &str = concat!( + "GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved\n", + " using the \"ours\" side. The commit tree contains additional directories:\n", + " .conflict-side-0 \u{2014} our tree\n", + " .conflict-side-1 \u{2014} their tree\n", + " .conflict-base-0 \u{2014} the merge base tree\n", + " .auto-resolution \u{2014} the auto-resolved tree\n", + " .conflict-files \u{2014} metadata about conflicted files\n", + " To manually resolve, check out this commit, remove the directories\n", + " listed above, resolve the conflicts, and amend the commit." +); + +/// Add conflict markers to a commit message: prepend `[conflict] ` to the +/// subject and append the `GitButler-Conflict` multi-line trailer after any +/// existing trailers. +/// +/// A single trailing newline is trimmed from the input before markers are +/// added; callers that need byte-exact round-trips should account for this. +pub fn add_conflict_markers(message: &BStr) -> BString { + let trimmed = message + .as_bytes() + .strip_suffix(b"\n") + .unwrap_or(message.as_bytes()); + + let capacity = CONFLICT_MESSAGE_PREFIX.len() + trimmed.len() + 2 + CONFLICT_TRAILER.len() + 1; + let mut result = BString::from(Vec::with_capacity(capacity)); + result.extend_from_slice(CONFLICT_MESSAGE_PREFIX.as_bytes()); + result.extend_from_slice(trimmed); + + // Trailers must be in the last paragraph — join directly if one exists. + if ends_in_trailer_block(trimmed) { + result.push(b'\n'); + } else { + result.extend_from_slice(b"\n\n"); + } + result.extend_from_slice(CONFLICT_TRAILER.as_bytes()); + result.push(b'\n'); + result +} + +/// Strip conflict markers from a commit message. +/// Returns the message unchanged when it is not conflicted (per +/// [`message_is_conflicted`]). +/// +/// Strips the `[conflict] ` subject prefix (if present) and the +/// `GitButler-Conflict` trailer line together with all its indented +/// continuation lines. +/// +/// Note: the returned message may not be byte-identical to the original — +/// trailing newlines are not preserved and line endings may be normalized. +pub fn strip_conflict_markers(message: &BStr) -> BString { + if !message_is_conflicted(message) { + return message.to_owned(); + } + + let bytes = message.as_bytes(); + + // Strip the subject prefix if present. + let without_prefix = bytes + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .unwrap_or(bytes); + + // Remove the GitButler-Conflict trailer only from the last paragraph + // to avoid accidentally stripping user-authored content earlier in the body + // that happens to match the trailer token. + let lines: Vec<&[u8]> = without_prefix.lines().collect(); + let mut result_lines: Vec<&[u8]> = Vec::with_capacity(lines.len()); + + // Find the start of the last paragraph (after the last blank line). + let last_para_start = lines + .iter() + .enumerate() + .rev() + .find(|(_, l)| l.is_empty()) + .map(|(i, _)| i + 1) + .unwrap_or(0); + + // Copy everything before the last paragraph unchanged. + let mut i = 0; + while i < last_para_start { + result_lines.push(lines[i]); + i += 1; + } + + // In the last paragraph, strip the conflict trailer and its continuation lines. + while i < lines.len() { + let line = lines[i]; + if line_starts_with_conflict_trailer(line) { + i += 1; + while i < lines.len() && lines[i].first().is_some_and(|b| b.is_ascii_whitespace()) { + i += 1; + } + } else { + result_lines.push(line); + i += 1; + } + } + + // Drop trailing blank lines left behind after removing the trailer. + while result_lines.last().is_some_and(|l| l.is_empty()) { + result_lines.pop(); + } + + let mut result = BString::from(Vec::new()); + for (idx, line) in result_lines.iter().enumerate() { + if idx > 0 { + result.push(b'\n'); + } + result.extend_from_slice(line); + } + result +} + +/// Returns `true` when the commit message contains a `GitButler-Conflict:` +/// trailer in the last paragraph. The `[conflict] ` subject prefix is +/// informational and not required for detection. +/// +/// Trailing blank lines are skipped so that messages edited by users or tools +/// that append newlines are still detected correctly. +/// +/// Note: once the upstream fix for `gix`'s `BodyRef::from_bytes` (which +/// currently fails to detect trailers that are the sole body content) is +/// released, this can be simplified to use `gix`'s trailer parser directly. +pub fn message_is_conflicted(message: &BStr) -> bool { + let bytes = message.as_bytes(); + let mut in_content = false; + for line in bytes.lines().rev() { + if line.is_empty() { + if in_content { + break; + } + // Skip trailing blank lines before the last paragraph. + continue; + } + in_content = true; + if line_starts_with_conflict_trailer(line) { + return true; + } + } + false +} + +/// If `old_message` is conflicted but `new_message` is not, re-apply the +/// conflict markers to `new_message`. This is used during reword and squash +/// so that editing a conflicted commit's message doesn't silently drop the +/// conflict state. +/// +/// Strips any existing partial markers from `new_message` before re-adding +/// to avoid double-prefixing or duplicate trailers. +pub fn rewrite_conflict_markers_on_message_change( + old_message: &BStr, + new_message: BString, +) -> BString { + if message_is_conflicted(old_message) && !message_is_conflicted(new_message.as_ref()) { + // Strip the `[conflict] ` prefix if the user left it in, + // then re-add the full set of markers. + let clean = new_message + .as_bytes() + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .map(BString::from) + .unwrap_or(new_message); + add_conflict_markers(clean.as_ref()) + } else { + new_message + } +} + +/// Returns `true` if `bytes` ends with a git trailer block — a paragraph where +/// every line is either a `Token: value` trailer or an indented continuation. +fn ends_in_trailer_block(bytes: &[u8]) -> bool { + let lines: Vec<&[u8]> = bytes.lines().collect(); + + // Trailers must be in a paragraph separated by a blank line from the subject. + // If there is no blank line, there is no trailer block. + let Some(blank_pos) = lines + .iter() + .enumerate() + .rev() + .find(|(_, l)| l.is_empty()) + .map(|(i, _)| i) + else { + return false; + }; + let para_start = blank_pos + 1; + + let para = &lines[para_start..]; + if para.is_empty() { + return false; + } + + let mut found_any = false; + let mut prev_was_trailer_or_continuation = false; + for line in para { + let is_continuation = line.first().is_some_and(|b| b.is_ascii_whitespace()); + if is_continuation { + if !prev_was_trailer_or_continuation { + return false; + } + prev_was_trailer_or_continuation = true; + } else if is_trailer_line(line) { + found_any = true; + prev_was_trailer_or_continuation = true; + } else { + return false; + } + } + found_any +} + +/// Returns `true` for lines of the form `Token: value` where `Token` contains +/// no spaces and the value is non-empty (i.e. `: ` follows the token). +fn is_trailer_line(line: &[u8]) -> bool { + let Some(colon) = line.find_byte(b':') else { + return false; + }; + if colon == 0 { + return false; + } + let token = &line[..colon]; + !token.contains(&b' ') && line.get(colon + 1) == Some(&b' ') +} + +/// Returns `true` when `line` starts with `GitButler-Conflict:`. +fn line_starts_with_conflict_trailer(line: &[u8]) -> bool { + line.strip_prefix(CONFLICT_TRAILER_TOKEN.as_bytes()) + .is_some_and(|rest| rest.first() == Some(&b':')) +} + +#[cfg(test)] +mod conflict_marker_tests { + use bstr::BStr; + + use super::*; + + fn marked(msg: &str) -> String { + String::from_utf8(add_conflict_markers(BStr::new(msg)).into()).unwrap() + } + + fn stripped(msg: &str) -> String { + String::from_utf8(strip_conflict_markers(BStr::new(msg)).into()).unwrap() + } + + fn is_conflicted(msg: &str) -> bool { + message_is_conflicted(BStr::new(msg)) + } + + /// Round-trip: add then strip returns the original (modulo the trailing + /// newline that `add_conflict_markers` always trims). + #[test] + fn simple_subject_roundtrip() { + let original = "fix the bug"; + let result = stripped(&marked(original)); + assert_eq!(result, original); + assert!(is_conflicted(&marked(original))); + } + + #[test] + fn trailing_newline_is_trimmed_by_add() { + // add_conflict_markers trims a trailing newline; strip reflects that. + assert_eq!(stripped(&marked("fix the bug\n")), "fix the bug"); + } + + #[test] + fn subject_and_body_roundtrip() { + let original = "fix the bug\n\nDetailed explanation here."; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn existing_trailers_are_preserved_and_ours_comes_last() { + let original = "fix the bug\n\nChange-Id: I1234567\nSigned-off-by: User "; + let result = marked(original); + assert!(is_conflicted(&result)); + + // Existing trailers must still be present + assert!(result.contains("Change-Id: I1234567\n")); + assert!(result.contains("Signed-off-by: User \n")); + + // Our trailer must come after the existing ones + let signed_pos = result.find("Signed-off-by:").unwrap(); + let conflict_pos = result.find(CONFLICT_TRAILER_TOKEN).unwrap(); + assert!( + conflict_pos > signed_pos, + "GitButler-Conflict trailer must follow existing trailers" + ); + + // Roundtrip + assert_eq!(stripped(&result), original); + } + + #[test] + fn subject_with_only_trailers_roundtrip() { + let original = "fix the bug\n\nChange-Id: I1234567"; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn body_and_trailers_roundtrip() { + let original = + "fix the bug\n\nSome explanation.\n\nChange-Id: I1234567\nSigned-off-by: A "; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn description_is_the_trailer_value_not_a_separate_paragraph() { + let result = marked("subject"); + // The description must appear on the same line as GitButler-Conflict: + // (or as indented continuation lines), not as a separate paragraph. + let trailer_start = format!("{CONFLICT_TRAILER_TOKEN}:"); + let conflict_line = result + .lines() + .find(|l| l.starts_with(&trailer_start)) + .expect("trailer line must exist"); + assert!( + conflict_line.len() > trailer_start.len(), + "trailer token must have an inline value, got: {conflict_line:?}" + ); + } + + #[test] + fn prefix_without_trailer_is_not_conflicted() { + assert!(!is_conflicted("[conflict] looks real but no trailer")); + } + + #[test] + fn trailer_without_prefix_is_still_conflicted() { + let msg = "normal commit\n\nGitButler-Conflict: sneaky"; + // Detection depends only on the trailer, not the prefix. + assert!(is_conflicted(msg)); + // Strip removes the trailer even without the prefix. + assert_eq!(stripped(msg), "normal commit"); + } + + #[test] + fn add_is_idempotent_when_guarded() { + let original = "subject"; + let once = marked(original); + // Callers guard with message_is_conflicted; verify that guard works + assert!(is_conflicted(&once)); + // Stripping twice is also stable + assert_eq!(stripped(&once), stripped(&stripped(&once))); + } + + #[test] + fn trailing_blank_lines_after_trailer_still_detected() { + let msg = format!("subject\n\n{CONFLICT_TRAILER}\n\n"); + assert!( + is_conflicted(&msg), + "trailing blank lines must not break detection" + ); + } + + /// The trailer token appearing in the body (not the last paragraph) must + /// not be stripped — only the actual trailer in the last paragraph is removed. + #[test] + fn strip_only_removes_trailer_from_last_paragraph() { + let body_with_token = format!( + "[conflict] subject\n\nGitButler-Conflict: mentioned in body\n\n{CONFLICT_TRAILER}\n" + ); + assert!(is_conflicted(&body_with_token)); + let result = stripped(&body_with_token); + assert!( + result.contains("GitButler-Conflict: mentioned in body"), + "body occurrence must be preserved, got: {result:?}" + ); + assert!( + !result.contains("This is a GitButler-managed"), + "the trailer itself must be removed" + ); + } + + #[test] + fn rewrite_does_not_double_prefix() { + let original = "fix bug"; + let conflicted = marked(original); + // Simulate a new message that already has the prefix but no trailer. + let partial = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); + let result = rewrite_conflict_markers_on_message_change( + BStr::new(&conflicted), + BString::from(partial), + ); + let result_str = std::str::from_utf8(result.as_ref()).unwrap(); + // Must not produce "[conflict] [conflict] fix bug". + let prefix_count = result_str.matches(CONFLICT_MESSAGE_PREFIX).count(); + assert_eq!(prefix_count, 1, "prefix must appear exactly once"); + assert!(is_conflicted(result_str)); + } + + /// Verify that gix parses the `GitButler-Conflict` trailer alongside + /// other standard trailers (Byron's review feedback). + #[test] + fn gix_parses_conflict_trailer_with_existing_trailers() { + let original = + "fix the bug\n\nSome body.\n\nChange-Id: I1234567\nSigned-off-by: A "; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + let body = msg.body().expect("message must have a body"); + let trailers: Vec<_> = body.trailers().collect(); + + let tokens: Vec<&str> = trailers.iter().map(|t| t.token.to_str().unwrap()).collect(); + assert!( + tokens.contains(&"Change-Id"), + "Change-Id trailer must be parseable by gix, got: {tokens:?}" + ); + assert!( + tokens.contains(&"Signed-off-by"), + "Signed-off-by trailer must be parseable by gix, got: {tokens:?}" + ); + assert!( + tokens.contains(&"GitButler-Conflict"), + "GitButler-Conflict trailer must be parseable by gix, got: {tokens:?}" + ); + } + + /// The `GitButler-Conflict` trailer must always be the last trailer in + /// the message so it does not interfere with other trailer-based tools. + #[test] + fn conflict_trailer_is_last() { + let original = "fix bug\n\nSome body.\n\nChange-Id: I123\nSigned-off-by: A "; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + let body = msg.body().expect("message must have a body"); + let trailers: Vec<_> = body.trailers().collect(); + let last = trailers.last().expect("must have at least one trailer"); + assert_eq!( + last.token.to_str().unwrap(), + "GitButler-Conflict", + "conflict trailer must be the last trailer" + ); + } + + /// Verify gix sees the `[conflict]` prefix in the title even for + /// subject-only messages. + /// + /// Note: gix's trailer parser does not detect trailers when the body + /// consists of only a single trailer paragraph (no preceding body text). + /// Our manual detection handles this case; the gix interop tests below + /// verify that messages WITH body text are parsed correctly by standard + /// git trailer tools. + #[test] + fn subject_only_roundtrip_with_gix() { + let original = "fix the bug"; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + assert_eq!( + msg.title.to_str().unwrap(), + "[conflict] fix the bug", + "gix must see the prefixed title" + ); + + // Round-trip + assert_eq!(stripped(&result), original); + } +} diff --git a/crates/but-core/tests/fixtures/scenario/multiple-remotes-with-tracking-branches.sh b/crates/but-core/tests/fixtures/scenario/multiple-remotes-with-tracking-branches.sh index ea6acbb57e3..8a11759bfd1 100644 --- a/crates/but-core/tests/fixtures/scenario/multiple-remotes-with-tracking-branches.sh +++ b/crates/but-core/tests/fixtures/scenario/multiple-remotes-with-tracking-branches.sh @@ -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 diff --git a/crates/but-rebase/src/cherry_pick.rs b/crates/but-rebase/src/cherry_pick.rs index 546515f1422..c172bdd3b2c 100644 --- a/crates/but-rebase/src/cherry_pick.rs +++ b/crates/but-rebase/src/cherry_pick.rs @@ -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,22 @@ 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 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 to_rebase_is_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()); } else if headers.is_none() { let headers = Headers::from_config(&repo.config_snapshot()); new_commit @@ -204,10 +208,6 @@ pub(crate) mod function { treat_as_unresolved: gix::merge::tree::TreatAsUnresolved, ) -> anyhow::Result> { 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 +242,20 @@ 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 + 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.set_headers(&headers); Ok(crate::commit::create( repo, @@ -341,22 +346,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::>(); - - set.len() - } - - /// Return the `conflicted` header field value. - pub(crate) fn conflicted_header_field(&self) -> Option { - let entries = self.total_entries(); - Some(if entries > 0 { entries as u64 } else { 1 }) - } } } diff --git a/crates/but-rebase/src/graph_rebase/cherry_pick.rs b/crates/but-rebase/src/graph_rebase/cherry_pick.rs index 4f3e0e5e762..e8088726299 100644 --- a/crates/but-rebase/src/graph_rebase/cherry_pick.rs +++ b/crates/but-rebase/src/graph_rebase/cherry_pick.rs @@ -305,18 +305,21 @@ 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 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 to_rebase_is_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()); } else if headers.is_none() { let headers = Headers::from_config(&repo.config_snapshot()); new_commit @@ -347,10 +350,6 @@ fn commit_from_conflicted_tree<'repo>( sign_commit: SignCommit, ) -> anyhow::Result> { 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 +381,20 @@ 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 + 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.set_headers(&headers); Ok(crate::commit::create( repo, diff --git a/crates/but-rebase/src/lib.rs b/crates/but-rebase/src/lib.rs index 8838058f092..b5778848d2f 100644 --- a/crates/but-rebase/src/lib.rs +++ b/crates/but-rebase/src/lib.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 { 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, diff --git a/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs b/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs index b6a4d9a7724..b91e93e53be 100644 --- a/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs +++ b/crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.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), ) "); diff --git a/crates/but-rebase/tests/rebase/graph_rebase/conflictable_restriction.rs b/crates/but-rebase/tests/rebase/graph_rebase/conflictable_restriction.rs index e5e383d85e1..a9d621ee3f4 100644 --- a/crates/but-rebase/tests/rebase/graph_rebase/conflictable_restriction.rs +++ b/crates/but-rebase/tests/rebase/graph_rebase/conflictable_restriction.rs @@ -36,7 +36,7 @@ fn by_default_conflicts_are_allowed() -> Result<()> { insta::assert_snapshot!(overlayed, @" └── 👉►:0[0]:main[🌳] - ├── ·3411540 (⌂|1) ►c + ├── ·103b227 (⌂|1) ►c └── ·5e0ba46 (⌂|1) ►a, ►b └── ►:1[1]:base └── ·6155f21 (⌂|1) @@ -45,17 +45,26 @@ fn by_default_conflicts_are_allowed() -> Result<()> { assert_eq!(overlayed, graph_tree(&outcome.workspace.graph).to_string()); // We expect to see conflicted headers - insta::assert_snapshot!(cat_commit(&repo, "c")?, @" - tree c199bde16a034b8588f7c896ec3640c40ad017dd + insta::assert_snapshot!(cat_commit(&repo, "c")?, @r#" + tree d935c009623a61ebde94512baef22c76c5ccbaef parent 5e0ba4636be91de6216903697b269915d3db6c53 author author 946684800 +0000 committer Committer (Memory Override) 946771200 +0000 gitbutler-headers-version 2 change-id 1 - gitbutler-conflicted 1 - c - "); + [conflict] c + + GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved + using the "ours" side. The commit tree contains additional directories: + .conflict-side-0 — our tree + .conflict-side-1 — their tree + .conflict-base-0 — the merge base tree + .auto-resolution — the auto-resolved tree + .conflict-files — metadata about conflicted files + To manually resolve, check out this commit, remove the directories + listed above, resolve the conflicts, and amend the commit. + "#); Ok(()) } diff --git a/crates/but-rebase/tests/rebase/main.rs b/crates/but-rebase/tests/rebase/main.rs index 0b6da6161ec..3c9efd8a2ee 100644 --- a/crates/but-rebase/tests/rebase/main.rs +++ b/crates/but-rebase/tests/rebase/main.rs @@ -340,7 +340,7 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { .rebase()?; insta::assert_debug_snapshot!(out, @" RebaseOutput { - top_commit: Sha1(b811bdb2d96bfc96bf54030ce094edea09fc8db0), + top_commit: Sha1(976ad5208a756763180113d0a021610aba3c0dad), references: [], commit_mapping: [ ( @@ -362,22 +362,22 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { Sha1(8f0d33828e5c859c95fb9e9fc063374fdd482536), ), Sha1(68a2fc349e13a186e6d65871a31bad244d25e6f4), - Sha1(a5e5bdbc985301d4f814944f11f1cace8eac13a1), + Sha1(d3cf6a9c107edc10d76ff7842de61a1b8c2fe8a8), ), ( Some( Sha1(8f0d33828e5c859c95fb9e9fc063374fdd482536), ), Sha1(134887021e06909021776c023a608f8ef179e859), - Sha1(b811bdb2d96bfc96bf54030ce094edea09fc8db0), + Sha1(976ad5208a756763180113d0a021610aba3c0dad), ), ], } "); insta::assert_snapshot!(visualize_commit_graph(&repo, out.top_commit)?, @r" - *-. b811bdb Re-merge branches 'A', 'B' and 'C' + *-. 976ad52 Re-merge branches 'A', 'B' and 'C' |\ \ - | | * a5e5bdb C~1 + | | * d3cf6a9 [conflict] C~1 | | * eebaa8b C | | * a037d4a C~2 | * | a748762 (B) B: another 10 lines at the bottom @@ -402,7 +402,7 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { let conflict_commit_id = repo.rev_parse_single(format!("{}^3", out.top_commit).as_str())?; insta::assert_snapshot!(but_testsupport::visualize_tree(conflict_commit_id), @r#" - 9a27e64 + c581f79 ├── .auto-resolution:5b3a532 │ ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" │ └── new-file:100644:213ec44 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n21\n22\n23\n24\n25\n26\n27\n28\n29\n30\n" @@ -416,30 +416,38 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { ├── .conflict-side-1:71364f9 │ ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" │ └── new-file:100644:0ff3bbb "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n20\n" - ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this." ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" └── new-file:100644:213ec44 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n21\n22\n23\n24\n25\n26\n27\n28\n29\n30\n" "#); // gitbutler headers were added here to indicate conflict (change-id is frozen for testing) - insta::assert_snapshot!(conflict_commit_id.object()?.data.as_bstr(), @" - tree 9a27e6447201f9ae394c1e0aaef1536633943fe0 + insta::assert_snapshot!(conflict_commit_id.object()?.data.as_bstr(), @r#" + tree c581f7908b6eee5cf2cd3a481c8d167f0a47d3c3 parent eebaa8b32984736d7a835f805724c66a3988f01b author author 946684800 +0000 committer Committer (Memory Override) 946771200 +0000 gitbutler-headers-version 2 change-id 1 - gitbutler-conflicted 1 - C~1 - "); + [conflict] C~1 + + GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved + using the "ours" side. The commit tree contains additional directories: + .conflict-side-0 — our tree + .conflict-side-1 — their tree + .conflict-base-0 — the merge base tree + .auto-resolution — the auto-resolved tree + .conflict-files — metadata about conflicted files + To manually resolve, check out this commit, remove the directories + listed above, resolve the conflicts, and amend the commit. + "#); // And they are added to merge commits. insta::assert_snapshot!(out.top_commit.attach(&repo).object()?.data.as_bstr(), @" tree 6abc3da6f1642bfd5543ef97f98b924f4f232a96 parent add59d26b2ffd7468fcb44c2db48111dd8f481e5 parent a7487625f079bedf4d20e48f052312c010117b38 - parent a5e5bdbc985301d4f814944f11f1cace8eac13a1 + parent d3cf6a9c107edc10d76ff7842de61a1b8c2fe8a8 author author 946684800 +0000 committer Committer (Memory Override) 946771200 +0000 gitbutler-headers-version 2 @@ -482,7 +490,7 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { // The base doesn't have new file, and we pick that up from the base of `base` of // the previous conflict. `our` side then is the original our. insta::assert_snapshot!(visualize_tree(&repo, &out ), @r#" - 72b9cf3 + ee9bc70 ├── .auto-resolution:5b3a532 │ ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" │ └── new-file:100644:213ec44 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n21\n22\n23\n24\n25\n26\n27\n28\n29\n30\n" @@ -495,7 +503,6 @@ fn reorder_with_conflict_and_remerge_and_pick_from_conflicts() -> Result<()> { ├── .conflict-side-1:fa799da │ ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" │ └── new-file:100644:f00c965 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n" - ├── CONFLICT-README.txt:100644:2af04b7 "You have checked out a GitButler Conflicted commit. You probably didn\'t mean to do this." ├── file:100644:5ecf5f4 "50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n" └── new-file:100644:213ec44 "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n21\n22\n23\n24\n25\n26\n27\n28\n29\n30\n" "#); @@ -589,9 +596,9 @@ fn reversible_conflicts() -> anyhow::Result<()> { let conflict_tip = repo.rev_parse_single(format!("{}^3", out.top_commit).as_str())?; insta::assert_snapshot!(visualize_commit_graph(&repo, out.top_commit)?, @r" - *-. b811bdb Re-merge branches 'A', 'B' and 'C' + *-. 976ad52 Re-merge branches 'A', 'B' and 'C' |\ \ - | | * a5e5bdb C~1 + | | * d3cf6a9 [conflict] C~1 | | * eebaa8b C | | * a037d4a C~2 | * | a748762 (B) B: another 10 lines at the bottom diff --git a/crates/but-workspace/src/branch_details.rs b/crates/but-workspace/src/branch_details.rs index 9e4954f74b4..cad195ca97c 100644 --- a/crates/but-workspace/src/branch_details.rs +++ b/crates/but-workspace/src/branch_details.rs @@ -226,11 +226,17 @@ fn local_commits_gix( let committer: ui::Author = commit.committer.to_ref(&mut buf).into(); 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() + }; out.push(ui::Commit { id: info.id, parent_ids: commit.parents.iter().cloned().collect(), - message: commit.message.clone(), - has_conflicts: commit.is_conflicted(), + message, + has_conflicts: is_conflicted, state: CommitState::LocalAndRemote(info.id), created_at: i128::from(commit.committer.time.seconds) * 1000, author, diff --git a/crates/but-workspace/src/changeset.rs b/crates/but-workspace/src/changeset.rs index fda3ec18e16..2f865e0764d 100644 --- a/crates/but-workspace/src/changeset.rs +++ b/crates/but-workspace/src/changeset.rs @@ -659,7 +659,12 @@ fn commit_data_id(c: &crate::ref_info::Commit) -> anyhow::Result { hasher.update(&offset.to_le_bytes()); hasher.update(b"M"); - hasher.update(c.message.as_slice()); + // Trim trailing line endings before hashing so that messages differing + // only in trailing newlines still match. This is needed because + // `strip_conflict_markers` may consume trailing newlines that the + // original message had; the remote copy retains them. + let msg = c.message.trim_end_with(|c| c == '\n' || c == '\r'); + hasher.update(msg); Ok(Identifier::CommitData(hasher.try_finalize()?)) } diff --git a/crates/but-workspace/src/legacy/stacks.rs b/crates/but-workspace/src/legacy/stacks.rs index eaf9a0463b8..b485596bc76 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -660,8 +660,16 @@ impl TryFrom<&gix::Commit<'_>> for CommitData { type Error = anyhow::Error; fn try_from(commit: &gix::Commit<'_>) -> std::result::Result { + let raw_message: BString = commit.message_bstr().into(); + let mut message = but_core::commit::strip_conflict_markers(raw_message.as_ref()); + // Normalize trailing newlines so that messages differing only in a trailing + // newline still match (strip_conflict_markers may consume a trailing newline + // that the original message had). + while message.last() == Some(&b'\n') { + message.pop(); + } Ok(CommitData { - message: commit.message_bstr().into(), + message, author: commit.author()?.into(), }) } diff --git a/crates/but-workspace/src/ref_info.rs b/crates/but-workspace/src/ref_info.rs index 5a83161131b..61c62ec8e54 100644 --- a/crates/but-workspace/src/ref_info.rs +++ b/crates/but-workspace/src/ref_info.rs @@ -63,12 +63,23 @@ impl From> for Commit { fn from(value: but_core::Commit<'_>) -> Self { let has_conflicts = value.is_conflicted(); let change_id = value.headers().and_then(|hdr| hdr.change_id); + let id = value.id.into(); + let tree_id = value.tree; + let parent_ids = value.parents.iter().cloned().collect(); + let gix::objs::Commit { + message, author, .. + } = value.inner; + let message = if has_conflicts { + but_core::commit::strip_conflict_markers(message.as_ref()) + } else { + message + }; Commit { - id: value.id.into(), - tree_id: value.tree, - parent_ids: value.parents.iter().cloned().collect(), - message: value.inner.message, - author: value.inner.author, + id, + tree_id, + parent_ids, + message, + author, has_conflicts, change_id, refs: Vec::new(), @@ -92,12 +103,19 @@ 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 + }; Commit { id: graph_commit.id, parent_ids: commit.parents.into_iter().collect(), tree_id: commit.tree, - message: commit.message, - has_conflicts: hdr.as_ref().is_some_and(|hdr| hdr.is_conflicted()), + message, + has_conflicts, author: commit .author .to_ref(&mut gix::date::parse::TimeBuf::default()) diff --git a/crates/but-workspace/src/ui/mod.rs b/crates/but-workspace/src/ui/mod.rs index c825088ba32..b4e77c378b1 100644 --- a/crates/but-workspace/src/ui/mod.rs +++ b/crates/but-workspace/src/ui/mod.rs @@ -118,17 +118,23 @@ impl TryFrom> 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 = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); + let has_conflicts = but_core::commit::message_is_conflicted(commit.message) + || headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); 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() + }; Ok(Commit { id: commit_id, parent_ids: commit.parents().collect(), - message: commit.message.to_owned(), + message, has_conflicts, state: CommitState::LocalAndRemote(commit_id), created_at: i128::from(commit.time()?.seconds) * 1000, @@ -142,7 +148,8 @@ impl TryFrom> for Commit { impl From for Commit { fn from(CommitOwned { id, inner }: CommitOwned) -> Self { let headers = commit::Headers::try_from_commit(&inner); - let has_conflicts = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); + let has_conflicts = but_core::commit::message_is_conflicted(inner.message.as_ref()) + || headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); let change_id = headers .unwrap_or_default() .ensure_change_id(id) @@ -158,6 +165,11 @@ impl From for Commit { message, extra_headers: _, } = inner; + let message = if has_conflicts { + but_core::commit::strip_conflict_markers(message.as_ref()) + } else { + message + }; Commit { id, parent_ids: parents.into_iter().collect(), diff --git a/crates/but-workspace/tests/workspace/commit/squash_commits.rs b/crates/but-workspace/tests/workspace/commit/squash_commits.rs index 71bc24ba4e5..b240237ee74 100644 --- a/crates/but-workspace/tests/workspace/commit/squash_commits.rs +++ b/crates/but-workspace/tests/workspace/commit/squash_commits.rs @@ -245,7 +245,7 @@ fn squash_down_out_of_order_reorders_subject_below_target_for_shared_file_lineag ); insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @" - * 09ef005 (HEAD -> three, two) commit two + * 1450203 (HEAD -> three, two) [conflict] commit two * f297a08 (one) commit three "); diff --git a/crates/but-worktrees/tests/worktree/various.rs b/crates/but-worktrees/tests/worktree/various.rs index 7f991d21526..1df5f4a0113 100644 --- a/crates/but-worktrees/tests/worktree/various.rs +++ b/crates/but-worktrees/tests/worktree/various.rs @@ -158,10 +158,10 @@ fn causes_conflicts_above() -> anyhow::Result<()> { .stdout .as_bstr() .to_owned(); - insta::assert_snapshot!(unstable_log, @r" + insta::assert_snapshot!(unstable_log, @" * GitButler Workspace Commit (HEAD -> gitbutler/workspace) * feature-b: add line 2 (feature-b) - * feature-b: add line 1 + * [conflict] feature-b: add line 1 * Integrated worktree (feature-a) * feature-a: add line 2 * feature-a: add line 1 diff --git a/crates/but/tests/but/command/rub.rs b/crates/but/tests/but/command/rub.rs index 7005e45dd76..0949064b657 100644 --- a/crates/but/tests/but/command/rub.rs +++ b/crates/but/tests/but/command/rub.rs @@ -361,7 +361,7 @@ Uncommitted changes ... "changes": [ { - "cliId": "76:nk", + "cliId": "fd:nk", "filePath": "a.txt", "changeType": "modified" } diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/reorder.rs b/crates/gitbutler-branch-actions/tests/branch-actions/reorder.rs index 84b9a654220..28bdeb7d945 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/reorder.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/reorder.rs @@ -500,7 +500,8 @@ fn test_ctx(ctx: &Context) -> Result { .iter() .map(|id| { let commit = repo.find_commit(*id).unwrap(); - (commit.message_bstr().to_string(), *id) + let msg = but_core::commit::strip_conflict_markers(commit.message_bstr()); + (msg.to_string(), *id) }) .collect(); let bottom_commits: HashMap = branches[0] @@ -509,7 +510,8 @@ fn test_ctx(ctx: &Context) -> Result { .iter() .map(|id| { let commit = repo.find_commit(*id).unwrap(); - (commit.message_bstr().to_string(), *id) + let msg = but_core::commit::strip_conflict_markers(commit.message_bstr()); + (msg.to_string(), *id) }) .collect(); diff --git a/crates/gitbutler-commit/src/commit_ext.rs b/crates/gitbutler-commit/src/commit_ext.rs index ac5f4a9dd99..895ca7949e2 100644 --- a/crates/gitbutler-commit/src/commit_ext.rs +++ b/crates/gitbutler-commit/src/commit_ext.rs @@ -1,4 +1,5 @@ use bstr::BStr; +use but_core::commit::message_is_conflicted; use but_core::{ChangeId, commit::Headers}; /// Extension trait for `gix::Commit`. @@ -27,13 +28,16 @@ impl CommitExt for gix::Commit<'_> { } fn is_conflicted(&self) -> bool { - self.decode() - .ok() - .and_then(|commit| { - let headers = Headers::try_from_commit_headers(|| commit.extra_headers())?; - Some(headers.conflicted? > 0) - }) - .unwrap_or(false) + // 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 + } } } diff --git a/crates/gitbutler-edit-mode/src/lib.rs b/crates/gitbutler-edit-mode/src/lib.rs index a399ba373b0..2f172d049ba 100644 --- a/crates/gitbutler-edit-mode/src/lib.rs +++ b/crates/gitbutler-edit-mode/src/lib.rs @@ -145,14 +145,26 @@ fn checkout_edit_branch(ctx: &Context, commit_id: gix::ObjectId) -> Result<()> { let their_commit_msg = commit .message() .and_then(|m| m.lines().next()) - .map(|l| l.chars().take(80).collect::()) + .map(|l| { + l.strip_prefix(but_core::commit::CONFLICT_MESSAGE_PREFIX) + .unwrap_or(l) + .chars() + .take(80) + .collect::() + }) .unwrap_or("".into()); let their_label = format!("Current commit: {their_commit_msg}"); let our_commit_msg = commit_parent .message() .and_then(|m| m.lines().next()) - .map(|l| l.chars().take(80).collect::()) + .map(|l| { + l.strip_prefix(but_core::commit::CONFLICT_MESSAGE_PREFIX) + .unwrap_or(l) + .chars() + .take(80) + .collect::() + }) .unwrap_or("".into()); let our_label = format!("New base: {our_commit_msg}"); @@ -268,7 +280,7 @@ pub(crate) fn save_and_return_to_workspace(ctx: &Context, perm: &mut RepoExclusi let new_commit_oid = if decoded_head_commit.tree() == tree_id { head_commit.id } else { - let commit = gix::objs::Commit::try_from(decoded_head_commit.clone())?; + let mut commit = gix::objs::Commit::try_from(decoded_head_commit.clone())?; let extra_headers: Vec<(BString, BString)> = Headers::try_from_commit(&commit) .map(|commit_headers| { let headers = Headers { @@ -278,6 +290,10 @@ 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()); + } but_rebase::commit::create( repo, gix::objs::Commit { diff --git a/crates/gitbutler-repo/src/rebase.rs b/crates/gitbutler-repo/src/rebase.rs index a7d13019580..6ba46de5306 100644 --- a/crates/gitbutler-repo/src/rebase.rs +++ b/crates/gitbutler-repo/src/rebase.rs @@ -1,6 +1,7 @@ use std::path::PathBuf; use anyhow::{Context as _, Result}; +use bstr::ByteSlice as _; use but_core::{ RepositoryExt, commit::{ConflictEntries, Headers}, @@ -112,7 +113,7 @@ pub fn merge_commits( let tree_oid; let forced_resolution = gix::merge::tree::TreatAsUnresolved::forced_resolution(); - let commit_headers = if merge_result.has_unresolved_conflicts(forced_resolution) { + let is_conflicted = if merge_result.has_unresolved_conflicts(forced_resolution) { let conflicted_files = extract_conflicted_files(merged_tree_id, merge_result, forced_resolution)?; @@ -151,21 +152,17 @@ pub fn merge_commits( 0o100644, )?; - // 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.blob(readme_content)?; - tree_writer.insert("CONFLICT-README.txt", readme_blob, 0o100644)?; - tree_oid = tree_writer.write().context("failed to write tree")?; - conflicted_files.to_headers() + true } else { tree_oid = merged_tree_id.to_git2(); - #[expect( - deprecated, - reason = "We should use a synthetic ID instead, but that needs the existing commit id if available" - )] - Headers::new_with_random_change_id() + false + }; + + let message = if is_conflicted { + but_core::commit::add_conflict_markers(resulting_name.as_bytes().as_bstr()) + } else { + resulting_name.into() }; let (author, committer) = gix_repository.commit_signatures()?; @@ -174,38 +171,18 @@ pub fn merge_commits( None, author, committer, - resulting_name.into(), + message.as_ref(), tree_oid.to_gix(), &[target_commit.id, incoming_commit.id], - Some(commit_headers), + Some({ + #[expect( + deprecated, + reason = "We should use a synthetic ID instead, but that needs the existing commit id if available" + )] + Headers::new_with_random_change_id() + }), ) .context("failed to create commit")?; Ok(commit_oid) } - -trait ToHeaders { - /// Assure that the returned headers will always indicate a conflict. - /// This is a fail-safe in case this instance has no paths stored as auto-resolution - /// removed the path that would otherwise be conflicting. - /// In other words: conflicting index entries aren't reliable when conflicts were resolved - /// with the 'ours' strategy. - fn to_headers(&self) -> Headers; -} - -impl ToHeaders for ConflictEntries { - fn to_headers(&self) -> Headers { - #[expect( - deprecated, - reason = "We should use a synthetic ID instead, but that needs the existing commit id if available" - )] - let default_headers = Headers::new_with_random_change_id(); - Headers { - conflicted: Some({ - let entries = self.total_entries(); - if entries > 0 { entries as u64 } else { 1 } - }), - ..default_headers - } - } -} From 6b55c6072ba7f9f91caff8261dcf515f0f96cce5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 15 Apr 2026 07:33:26 +0800 Subject: [PATCH 2/3] review - more structured `but-core` addition - idempotent `add_conflict_markers` - simplify marker removal Co-authored-by: GPT 5.4 --- crates/but-core/src/commit/conflict.rs | 507 ++++++++++++++++++ .../but-core/src/{commit.rs => commit/mod.rs} | 476 +--------------- crates/but-rebase/src/cherry_pick.rs | 24 +- .../src/graph_rebase/cherry_pick.rs | 23 +- crates/but-workspace/src/branch_details.rs | 6 +- crates/but-workspace/src/ref_info.rs | 15 +- crates/but-workspace/src/ui/mod.rs | 19 +- crates/gitbutler-commit/src/commit_ext.rs | 16 +- crates/gitbutler-edit-mode/src/lib.rs | 5 +- 9 files changed, 544 insertions(+), 547 deletions(-) create mode 100644 crates/but-core/src/commit/conflict.rs rename crates/but-core/src/{commit.rs => commit/mod.rs} (59%) diff --git a/crates/but-core/src/commit/conflict.rs b/crates/but-core/src/commit/conflict.rs new file mode 100644 index 00000000000..33f90de293c --- /dev/null +++ b/crates/but-core/src/commit/conflict.rs @@ -0,0 +1,507 @@ +use bstr::{BStr, BString, ByteSlice}; + +use super::Headers; + +/// The prefix prepended to the commit subject line to mark a conflicted commit. +pub const CONFLICT_MESSAGE_PREFIX: &str = "[conflict] "; + +/// The git trailer token used to identify GitButler-managed conflicted commits. +/// The description explaining the conflict is embedded as the multi-line trailer +/// value, with continuation lines indented by 3 spaces per git convention. +const CONFLICT_TRAILER_TOKEN: &str = "GitButler-Conflict"; + +/// The full multi-line git trailer appended to conflicted commit messages. +/// The description is the trailer value; continuation lines are indented with +/// 3 spaces so standard git trailer tools can parse and manipulate them. +const CONFLICT_TRAILER: &str = concat!( + "GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved\n", + " using the \"ours\" side. The commit tree contains additional directories:\n", + " .conflict-side-0 \u{2014} our tree\n", + " .conflict-side-1 \u{2014} their tree\n", + " .conflict-base-0 \u{2014} the merge base tree\n", + " .auto-resolution \u{2014} the auto-resolved tree\n", + " .conflict-files \u{2014} metadata about conflicted files\n", + " To manually resolve, check out this commit, remove the directories\n", + " listed above, resolve the conflicts, and amend the commit." +); + +/// Add conflict markers to a commit `message`: prepend `[conflict] ` to the +/// subject and append the `GitButler-Conflict` multi-line trailer after any +/// existing trailers, and return the new message. +/// The `message` is returned unchanged if it already contained conflict markers. +/// +/// A single trailing newline is trimmed from the input before markers are +/// added; callers that need byte-exact round-trips should account for this. +pub fn add_conflict_markers(message: &BStr) -> BString { + if message_is_conflicted(message) { + return message.into(); + } + let trimmed = message + .as_bytes() + .strip_suffix(b"\n") + .unwrap_or(message.as_bytes()); + + let capacity = CONFLICT_MESSAGE_PREFIX.len() + trimmed.len() + 2 + CONFLICT_TRAILER.len() + 1; + let mut result = BString::from(Vec::with_capacity(capacity)); + result.extend_from_slice(CONFLICT_MESSAGE_PREFIX.as_bytes()); + result.extend_from_slice(trimmed); + + // Trailers must be in the last paragraph — join directly if one exists. + if ends_in_trailer_block(trimmed) { + result.push(b'\n'); + } else { + result.extend_from_slice(b"\n\n"); + } + result.extend_from_slice(CONFLICT_TRAILER.as_bytes()); + result.push(b'\n'); + result +} + +/// Strip conflict markers from a commit message. +/// Returns the message unchanged when it is not conflicted (per +/// [`message_is_conflicted`]). +/// +/// Strips the `[conflict] ` subject prefix (if present) and the +/// `GitButler-Conflict` trailer line together with all its indented +/// continuation lines. +/// +/// Note: the returned message may not be byte-identical to the original — +/// trailing newlines are not preserved and line endings may be normalized. +pub fn strip_conflict_markers(message: &BStr) -> BString { + if !message_is_conflicted(message) { + return message.to_owned(); + } + + let bytes = message.as_bytes(); + + // Strip the subject prefix if present. + let without_prefix = bytes + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .unwrap_or(bytes); + + // Remove the GitButler-Conflict trailer only from the last paragraph + // to avoid accidentally stripping user-authored content earlier in the body + // that happens to match the trailer token. + let lines: Vec<&[u8]> = without_prefix.lines().collect(); + let mut result_lines: Vec<&[u8]> = Vec::with_capacity(lines.len()); + + // Find the start of the last paragraph (after the last blank line). + let last_para_start = lines + .iter() + .enumerate() + .rev() + .find(|(_, l)| l.is_empty()) + .map(|(i, _)| i + 1) + .unwrap_or(0); + + // Copy everything before the last paragraph unchanged. + let mut i = 0; + while i < last_para_start { + result_lines.push(lines[i]); + i += 1; + } + + // In the last paragraph, strip the conflict trailer and its continuation lines. + while i < lines.len() { + let line = lines[i]; + if line_starts_with_conflict_trailer(line) { + i += 1; + while i < lines.len() && lines[i].first().is_some_and(|b| b.is_ascii_whitespace()) { + i += 1; + } + } else { + result_lines.push(line); + i += 1; + } + } + + // Drop trailing blank lines left behind after removing the trailer. + while result_lines.last().is_some_and(|l| l.is_empty()) { + result_lines.pop(); + } + + let mut result = BString::from(Vec::new()); + for (idx, line) in result_lines.iter().enumerate() { + if idx > 0 { + result.push(b'\n'); + } + result.extend_from_slice(line); + } + result +} + +/// Returns `true` when the commit is conflicted either by message marker +/// (current encoding) or by the legacy `gitbutler-conflicted` header. +pub fn is_conflicted(message: &BStr, headers: Option<&Headers>) -> bool { + message_is_conflicted(message) || headers.is_some_and(Headers::is_conflicted) +} + +/// Returns `true` when the commit message contains a `GitButler-Conflict:` +/// trailer in the last paragraph. The `[conflict] ` subject prefix is +/// informational and not required for detection. +/// +/// Trailing blank lines are skipped so that messages edited by users or tools +/// that append newlines are still detected correctly. +pub fn message_is_conflicted(message: &BStr) -> bool { + let bytes = message.as_bytes(); + let mut in_content = false; + for line in bytes.lines().rev() { + if line.is_empty() { + if in_content { + break; + } + // Skip trailing blank lines before the last paragraph. + continue; + } + in_content = true; + if line_starts_with_conflict_trailer(line) { + return true; + } + } + false +} + +/// If `old_message` is conflicted but `new_message` is not, re-apply the +/// conflict markers to `new_message`. This is used during reword and squash +/// so that editing a conflicted commit's message doesn't silently drop the +/// conflict state. +/// +/// Strips any existing partial markers from `new_message` before re-adding +/// to avoid double-prefixing or duplicate trailers. +pub fn rewrite_conflict_markers_on_message_change( + old_message: &BStr, + new_message: BString, +) -> BString { + if message_is_conflicted(old_message) && !message_is_conflicted(new_message.as_ref()) { + // Strip the `[conflict] ` prefix if the user left it in, + // then re-add the full set of markers. + let clean = new_message + .as_bytes() + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .map(BString::from) + .unwrap_or(new_message); + add_conflict_markers(clean.as_ref()) + } else { + new_message + } +} + +/// Returns `true` if `bytes` ends with a git trailer block — a paragraph where +/// every line is either a `Token: value` trailer or an indented continuation. +fn ends_in_trailer_block(bytes: &[u8]) -> bool { + let lines: Vec<&[u8]> = bytes.lines().collect(); + + // Trailers must be in a paragraph separated by a blank line from the subject. + // If there is no blank line, there is no trailer block. + let Some(blank_pos) = lines + .iter() + .enumerate() + .rev() + .find(|(_, l)| l.is_empty()) + .map(|(i, _)| i) + else { + return false; + }; + let para_start = blank_pos + 1; + + let para = &lines[para_start..]; + if para.is_empty() { + return false; + } + + let mut found_any = false; + let mut prev_was_trailer_or_continuation = false; + for line in para { + let is_continuation = line.first().is_some_and(|b| b.is_ascii_whitespace()); + if is_continuation { + if !prev_was_trailer_or_continuation { + return false; + } + prev_was_trailer_or_continuation = true; + } else if is_trailer_line(line) { + found_any = true; + prev_was_trailer_or_continuation = true; + } else { + return false; + } + } + found_any +} + +/// Returns `true` for lines of the form `Token: value` where `Token` contains +/// no spaces and the value is non-empty (i.e. `: ` follows the token). +fn is_trailer_line(line: &[u8]) -> bool { + let Some(colon) = line.find_byte(b':') else { + return false; + }; + if colon == 0 { + return false; + } + let token = &line[..colon]; + !token.contains(&b' ') && line.get(colon + 1) == Some(&b' ') +} + +/// Returns `true` when `line` starts with `GitButler-Conflict:`. +fn line_starts_with_conflict_trailer(line: &[u8]) -> bool { + line.strip_prefix(CONFLICT_TRAILER_TOKEN.as_bytes()) + .is_some_and(|rest| rest.first() == Some(&b':')) +} + +#[cfg(test)] +mod tests { + use crate::commit::Headers; + use bstr::BStr; + + use super::*; + + fn marked(msg: &str) -> String { + String::from_utf8(add_conflict_markers(BStr::new(msg)).into()).unwrap() + } + + fn stripped(msg: &str) -> String { + String::from_utf8(strip_conflict_markers(BStr::new(msg)).into()).unwrap() + } + + fn message_is_marked_conflicted(msg: &str) -> bool { + message_is_conflicted(BStr::new(msg)) + } + + /// Round-trip: add then strip returns the original (modulo the trailing + /// newline that `add_conflict_markers` always trims). + #[test] + fn simple_subject_roundtrip() { + let original = "fix the bug"; + let result = stripped(&marked(original)); + assert_eq!(result, original); + assert!(message_is_marked_conflicted(&marked(original))); + } + + #[test] + fn trailing_newline_is_trimmed_by_add() { + // add_conflict_markers trims a trailing newline; strip reflects that. + assert_eq!(stripped(&marked("fix the bug\n")), "fix the bug"); + } + + #[test] + fn subject_and_body_roundtrip() { + let original = "fix the bug\n\nDetailed explanation here."; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn existing_trailers_are_preserved_and_ours_comes_last() { + let original = "fix the bug\n\nChange-Id: I1234567\nSigned-off-by: User "; + let result = marked(original); + assert!(message_is_marked_conflicted(&result)); + + // Existing trailers must still be present + assert!(result.contains("Change-Id: I1234567\n")); + assert!(result.contains("Signed-off-by: User \n")); + + // Our trailer must come after the existing ones + let signed_pos = result.find("Signed-off-by:").unwrap(); + let conflict_pos = result.find(CONFLICT_TRAILER_TOKEN).unwrap(); + assert!( + conflict_pos > signed_pos, + "GitButler-Conflict trailer must follow existing trailers" + ); + + // Roundtrip + assert_eq!(stripped(&result), original); + } + + #[test] + fn subject_with_only_trailers_roundtrip() { + let original = "fix the bug\n\nChange-Id: I1234567"; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn body_and_trailers_roundtrip() { + let original = + "fix the bug\n\nSome explanation.\n\nChange-Id: I1234567\nSigned-off-by: A "; + assert_eq!(stripped(&marked(original)), original); + } + + #[test] + fn description_is_the_trailer_value_not_a_separate_paragraph() { + let result = marked("subject"); + // The description must appear on the same line as GitButler-Conflict: + // (or as indented continuation lines), not as a separate paragraph. + let trailer_start = format!("{CONFLICT_TRAILER_TOKEN}:"); + let conflict_line = result + .lines() + .find(|l| l.starts_with(&trailer_start)) + .expect("trailer line must exist"); + assert!( + conflict_line.len() > trailer_start.len(), + "trailer token must have an inline value, got: {conflict_line:?}" + ); + } + + #[test] + fn prefix_without_trailer_is_not_conflicted() { + assert!(!message_is_marked_conflicted( + "[conflict] looks real but no trailer" + )); + } + + #[test] + fn trailer_without_prefix_is_still_conflicted() { + let msg = "normal commit\n\nGitButler-Conflict: sneaky"; + // Detection depends only on the trailer, not the prefix. + assert!(message_is_marked_conflicted(msg)); + // Strip removes the trailer even without the prefix. + assert_eq!(stripped(msg), "normal commit"); + } + + #[test] + fn add_is_idempotent() { + let original = "subject"; + let once = marked(original); + let twice = marked(&once); + + assert!(message_is_marked_conflicted(&once)); + assert_eq!(twice, once); + assert_eq!(stripped(&once), stripped(&stripped(&once))); + } + + #[test] + fn strip_is_idempotent() { + let original = marked("subject"); + let once = stripped(&original); + let twice = stripped(&once); + + assert_eq!(twice, once); + } + + #[test] + fn trailing_blank_lines_after_trailer_still_detected() { + let msg = format!("subject\n\n{CONFLICT_TRAILER}\n\n"); + assert!( + message_is_marked_conflicted(&msg), + "trailing blank lines must not break detection" + ); + } + + /// The trailer token appearing in the body (not the last paragraph) must + /// not be stripped — only the actual trailer in the last paragraph is removed. + #[test] + fn strip_only_removes_trailer_from_last_paragraph() { + let body_with_token = format!( + "[conflict] subject\n\nGitButler-Conflict: mentioned in body\n\n{CONFLICT_TRAILER}\n" + ); + assert!(message_is_marked_conflicted(&body_with_token)); + let result = stripped(&body_with_token); + assert!( + result.contains("GitButler-Conflict: mentioned in body"), + "body occurrence must be preserved, got: {result:?}" + ); + assert!( + !result.contains("This is a GitButler-managed"), + "the trailer itself must be removed" + ); + } + + #[test] + fn rewrite_does_not_double_prefix() { + let original = "fix bug"; + let conflicted = marked(original); + // Simulate a new message that already has the prefix but no trailer. + let partial = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); + let result = rewrite_conflict_markers_on_message_change( + BStr::new(&conflicted), + BString::from(partial), + ); + let result_str = std::str::from_utf8(result.as_ref()).unwrap(); + // Must not produce "[conflict] [conflict] fix bug". + let prefix_count = result_str.matches(CONFLICT_MESSAGE_PREFIX).count(); + assert_eq!(prefix_count, 1, "prefix must appear exactly once"); + assert!(message_is_marked_conflicted(result_str)); + } + + #[test] + fn detects_conflicts_from_headers_too() { + assert!(is_conflicted( + BStr::new("ordinary message"), + Some(&Headers { + change_id: None, + conflicted: Some(1), + }), + )); + assert!(!is_conflicted( + BStr::new("ordinary message"), + Some(&Headers::default()), + )); + assert!(!is_conflicted(BStr::new("ordinary message"), None)); + } + + /// Verify that gix parses the `GitButler-Conflict` trailer alongside + /// other standard trailers (Byron's review feedback). + #[test] + fn gix_parses_conflict_trailer_with_existing_trailers() { + let original = + "fix the bug\n\nSome body.\n\nChange-Id: I1234567\nSigned-off-by: A "; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + let body = msg.body().expect("message must have a body"); + let trailers: Vec<_> = body.trailers().collect(); + + let tokens: Vec<&str> = trailers.iter().map(|t| t.token.to_str().unwrap()).collect(); + assert!( + tokens.contains(&"Change-Id"), + "Change-Id trailer must be parseable by gix, got: {tokens:?}" + ); + assert!( + tokens.contains(&"Signed-off-by"), + "Signed-off-by trailer must be parseable by gix, got: {tokens:?}" + ); + assert!( + tokens.contains(&"GitButler-Conflict"), + "GitButler-Conflict trailer must be parseable by gix, got: {tokens:?}" + ); + } + + /// The `GitButler-Conflict` trailer must always be the last trailer in + /// the message so it does not interfere with other trailer-based tools. + #[test] + fn conflict_trailer_is_last() { + let original = "fix bug\n\nSome body.\n\nChange-Id: I123\nSigned-off-by: A "; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + let body = msg.body().expect("message must have a body"); + let trailers: Vec<_> = body.trailers().collect(); + let last = trailers.last().expect("must have at least one trailer"); + assert_eq!( + last.token.to_str().unwrap(), + "GitButler-Conflict", + "conflict trailer must be the last trailer" + ); + } + + /// Verify gix sees the `[conflict]` prefix in the title even for + /// subject-only messages. + /// + /// Note: gix's trailer parser does not detect trailers when the body + /// consists of only a single trailer paragraph (no preceding body text). + /// Our manual detection handles this case; the gix interop tests below + /// verify that messages WITH body text are parsed correctly by standard + /// git trailer tools. + #[test] + fn subject_only_roundtrip_with_gix() { + let original = "fix the bug"; + let result = marked(original); + + let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); + assert_eq!( + msg.title.to_str().unwrap(), + "[conflict] fix the bug", + "gix must see the prefixed title" + ); + + // Round-trip + assert_eq!(stripped(&result), original); + } +} diff --git a/crates/but-core/src/commit.rs b/crates/but-core/src/commit/mod.rs similarity index 59% rename from crates/but-core/src/commit.rs rename to crates/but-core/src/commit/mod.rs index 09459c3ed44..e60b7c438f8 100644 --- a/crates/but-core/src/commit.rs +++ b/crates/but-core/src/commit/mod.rs @@ -559,8 +559,7 @@ impl<'repo> Commit<'repo> { /// 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 { - message_is_conflicted(self.inner.message.as_ref()) - || 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, @@ -691,471 +690,8 @@ impl ConflictEntries { } } -/// The prefix prepended to the commit subject line to mark a conflicted commit. -pub const CONFLICT_MESSAGE_PREFIX: &str = "[conflict] "; - -/// The git trailer token used to identify GitButler-managed conflicted commits. -/// The description explaining the conflict is embedded as the multi-line trailer -/// value, with continuation lines indented by 3 spaces per git convention. -const CONFLICT_TRAILER_TOKEN: &str = "GitButler-Conflict"; - -/// The full multi-line git trailer appended to conflicted commit messages. -/// The description is the trailer value; continuation lines are indented with -/// 3 spaces so standard git trailer tools can parse and manipulate them. -const CONFLICT_TRAILER: &str = concat!( - "GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved\n", - " using the \"ours\" side. The commit tree contains additional directories:\n", - " .conflict-side-0 \u{2014} our tree\n", - " .conflict-side-1 \u{2014} their tree\n", - " .conflict-base-0 \u{2014} the merge base tree\n", - " .auto-resolution \u{2014} the auto-resolved tree\n", - " .conflict-files \u{2014} metadata about conflicted files\n", - " To manually resolve, check out this commit, remove the directories\n", - " listed above, resolve the conflicts, and amend the commit." -); - -/// Add conflict markers to a commit message: prepend `[conflict] ` to the -/// subject and append the `GitButler-Conflict` multi-line trailer after any -/// existing trailers. -/// -/// A single trailing newline is trimmed from the input before markers are -/// added; callers that need byte-exact round-trips should account for this. -pub fn add_conflict_markers(message: &BStr) -> BString { - let trimmed = message - .as_bytes() - .strip_suffix(b"\n") - .unwrap_or(message.as_bytes()); - - let capacity = CONFLICT_MESSAGE_PREFIX.len() + trimmed.len() + 2 + CONFLICT_TRAILER.len() + 1; - let mut result = BString::from(Vec::with_capacity(capacity)); - result.extend_from_slice(CONFLICT_MESSAGE_PREFIX.as_bytes()); - result.extend_from_slice(trimmed); - - // Trailers must be in the last paragraph — join directly if one exists. - if ends_in_trailer_block(trimmed) { - result.push(b'\n'); - } else { - result.extend_from_slice(b"\n\n"); - } - result.extend_from_slice(CONFLICT_TRAILER.as_bytes()); - result.push(b'\n'); - result -} - -/// Strip conflict markers from a commit message. -/// Returns the message unchanged when it is not conflicted (per -/// [`message_is_conflicted`]). -/// -/// Strips the `[conflict] ` subject prefix (if present) and the -/// `GitButler-Conflict` trailer line together with all its indented -/// continuation lines. -/// -/// Note: the returned message may not be byte-identical to the original — -/// trailing newlines are not preserved and line endings may be normalized. -pub fn strip_conflict_markers(message: &BStr) -> BString { - if !message_is_conflicted(message) { - return message.to_owned(); - } - - let bytes = message.as_bytes(); - - // Strip the subject prefix if present. - let without_prefix = bytes - .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) - .unwrap_or(bytes); - - // Remove the GitButler-Conflict trailer only from the last paragraph - // to avoid accidentally stripping user-authored content earlier in the body - // that happens to match the trailer token. - let lines: Vec<&[u8]> = without_prefix.lines().collect(); - let mut result_lines: Vec<&[u8]> = Vec::with_capacity(lines.len()); - - // Find the start of the last paragraph (after the last blank line). - let last_para_start = lines - .iter() - .enumerate() - .rev() - .find(|(_, l)| l.is_empty()) - .map(|(i, _)| i + 1) - .unwrap_or(0); - - // Copy everything before the last paragraph unchanged. - let mut i = 0; - while i < last_para_start { - result_lines.push(lines[i]); - i += 1; - } - - // In the last paragraph, strip the conflict trailer and its continuation lines. - while i < lines.len() { - let line = lines[i]; - if line_starts_with_conflict_trailer(line) { - i += 1; - while i < lines.len() && lines[i].first().is_some_and(|b| b.is_ascii_whitespace()) { - i += 1; - } - } else { - result_lines.push(line); - i += 1; - } - } - - // Drop trailing blank lines left behind after removing the trailer. - while result_lines.last().is_some_and(|l| l.is_empty()) { - result_lines.pop(); - } - - let mut result = BString::from(Vec::new()); - for (idx, line) in result_lines.iter().enumerate() { - if idx > 0 { - result.push(b'\n'); - } - result.extend_from_slice(line); - } - result -} - -/// Returns `true` when the commit message contains a `GitButler-Conflict:` -/// trailer in the last paragraph. The `[conflict] ` subject prefix is -/// informational and not required for detection. -/// -/// Trailing blank lines are skipped so that messages edited by users or tools -/// that append newlines are still detected correctly. -/// -/// Note: once the upstream fix for `gix`'s `BodyRef::from_bytes` (which -/// currently fails to detect trailers that are the sole body content) is -/// released, this can be simplified to use `gix`'s trailer parser directly. -pub fn message_is_conflicted(message: &BStr) -> bool { - let bytes = message.as_bytes(); - let mut in_content = false; - for line in bytes.lines().rev() { - if line.is_empty() { - if in_content { - break; - } - // Skip trailing blank lines before the last paragraph. - continue; - } - in_content = true; - if line_starts_with_conflict_trailer(line) { - return true; - } - } - false -} - -/// If `old_message` is conflicted but `new_message` is not, re-apply the -/// conflict markers to `new_message`. This is used during reword and squash -/// so that editing a conflicted commit's message doesn't silently drop the -/// conflict state. -/// -/// Strips any existing partial markers from `new_message` before re-adding -/// to avoid double-prefixing or duplicate trailers. -pub fn rewrite_conflict_markers_on_message_change( - old_message: &BStr, - new_message: BString, -) -> BString { - if message_is_conflicted(old_message) && !message_is_conflicted(new_message.as_ref()) { - // Strip the `[conflict] ` prefix if the user left it in, - // then re-add the full set of markers. - let clean = new_message - .as_bytes() - .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) - .map(BString::from) - .unwrap_or(new_message); - add_conflict_markers(clean.as_ref()) - } else { - new_message - } -} - -/// Returns `true` if `bytes` ends with a git trailer block — a paragraph where -/// every line is either a `Token: value` trailer or an indented continuation. -fn ends_in_trailer_block(bytes: &[u8]) -> bool { - let lines: Vec<&[u8]> = bytes.lines().collect(); - - // Trailers must be in a paragraph separated by a blank line from the subject. - // If there is no blank line, there is no trailer block. - let Some(blank_pos) = lines - .iter() - .enumerate() - .rev() - .find(|(_, l)| l.is_empty()) - .map(|(i, _)| i) - else { - return false; - }; - let para_start = blank_pos + 1; - - let para = &lines[para_start..]; - if para.is_empty() { - return false; - } - - let mut found_any = false; - let mut prev_was_trailer_or_continuation = false; - for line in para { - let is_continuation = line.first().is_some_and(|b| b.is_ascii_whitespace()); - if is_continuation { - if !prev_was_trailer_or_continuation { - return false; - } - prev_was_trailer_or_continuation = true; - } else if is_trailer_line(line) { - found_any = true; - prev_was_trailer_or_continuation = true; - } else { - return false; - } - } - found_any -} - -/// Returns `true` for lines of the form `Token: value` where `Token` contains -/// no spaces and the value is non-empty (i.e. `: ` follows the token). -fn is_trailer_line(line: &[u8]) -> bool { - let Some(colon) = line.find_byte(b':') else { - return false; - }; - if colon == 0 { - return false; - } - let token = &line[..colon]; - !token.contains(&b' ') && line.get(colon + 1) == Some(&b' ') -} - -/// Returns `true` when `line` starts with `GitButler-Conflict:`. -fn line_starts_with_conflict_trailer(line: &[u8]) -> bool { - line.strip_prefix(CONFLICT_TRAILER_TOKEN.as_bytes()) - .is_some_and(|rest| rest.first() == Some(&b':')) -} - -#[cfg(test)] -mod conflict_marker_tests { - use bstr::BStr; - - use super::*; - - fn marked(msg: &str) -> String { - String::from_utf8(add_conflict_markers(BStr::new(msg)).into()).unwrap() - } - - fn stripped(msg: &str) -> String { - String::from_utf8(strip_conflict_markers(BStr::new(msg)).into()).unwrap() - } - - fn is_conflicted(msg: &str) -> bool { - message_is_conflicted(BStr::new(msg)) - } - - /// Round-trip: add then strip returns the original (modulo the trailing - /// newline that `add_conflict_markers` always trims). - #[test] - fn simple_subject_roundtrip() { - let original = "fix the bug"; - let result = stripped(&marked(original)); - assert_eq!(result, original); - assert!(is_conflicted(&marked(original))); - } - - #[test] - fn trailing_newline_is_trimmed_by_add() { - // add_conflict_markers trims a trailing newline; strip reflects that. - assert_eq!(stripped(&marked("fix the bug\n")), "fix the bug"); - } - - #[test] - fn subject_and_body_roundtrip() { - let original = "fix the bug\n\nDetailed explanation here."; - assert_eq!(stripped(&marked(original)), original); - } - - #[test] - fn existing_trailers_are_preserved_and_ours_comes_last() { - let original = "fix the bug\n\nChange-Id: I1234567\nSigned-off-by: User "; - let result = marked(original); - assert!(is_conflicted(&result)); - - // Existing trailers must still be present - assert!(result.contains("Change-Id: I1234567\n")); - assert!(result.contains("Signed-off-by: User \n")); - - // Our trailer must come after the existing ones - let signed_pos = result.find("Signed-off-by:").unwrap(); - let conflict_pos = result.find(CONFLICT_TRAILER_TOKEN).unwrap(); - assert!( - conflict_pos > signed_pos, - "GitButler-Conflict trailer must follow existing trailers" - ); - - // Roundtrip - assert_eq!(stripped(&result), original); - } - - #[test] - fn subject_with_only_trailers_roundtrip() { - let original = "fix the bug\n\nChange-Id: I1234567"; - assert_eq!(stripped(&marked(original)), original); - } - - #[test] - fn body_and_trailers_roundtrip() { - let original = - "fix the bug\n\nSome explanation.\n\nChange-Id: I1234567\nSigned-off-by: A "; - assert_eq!(stripped(&marked(original)), original); - } - - #[test] - fn description_is_the_trailer_value_not_a_separate_paragraph() { - let result = marked("subject"); - // The description must appear on the same line as GitButler-Conflict: - // (or as indented continuation lines), not as a separate paragraph. - let trailer_start = format!("{CONFLICT_TRAILER_TOKEN}:"); - let conflict_line = result - .lines() - .find(|l| l.starts_with(&trailer_start)) - .expect("trailer line must exist"); - assert!( - conflict_line.len() > trailer_start.len(), - "trailer token must have an inline value, got: {conflict_line:?}" - ); - } - - #[test] - fn prefix_without_trailer_is_not_conflicted() { - assert!(!is_conflicted("[conflict] looks real but no trailer")); - } - - #[test] - fn trailer_without_prefix_is_still_conflicted() { - let msg = "normal commit\n\nGitButler-Conflict: sneaky"; - // Detection depends only on the trailer, not the prefix. - assert!(is_conflicted(msg)); - // Strip removes the trailer even without the prefix. - assert_eq!(stripped(msg), "normal commit"); - } - - #[test] - fn add_is_idempotent_when_guarded() { - let original = "subject"; - let once = marked(original); - // Callers guard with message_is_conflicted; verify that guard works - assert!(is_conflicted(&once)); - // Stripping twice is also stable - assert_eq!(stripped(&once), stripped(&stripped(&once))); - } - - #[test] - fn trailing_blank_lines_after_trailer_still_detected() { - let msg = format!("subject\n\n{CONFLICT_TRAILER}\n\n"); - assert!( - is_conflicted(&msg), - "trailing blank lines must not break detection" - ); - } - - /// The trailer token appearing in the body (not the last paragraph) must - /// not be stripped — only the actual trailer in the last paragraph is removed. - #[test] - fn strip_only_removes_trailer_from_last_paragraph() { - let body_with_token = format!( - "[conflict] subject\n\nGitButler-Conflict: mentioned in body\n\n{CONFLICT_TRAILER}\n" - ); - assert!(is_conflicted(&body_with_token)); - let result = stripped(&body_with_token); - assert!( - result.contains("GitButler-Conflict: mentioned in body"), - "body occurrence must be preserved, got: {result:?}" - ); - assert!( - !result.contains("This is a GitButler-managed"), - "the trailer itself must be removed" - ); - } - - #[test] - fn rewrite_does_not_double_prefix() { - let original = "fix bug"; - let conflicted = marked(original); - // Simulate a new message that already has the prefix but no trailer. - let partial = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); - let result = rewrite_conflict_markers_on_message_change( - BStr::new(&conflicted), - BString::from(partial), - ); - let result_str = std::str::from_utf8(result.as_ref()).unwrap(); - // Must not produce "[conflict] [conflict] fix bug". - let prefix_count = result_str.matches(CONFLICT_MESSAGE_PREFIX).count(); - assert_eq!(prefix_count, 1, "prefix must appear exactly once"); - assert!(is_conflicted(result_str)); - } - - /// Verify that gix parses the `GitButler-Conflict` trailer alongside - /// other standard trailers (Byron's review feedback). - #[test] - fn gix_parses_conflict_trailer_with_existing_trailers() { - let original = - "fix the bug\n\nSome body.\n\nChange-Id: I1234567\nSigned-off-by: A "; - let result = marked(original); - - let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); - let body = msg.body().expect("message must have a body"); - let trailers: Vec<_> = body.trailers().collect(); - - let tokens: Vec<&str> = trailers.iter().map(|t| t.token.to_str().unwrap()).collect(); - assert!( - tokens.contains(&"Change-Id"), - "Change-Id trailer must be parseable by gix, got: {tokens:?}" - ); - assert!( - tokens.contains(&"Signed-off-by"), - "Signed-off-by trailer must be parseable by gix, got: {tokens:?}" - ); - assert!( - tokens.contains(&"GitButler-Conflict"), - "GitButler-Conflict trailer must be parseable by gix, got: {tokens:?}" - ); - } - - /// The `GitButler-Conflict` trailer must always be the last trailer in - /// the message so it does not interfere with other trailer-based tools. - #[test] - fn conflict_trailer_is_last() { - let original = "fix bug\n\nSome body.\n\nChange-Id: I123\nSigned-off-by: A "; - let result = marked(original); - - let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); - let body = msg.body().expect("message must have a body"); - let trailers: Vec<_> = body.trailers().collect(); - let last = trailers.last().expect("must have at least one trailer"); - assert_eq!( - last.token.to_str().unwrap(), - "GitButler-Conflict", - "conflict trailer must be the last trailer" - ); - } - - /// Verify gix sees the `[conflict]` prefix in the title even for - /// subject-only messages. - /// - /// Note: gix's trailer parser does not detect trailers when the body - /// consists of only a single trailer paragraph (no preceding body text). - /// Our manual detection handles this case; the gix interop tests below - /// verify that messages WITH body text are parsed correctly by standard - /// git trailer tools. - #[test] - fn subject_only_roundtrip_with_gix() { - let original = "fix the bug"; - let result = marked(original); - - let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); - assert_eq!( - msg.title.to_str().unwrap(), - "[conflict] fix the bug", - "gix must see the prefixed title" - ); - - // Round-trip - assert_eq!(stripped(&result), original); - } -} +mod conflict; +pub use conflict::{ + CONFLICT_MESSAGE_PREFIX, add_conflict_markers, is_conflicted, message_is_conflicted, + rewrite_conflict_markers_on_message_change, strip_conflict_markers, +}; diff --git a/crates/but-rebase/src/cherry_pick.rs b/crates/but-rebase/src/cherry_pick.rs index c172bdd3b2c..626d323dba9 100644 --- a/crates/but-rebase/src/cherry_pick.rs +++ b/crates/but-rebase/src/cherry_pick.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( diff --git a/crates/but-rebase/src/graph_rebase/cherry_pick.rs b/crates/but-rebase/src/graph_rebase/cherry_pick.rs index e8088726299..ed1619e6be6 100644 --- a/crates/but-rebase/src/graph_rebase/cherry_pick.rs +++ b/crates/but-rebase/src/graph_rebase/cherry_pick.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( diff --git a/crates/but-workspace/src/branch_details.rs b/crates/but-workspace/src/branch_details.rs index cad195ca97c..98b26b5fb9c 100644 --- a/crates/but-workspace/src/branch_details.rs +++ b/crates/but-workspace/src/branch_details.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(), diff --git a/crates/but-workspace/src/ref_info.rs b/crates/but-workspace/src/ref_info.rs index 61c62ec8e54..c4ffa3d7dac 100644 --- a/crates/but-workspace/src/ref_info.rs +++ b/crates/but-workspace/src/ref_info.rs @@ -69,11 +69,7 @@ impl From> 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(), diff --git a/crates/but-workspace/src/ui/mod.rs b/crates/but-workspace/src/ui/mod.rs index b4e77c378b1..69650bf7ba7 100644 --- a/crates/but-workspace/src/ui/mod.rs +++ b/crates/but-workspace/src/ui/mod.rs @@ -118,19 +118,14 @@ impl TryFrom> 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> for Commit { impl From 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 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(), diff --git a/crates/gitbutler-commit/src/commit_ext.rs b/crates/gitbutler-commit/src/commit_ext.rs index 895ca7949e2..1b6517bc7bd 100644 --- a/crates/gitbutler-commit/src/commit_ext.rs +++ b/crates/gitbutler-commit/src/commit_ext.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()) } } diff --git a/crates/gitbutler-edit-mode/src/lib.rs b/crates/gitbutler-edit-mode/src/lib.rs index 2f172d049ba..4a8d13421d8 100644 --- a/crates/gitbutler-edit-mode/src/lib.rs +++ b/crates/gitbutler-edit-mode/src/lib.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 { From 79ac145ec8986dbc764aa8bbd80e81289e81cf1b Mon Sep 17 00:00:00 2001 From: "GPT 5.4" Date: Wed, 15 Apr 2026 08:30:44 +0800 Subject: [PATCH 3/3] refactor - use `gix` more and avoid line-ending change Co-authored-by: Sebastian Thiel --- crates/but-core/src/commit/conflict.rs | 414 ++++++++++-------- crates/but-core/src/commit/mod.rs | 2 +- crates/but-workspace/src/legacy/stacks.rs | 8 +- crates/gitbutler-edit-mode/src/lib.rs | 43 +- crates/gitbutler-edit-mode/tests/edit_mode.rs | 2 +- .../tests/fixtures/edit_mode.sh | 4 +- 6 files changed, 245 insertions(+), 228 deletions(-) diff --git a/crates/but-core/src/commit/conflict.rs b/crates/but-core/src/commit/conflict.rs index 33f90de293c..f0ed06c646d 100644 --- a/crates/but-core/src/commit/conflict.rs +++ b/crates/but-core/src/commit/conflict.rs @@ -3,12 +3,12 @@ use bstr::{BStr, BString, ByteSlice}; use super::Headers; /// The prefix prepended to the commit subject line to mark a conflicted commit. -pub const CONFLICT_MESSAGE_PREFIX: &str = "[conflict] "; +const CONFLICT_MESSAGE_PREFIX: &[u8] = b"[conflict] "; /// The git trailer token used to identify GitButler-managed conflicted commits. /// The description explaining the conflict is embedded as the multi-line trailer /// value, with continuation lines indented by 3 spaces per git convention. -const CONFLICT_TRAILER_TOKEN: &str = "GitButler-Conflict"; +const CONFLICT_TRAILER_TOKEN: &[u8] = b"GitButler-Conflict"; /// The full multi-line git trailer appended to conflicted commit messages. /// The description is the trailer value; continuation lines are indented with @@ -36,24 +36,26 @@ pub fn add_conflict_markers(message: &BStr) -> BString { if message_is_conflicted(message) { return message.into(); } - let trimmed = message - .as_bytes() - .strip_suffix(b"\n") - .unwrap_or(message.as_bytes()); - - let capacity = CONFLICT_MESSAGE_PREFIX.len() + trimmed.len() + 2 + CONFLICT_TRAILER.len() + 1; - let mut result = BString::from(Vec::with_capacity(capacity)); - result.extend_from_slice(CONFLICT_MESSAGE_PREFIX.as_bytes()); - result.extend_from_slice(trimmed); - - // Trailers must be in the last paragraph — join directly if one exists. - if ends_in_trailer_block(trimmed) { - result.push(b'\n'); - } else { - result.extend_from_slice(b"\n\n"); - } - result.extend_from_slice(CONFLICT_TRAILER.as_bytes()); - result.push(b'\n'); + let trimmed = strip_single_trailing_newline(message.as_bytes()); + let clean = trimmed + .strip_prefix(CONFLICT_MESSAGE_PREFIX) + .unwrap_or(trimmed); + let line_ending = line_ending_for(message.as_bytes()); + let has_trailer_block = gix::objs::commit::MessageRef::from_bytes(clean) + .body() + .is_some_and(|body| body.trailers().next().is_some()); + + let mut result = BString::default(); + result.extend_from_slice(CONFLICT_MESSAGE_PREFIX); + result.extend_from_slice(clean); + + // Keep trailers in the last paragraph if the message already has a trailer block. + result.extend_from_slice(line_ending); + if !has_trailer_block { + result.extend_from_slice(line_ending); + } + push_with_line_endings(&mut result, CONFLICT_TRAILER.as_bytes(), line_ending); + result.extend_from_slice(line_ending); result } @@ -66,68 +68,47 @@ pub fn add_conflict_markers(message: &BStr) -> BString { /// continuation lines. /// /// Note: the returned message may not be byte-identical to the original — -/// trailing newlines are not preserved and line endings may be normalized. +/// trailing newlines are not preserved. pub fn strip_conflict_markers(message: &BStr) -> BString { - if !message_is_conflicted(message) { - return message.to_owned(); - } - - let bytes = message.as_bytes(); + let msg_bytes = message.as_bytes(); + let line_ending = line_ending_for(msg_bytes); // Strip the subject prefix if present. - let without_prefix = bytes - .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) - .unwrap_or(bytes); - - // Remove the GitButler-Conflict trailer only from the last paragraph - // to avoid accidentally stripping user-authored content earlier in the body - // that happens to match the trailer token. - let lines: Vec<&[u8]> = without_prefix.lines().collect(); - let mut result_lines: Vec<&[u8]> = Vec::with_capacity(lines.len()); - - // Find the start of the last paragraph (after the last blank line). - let last_para_start = lines - .iter() - .enumerate() - .rev() - .find(|(_, l)| l.is_empty()) - .map(|(i, _)| i + 1) - .unwrap_or(0); - - // Copy everything before the last paragraph unchanged. - let mut i = 0; - while i < last_para_start { - result_lines.push(lines[i]); - i += 1; - } - - // In the last paragraph, strip the conflict trailer and its continuation lines. - while i < lines.len() { - let line = lines[i]; - if line_starts_with_conflict_trailer(line) { - i += 1; - while i < lines.len() && lines[i].first().is_some_and(|b| b.is_ascii_whitespace()) { - i += 1; - } - } else { - result_lines.push(line); - i += 1; + let without_prefix = msg_bytes + .strip_prefix(CONFLICT_MESSAGE_PREFIX) + .unwrap_or(msg_bytes); + let message = gix::objs::commit::MessageRef::from_bytes(without_prefix); + let trailer_bytes = conflict_trailer_bytes(line_ending); + let trailer_start = message.body.and_then(|body| { + let body_offset = subslice_offset(without_prefix, body.as_bytes()); + gix::objs::commit::message::BodyRef::from_bytes(body.as_bytes()) + .trailers() + .find(|trailer| trailer.token.eq_ignore_ascii_case(CONFLICT_TRAILER_TOKEN)) + .map(|trailer| body_offset + subslice_offset(body.as_bytes(), trailer.token.as_bytes())) + }); + + if let Some(trailer_start) = trailer_start { + if without_prefix[trailer_start..].starts_with(trailer_bytes.as_slice()) { + let before = &without_prefix[..trailer_start]; + let after = &without_prefix[trailer_start + trailer_bytes.len()..]; + let mut out = BString::from(before); + out.extend_from_slice(after); + return trim_trailing_line_endings(out.as_ref()).into(); } - } - // Drop trailing blank lines left behind after removing the trailer. - while result_lines.last().is_some_and(|l| l.is_empty()) { - result_lines.pop(); + return trim_trailing_line_endings(&without_prefix[..trailer_start]).into(); } - let mut result = BString::from(Vec::new()); - for (idx, line) in result_lines.iter().enumerate() { - if idx > 0 { - result.push(b'\n'); - } - result.extend_from_slice(line); - } - result + without_prefix.find(trailer_bytes.as_slice()).map_or_else( + || msg_bytes.into(), + |trailer_start| { + let before = trim_trailing_line_endings(&without_prefix[..trailer_start]); + let after = &without_prefix[trailer_start + trailer_bytes.len()..]; + let mut out = BString::from(before); + out.extend_from_slice(after); + out + }, + ) } /// Returns `true` when the commit is conflicted either by message marker @@ -137,28 +118,18 @@ pub fn is_conflicted(message: &BStr, headers: Option<&Headers>) -> bool { } /// Returns `true` when the commit message contains a `GitButler-Conflict:` -/// trailer in the last paragraph. The `[conflict] ` subject prefix is -/// informational and not required for detection. +/// trailer. The `[conflict] ` subject prefix is informational and +/// not required for detection. /// /// Trailing blank lines are skipped so that messages edited by users or tools /// that append newlines are still detected correctly. pub fn message_is_conflicted(message: &BStr) -> bool { - let bytes = message.as_bytes(); - let mut in_content = false; - for line in bytes.lines().rev() { - if line.is_empty() { - if in_content { - break; - } - // Skip trailing blank lines before the last paragraph. - continue; - } - in_content = true; - if line_starts_with_conflict_trailer(line) { - return true; - } - } - false + let message = gix::objs::commit::MessageRef::from_bytes(message.as_bytes()); + let Some(body) = message.body() else { + return false; + }; + body.trailers() + .any(|trailer| trailer.token.eq_ignore_ascii_case(CONFLICT_TRAILER_TOKEN)) } /// If `old_message` is conflicted but `new_message` is not, re-apply the @@ -173,86 +144,73 @@ pub fn rewrite_conflict_markers_on_message_change( new_message: BString, ) -> BString { if message_is_conflicted(old_message) && !message_is_conflicted(new_message.as_ref()) { - // Strip the `[conflict] ` prefix if the user left it in, - // then re-add the full set of markers. - let clean = new_message - .as_bytes() - .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) - .map(BString::from) - .unwrap_or(new_message); - add_conflict_markers(clean.as_ref()) + add_conflict_markers(new_message.as_ref()) } else { new_message } } -/// Returns `true` if `bytes` ends with a git trailer block — a paragraph where -/// every line is either a `Token: value` trailer or an indented continuation. -fn ends_in_trailer_block(bytes: &[u8]) -> bool { - let lines: Vec<&[u8]> = bytes.lines().collect(); - - // Trailers must be in a paragraph separated by a blank line from the subject. - // If there is no blank line, there is no trailer block. - let Some(blank_pos) = lines - .iter() - .enumerate() - .rev() - .find(|(_, l)| l.is_empty()) - .map(|(i, _)| i) - else { - return false; - }; - let para_start = blank_pos + 1; - - let para = &lines[para_start..]; - if para.is_empty() { - return false; +fn line_ending_for(message: &[u8]) -> &'static [u8] { + if message.find(b"\r\n").is_some() { + b"\r\n" + } else { + b"\n" } +} - let mut found_any = false; - let mut prev_was_trailer_or_continuation = false; - for line in para { - let is_continuation = line.first().is_some_and(|b| b.is_ascii_whitespace()); - if is_continuation { - if !prev_was_trailer_or_continuation { - return false; - } - prev_was_trailer_or_continuation = true; - } else if is_trailer_line(line) { - found_any = true; - prev_was_trailer_or_continuation = true; - } else { - return false; - } - } - found_any +fn strip_single_trailing_newline(bytes: &[u8]) -> &[u8] { + bytes + .strip_suffix(b"\r\n") + .or_else(|| bytes.strip_suffix(b"\n")) + .unwrap_or(bytes) } -/// Returns `true` for lines of the form `Token: value` where `Token` contains -/// no spaces and the value is non-empty (i.e. `: ` follows the token). -fn is_trailer_line(line: &[u8]) -> bool { - let Some(colon) = line.find_byte(b':') else { - return false; - }; - if colon == 0 { - return false; +fn subslice_offset(base: &[u8], subslice: &[u8]) -> usize { + subslice.as_ptr() as usize - base.as_ptr() as usize +} + +fn trim_trailing_line_endings(mut bytes: &[u8]) -> &[u8] { + while let Some(stripped) = bytes + .strip_suffix(b"\r\n") + .or_else(|| bytes.strip_suffix(b"\n")) + .or_else(|| bytes.strip_suffix(b"\r")) + { + bytes = stripped; } - let token = &line[..colon]; - !token.contains(&b' ') && line.get(colon + 1) == Some(&b' ') + bytes +} + +fn conflict_trailer_bytes(line_ending: &[u8]) -> BString { + let mut out = BString::default(); + push_with_line_endings(&mut out, CONFLICT_TRAILER.as_bytes(), line_ending); + out.extend_from_slice(line_ending); + out } -/// Returns `true` when `line` starts with `GitButler-Conflict:`. -fn line_starts_with_conflict_trailer(line: &[u8]) -> bool { - line.strip_prefix(CONFLICT_TRAILER_TOKEN.as_bytes()) - .is_some_and(|rest| rest.first() == Some(&b':')) +fn push_with_line_endings(out: &mut BString, text: &[u8], line_ending: &[u8]) { + let mut start = 0; + for (idx, byte) in text.iter().enumerate() { + if *byte == b'\n' { + out.extend_from_slice(&text[start..idx]); + out.extend_from_slice(line_ending); + start = idx + 1; + } + } + out.extend_from_slice(&text[start..]); } #[cfg(test)] mod tests { - use crate::commit::Headers; - use bstr::BStr; + use crate::commit::{ + Headers, add_conflict_markers, is_conflicted, message_is_conflicted, + rewrite_conflict_markers_on_message_change, strip_conflict_markers, + }; + use bstr::{BStr, BString, ByteSlice}; + + use super::CONFLICT_TRAILER; - use super::*; + const CONFLICT_MESSAGE_PREFIX: &str = "[conflict] "; + const CONFLICT_TRAILER_TOKEN: &str = "GitButler-Conflict"; fn marked(msg: &str) -> String { String::from_utf8(add_conflict_markers(BStr::new(msg)).into()).unwrap() @@ -266,6 +224,18 @@ mod tests { message_is_conflicted(BStr::new(msg)) } + fn assert_only_crlf_line_endings(message: &[u8]) { + for (idx, byte) in message.iter().enumerate() { + if *byte == b'\n' { + assert_eq!( + idx.checked_sub(1).and_then(|idx| message.get(idx)), + Some(&b'\r'), + "found bare LF at byte index {idx} in {message:?}" + ); + } + } + } + /// Round-trip: add then strip returns the original (modulo the trailing /// newline that `add_conflict_markers` always trims). #[test] @@ -366,6 +336,16 @@ mod tests { assert_eq!(stripped(&once), stripped(&stripped(&once))); } + #[test] + fn add_does_not_double_prefix_prefix_only_messages() { + let partial = format!("{CONFLICT_MESSAGE_PREFIX}subject"); + let result = marked(&partial); + + assert_eq!(result.matches(CONFLICT_MESSAGE_PREFIX).count(), 1); + assert!(message_is_marked_conflicted(&result)); + assert_eq!(stripped(&result), "subject"); + } + #[test] fn strip_is_idempotent() { let original = marked("subject"); @@ -403,15 +383,43 @@ mod tests { ); } + #[test] + fn strip_removes_our_trailer_even_with_arbitrary_suffix_after_it() { + let original = "subject\n\nbody"; + let suffixed = format!( + "{marked_message}\n\nthis suffix is arbitrary prose, not a trailer", + marked_message = marked(original) + ); + + assert_eq!( + stripped(&suffixed), + "subject\n\nbody\n\nthis suffix is arbitrary prose, not a trailer" + ); + } + + #[test] + fn strip_preserves_following_official_trailer_in_same_trailer_block() { + let original = "subject\n\nbody"; + let suffixed = format!( + "{marked_message}Signed-off-by: A U Thor \n", + marked_message = marked(original) + ); + + assert_eq!( + stripped(&suffixed), + "subject\n\nbody\n\nSigned-off-by: A U Thor " + ); + } + #[test] fn rewrite_does_not_double_prefix() { let original = "fix bug"; let conflicted = marked(original); // Simulate a new message that already has the prefix but no trailer. - let partial = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); + let new_message_with_accidental_prefix = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); let result = rewrite_conflict_markers_on_message_change( - BStr::new(&conflicted), - BString::from(partial), + conflicted.as_str().into(), + new_message_with_accidental_prefix.into(), ); let result_str = std::str::from_utf8(result.as_ref()).unwrap(); // Must not produce "[conflict] [conflict] fix bug". @@ -436,33 +444,6 @@ mod tests { assert!(!is_conflicted(BStr::new("ordinary message"), None)); } - /// Verify that gix parses the `GitButler-Conflict` trailer alongside - /// other standard trailers (Byron's review feedback). - #[test] - fn gix_parses_conflict_trailer_with_existing_trailers() { - let original = - "fix the bug\n\nSome body.\n\nChange-Id: I1234567\nSigned-off-by: A "; - let result = marked(original); - - let msg = gix::objs::commit::MessageRef::from_bytes(BStr::new(&result)); - let body = msg.body().expect("message must have a body"); - let trailers: Vec<_> = body.trailers().collect(); - - let tokens: Vec<&str> = trailers.iter().map(|t| t.token.to_str().unwrap()).collect(); - assert!( - tokens.contains(&"Change-Id"), - "Change-Id trailer must be parseable by gix, got: {tokens:?}" - ); - assert!( - tokens.contains(&"Signed-off-by"), - "Signed-off-by trailer must be parseable by gix, got: {tokens:?}" - ); - assert!( - tokens.contains(&"GitButler-Conflict"), - "GitButler-Conflict trailer must be parseable by gix, got: {tokens:?}" - ); - } - /// The `GitButler-Conflict` trailer must always be the last trailer in /// the message so it does not interfere with other trailer-based tools. #[test] @@ -483,12 +464,6 @@ mod tests { /// Verify gix sees the `[conflict]` prefix in the title even for /// subject-only messages. - /// - /// Note: gix's trailer parser does not detect trailers when the body - /// consists of only a single trailer paragraph (no preceding body text). - /// Our manual detection handles this case; the gix interop tests below - /// verify that messages WITH body text are parsed correctly by standard - /// git trailer tools. #[test] fn subject_only_roundtrip_with_gix() { let original = "fix the bug"; @@ -504,4 +479,59 @@ mod tests { // Round-trip assert_eq!(stripped(&result), original); } + + #[test] + fn mutating_functions_preserve_windows_line_endings() { + let original = "fix the bug\r\n\r\nDetailed explanation here.\r\n\r\nChange-Id: I1234567\r\nSigned-off-by: A "; + let marked = add_conflict_markers(BStr::new(original)); + assert_only_crlf_line_endings(marked.as_ref()); + assert!(message_is_conflicted(marked.as_ref())); + + let stripped = strip_conflict_markers(marked.as_ref()); + assert_only_crlf_line_endings(stripped.as_ref()); + assert_eq!(stripped.as_bytes(), original.as_bytes()); + + let rewritten_message = BString::from( + "[conflict] rewritten subject\r\n\r\nUpdated body.\r\n\r\nChange-Id: I7654321", + ); + let rewritten = + rewrite_conflict_markers_on_message_change(marked.as_ref(), rewritten_message.clone()); + assert_only_crlf_line_endings(rewritten.as_ref()); + assert!(message_is_conflicted(rewritten.as_ref())); + assert_eq!( + strip_conflict_markers(rewritten.as_ref()).as_bytes(), + rewritten_message + .as_bytes() + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .unwrap() + ); + } + + #[test] + fn mutating_functions_preserve_mixed_line_endings() { + let original = "fix the bug\r\n\r\nDetailed explanation here.\n\nChange-Id: I1234567\r\nSigned-off-by: A "; + let marked = add_conflict_markers(BStr::new(original)); + assert!( + marked[CONFLICT_MESSAGE_PREFIX.len()..].starts_with(original.as_bytes()), + "existing message content must remain byte-for-byte intact" + ); + assert!(message_is_conflicted(marked.as_ref())); + + let stripped = strip_conflict_markers(marked.as_ref()); + assert_eq!(stripped.as_bytes(), original.as_bytes()); + + let rewritten_message = BString::from( + "[conflict] rewritten subject\n\r\nUpdated body.\r\n\nChange-Id: I7654321", + ); + let rewritten = + rewrite_conflict_markers_on_message_change(marked.as_ref(), rewritten_message.clone()); + assert!(message_is_conflicted(rewritten.as_ref())); + assert_eq!( + strip_conflict_markers(rewritten.as_ref()).as_bytes(), + rewritten_message + .as_bytes() + .strip_prefix(CONFLICT_MESSAGE_PREFIX.as_bytes()) + .unwrap() + ); + } } diff --git a/crates/but-core/src/commit/mod.rs b/crates/but-core/src/commit/mod.rs index e60b7c438f8..10c73dbba31 100644 --- a/crates/but-core/src/commit/mod.rs +++ b/crates/but-core/src/commit/mod.rs @@ -692,6 +692,6 @@ impl ConflictEntries { mod conflict; pub use conflict::{ - CONFLICT_MESSAGE_PREFIX, add_conflict_markers, is_conflicted, message_is_conflicted, + add_conflict_markers, is_conflicted, message_is_conflicted, rewrite_conflict_markers_on_message_change, strip_conflict_markers, }; diff --git a/crates/but-workspace/src/legacy/stacks.rs b/crates/but-workspace/src/legacy/stacks.rs index b485596bc76..9aec4b480db 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -661,13 +661,7 @@ impl TryFrom<&gix::Commit<'_>> for CommitData { fn try_from(commit: &gix::Commit<'_>) -> std::result::Result { let raw_message: BString = commit.message_bstr().into(); - let mut message = but_core::commit::strip_conflict_markers(raw_message.as_ref()); - // Normalize trailing newlines so that messages differing only in a trailing - // newline still match (strip_conflict_markers may consume a trailing newline - // that the original message had). - while message.last() == Some(&b'\n') { - message.pop(); - } + let message = but_core::commit::strip_conflict_markers(raw_message.as_ref()); Ok(CommitData { message, author: commit.author()?.into(), diff --git a/crates/gitbutler-edit-mode/src/lib.rs b/crates/gitbutler-edit-mode/src/lib.rs index 4a8d13421d8..678face2deb 100644 --- a/crates/gitbutler-edit-mode/src/lib.rs +++ b/crates/gitbutler-edit-mode/src/lib.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use anyhow::{Context as _, Result, bail}; -use bstr::BString; +use bstr::{BString, ByteSlice}; use but_core::{ Commit, RepositoryExt, TreeChange, commit::{Headers, SignCommit}, @@ -16,7 +16,7 @@ use but_oxidize::{ObjectIdExt as _, gix_to_git2_index}; use but_rebase::graph_rebase::{Editor, Pick, Step}; use git2::build::CheckoutBuilder; use gitbutler_cherry_pick::{ConflictedTreeKey, GixRepositoryExt as _}; -use gitbutler_commit::commit_ext::CommitExt; +use gitbutler_commit::commit_ext::{CommitExt, CommitMessageBstr}; use gitbutler_operating_modes::{ EDIT_BRANCH_REF, EditModeMetadata, INTEGRATION_BRANCH_REF, OperatingMode, WORKSPACE_BRANCH_REF, operating_mode, read_edit_mode_metadata, write_edit_mode_metadata, @@ -29,6 +29,17 @@ pub mod commands; const UNCOMMITTED_CHANGES_REF: &str = "refs/gitbutler/edit-uncommitted-changes"; +fn commit_title_to_merge_conflict_label(commit: &gix::Commit<'_>) -> String { + gix::objs::commit::MessageRef::from_bytes( + but_core::commit::strip_conflict_markers(commit.message_bstr()).as_ref(), + ) + .title + .to_str_lossy() + .chars() + .take(80) + .collect() +} + /// Returns an index of the tree of `commit` if it is unconflicted, *or* produce a merged tree /// if `commit` is conflicted. That tree is turned into an index that records the conflicts that occurred /// during the merge. @@ -117,11 +128,11 @@ fn checkout_edit_branch(ctx: &Context, commit_id: gix::ObjectId) -> Result<()> { let repo = &*ctx.repo.get()?; #[expect(deprecated, reason = "checkout/index materialization boundary")] let git2_repo = &*ctx.git2_repo.get()?; - let commit = git2_repo.find_commit(commit_id.to_git2())?; + let commit = commit_id.attach(repo).object()?.try_into_commit()?; // Checkout commits's parent let commit_parent_id = find_or_create_base_commit(repo, commit_id)?; - let commit_parent = git2_repo.find_commit(commit_parent_id.to_git2())?; + let commit_parent = commit_parent_id.attach(repo).object()?.try_into_commit()?; let edit_branch_ref: gix::refs::FullName = EDIT_BRANCH_REF.try_into()?; repo.reference( edit_branch_ref.as_ref(), @@ -142,30 +153,10 @@ fn checkout_edit_branch(ctx: &Context, commit_id: gix::ObjectId) -> Result<()> { // TODO this may not be necessary if the commit is unconflicted let mut index = get_commit_index(ctx, commit_id)?; - let their_commit_msg = commit - .message() - .and_then(|m| m.lines().next()) - .map(|l| { - l.strip_prefix(but_core::commit::CONFLICT_MESSAGE_PREFIX) - .unwrap_or(l) - .chars() - .take(80) - .collect::() - }) - .unwrap_or("".into()); + let their_commit_msg = commit_title_to_merge_conflict_label(&commit); let their_label = format!("Current commit: {their_commit_msg}"); - let our_commit_msg = commit_parent - .message() - .and_then(|m| m.lines().next()) - .map(|l| { - l.strip_prefix(but_core::commit::CONFLICT_MESSAGE_PREFIX) - .unwrap_or(l) - .chars() - .take(80) - .collect::() - }) - .unwrap_or("".into()); + let our_commit_msg = commit_title_to_merge_conflict_label(&commit_parent); let our_label = format!("New base: {our_commit_msg}"); git2_repo.checkout_index( diff --git a/crates/gitbutler-edit-mode/tests/edit_mode.rs b/crates/gitbutler-edit-mode/tests/edit_mode.rs index 29978feb779..27c7a5a297a 100644 --- a/crates/gitbutler-edit-mode/tests/edit_mode.rs +++ b/crates/gitbutler-edit-mode/tests/edit_mode.rs @@ -362,7 +362,7 @@ fn enter_edit_mode_checks_out_conflicted_commit() -> Result<()> { ); insta::assert_debug_snapshot!( repo.head_commit()?.message()?.summary(), - @r#""Changes to make millions""# + @r#""[conflict] Changes to make millions""# ); insta::assert_snapshot!( diff --git a/crates/gitbutler-edit-mode/tests/fixtures/edit_mode.sh b/crates/gitbutler-edit-mode/tests/fixtures/edit_mode.sh index a18512b6109..4aecc7b9b4a 100644 --- a/crates/gitbutler-edit-mode/tests/fixtures/edit_mode.sh +++ b/crates/gitbutler-edit-mode/tests/fixtures/edit_mode.sh @@ -74,7 +74,9 @@ gitbutler-headers-version 2 change-id 00000000-0000-0000-0000-000000000001 gitbutler-conflicted 1 -Changes to make millions +[conflict] Changes to make millions + +GitButler-Conflict: fixture marker EOF )