cli: branch: let find_*_branches() return (name, target) pairs

This will help inline view.remove_branch() in cmd_branch_forget(). I don't
care much about owned (String, _) vs (&str, _), but we can't simplify the
lifetime issue in find_forgettable_branches() anyway. So I made all callers
pass cloned Arc<ReadonlyRepo> and borrow (name, target) pairs from there.

cli/src/commands/branch/delete.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools as _;
 use jj_lib::op_store::RefTarget;
 use jj_lib::str_util::StringPattern;
 
@@ -39,16 +40,22 @@ pub fn cmd_branch_delete(
     args: &BranchDeleteArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let view = workspace_command.repo().view();
-    let names = find_local_branches(view, &args.names)?;
+    let repo = workspace_command.repo().clone();
+    let matched_branches = find_local_branches(repo.view(), &args.names)?;
     let mut tx = workspace_command.start_transaction();
-    for branch_name in names.iter() {
+    for (name, _) in &matched_branches {
         tx.mut_repo()
-            .set_local_branch_target(branch_name, RefTarget::absent());
+            .set_local_branch_target(name, RefTarget::absent());
     }
-    tx.finish(ui, format!("delete branch {}", names.join(", ")))?;
-    if names.len() > 1 {
-        writeln!(ui.status(), "Deleted {} branches.", names.len())?;
+    tx.finish(
+        ui,
+        format!(
+            "delete branch {}",
+            matched_branches.iter().map(|(name, _)| name).join(", ")
+        ),
+    )?;
+    if matched_branches.len() > 1 {
+        writeln!(ui.status(), "Deleted {} branches.", matched_branches.len())?;
     }
     Ok(())
 }

cli/src/commands/branch/forget.rs

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools as _;
+use jj_lib::op_store::BranchTarget;
 use jj_lib::str_util::StringPattern;
 use jj_lib::view::View;
 
@@ -42,26 +44,30 @@ pub fn cmd_branch_forget(
     args: &BranchForgetArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let view = workspace_command.repo().view();
-    let names = find_forgettable_branches(view, &args.names)?;
+    let repo = workspace_command.repo().clone();
+    let matched_branches = find_forgettable_branches(repo.view(), &args.names)?;
     let mut tx = workspace_command.start_transaction();
-    for branch_name in names.iter() {
-        tx.mut_repo().remove_branch(branch_name);
+    for (name, _) in &matched_branches {
+        tx.mut_repo().remove_branch(name);
     }
-    tx.finish(ui, format!("forget branch {}", names.join(", ")))?;
-    if names.len() > 1 {
-        writeln!(ui.status(), "Forgot {} branches.", names.len())?;
+    tx.finish(
+        ui,
+        format!(
+            "forget branch {}",
+            matched_branches.iter().map(|(name, _)| name).join(", ")
+        ),
+    )?;
+    if matched_branches.len() > 1 {
+        writeln!(ui.status(), "Forgot {} branches.", matched_branches.len())?;
     }
     Ok(())
 }
 
-fn find_forgettable_branches(
-    view: &View,
+fn find_forgettable_branches<'a>(
+    view: &'a View,
     name_patterns: &[StringPattern],
-) -> Result<Vec<String>, CommandError> {
+) -> Result<Vec<(&'a str, BranchTarget<'a>)>, CommandError> {
     find_branches_with(name_patterns, |pattern| {
-        view.branches()
-            .filter(|(name, _)| pattern.matches(name))
-            .map(|(name, _)| name.to_owned())
+        view.branches().filter(|(name, _)| pattern.matches(name))
     })
 }

cli/src/commands/branch/mod.rs

@@ -85,33 +85,32 @@ pub fn cmd_branch(
     }
 }
 
-fn find_local_branches(
-    view: &View,
+fn find_local_branches<'a>(
+    view: &'a View,
     name_patterns: &[StringPattern],
-) -> Result<Vec<String>, CommandError> {
+) -> Result<Vec<(&'a str, &'a RefTarget)>, CommandError> {
     find_branches_with(name_patterns, |pattern| {
         view.local_branches_matching(pattern)
-            .map(|(name, _)| name.to_owned())
     })
 }
 
-fn find_branches_with<'a, I: Iterator<Item = String>>(
-    name_patterns: &'a [StringPattern],
-    mut find_matches: impl FnMut(&'a StringPattern) -> I,
-) -> Result<Vec<String>, CommandError> {
-    let mut matching_branches: Vec<String> = vec![];
+fn find_branches_with<'a, 'b, V, I: Iterator<Item = (&'a str, V)>>(
+    name_patterns: &'b [StringPattern],
+    mut find_matches: impl FnMut(&'b StringPattern) -> I,
+) -> Result<Vec<I::Item>, CommandError> {
+    let mut matching_branches: Vec<I::Item> = vec![];
     let mut unmatched_patterns = vec![];
     for pattern in name_patterns {
-        let mut names = find_matches(pattern).peekable();
-        if names.peek().is_none() {
+        let mut matches = find_matches(pattern).peekable();
+        if matches.peek().is_none() {
             unmatched_patterns.push(pattern);
         }
-        matching_branches.extend(names);
+        matching_branches.extend(matches);
     }
     match &unmatched_patterns[..] {
         [] => {
-            matching_branches.sort_unstable();
-            matching_branches.dedup();
+            matching_branches.sort_unstable_by_key(|(name, _)| *name);
+            matching_branches.dedup_by_key(|(name, _)| *name);
             Ok(matching_branches)
         }
         [pattern] if pattern.is_exact() => Err(user_error(format!("No such branch: {pattern}"))),

cli/src/commands/branch/move.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use itertools::Itertools as _;
 use jj_lib::backend::CommitId;
 use jj_lib::object_id::ObjectId as _;
 use jj_lib::op_store::RefTarget;
@@ -63,10 +64,9 @@ pub fn cmd_branch_move(
     args: &BranchMoveArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let repo = workspace_command.repo().as_ref();
-    let view = repo.view();
+    let repo = workspace_command.repo().clone();
 
-    let branch_names = {
+    let matched_branches = {
         let is_source_commit = if !args.from.is_empty() {
             workspace_command
                 .parse_union_revsets(&args.from)?
@@ -77,28 +77,28 @@ pub fn cmd_branch_move(
         };
         if !args.names.is_empty() {
             find_branches_with(&args.names, |pattern| {
-                view.local_branches_matching(pattern)
+                repo.view()
+                    .local_branches_matching(pattern)
                     .filter(|(_, target)| target.added_ids().any(&is_source_commit))
-                    .map(|(name, _)| name.to_owned())
             })?
         } else {
-            view.local_branches()
+            repo.view()
+                .local_branches()
                 .filter(|(_, target)| target.added_ids().any(&is_source_commit))
-                .map(|(name, _)| name.to_owned())
                 .collect()
         }
     };
     let target_commit = workspace_command.resolve_single_rev(&args.to)?;
 
-    if branch_names.is_empty() {
+    if matched_branches.is_empty() {
         writeln!(ui.status(), "No branches to update.")?;
         return Ok(());
     }
-    if branch_names.len() > 1 {
+    if matched_branches.len() > 1 {
         writeln!(
             ui.warning_default(),
             "Updating multiple branches: {}",
-            branch_names.join(", "),
+            matched_branches.iter().map(|(name, _)| name).join(", "),
         )?;
         if args.names.is_empty() {
             writeln!(ui.hint_default(), "Specify branch by name to update one.")?;
@@ -106,9 +106,9 @@ pub fn cmd_branch_move(
     }
 
     if !args.allow_backwards {
-        if let Some(name) = branch_names
+        if let Some((name, _)) = matched_branches
             .iter()
-            .find(|name| !is_fast_forward(repo, view.get_local_branch(name), target_commit.id()))
+            .find(|(_, old_target)| !is_fast_forward(repo.as_ref(), old_target, target_commit.id()))
         {
             return Err(user_error_with_hint(
                 format!("Refusing to move branch backwards or sideways: {name}"),
@@ -118,7 +118,7 @@ pub fn cmd_branch_move(
     }
 
     let mut tx = workspace_command.start_transaction();
-    for name in &branch_names {
+    for (name, _) in &matched_branches {
         tx.mut_repo()
             .set_local_branch_target(name, RefTarget::normal(target_commit.id().clone()));
     }
@@ -126,7 +126,7 @@ pub fn cmd_branch_move(
         ui,
         format!(
             "point branch {names} to commit {id}",
-            names = branch_names.join(", "),
+            names = matched_branches.iter().map(|(name, _)| name).join(", "),
             id = target_commit.id().hex()
         ),
     )?;