From cdc0cc360162d96d15d0abca736c8fb8b2003369 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Thu, 4 Jul 2024 18:44:43 +0900
Subject: [PATCH] cli: show commit summary at end of "branch move"

It's nice to see the result of "branch move", "create", etc., and this is more
important in "branch move" because the source branches can be specified in an
abstracted way. I originally considered printing a list of affected branches,
but it looked rather verbose. Since the destination revision is unique, we can
use commit_summary template instead.

This patch also removes a warning about multiple branches because the branch
names are included in the commit summary. I think the hint message is good
enough to signal possible mistake.
---
 cli/src/commands/branch/move.rs  | 27 ++++++++++++++-------------
 cli/tests/test_branch_command.rs | 17 +++++++++++------
 2 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/cli/src/commands/branch/move.rs b/cli/src/commands/branch/move.rs
index ac5b414212..75ff77d1c6 100644
--- a/cli/src/commands/branch/move.rs
+++ b/cli/src/commands/branch/move.rs
@@ -66,6 +66,7 @@ pub fn cmd_branch_move(
     let mut workspace_command = command.workspace_helper(ui)?;
     let repo = workspace_command.repo().clone();
 
+    let target_commit = workspace_command.resolve_single_rev(&args.to)?;
     let matched_branches = {
         let is_source_commit = if !args.from.is_empty() {
             workspace_command
@@ -75,7 +76,7 @@ pub fn cmd_branch_move(
         } else {
             Box::new(|_: &CommitId| true)
         };
-        if !args.names.is_empty() {
+        let mut branches = if !args.names.is_empty() {
             find_branches_with(&args.names, |pattern| {
                 repo.view()
                     .local_branches_matching(pattern)
@@ -86,24 +87,16 @@ pub fn cmd_branch_move(
                 .local_branches()
                 .filter(|(_, target)| target.added_ids().any(&is_source_commit))
                 .collect()
-        }
+        };
+        // Noop matches aren't error, but should be excluded from stats.
+        branches.retain(|(_, old_target)| old_target.as_normal() != Some(target_commit.id()));
+        branches
     };
-    let target_commit = workspace_command.resolve_single_rev(&args.to)?;
 
     if matched_branches.is_empty() {
         writeln!(ui.status(), "No branches to update.")?;
         return Ok(());
     }
-    if matched_branches.len() > 1 {
-        writeln!(
-            ui.warning_default(),
-            "Updating multiple branches: {}",
-            matched_branches.iter().map(|(name, _)| name).join(", "),
-        )?;
-        if args.names.is_empty() {
-            writeln!(ui.hint_default(), "Specify branch by name to update one.")?;
-        }
-    }
 
     if !args.allow_backwards {
         if let Some((name, _)) = matched_branches
@@ -131,5 +124,13 @@ pub fn cmd_branch_move(
         ),
     )?;
 
+    if let Some(mut formatter) = ui.status_formatter() {
+        write!(formatter, "Moved {} branches to ", matched_branches.len())?;
+        workspace_command.write_commit_summary(formatter.as_mut(), &target_commit)?;
+        writeln!(formatter)?;
+    }
+    if matched_branches.len() > 1 && args.names.is_empty() {
+        writeln!(ui.hint_default(), "Specify branch by name to update one.")?;
+    }
     Ok(())
 }
diff --git a/cli/tests/test_branch_command.rs b/cli/tests/test_branch_command.rs
index 4ca8b0fb06..d5330aaa45 100644
--- a/cli/tests/test_branch_command.rs
+++ b/cli/tests/test_branch_command.rs
@@ -145,7 +145,9 @@ fn test_branch_move() {
     insta::assert_snapshot!(stderr, @"");
 
     let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "move", "foo"]);
-    insta::assert_snapshot!(stderr, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Moved 1 branches to mzvwutvl 167f90e7 foo | (empty) (no description set)
+    "###);
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "move", "--to=@-", "foo"]);
     insta::assert_snapshot!(stderr, @r###"
@@ -157,7 +159,9 @@ fn test_branch_move() {
         &repo_path,
         &["branch", "move", "--to=@-", "--allow-backwards", "foo"],
     );
-    insta::assert_snapshot!(stderr, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Moved 1 branches to qpvuntsm 230dd059 foo | (empty) (no description set)
+    "###);
 
     // Delete branch locally, but is still tracking remote
     test_env.jj_cmd_ok(&repo_path, &["describe", "@-", "-mcommit"]);
@@ -243,13 +247,13 @@ fn test_branch_move_matching() {
     // Noop move
     let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "move", "--to=a1", "a2"]);
     insta::assert_snapshot!(stderr, @r###"
-    Nothing changed.
+    No branches to update.
     "###);
 
     // Move from multiple revisions
     let (_stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "move", "--from=::@"]);
     insta::assert_snapshot!(stderr, @r###"
-    Warning: Updating multiple branches: b1, c1
+    Moved 2 branches to vruxwmqv a2781dd9 b1 c1 | (empty) head2
     Hint: Specify branch by name to update one.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@@ -266,7 +270,6 @@ fn test_branch_move_matching() {
     // Try to move multiple branches, but one of them isn't fast-forward
     let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "move", "glob:?1"]);
     insta::assert_snapshot!(stderr, @r###"
-    Warning: Updating multiple branches: a1, b1, c1
     Error: Refusing to move branch backwards or sideways: a1
     Hint: Use --allow-backwards to allow it.
     "###);
@@ -285,7 +288,9 @@ fn test_branch_move_matching() {
         &repo_path,
         &["branch", "move", "--from=::a1+", "--to=a1+", "glob:?1"],
     );
-    insta::assert_snapshot!(stderr, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Moved 1 branches to kkmpptxz 6b5e840e a1 | (empty) head1
+    "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @   a2781dd9ee37
     ◉  c1 f4f38657a3dd