diff --git a/cli/src/commands/branch/create.rs b/cli/src/commands/branch/create.rs index 42cd8ee955..9c6cbe7157 100644 --- a/cli/src/commands/branch/create.rs +++ b/cli/src/commands/branch/create.rs @@ -16,7 +16,6 @@ use clap::builder::NonEmptyStringValueParser; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::RefTarget; -use super::make_branch_term; use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::{user_error_with_hint, CommandError}; use crate::ui::Ui; @@ -69,9 +68,9 @@ pub fn cmd_branch_create( tx.finish( ui, format!( - "create {} pointing to commit {}", - make_branch_term(branch_names), - target_commit.id().hex() + "create branch {names} pointing to commit {id}", + names = branch_names.join(", "), + id = target_commit.id().hex() ), )?; Ok(()) diff --git a/cli/src/commands/branch/delete.rs b/cli/src/commands/branch/delete.rs index 2664c9519a..c9d5700766 100644 --- a/cli/src/commands/branch/delete.rs +++ b/cli/src/commands/branch/delete.rs @@ -12,10 +12,11 @@ // 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; -use super::{find_local_branches, make_branch_term}; +use super::find_local_branches; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; use crate::ui::Ui; @@ -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 {}", make_branch_term(&names)))?; - 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(()) } diff --git a/cli/src/commands/branch/forget.rs b/cli/src/commands/branch/forget.rs index b87cffc6a2..a0b3b05a56 100644 --- a/cli/src/commands/branch/forget.rs +++ b/cli/src/commands/branch/forget.rs @@ -12,10 +12,12 @@ // See the License for the specific language governing permissions and // limitations under the License. +use itertools::Itertools as _; +use jj_lib::op_store::{BranchTarget, RefTarget, RemoteRef}; use jj_lib::str_util::StringPattern; use jj_lib::view::View; -use super::{find_branches_with, make_branch_term}; +use super::find_branches_with; use crate::cli_util::CommandHelper; use crate::command_error::CommandError; use crate::ui::Ui; @@ -42,26 +44,35 @@ 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, branch_target) in &matched_branches { + tx.mut_repo() + .set_local_branch_target(name, RefTarget::absent()); + for (remote_name, _) in &branch_target.remote_refs { + tx.mut_repo() + .set_remote_branch(name, remote_name, RemoteRef::absent()); + } } - tx.finish(ui, format!("forget {}", make_branch_term(&names)))?; - 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, CommandError> { +) -> Result)>, 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)) }) } diff --git a/cli/src/commands/branch/mod.rs b/cli/src/commands/branch/mod.rs index dfd6a29f3b..71e3d7d71b 100644 --- a/cli/src/commands/branch/mod.rs +++ b/cli/src/commands/branch/mod.rs @@ -22,8 +22,6 @@ mod set; mod track; mod untrack; -use std::fmt; - use itertools::Itertools as _; use jj_lib::backend::CommitId; use jj_lib::op_store::{RefTarget, RemoteRef}; @@ -87,40 +85,32 @@ pub fn cmd_branch( } } -fn make_branch_term(branch_names: &[impl fmt::Display]) -> String { - match branch_names { - [branch_name] => format!("branch {}", branch_name), - branch_names => format!("branches {}", branch_names.iter().join(", ")), - } -} - -fn find_local_branches( - view: &View, +fn find_local_branches<'a>( + view: &'a View, name_patterns: &[StringPattern], -) -> Result, CommandError> { +) -> Result, CommandError> { find_branches_with(name_patterns, |pattern| { view.local_branches_matching(pattern) - .map(|(name, _)| name.to_owned()) }) } -fn find_branches_with<'a, I: Iterator>( - name_patterns: &'a [StringPattern], - mut find_matches: impl FnMut(&'a StringPattern) -> I, -) -> Result, CommandError> { - let mut matching_branches: Vec = vec![]; +fn find_branches_with<'a, 'b, V, I: Iterator>( + name_patterns: &'b [StringPattern], + mut find_matches: impl FnMut(&'b StringPattern) -> I, +) -> Result, CommandError> { + let mut matching_branches: Vec = 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}"))), diff --git a/cli/src/commands/branch/move.rs b/cli/src/commands/branch/move.rs index e0cd00348c..ac5b414212 100644 --- a/cli/src/commands/branch/move.rs +++ b/cli/src/commands/branch/move.rs @@ -12,12 +12,13 @@ // 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; use jj_lib::str_util::StringPattern; -use super::{find_branches_with, is_fast_forward, make_branch_term}; +use super::{find_branches_with, is_fast_forward}; use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::{user_error_with_hint, CommandError}; use crate::ui::Ui; @@ -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,16 +118,16 @@ 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())); } tx.finish( ui, format!( - "point {} to commit {}", - make_branch_term(&branch_names), - target_commit.id().hex() + "point branch {names} to commit {id}", + names = matched_branches.iter().map(|(name, _)| name).join(", "), + id = target_commit.id().hex() ), )?; diff --git a/cli/src/commands/branch/rename.rs b/cli/src/commands/branch/rename.rs index b2eb66ab21..a589778575 100644 --- a/cli/src/commands/branch/rename.rs +++ b/cli/src/commands/branch/rename.rs @@ -15,7 +15,6 @@ use jj_lib::op_store::RefTarget; use jj_lib::str_util::StringPattern; -use super::make_branch_term; use crate::cli_util::CommandHelper; use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; @@ -55,14 +54,7 @@ pub fn cmd_branch_rename( .set_local_branch_target(new_branch, ref_target); tx.mut_repo() .set_local_branch_target(old_branch, RefTarget::absent()); - tx.finish( - ui, - format!( - "rename {} to {}", - make_branch_term(&[old_branch]), - make_branch_term(&[new_branch]), - ), - )?; + tx.finish(ui, format!("rename branch {old_branch} to {new_branch}"))?; let view = workspace_command.repo().view(); if view diff --git a/cli/src/commands/branch/set.rs b/cli/src/commands/branch/set.rs index 3757fc8106..2dfbf4d942 100644 --- a/cli/src/commands/branch/set.rs +++ b/cli/src/commands/branch/set.rs @@ -16,7 +16,7 @@ use clap::builder::NonEmptyStringValueParser; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::RefTarget; -use super::{is_fast_forward, make_branch_term}; +use super::is_fast_forward; use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::{user_error_with_hint, CommandError}; use crate::ui::Ui; @@ -77,9 +77,9 @@ pub fn cmd_branch_set( tx.finish( ui, format!( - "point {} to commit {}", - make_branch_term(branch_names), - target_commit.id().hex() + "point branch {names} to commit {id}", + names = branch_names.join(", "), + id = target_commit.id().hex() ), )?; diff --git a/cli/src/commands/branch/track.rs b/cli/src/commands/branch/track.rs index 3e0a4b77d8..5dab3a29d0 100644 --- a/cli/src/commands/branch/track.rs +++ b/cli/src/commands/branch/track.rs @@ -14,7 +14,9 @@ use std::collections::HashMap; -use super::{find_remote_branches, make_branch_term}; +use itertools::Itertools as _; + +use super::find_remote_branches; use crate::cli_util::{CommandHelper, RemoteBranchNamePattern}; use crate::command_error::CommandError; use crate::commit_templater::{CommitTemplateLanguage, RefName}; @@ -61,7 +63,10 @@ pub fn cmd_branch_track( tx.mut_repo() .track_remote_branch(&name.branch, &name.remote); } - tx.finish(ui, format!("track remote {}", make_branch_term(&names)))?; + tx.finish( + ui, + format!("track remote branch {}", names.iter().join(", ")), + )?; if names.len() > 1 { writeln!( ui.status(), diff --git a/cli/src/commands/branch/untrack.rs b/cli/src/commands/branch/untrack.rs index 44f5fe0fd8..498ecfe29b 100644 --- a/cli/src/commands/branch/untrack.rs +++ b/cli/src/commands/branch/untrack.rs @@ -12,9 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. +use itertools::Itertools as _; use jj_lib::git; -use super::{find_remote_branches, make_branch_term}; +use super::find_remote_branches; use crate::cli_util::{CommandHelper, RemoteBranchNamePattern}; use crate::command_error::CommandError; use crate::ui::Ui; @@ -65,7 +66,10 @@ pub fn cmd_branch_untrack( tx.mut_repo() .untrack_remote_branch(&name.branch, &name.remote); } - tx.finish(ui, format!("untrack remote {}", make_branch_term(&names)))?; + tx.finish( + ui, + format!("untrack remote branch {}", names.iter().join(", ")), + )?; if names.len() > 1 { writeln!( ui.status(), diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 1382dc9224..e90158284d 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1428,16 +1428,6 @@ impl MutableRepo { self.view.mark_dirty(); } - /// Returns true if any local or remote branch of the given `name` exists. - #[must_use] - pub fn has_branch(&self, name: &str) -> bool { - self.view.with_ref(|v| v.has_branch(name)) - } - - pub fn remove_branch(&mut self, name: &str) { - self.view_mut().remove_branch(name); - } - pub fn get_local_branch(&self, name: &str) -> RefTarget { self.view.with_ref(|v| v.get_local_branch(name).clone()) } diff --git a/lib/src/view.rs b/lib/src/view.rs index 1125bd0b2b..4906de0aa0 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -96,25 +96,6 @@ impl View { self.data.head_ids.remove(head_id); } - /// Returns true if any local or remote branch of the given `name` exists. - #[must_use] - pub fn has_branch(&self, name: &str) -> bool { - self.data.local_branches.contains_key(name) - || self - .data - .remote_views - .values() - .any(|remote_view| remote_view.branches.contains_key(name)) - } - - // TODO: maybe rename to forget_branch() because this seems unusual operation? - pub fn remove_branch(&mut self, name: &str) { - self.data.local_branches.remove(name); - for remote_view in self.data.remote_views.values_mut() { - remote_view.branches.remove(name); - } - } - /// Iterates local branch `(name, target)`s in lexicographical order. pub fn local_branches(&self) -> impl Iterator { self.data diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 3f8a98a8cb..5ae15842f0 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -534,7 +534,7 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { state: RemoteRefState::Tracking, }, ); - assert!(view.has_branch("main")); // branch #3 of 3 + assert!(view.get_local_branch("main").is_present()); // branch #3 of 3 // Simulate fetching from a remote where feature-remote-only and // feature-remote-and-local branches were deleted. This leads to the @@ -550,8 +550,11 @@ fn test_import_refs_reimport_with_deleted_remote_ref() { let view = repo.view(); // The local branches were indeed deleted assert_eq!(view.branches().count(), 2); - assert!(view.has_branch("main")); - assert!(!view.has_branch("feature-remote-only")); + assert!(view.get_local_branch("main").is_present()); + assert!(view.get_local_branch("feature-remote-only").is_absent()); + assert!(view + .get_remote_branch("feature-remote-only", "origin") + .is_absent()); assert!(view .get_local_branch("feature-remote-and-local") .is_absent()); @@ -653,7 +656,7 @@ fn test_import_refs_reimport_with_moved_remote_ref() { state: RemoteRefState::Tracking, }, ); - assert!(view.has_branch("main")); // branch #3 of 3 + assert!(view.get_local_branch("main").is_present()); // branch #3 of 3 // Simulate fetching from a remote where feature-remote-only and // feature-remote-and-local branches were moved. This leads to the @@ -711,7 +714,7 @@ fn test_import_refs_reimport_with_moved_remote_ref() { state: RemoteRefState::Tracking, }, ); - assert!(view.has_branch("main")); // branch #3 of 3 + assert!(view.get_local_branch("main").is_present()); // branch #3 of 3 let expected_heads = hashset! { jj_id(&commit_main), jj_id(&new_commit_remote_and_local), @@ -1185,9 +1188,13 @@ fn test_import_some_refs() { view.get_remote_branch("feature4", "origin"), &commit_feat4_remote_ref ); - assert!(!view.has_branch("main")); + assert!(view.get_local_branch("main").is_absent()); + assert!(view.get_remote_branch("main", "git").is_absent()); + assert!(view.get_remote_branch("main", "origin").is_absent()); assert!(!view.heads().contains(&jj_id(&commit_main))); - assert!(!view.has_branch("ignored")); + assert!(view.get_local_branch("ignored").is_absent()); + assert!(view.get_remote_branch("ignored", "git").is_absent()); + assert!(view.get_remote_branch("ignored", "origin").is_absent()); assert!(!view.heads().contains(&jj_id(&commit_ign))); // Delete branch feature1, feature3 and feature4 in git repository and import @@ -2375,7 +2382,11 @@ fn test_fetch_prune_deleted_ref() { ) .unwrap(); // Test the setup - assert!(tx.mut_repo().has_branch("main")); + assert!(tx.mut_repo().get_local_branch("main").is_present()); + assert!(tx + .mut_repo() + .get_remote_branch("main", "origin") + .is_present()); test_data .origin_repo @@ -2394,7 +2405,11 @@ fn test_fetch_prune_deleted_ref() { ) .unwrap(); assert_eq!(stats.import_stats.abandoned_commits, vec![jj_id(&commit)]); - assert!(!tx.mut_repo().has_branch("main")); + assert!(tx.mut_repo().get_local_branch("main").is_absent()); + assert!(tx + .mut_repo() + .get_remote_branch("main", "origin") + .is_absent()); } #[test]