From 92ba6d336dfe07ccb336ced06ae59d06267fb3bb Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 6 Aug 2024 18:25:34 +0900
Subject: [PATCH] cli: op diff: be more strict about multiple diff
 sources/destinations

As the doc comment states, this function only supports change of "a single
added and removed commit", "only a single added", or "single removed commit."
It doesn't make sense to show diffs from the parent commit ignoring multiple
removed commits for example.
---
 cli/src/commands/operation/diff.rs | 45 +++++++++++++++---------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/cli/src/commands/operation/diff.rs b/cli/src/commands/operation/diff.rs
index f26c3dd769..21cb428753 100644
--- a/cli/src/commands/operation/diff.rs
+++ b/cli/src/commands/operation/diff.rs
@@ -552,30 +552,31 @@ fn show_change_diff(
     formatter: &mut dyn Formatter,
     repo: &dyn Repo,
     diff_renderer: &DiffRenderer,
-    modified_change: &ModifiedChange,
+    change: &ModifiedChange,
     width: usize,
 ) -> Result<(), CommandError> {
-    if modified_change.added_commits.len() == 1 && modified_change.removed_commits.len() == 1 {
-        let commit = &modified_change.added_commits[0];
-        let predecessor = &modified_change.removed_commits[0];
-        let predecessor_tree = rebase_to_dest_parent(repo, predecessor, commit)?;
-        let tree = commit.tree()?;
-        diff_renderer.show_diff(
-            ui,
-            formatter,
-            &predecessor_tree,
-            &tree,
-            &EverythingMatcher,
-            width,
-        )?;
-    } else if modified_change.added_commits.len() == 1 {
-        let commit = &modified_change.added_commits[0];
-        diff_renderer.show_patch(ui, formatter, commit, &EverythingMatcher, width)?;
-    } else if modified_change.removed_commits.len() == 1 {
-        // TODO: Should we show a reverse diff?
-        let commit = &modified_change.removed_commits[0];
-        diff_renderer.show_patch(ui, formatter, commit, &EverythingMatcher, width)?;
+    match (&*change.removed_commits, &*change.added_commits) {
+        ([predecessor], [commit]) => {
+            let predecessor_tree = rebase_to_dest_parent(repo, predecessor, commit)?;
+            let tree = commit.tree()?;
+            diff_renderer.show_diff(
+                ui,
+                formatter,
+                &predecessor_tree,
+                &tree,
+                &EverythingMatcher,
+                width,
+            )?;
+        }
+        ([], [commit]) => {
+            diff_renderer.show_patch(ui, formatter, commit, &EverythingMatcher, width)?;
+        }
+        ([commit], []) => {
+            // TODO: Should we show a reverse diff?
+            diff_renderer.show_patch(ui, formatter, commit, &EverythingMatcher, width)?;
+        }
+        ([_, _, ..], _) | (_, [_, _, ..]) => {}
+        ([], []) => panic!("ModifiedChange should have at least one entry"),
     }
-
     Ok(())
 }