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>

Author: Byron

crates/but-api/src/diff.rs

@@ -35,7 +35,7 @@ pub mod json {
         fn from(value: but_core::diff::CommitDetails) -> Self {
             let but_core::diff::CommitDetails {
                 commit,
-                diff_with_first_parent,
+                changes: diff_with_first_parent,
                 line_stats,
                 conflict_entries,
             } = value;

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<crate::TreeChange>,
+    /// 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<crate::TreeChange>,
     /// The stats of the changes, which are computed only when explicitly requested.
     pub line_stats: Option<LineStats>,
     /// 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<Self> {
         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,
         })

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<Item = gix::ObjectId>,
+) -> anyhow::Result<Option<gix::ObjectId>> {
+    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 {

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<TreeChanges> {
-    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 })

crates/but-core/tests/core/diff/ui.rs

@@ -739,3 +739,48 @@ pub fn repo(name: &str) -> anyhow::Result<gix::Repository> {
         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(())
+}

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<JsonChange> = 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()

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<String> {
     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:",

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,

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| {

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(