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..9ceb096f368 100644 --- a/crates/but-core/src/diff/commit_details.rs +++ b/crates/but-core/src/diff/commit_details.rs @@ -4,7 +4,7 @@ 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, + 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 +30,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(commit.parent_ids().map(|id| id.detach()))?; let changes = crate::diff::TreeChanges::from_trees(repo, first_parent_commit_id, commit_id.detach())?; @@ -42,7 +43,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..73091114e1f 100644 --- a/crates/but-core/src/diff/mod.rs +++ b/crates/but-core/src/diff/mod.rs @@ -107,6 +107,14 @@ impl ModeFlags { } } } +/// Select the commit/tree to diff against: +/// - root commits use `None` (empty tree), +/// - all non-root commits (including merge commits) use the first parent. +pub(crate) fn diff_base_commit_id( + parent_ids: impl Iterator, +) -> anyhow::Result> { + Ok(parent_ids.into_iter().next()) +} #[cfg(test)] mod tests { diff --git a/crates/but-core/src/diff/ui.rs b/crates/but-core/src/diff/ui.rs index d449ada9f59..a9ec0dfcb36 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(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..0aa7d8b0143 100644 --- a/crates/but-core/tests/core/diff/ui.rs +++ b/crates/but-core/tests/core/diff/ui.rs @@ -739,3 +739,37 @@ pub fn repo(name: &str) -> anyhow::Result { gix::open::Options::isolated(), )?) } + +#[test] +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, 9); + 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 + +"# + ); + + 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(