diff --git a/CHANGELOG.md b/CHANGELOG.md index 91fe17d0d1..b030545ec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `branches()`/`remote_branches()`/`author()`/`committer()`/`description()` revsets now support glob matching. +* `jj branch delete`/`forget` now support [string pattern + syntax](docs/revsets.md#string-patterns). The `--glob` option is deprecated in + favor of `glob:` pattern. + ### Fixed bugs * Updating the working copy to a commit where a file that's currently ignored diff --git a/Cargo.lock b/Cargo.lock index ee9f25edff..12b89c6e9d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1003,7 +1003,6 @@ dependencies = [ "esl01-renderdag", "futures 0.3.28", "git2", - "glob", "hex", "indexmap", "insta", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 9d4c4e87df..44ed2c750b 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -41,7 +41,6 @@ dirs = { workspace = true } esl01-renderdag = { workspace = true } futures = { workspace = true } git2 = { workspace = true } -glob = { workspace = true } hex = { workspace = true } indexmap = { workspace = true } itertools = { workspace = true } diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 3a9e7433ba..d9b7876fc3 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -379,12 +379,6 @@ impl From for CommandError { } } -impl From for CommandError { - fn from(err: glob::PatternError) -> Self { - user_error(format!("Failed to compile glob: {err}")) - } -} - impl From for CommandError { fn from(err: clap::Error) -> Self { CommandError::ClapCliError(Arc::new(err)) diff --git a/cli/src/commands/branch.rs b/cli/src/commands/branch.rs index b246344afb..0e5349d64f 100644 --- a/cli/src/commands/branch.rs +++ b/cli/src/commands/branch.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeSet, HashSet}; +use std::collections::HashSet; use std::fmt; use std::io::Write as _; use std::str::FromStr; @@ -10,7 +10,7 @@ use jj_lib::git; use jj_lib::op_store::RefTarget; use jj_lib::repo::Repo; use jj_lib::revset::{self, RevsetExpression}; -use jj_lib::str_util::StringPattern; +use jj_lib::str_util::{StringPattern, StringPatternParseError}; use jj_lib::view::View; use crate::cli_util::{user_error, user_error_with_hint, CommandError, CommandHelper, RevisionArg}; @@ -54,13 +54,17 @@ pub struct BranchCreateArgs { /// next push. #[derive(clap::Args, Clone, Debug)] pub struct BranchDeleteArgs { - /// The branches to delete. - #[arg(required_unless_present_any(& ["glob"]))] - names: Vec, - - /// A glob pattern indicating branches to delete. - #[arg(long)] - pub glob: Vec, + /// The branches to delete + /// + /// By default, the specified name matches exactly. Use `glob:` prefix to + /// select branches by wildcard pattern. For details, see + /// https://github.com/martinvonz/jj/blob/main/docs/revsets.md#string-patterns. + #[arg(required_unless_present_any(&["glob"]), value_parser = parse_name_pattern)] + pub names: Vec, + + /// Deprecated. Please prefix the pattern with `glob:` instead. + #[arg(long, hide = true, value_parser = StringPattern::glob)] + pub glob: Vec, } /// List branches and their targets @@ -95,13 +99,17 @@ pub struct BranchListArgs { /// recreated on future pulls if it still exists in the remote. #[derive(clap::Args, Clone, Debug)] pub struct BranchForgetArgs { - /// The branches to forget. - #[arg(required_unless_present_any(& ["glob"]))] - pub names: Vec, - - /// A glob pattern indicating branches to forget. - #[arg(long)] - pub glob: Vec, + /// The branches to forget + /// + /// By default, the specified name matches exactly. Use `glob:` prefix to + /// select branches by wildcard pattern. For details, see + /// https://github.com/martinvonz/jj/blob/main/docs/revsets.md#string-patterns. + #[arg(required_unless_present_any(&["glob"]), value_parser = parse_name_pattern)] + pub names: Vec, + + /// Deprecated. Please prefix the pattern with `glob:` instead. + #[arg(long, hide = true, value_parser = StringPattern::glob)] + pub glob: Vec, } /// Update a given branch to point to a certain commit. @@ -276,48 +284,60 @@ fn cmd_branch_set( Ok(()) } -/// This function may return the same branch more than once -fn find_globs( +fn parse_name_pattern(src: &str) -> Result { + if let Some((kind, pat)) = src.split_once(':') { + StringPattern::from_str_kind(pat, kind) + } else { + Ok(StringPattern::exact(src)) + } +} + +fn find_local_branches( + view: &View, + name_patterns: &[StringPattern], +) -> Result, CommandError> { + find_branches_with(name_patterns, |pattern| { + view.local_branches_matching(pattern) + .map(|(name, _)| name.to_owned()) + }) +} + +fn find_forgettable_branches( view: &View, - globs: &[String], - allow_deleted: bool, + name_patterns: &[StringPattern], +) -> Result, CommandError> { + find_branches_with(name_patterns, |pattern| { + view.branches() + .filter(|(name, _)| pattern.matches(name)) + .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![]; - let mut failed_globs = vec![]; - for glob_str in globs { - let glob = glob::Pattern::new(glob_str)?; - let names = view - .branches() - .filter_map(|(branch_name, branch_target)| { - if glob.matches(branch_name) - && (allow_deleted || branch_target.local_target.is_present()) - { - Some(branch_name.to_owned()) - } else { - None - } - }) - .collect_vec(); - if names.is_empty() { - failed_globs.push(glob); + let mut unmatched_patterns = vec![]; + for pattern in name_patterns { + let mut names = find_matches(pattern).peekable(); + if names.peek().is_none() { + unmatched_patterns.push(pattern); } matching_branches.extend(names); } - match &failed_globs[..] { - [] => { /* No problem */ } - [glob] => { - return Err(user_error(format!( - "The provided glob '{glob}' did not match any branches" - ))) + match &unmatched_patterns[..] { + [] => { + matching_branches.sort_unstable(); + matching_branches.dedup(); + Ok(matching_branches) } - globs => { - return Err(user_error(format!( - "The provided globs '{}' did not match any branches", - globs.iter().join("', '") - ))) - } - }; - Ok(matching_branches) + [pattern] if pattern.is_exact() => Err(user_error(format!("No such branch: {pattern}"))), + patterns => Err(user_error(format!( + "No matching branches for patterns: {}", + patterns.iter().join(", ") + ))), + } } fn cmd_branch_delete( @@ -327,20 +347,16 @@ fn cmd_branch_delete( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let view = workspace_command.repo().view(); - for branch_name in &args.names { - if workspace_command - .repo() - .view() - .get_local_branch(branch_name) - .is_absent() - { - return Err(user_error(format!("No such branch: {branch_name}"))); - } + if !args.glob.is_empty() { + writeln!( + ui.warning(), + "--glob has been deprecated. Please prefix the pattern with `glob:` instead." + )?; } - let globbed_names = find_globs(view, &args.glob, false)?; - let names: BTreeSet = args.names.iter().cloned().chain(globbed_names).collect(); - let branch_term = make_branch_term(names.iter().collect_vec().as_slice()); - let mut tx = workspace_command.start_transaction(&format!("delete {branch_term}")); + let name_patterns = [&args.names[..], &args.glob[..]].concat(); + let names = find_local_branches(view, &name_patterns)?; + let mut tx = + workspace_command.start_transaction(&format!("delete {}", make_branch_term(&names))); for branch_name in names.iter() { tx.mut_repo() .set_local_branch_target(branch_name, RefTarget::absent()); @@ -359,13 +375,16 @@ fn cmd_branch_forget( ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; let view = workspace_command.repo().view(); - if let Some(branch_name) = args.names.iter().find(|name| !view.has_branch(name)) { - return Err(user_error(format!("No such branch: {branch_name}"))); + if !args.glob.is_empty() { + writeln!( + ui.warning(), + "--glob has been deprecated. Please prefix the pattern with `glob:` instead." + )?; } - let globbed_names = find_globs(view, &args.glob, true)?; - let names: BTreeSet = args.names.iter().cloned().chain(globbed_names).collect(); - let branch_term = make_branch_term(names.iter().collect_vec().as_slice()); - let mut tx = workspace_command.start_transaction(&format!("forget {branch_term}")); + let name_patterns = [&args.names[..], &args.glob[..]].concat(); + let names = find_forgettable_branches(view, &name_patterns)?; + let mut tx = + workspace_command.start_transaction(&format!("forget {}", make_branch_term(&names))); for branch_name in names.iter() { tx.mut_repo().remove_branch(branch_name); } diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index 2def802aee..5e12df9f74 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -313,6 +313,8 @@ fn cmd_git_fetch( "fetch from git remote(s) {}", remotes.iter().join(",") )); + // TODO: maybe this should error out if the pattern contained meta + // characters and is not prefixed with "glob:". let branches = args.branch.iter().map(|b| b.as_str()).collect_vec(); for remote in remotes { let stats = with_remote_callbacks(ui, |cb| { diff --git a/cli/tests/test_branch_command.rs b/cli/tests/test_branch_command.rs index 973b73eeea..7b1f3693fe 100644 --- a/cli/tests/test_branch_command.rs +++ b/cli/tests/test_branch_command.rs @@ -99,6 +99,13 @@ fn test_branch_forget_glob() { test_env.jj_cmd_ok(&repo_path, &["branch", "forget", "--glob", "foo-[1-3]"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + Forgot 2 branches. + "###); + test_env.jj_cmd_ok(&repo_path, &["undo"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "forget", "glob:foo-[1-3]"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" Forgot 2 branches. "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @@ -110,36 +117,33 @@ fn test_branch_forget_glob() { // multiple glob patterns, shouldn't produce an error. let (stdout, stderr) = test_env.jj_cmd_ok( &repo_path, - &[ - "branch", "forget", "foo-4", "--glob", "foo-*", "--glob", "foo-*", - ], + &["branch", "forget", "foo-4", "--glob", "foo-*", "glob:foo-*"], ); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(stderr, @r###" + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ bar-2 230dd059e1b0 ◉ 000000000000 "###); // Malformed glob - let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "forget", "--glob", "foo-[1-3"]); + let stderr = test_env.jj_cmd_cli_error(&repo_path, &["branch", "forget", "glob:foo-[1-3"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to compile glob: Pattern syntax error near position 4: invalid range pattern + error: invalid value 'glob:foo-[1-3' for '[NAMES]...': Pattern syntax error near position 4: invalid range pattern + + For more information, try '--help'. "###); // We get an error if none of the globs match anything let stderr = test_env.jj_cmd_failure( &repo_path, - &[ - "branch", - "forget", - "--glob=bar*", - "--glob=baz*", - "--glob=boom*", - ], + &["branch", "forget", "glob:bar*", "glob:baz*", "--glob=boom*"], ); insta::assert_snapshot!(stderr, @r###" - Error: The provided globs 'baz*', 'boom*' did not match any branches + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + Error: No matching branches for patterns: baz*, boom* "###); } @@ -177,6 +181,13 @@ fn test_branch_delete_glob() { test_env.jj_cmd_ok(&repo_path, &["branch", "delete", "--glob", "foo-[1-3]"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + Deleted 2 branches. + "###); + test_env.jj_cmd_ok(&repo_path, &["undo"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "delete", "glob:foo-[1-3]"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" Deleted 2 branches. "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @@ -188,19 +199,20 @@ fn test_branch_delete_glob() { // forget`, it's not allowed to delete already deleted branches. let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "delete", "--glob=foo-[1-3]"]); insta::assert_snapshot!(stderr, @r###" - Error: The provided glob 'foo-[1-3]' did not match any branches + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + Error: No matching branches for patterns: foo-[1-3] "###); // Deleting a branch via both explicit name and glob pattern, or with // multiple glob patterns, shouldn't produce an error. let (stdout, stderr) = test_env.jj_cmd_ok( &repo_path, - &[ - "branch", "delete", "foo-4", "--glob", "foo-*", "--glob", "foo-*", - ], + &["branch", "delete", "foo-4", "--glob", "foo-*", "glob:foo-*"], ); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(stderr, @""); + insta::assert_snapshot!(stderr, @r###" + --glob has been deprecated. Please prefix the pattern with `glob:` instead. + "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ bar-2 foo-1@origin foo-3@origin foo-4@origin 6fbf398c2d59 ◉ 000000000000 @@ -225,9 +237,19 @@ fn test_branch_delete_glob() { "###); // Malformed glob - let stderr = test_env.jj_cmd_failure(&repo_path, &["branch", "delete", "--glob", "foo-[1-3"]); + let stderr = test_env.jj_cmd_cli_error(&repo_path, &["branch", "delete", "glob:foo-[1-3"]); insta::assert_snapshot!(stderr, @r###" - Error: Failed to compile glob: Pattern syntax error near position 4: invalid range pattern + error: invalid value 'glob:foo-[1-3' for '[NAMES]...': Pattern syntax error near position 4: invalid range pattern + + For more information, try '--help'. + "###); + + // Unknown pattern kind + let stderr = test_env.jj_cmd_cli_error(&repo_path, &["branch", "forget", "whatever:branch"]); + insta::assert_snapshot!(stderr, @r###" + error: invalid value 'whatever:branch' for '[NAMES]...': Invalid string pattern kind "whatever" + + For more information, try '--help'. "###); } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index ce67dc710c..401b8f8828 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -2135,9 +2135,9 @@ fn resolve_commit_ref( RevsetCommitRef::VisibleHeads => Ok(repo.view().heads().iter().cloned().collect_vec()), RevsetCommitRef::Root => Ok(vec![repo.store().root_commit_id().clone()]), RevsetCommitRef::Branches(pattern) => { - let view_data = repo.view().store_view(); - let commit_ids = pattern - .filter_btree_map(&view_data.local_branches) + let commit_ids = repo + .view() + .local_branches_matching(pattern) .flat_map(|(_, target)| target.added_ids()) .cloned() .collect(); @@ -2147,10 +2147,9 @@ fn resolve_commit_ref( branch_pattern, remote_pattern, } => { - let view_data = repo.view().store_view(); - let commit_ids = remote_pattern - .filter_btree_map(&view_data.remote_views) - .flat_map(|(_, remote_view)| branch_pattern.filter_btree_map(&remote_view.branches)) + let commit_ids = repo + .view() + .remote_branches_matching(branch_pattern, remote_pattern) .flat_map(|(_, remote_ref)| remote_ref.target.added_ids()) .cloned() .collect(); diff --git a/lib/src/str_util.rs b/lib/src/str_util.rs index e3b8476e0f..90c297794a 100644 --- a/lib/src/str_util.rs +++ b/lib/src/str_util.rs @@ -16,6 +16,7 @@ use std::borrow::Borrow; use std::collections::BTreeMap; +use std::fmt; use either::Either; use thiserror::Error; @@ -49,6 +50,11 @@ impl StringPattern { StringPattern::Substring(String::new()) } + /// Creates pattern that matches exactly. + pub fn exact(src: impl Into) -> Self { + StringPattern::Exact(src.into()) + } + /// Parses the given string as glob pattern. pub fn glob(src: &str) -> Result { // TODO: might be better to do parsing and compilation separately since @@ -61,13 +67,18 @@ impl StringPattern { /// Parses the given string as pattern of the specified `kind`. pub fn from_str_kind(src: &str, kind: &str) -> Result { match kind { - "exact" => Ok(StringPattern::Exact(src.to_owned())), + "exact" => Ok(StringPattern::exact(src)), "glob" => StringPattern::glob(src), "substring" => Ok(StringPattern::Substring(src.to_owned())), _ => Err(StringPatternParseError::InvalidKind(kind.to_owned())), } } + /// Returns true if this pattern matches input strings exactly. + pub fn is_exact(&self) -> bool { + self.as_exact().is_some() + } + /// Returns a literal pattern if this should match input strings exactly. /// /// This can be used to optimize map lookup by exact key. @@ -78,6 +89,15 @@ impl StringPattern { } } + /// Returns the original string of this pattern. + pub fn as_str(&self) -> &str { + match self { + StringPattern::Exact(literal) => literal, + StringPattern::Glob(pattern) => pattern.as_str(), + StringPattern::Substring(needle) => needle, + } + } + /// Returns true if this pattern matches the `haystack`. pub fn matches(&self, haystack: &str) -> bool { match self { @@ -99,3 +119,10 @@ impl StringPattern { } } } + +impl fmt::Display for StringPattern { + /// Shows the original string of this pattern. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.as_str()) + } +} diff --git a/lib/src/view.rs b/lib/src/view.rs index 1d67d3d6c9..beb9a0b20d 100644 --- a/lib/src/view.rs +++ b/lib/src/view.rs @@ -26,6 +26,7 @@ use crate::op_store::{ BranchTarget, RefTarget, RefTargetOptionExt as _, RemoteRef, RemoteRefState, WorkspaceId, }; use crate::refs::{merge_ref_targets, TrackingRefPair}; +use crate::str_util::StringPattern; #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Hash, Debug)] pub enum RefName { @@ -180,6 +181,17 @@ impl View { .map(|(name, target)| (name.as_ref(), target)) } + /// Iterates local branch `(name, target)`s matching the given pattern. + /// Entries are sorted by `name`. + pub fn local_branches_matching<'a: 'b, 'b>( + &'a self, + pattern: &'b StringPattern, + ) -> impl Iterator + 'b { + pattern + .filter_btree_map(&self.data.local_branches) + .map(|(name, target)| (name.as_ref(), target)) + } + pub fn get_local_branch(&self, name: &str) -> &RefTarget { self.data.local_branches.get(name).flatten() } @@ -215,6 +227,27 @@ impl View { .flatten() } + /// Iterates remote branch `((name, remote_name), remote_ref)`s matching the + /// given patterns. Entries are sorted by `(name, remote_name)`. + pub fn remote_branches_matching<'a: 'b, 'b>( + &'a self, + branch_pattern: &'b StringPattern, + remote_pattern: &'b StringPattern, + ) -> impl Iterator + 'b { + // Use kmerge instead of flat_map for consistency with all_remote_branches(). + remote_pattern + .filter_btree_map(&self.data.remote_views) + .map(|(remote_name, remote_view)| { + branch_pattern.filter_btree_map(&remote_view.branches).map( + |(branch_name, remote_ref)| { + let full_name = (branch_name.as_ref(), remote_name.as_ref()); + (full_name, remote_ref) + }, + ) + }) + .kmerge_by(|(full_name1, _), (full_name2, _)| full_name1 < full_name2) + } + pub fn get_remote_branch(&self, name: &str, remote_name: &str) -> &RemoteRef { if let Some(remote_view) = self.data.remote_views.get(remote_name) { remote_view.branches.get(name).flatten()