diff --git a/crates/but-core/src/commit/conflict.rs b/crates/but-core/src/commit/conflict.rs new file mode 100644 index 00000000000..f0ed06c646d --- /dev/null +++ b/crates/but-core/src/commit/conflict.rs @@ -0,0 +1,537 @@ +use bstr::{BStr, BString, ByteSlice}; + +use super::Headers; + +/// The prefix prepended to the commit subject line to mark a conflicted commit. +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: &[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 +/// 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 = 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 +} + +/// 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. +pub fn strip_conflict_markers(message: &BStr) -> BString { + let msg_bytes = message.as_bytes(); + let line_ending = line_ending_for(msg_bytes); + + // Strip the subject prefix if present. + 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(); + } + + return trim_trailing_line_endings(&without_prefix[..trailer_start]).into(); + } + + 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 +/// (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. 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 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 +/// 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()) { + add_conflict_markers(new_message.as_ref()) + } else { + new_message + } +} + +fn line_ending_for(message: &[u8]) -> &'static [u8] { + if message.find(b"\r\n").is_some() { + b"\r\n" + } else { + b"\n" + } +} + +fn strip_single_trailing_newline(bytes: &[u8]) -> &[u8] { + bytes + .strip_suffix(b"\r\n") + .or_else(|| bytes.strip_suffix(b"\n")) + .unwrap_or(bytes) +} + +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; + } + 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 +} + +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, 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; + + 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() + } + + 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)) + } + + 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] + 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 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"); + 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 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 new_message_with_accidental_prefix = format!("{CONFLICT_MESSAGE_PREFIX}fix bug"); + let result = rewrite_conflict_markers_on_message_change( + 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". + 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)); + } + + /// 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. + #[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); + } + + #[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.rs b/crates/but-core/src/commit/mod.rs similarity index 98% rename from crates/but-core/src/commit.rs rename to crates/but-core/src/commit/mod.rs index 5a51003706f..10c73dbba31 100644 --- a/crates/but-core/src/commit.rs +++ b/crates/but-core/src/commit/mod.rs @@ -555,8 +555,11 @@ impl<'repo> Commit<'repo> { } /// Return `true` if this commit contains a tree that is conflicted. + /// + /// Checks the commit message for conflict markers first (new style), + /// then falls back to the `gitbutler-conflicted` header (legacy). pub fn is_conflicted(&self) -> bool { - self.headers().is_some_and(|hdr| hdr.is_conflicted()) + is_conflicted(self.inner.message.as_ref(), self.headers().as_ref()) } /// If the commit is conflicted, then it returns the auto-resolution tree, @@ -686,3 +689,9 @@ impl ConflictEntries { set.len() } } + +mod conflict; +pub use conflict::{ + add_conflict_markers, is_conflicted, message_is_conflicted, + rewrite_conflict_markers_on_message_change, strip_conflict_markers, +}; 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..626d323dba9 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,16 @@ pub(crate) mod function { } let headers = to_rebase.headers(); - let to_rebase_is_conflicted = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); let mut new_commit = to_rebase.inner; new_commit.tree = resolved_tree_id.detach(); // Assure the commit isn't thinking it's conflicted. - if to_rebase_is_conflicted { - if let Some(pos) = new_commit - .extra_headers() - .find_pos(HEADERS_CONFLICTED_FIELD) - { - new_commit.extra_headers.remove(pos); - } + new_commit.message = but_core::commit::strip_conflict_markers(new_commit.message.as_ref()); + if let Some(pos) = new_commit + .extra_headers() + .find_pos(HEADERS_CONFLICTED_FIELD) + { + new_commit.extra_headers.remove(pos); } else if headers.is_none() { let headers = Headers::from_config(&repo.config_snapshot()); new_commit @@ -204,10 +202,6 @@ pub(crate) mod function { treat_as_unresolved: gix::merge::tree::TreatAsUnresolved, ) -> anyhow::Result> { let repo = resolved_tree_id.repo; - // in case someone checks this out with vanilla Git, we should warn why it looks like this - let readme_content = - b"You have checked out a GitButler Conflicted commit. You probably didn't mean to do this."; - let readme_blob = repo.write_blob(readme_content)?; let conflicted_files = extract_conflicted_files(resolved_tree_id, cherry_pick, treat_as_unresolved)?; @@ -242,15 +236,18 @@ pub(crate) mod function { resolved_tree_id, )?; tree.upsert(".conflict-files", EntryKind::Blob, conflicted_files_blob)?; - tree.upsert("CONFLICT-README.txt", EntryKind::Blob, readme_blob)?; let mut headers = to_rebase .headers() .unwrap_or_else(|| Headers::from_config(&repo.config_snapshot())); - headers.conflicted = conflicted_files.conflicted_header_field(); + headers.conflicted = None; to_rebase.tree = tree.write().context("failed to write tree")?.detach(); set_parent(&mut to_rebase, head.id.detach())?; + // Add conflict markers to the commit message + to_rebase.inner.message = + but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref()); + to_rebase.set_headers(&headers); Ok(crate::commit::create( repo, @@ -341,22 +338,5 @@ pub(crate) mod function { || !self.our_entries.is_empty() || !self.their_entries.is_empty() } - - fn total_entries(&self) -> usize { - let set = self - .ancestor_entries - .iter() - .chain(self.our_entries.iter()) - .chain(self.their_entries.iter()) - .collect::>(); - - 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..ed1619e6be6 100644 --- a/crates/but-rebase/src/graph_rebase/cherry_pick.rs +++ b/crates/but-rebase/src/graph_rebase/cherry_pick.rs @@ -305,18 +305,16 @@ fn commit_from_unconflicted_tree<'repo>( let repo = to_rebase.id.repo; let headers = to_rebase.headers(); - let to_rebase_is_conflicted = headers.as_ref().is_some_and(|hdr| hdr.is_conflicted()); let mut new_commit = to_rebase.inner; new_commit.tree = resolved_tree_id.detach(); // Ensure the commit isn't thinking it's conflicted. - if to_rebase_is_conflicted { - if let Some(pos) = new_commit - .extra_headers() - .find_pos(HEADERS_CONFLICTED_FIELD) - { - new_commit.extra_headers.remove(pos); - } + new_commit.message = but_core::commit::strip_conflict_markers(new_commit.message.as_ref()); + if let Some(pos) = new_commit + .extra_headers() + .find_pos(HEADERS_CONFLICTED_FIELD) + { + new_commit.extra_headers.remove(pos); } else if headers.is_none() { let headers = Headers::from_config(&repo.config_snapshot()); new_commit @@ -347,10 +345,6 @@ fn commit_from_conflicted_tree<'repo>( sign_commit: SignCommit, ) -> anyhow::Result> { let repo = resolved_tree_id.repo; - // in case someone checks this out with vanilla Git, we should warn why it looks like this - let readme_content = - b"You have checked out a GitButler Conflicted commit. You probably didn't mean to do this."; - let readme_blob = repo.write_blob(readme_content)?; let conflicted_files = extract_conflicted_files(resolved_tree_id, cherry_pick, treat_as_unresolved)?; @@ -382,15 +376,18 @@ fn commit_from_conflicted_tree<'repo>( resolved_tree_id, )?; tree.upsert(".conflict-files", EntryKind::Blob, conflicted_files_blob)?; - tree.upsert("CONFLICT-README.txt", EntryKind::Blob, readme_blob)?; let mut headers = to_rebase .headers() .unwrap_or_else(|| Headers::from_config(&repo.config_snapshot())); - headers.conflicted = conflicted_files.conflicted_header_field(); + headers.conflicted = None; to_rebase.tree = tree.write().context("failed to write tree")?.detach(); to_rebase.parents = parents.into(); + // Add conflict markers to the commit message + to_rebase.inner.message = + but_core::commit::add_conflict_markers(to_rebase.inner.message.as_ref()); + to_rebase.set_headers(&headers); Ok(crate::commit::create( repo, 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..98b26b5fb9c 100644 --- a/crates/but-workspace/src/branch_details.rs +++ b/crates/but-workspace/src/branch_details.rs @@ -226,11 +226,13 @@ 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 = but_core::commit::strip_conflict_markers(commit.message.as_ref()); 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..9aec4b480db 100644 --- a/crates/but-workspace/src/legacy/stacks.rs +++ b/crates/but-workspace/src/legacy/stacks.rs @@ -660,8 +660,10 @@ 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 message = but_core::commit::strip_conflict_markers(raw_message.as_ref()); 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..c4ffa3d7dac 100644 --- a/crates/but-workspace/src/ref_info.rs +++ b/crates/but-workspace/src/ref_info.rs @@ -63,12 +63,19 @@ 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 = but_core::commit::strip_conflict_markers(message.as_ref()); 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 +99,14 @@ impl Commit { graph_commit: &but_graph::Commit, ) -> Self { let hdr = but_core::commit::Headers::try_from_commit(&commit); + 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(), 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..69650bf7ba7 100644 --- a/crates/but-workspace/src/ui/mod.rs +++ b/crates/but-workspace/src/ui/mod.rs @@ -118,17 +118,18 @@ 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::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 = but_core::commit::strip_conflict_markers(commit.message); 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 +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 = 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) @@ -158,6 +160,7 @@ impl From for Commit { message, extra_headers: _, } = inner; + let message = but_core::commit::strip_conflict_markers(message.as_ref()); 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..1b6517bc7bd 100644 --- a/crates/gitbutler-commit/src/commit_ext.rs +++ b/crates/gitbutler-commit/src/commit_ext.rs @@ -27,13 +27,11 @@ 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) + 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 a399ba373b0..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,18 +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.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.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( @@ -268,7 +271,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 +281,7 @@ pub(crate) fn save_and_return_to_workspace(ctx: &Context, perm: &mut RepoExclusi (&headers).into() }) .unwrap_or_default(); + 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-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 ) 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 - } - } -}