add branch delete/forget glob:pattern, deprecate --glob option

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

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 }

cli/src/cli_util.rs

@@ -379,12 +379,6 @@ impl From<FsPathParseError> for CommandError {
     }
 }
 
-impl From<glob::PatternError> for CommandError {
-    fn from(err: glob::PatternError) -> Self {
-        user_error(format!("Failed to compile glob: {err}"))
-    }
-}
-
 impl From<clap::Error> for CommandError {
     fn from(err: clap::Error) -> Self {
         CommandError::ClapCliError(Arc::new(err))

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<String>,
-
-    /// A glob pattern indicating branches to delete.
-    #[arg(long)]
-    pub glob: Vec<String>,
+    /// 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<StringPattern>,
+
+    /// Deprecated. Please prefix the pattern with `glob:` instead.
+    #[arg(long, hide = true, value_parser = StringPattern::glob)]
+    pub glob: Vec<StringPattern>,
 }
 
 /// 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<String>,
-
-    /// A glob pattern indicating branches to forget.
-    #[arg(long)]
-    pub glob: Vec<String>,
+    /// 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<StringPattern>,
+
+    /// Deprecated. Please prefix the pattern with `glob:` instead.
+    #[arg(long, hide = true, value_parser = StringPattern::glob)]
+    pub glob: Vec<StringPattern>,
 }
 
 /// 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<StringPattern, StringPatternParseError> {
+    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<Vec<String>, 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<Vec<String>, 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<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![];
-    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<String> = 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<String> = 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);
     }

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| {

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'.
     "###);
 }