From ed2cf5a3a4b80cc26ff675ee30e502e49717c601 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 12 Mar 2026 05:04:33 +0000 Subject: [PATCH 1/2] fix: diff merge commits against merge-base with regression test (#) Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> --- crates/but-api/src/diff.rs | 4 +- crates/but-core/src/diff/commit_details.rs | 10 +++-- crates/but-core/src/diff/mod.rs | 24 ++++++++++ crates/but-core/src/diff/ui.rs | 9 +--- crates/but-core/tests/core/diff/ui.rs | 45 +++++++++++++++++++ crates/but/src/command/legacy/diff/show.rs | 4 +- crates/but/src/command/legacy/reword.rs | 9 ++-- crates/but/src/command/legacy/status/mod.rs | 2 +- crates/but/src/tui/diff_viewer.rs | 2 +- .../tests/virtual_branches/mod.rs | 2 +- 10 files changed, 88 insertions(+), 23 deletions(-) diff --git a/crates/but-api/src/diff.rs b/crates/but-api/src/diff.rs index 144234de91b..922fcd44b63 100644 --- a/crates/but-api/src/diff.rs +++ b/crates/but-api/src/diff.rs @@ -35,14 +35,14 @@ pub mod json { fn from(value: but_core::diff::CommitDetails) -> Self { let but_core::diff::CommitDetails { commit, - diff_with_first_parent, + changes, line_stats, conflict_entries, } = value; CommitDetails { commit: commit.into(), - changes: diff_with_first_parent.into_iter().map(Into::into).collect(), + changes: changes.into_iter().map(Into::into).collect(), line_stats, conflict_entries, } diff --git a/crates/but-core/src/diff/commit_details.rs b/crates/but-core/src/diff/commit_details.rs index 72c5dabfc6c..28ff21ae7bd 100644 --- a/crates/but-core/src/diff/commit_details.rs +++ b/crates/but-core/src/diff/commit_details.rs @@ -3,8 +3,9 @@ pub struct CommitDetails { /// The fully decoded commit. pub commit: crate::CommitOwned, - /// The changes between the tree of the first parent and this commit. - pub diff_with_first_parent: Vec, + /// The changes between the tree of the first parent and this commit, or the merge-base between + /// all parents of this commit and this commit if it's a merge-commit. + pub changes: Vec, /// The stats of the changes, which are computed only when explicitly requested. pub line_stats: Option, /// Represents what was causing a particular commit to conflict when rebased. @@ -30,7 +31,8 @@ impl CommitDetails { pub fn from_commit_id(commit_id: gix::Id, line_stats: bool) -> anyhow::Result { let repo = commit_id.repo; let commit = repo.find_commit(commit_id)?; - let first_parent_commit_id = commit.parent_ids().map(|id| id.detach()).next(); + let first_parent_commit_id = + super::diff_base_commit_id(repo, commit.parent_ids().map(|id| id.detach()))?; let changes = crate::diff::TreeChanges::from_trees(repo, first_parent_commit_id, commit_id.detach())?; @@ -42,7 +44,7 @@ impl CommitDetails { let conflict_entries = commit.conflict_entries()?; Ok(CommitDetails { commit: commit.detach(), - diff_with_first_parent: changes.into_tree_changes(), + changes: changes.into_tree_changes(), line_stats: line_stats.map(Into::into), conflict_entries, }) diff --git a/crates/but-core/src/diff/mod.rs b/crates/but-core/src/diff/mod.rs index 9177da39e65..7520d38d9eb 100644 --- a/crates/but-core/src/diff/mod.rs +++ b/crates/but-core/src/diff/mod.rs @@ -107,6 +107,30 @@ impl ModeFlags { } } } +/// Select the commit/tree to diff against: +/// - root commits use `None` (empty tree), +/// - regular commits use their first and only parent, +/// - merge commits use the merge-base of all parents. +pub(crate) fn diff_base_commit_id( + repo: &gix::Repository, + mut parent_ids: impl Iterator, +) -> anyhow::Result> { + let Some(first_parent_id) = parent_ids.next() else { + return Ok(None); + }; + let Some(second_parent_id) = parent_ids.next() else { + return Ok(Some(first_parent_id)); + }; + + Ok(Some( + repo.merge_base_octopus( + [first_parent_id, second_parent_id] + .into_iter() + .chain(parent_ids), + )? + .detach(), + )) +} #[cfg(test)] mod tests { diff --git a/crates/but-core/src/diff/ui.rs b/crates/but-core/src/diff/ui.rs index d449ada9f59..e3e1274c67e 100644 --- a/crates/but-core/src/diff/ui.rs +++ b/crates/but-core/src/diff/ui.rs @@ -17,13 +17,8 @@ pub fn commit_changes_with_line_stats_by_worktree_dir( repo: &gix::Repository, commit_id: gix::ObjectId, ) -> anyhow::Result { - let parent_id = commit_id - .attach(repo) - .object()? - .into_commit() - .parent_ids() - .map(|id| id.detach()) - .next(); + let commit = commit_id.attach(repo).object()?.into_commit(); + let parent_id = super::diff_base_commit_id(repo, commit.parent_ids().map(|id| id.detach()))?; let (changes, stats) = super::tree_changes_with_line_stats(repo, parent_id, commit_id) .map(|(c, s)| (c.into_iter().map(Into::into).collect(), s.into()))?; Ok(TreeChanges { changes, stats }) diff --git a/crates/but-core/tests/core/diff/ui.rs b/crates/but-core/tests/core/diff/ui.rs index d1b705b631b..a85837bddc6 100644 --- a/crates/but-core/tests/core/diff/ui.rs +++ b/crates/but-core/tests/core/diff/ui.rs @@ -739,3 +739,48 @@ pub fn repo(name: &str) -> anyhow::Result { gix::open::Options::isolated(), )?) } + +#[test] +fn merge_commit_ui_diff_uses_merge_base() -> anyhow::Result<()> { + let repo = + but_testsupport::read_only_in_memory_scenario("merge-with-two-branches-line-offset")?; + let merge_commit_id = repo.rev_parse_single("merge")?.detach(); + let actual = + but_core::diff::ui::commit_changes_with_line_stats_by_worktree_dir(&repo, merge_commit_id)?; + assert_eq!(actual.stats.lines_added, 19); + assert_eq!(actual.stats.lines_removed, 0); + assert_eq!(actual.stats.files_changed, 1); + + let actual_diff = actual.try_to_unidiff(&repo, 0)?; + let actual_diff: &[u8] = actual_diff.as_ref(); + assert_eq!( + actual_diff, + br#"--- a/file ++++ b/file +@@ -1,0 +1,9 @@ ++1 ++2 ++3 ++4 ++5 ++6 ++7 ++8 ++9 +@@ -12,0 +21,10 @@ ++21 ++22 ++23 ++24 ++25 ++26 ++27 ++28 ++29 ++30 + +"# + ); + + Ok(()) +} diff --git a/crates/but/src/command/legacy/diff/show.rs b/crates/but/src/command/legacy/diff/show.rs index bde69d0866c..f7cbedef05a 100644 --- a/crates/but/src/command/legacy/diff/show.rs +++ b/crates/but/src/command/legacy/diff/show.rs @@ -106,7 +106,7 @@ pub(crate) fn commit( if let Some(json_out) = out.for_json() { let changes: Vec = result - .diff_with_first_parent + .changes .into_iter() .filter(|change| path.as_ref().is_none_or(|p| p == &change.path)) .map(|change| { @@ -120,7 +120,7 @@ pub(crate) fn commit( let output = JsonDiffOutput { changes }; json_out.write_value(output)?; } else if let Some(out) = out.for_human_or_shell() { - for change in result.diff_with_first_parent { + for change in result.changes { if path.as_ref().is_none_or(|p| p == &change.path) { let patch = but_api::legacy::diff::tree_change_diffs(ctx, change.clone().into()) .ok() diff --git a/crates/but/src/command/legacy/reword.rs b/crates/but/src/command/legacy/reword.rs index f1ecf697553..58067e23e65 100644 --- a/crates/but/src/command/legacy/reword.rs +++ b/crates/but/src/command/legacy/reword.rs @@ -127,13 +127,12 @@ fn edit_commit_message_by_id( prepare_provided_message(message, "commit message").unwrap_or_else(|| { let changed_files = get_changed_files_from_commit_details(&commit_details); - let should_show_diff = show_diff_in_editor.should_show_diff(|| { - estimate_diff_blob_size(&commit_details.diff_with_first_parent, ctx) - })?; + let should_show_diff = show_diff_in_editor + .should_show_diff(|| estimate_diff_blob_size(&commit_details.changes, ctx))?; let diff = should_show_diff .then(|| { commit_details - .diff_with_first_parent + .changes .iter() .map(|change| change.unified_diff(&*ctx.repo.get()?, 3)) .filter_map(|diff| diff.transpose()) @@ -174,7 +173,7 @@ fn get_changed_files_from_commit_details( ) -> Vec { let mut files = Vec::new(); - for change in &commit_details.diff_with_first_parent { + for change in &commit_details.changes { let status = match &change.status { but_core::TreeStatus::Addition { .. } => "new file:", but_core::TreeStatus::Deletion { .. } => "deleted:", diff --git a/crates/but/src/command/legacy/status/mod.rs b/crates/but/src/command/legacy/status/mod.rs index bab2f695b2c..9f4e247b641 100644 --- a/crates/but/src/command/legacy/status/mod.rs +++ b/crates/but/src/command/legacy/status/mod.rs @@ -789,7 +789,7 @@ pub fn print_group( &repo, commit.short_id.clone(), &commit.inner, - CommitChanges::Remote(&details.diff_with_first_parent), + CommitChanges::Remote(&details.changes), CommitClassification::Upstream, false, show_files, diff --git a/crates/but/src/tui/diff_viewer.rs b/crates/but/src/tui/diff_viewer.rs index d0151425d90..2ec77f95100 100644 --- a/crates/but/src/tui/diff_viewer.rs +++ b/crates/but/src/tui/diff_viewer.rs @@ -116,7 +116,7 @@ impl DiffFileEntry { but_api::diff::commit_details(ctx, commit_id, but_api::diff::ComputeLineStats::No)?; result - .diff_with_first_parent + .changes .into_iter() .filter(|change| path_filter.as_ref().is_none_or(|p| p == &change.path)) .map(|change| { diff --git a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs index 45dee5d6a11..63fa58b6d59 100644 --- a/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs +++ b/crates/gitbutler-branch-actions/tests/virtual_branches/mod.rs @@ -89,7 +89,7 @@ pub fn list_commit_files( gix::prelude::ObjectIdExt::attach(commit_id, &repo), false, ) - .map(|d| d.diff_with_first_parent) + .map(|d| d.changes) } pub fn create_commit( From 28c909d82454d29eae81971fc79f426d2c84e8a3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 12 Mar 2026 07:11:56 +0000 Subject: [PATCH 2/2] fix: restore first-parent merge commit diff semantics Co-authored-by: Byron <63622+Byron@users.noreply.github.com> --- crates/but-core/src/diff/commit_details.rs | 5 ++--- crates/but-core/src/diff/mod.rs | 22 +++------------------- crates/but-core/src/diff/ui.rs | 2 +- crates/but-core/tests/core/diff/ui.rs | 15 ++------------- 4 files changed, 8 insertions(+), 36 deletions(-) diff --git a/crates/but-core/src/diff/commit_details.rs b/crates/but-core/src/diff/commit_details.rs index 28ff21ae7bd..9ceb096f368 100644 --- a/crates/but-core/src/diff/commit_details.rs +++ b/crates/but-core/src/diff/commit_details.rs @@ -3,8 +3,7 @@ pub struct CommitDetails { /// The fully decoded commit. pub commit: crate::CommitOwned, - /// The changes between the tree of the first parent and this commit, or the merge-base between - /// all parents of this commit and this commit if it's a merge-commit. + /// The changes between the tree of the first parent and this commit. pub changes: Vec, /// The stats of the changes, which are computed only when explicitly requested. pub line_stats: Option, @@ -32,7 +31,7 @@ impl CommitDetails { let repo = commit_id.repo; let commit = repo.find_commit(commit_id)?; let first_parent_commit_id = - super::diff_base_commit_id(repo, commit.parent_ids().map(|id| id.detach()))?; + super::diff_base_commit_id(commit.parent_ids().map(|id| id.detach()))?; let changes = crate::diff::TreeChanges::from_trees(repo, first_parent_commit_id, commit_id.detach())?; diff --git a/crates/but-core/src/diff/mod.rs b/crates/but-core/src/diff/mod.rs index 7520d38d9eb..73091114e1f 100644 --- a/crates/but-core/src/diff/mod.rs +++ b/crates/but-core/src/diff/mod.rs @@ -109,27 +109,11 @@ impl ModeFlags { } /// Select the commit/tree to diff against: /// - root commits use `None` (empty tree), -/// - regular commits use their first and only parent, -/// - merge commits use the merge-base of all parents. +/// - all non-root commits (including merge commits) use the first parent. pub(crate) fn diff_base_commit_id( - repo: &gix::Repository, - mut parent_ids: impl Iterator, + parent_ids: impl Iterator, ) -> anyhow::Result> { - let Some(first_parent_id) = parent_ids.next() else { - return Ok(None); - }; - let Some(second_parent_id) = parent_ids.next() else { - return Ok(Some(first_parent_id)); - }; - - Ok(Some( - repo.merge_base_octopus( - [first_parent_id, second_parent_id] - .into_iter() - .chain(parent_ids), - )? - .detach(), - )) + Ok(parent_ids.into_iter().next()) } #[cfg(test)] diff --git a/crates/but-core/src/diff/ui.rs b/crates/but-core/src/diff/ui.rs index e3e1274c67e..a9ec0dfcb36 100644 --- a/crates/but-core/src/diff/ui.rs +++ b/crates/but-core/src/diff/ui.rs @@ -18,7 +18,7 @@ pub fn commit_changes_with_line_stats_by_worktree_dir( commit_id: gix::ObjectId, ) -> anyhow::Result { let commit = commit_id.attach(repo).object()?.into_commit(); - let parent_id = super::diff_base_commit_id(repo, commit.parent_ids().map(|id| id.detach()))?; + let parent_id = super::diff_base_commit_id(commit.parent_ids().map(|id| id.detach()))?; let (changes, stats) = super::tree_changes_with_line_stats(repo, parent_id, commit_id) .map(|(c, s)| (c.into_iter().map(Into::into).collect(), s.into()))?; Ok(TreeChanges { changes, stats }) diff --git a/crates/but-core/tests/core/diff/ui.rs b/crates/but-core/tests/core/diff/ui.rs index a85837bddc6..0aa7d8b0143 100644 --- a/crates/but-core/tests/core/diff/ui.rs +++ b/crates/but-core/tests/core/diff/ui.rs @@ -741,13 +741,13 @@ pub fn repo(name: &str) -> anyhow::Result { } #[test] -fn merge_commit_ui_diff_uses_merge_base() -> anyhow::Result<()> { +fn merge_commit_ui_diff_uses_first_parent() -> anyhow::Result<()> { let repo = but_testsupport::read_only_in_memory_scenario("merge-with-two-branches-line-offset")?; let merge_commit_id = repo.rev_parse_single("merge")?.detach(); let actual = but_core::diff::ui::commit_changes_with_line_stats_by_worktree_dir(&repo, merge_commit_id)?; - assert_eq!(actual.stats.lines_added, 19); + assert_eq!(actual.stats.lines_added, 9); assert_eq!(actual.stats.lines_removed, 0); assert_eq!(actual.stats.files_changed, 1); @@ -767,17 +767,6 @@ fn merge_commit_ui_diff_uses_merge_base() -> anyhow::Result<()> { +7 +8 +9 -@@ -12,0 +21,10 @@ -+21 -+22 -+23 -+24 -+25 -+26 -+27 -+28 -+29 -+30 "# );