diff --git a/CHANGELOG.md b/CHANGELOG.md index de70d796bb..8cad318cd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * The minimum supported Rust version (MSRV) is now 1.71.0. +* The `--allow-large-revsets` flag for `jj rebase` and `jj new` was replaced by + the `multiple()` revset function. For example, use + `jj rebase -d 'multiple(foo-)'` instead of + `jj rebase --allow-large-revsets -d 'foo-'`. + * The `--allow-large-revsets` flag for `jj rebase` and `jj new` can no longer be used for allowing duplicate destinations. Include the potential duplicates - in a single expression instead (e.g. `jj new --allow-large-revsets 'x|y'`). + in a single expression instead (e.g. `jj new 'multiple(x|y)'`). ### New features diff --git a/docs/revsets.md b/docs/revsets.md index f5a135b01f..e70800ea7d 100644 --- a/docs/revsets.md +++ b/docs/revsets.md @@ -113,6 +113,9 @@ revsets (expressions) as arguments. Note that this is different from [Mercurial's](https://repo.mercurial-scm.org/hg/help/revsets) `roots(x)` function, which is equivalent to `x ~ x+`. +* `multiple(x)`: Indicates that you want to allow `x` to resolve to multiple + commits even when exactly 1 commit is normally expected. It has no effect in + contexts where multiple commits are allowed by default. * `latest(x[, count])`: Latest `count` commits in `x`, based on committer timestamp. The default `count` is 1. * `merges()`: Merge commits. diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index b7b1e10344..d10a5e0851 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -625,6 +625,7 @@ impl<'index> EvaluationContext<'index> { self.take_latest_revset(candidate_set.as_ref(), *count), )) } + ResolvedExpression::Multiple(expression) => self.evaluate(expression), ResolvedExpression::Union(expression1, expression2) => { let set1 = self.evaluate(expression1)?; let set2 = self.evaluate(expression2)?; diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 33043f67db..71f7247988 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -265,6 +265,12 @@ pub enum RevsetExpression { }, Heads(Rc), Roots(Rc), + /// Marker to indicate that multiple revisions are allowed. For use at the + /// top level in contexts where otherwise a single revision is expected, + /// such as in `jj rebase -d`. + // TODO: Convert this into a more generic `Sized {expression, range}`, or maybe make that its + // own variant. + Multiple(Rc), Latest { candidates: Rc, count: usize, @@ -331,6 +337,10 @@ impl RevsetExpression { Rc::new(RevsetExpression::CommitRef(RevsetCommitRef::GitHead)) } + pub fn multiple(self: &Rc) -> Rc { + Rc::new(RevsetExpression::Multiple(self.clone())) + } + pub fn latest(self: &Rc, count: usize) -> Rc { Rc::new(RevsetExpression::Latest { candidates: self.clone(), @@ -516,6 +526,7 @@ pub enum ResolvedExpression { }, Heads(Box), Roots(Box), + Multiple(Box), Latest { candidates: Box, count: usize, @@ -978,6 +989,11 @@ static BUILTIN_FUNCTION_MAP: Lazy> = Lazy: expect_no_arguments(name, arguments_pair)?; Ok(RevsetExpression::git_head()) }); + map.insert("multiple", |name, arguments_pair, state| { + let arg = expect_one_argument(name, arguments_pair)?; + let expression = parse_expression_rule(arg.into_inner(), state)?; + Ok(expression.multiple()) + }); map.insert("latest", |name, arguments_pair, state| { let ([candidates_arg], [count_opt_arg]) = expect_arguments(name, arguments_pair)?; let candidates = parse_expression_rule(candidates_arg.into_inner(), state)?; @@ -1312,6 +1328,9 @@ fn try_transform_expression( candidates, count: *count, }), + RevsetExpression::Multiple(expression) => { + transform_rec(expression, pre, post)?.map(RevsetExpression::Multiple) + } RevsetExpression::Filter(_) => None, RevsetExpression::AsFilter(candidates) => { transform_rec(candidates, pre, post)?.map(RevsetExpression::AsFilter) @@ -2019,6 +2038,9 @@ impl VisibilityResolutionContext<'_> { candidates: self.resolve(candidates).into(), count: *count, }, + RevsetExpression::Multiple(expression) => { + ResolvedExpression::Multiple(self.resolve(expression).into()) + } RevsetExpression::Filter(_) | RevsetExpression::AsFilter(_) => { // Top-level filter without intersection: e.g. "~author(_)" is represented as // `AsFilter(NotIn(Filter(Author(_))))`. @@ -2095,6 +2117,7 @@ impl VisibilityResolutionContext<'_> { | RevsetExpression::DagRange { .. } | RevsetExpression::Heads(_) | RevsetExpression::Roots(_) + | RevsetExpression::Multiple(_) | RevsetExpression::Latest { .. } => { ResolvedPredicateExpression::Set(self.resolve(expression).into()) } diff --git a/src/cli_util.rs b/src/cli_util.rs index dce668355b..0710547fa4 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -932,6 +932,8 @@ impl WorkspaceCommandHelper { ) } + /// Resolve a revset to a single revision. Return an error if the revset is + /// empty or has multiple revisions. pub fn resolve_single_rev(&self, revision_str: &str) -> Result { let revset_expression = self.parse_revset(revision_str)?; let revset = self.evaluate_revset(revset_expression)?; @@ -962,12 +964,41 @@ impl WorkspaceCommandHelper { } } + /// Resolve a revset any number of revisions (including 0). pub fn resolve_revset(&self, revision_str: &str) -> Result, CommandError> { let revset_expression = self.parse_revset(revision_str)?; let revset = self.evaluate_revset(revset_expression)?; Ok(revset.iter().commits(self.repo().store()).try_collect()?) } + /// Resolve a revset any number of revisions (including 0), but require the + /// user to indicate if they allow multiple revisions by wrapping the + /// expression in `multiple()`. + pub fn resolve_revset_default_single( + &self, + revision_str: &str, + ) -> Result, CommandError> { + let revset_expression = self.parse_revset(revision_str)?; + if matches!(revset_expression.as_ref(), RevsetExpression::Multiple(_)) { + self.resolve_revset(revision_str) + } else { + self.resolve_single_rev(revision_str) + .map_err(|err| match err { + CommandError::UserError { message, hint } => user_error_with_hint( + message, + format!( + "{old_hint}Wrap the expression in 'multiple()' to allow any number of \ + revisions (i.e. 'multiple({})').", + revision_str, + old_hint = hint.map(|hint| format!("{hint}\n")).unwrap_or_default() + ), + ), + err => err, + }) + .map(|commit| vec![commit]) + } + } + pub fn parse_revset( &self, revision_str: &str, @@ -1746,28 +1777,15 @@ pub fn resolve_multiple_nonempty_revsets( Ok(acc) } -pub fn resolve_multiple_nonempty_revsets_flag_guarded( +pub fn resolve_multiple_nonempty_revsets_default_single( workspace_command: &WorkspaceCommandHelper, revisions: &[RevisionArg], - allow_plural_revsets: bool, ) -> Result, CommandError> { let mut all_commits = IndexSet::new(); for revision_str in revisions { - if allow_plural_revsets { - let commits = workspace_command.resolve_revset(revision_str)?; - workspace_command.check_non_empty(&commits)?; - for commit in commits { - let commit_hash = short_commit_hash(commit.id()); - if !all_commits.insert(commit) { - return Err(user_error(format!( - r#"More than one revset resolved to revision {commit_hash}"#, - ))); - } - } - } else { - let commit = workspace_command - .resolve_single_rev(revision_str) - .map_err(append_large_revsets_hint_if_multiple_revisions)?; + let commits = workspace_command.resolve_revset_default_single(revision_str)?; + workspace_command.check_non_empty(&commits)?; + for commit in commits { let commit_hash = short_commit_hash(commit.id()); if !all_commits.insert(commit) { return Err(user_error(format!( @@ -1779,24 +1797,6 @@ pub fn resolve_multiple_nonempty_revsets_flag_guarded( Ok(all_commits) } -fn append_large_revsets_hint_if_multiple_revisions(err: CommandError) -> CommandError { - match err { - CommandError::UserError { message, hint } if message.contains("more than one revision") => { - CommandError::UserError { - message, - hint: { - Some(format!( - "{old_hint}If this was intentional, specify the `--allow-large-revsets` \ - argument", - old_hint = hint.map(|h| format!("{h}\n")).unwrap_or_default() - )) - }, - } - } - _ => err, - } -} - pub fn update_working_copy( repo: &Arc, wc: &mut WorkingCopy, diff --git a/src/commands/mod.rs b/src/commands/mod.rs index df17fda73f..bb7b4606f2 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -56,7 +56,7 @@ use maplit::{hashmap, hashset}; use crate::cli_util::{ check_stale_working_copy, get_config_file_path, print_checkout_stats, - resolve_multiple_nonempty_revsets, resolve_multiple_nonempty_revsets_flag_guarded, + resolve_multiple_nonempty_revsets, resolve_multiple_nonempty_revsets_default_single, run_ui_editor, serialize_config_value, short_commit_hash, user_error, user_error_with_hint, write_config_value_to_file, Args, CommandError, CommandHelper, DescriptionArg, LogContentFormat, RevisionArg, WorkspaceCommandHelper, @@ -527,9 +527,6 @@ struct NewArgs { /// The change description to use #[arg(long, short, default_value = "")] message: DescriptionArg, - /// Allow revsets expanding to multiple commits in a single argument - #[arg(long, short = 'L')] - allow_large_revsets: bool, /// Insert the new change between the target commit(s) and their children #[arg(long, short = 'A', visible_alias = "after")] insert_after: bool, @@ -857,10 +854,6 @@ struct RebaseArgs { /// commit) #[arg(long, short, required = true)] destination: Vec, - /// Allow revsets expanding to multiple commits in a single argument (for - /// those options that can be repeated) - #[arg(long, short = 'L')] - allow_large_revsets: bool, } /// Apply the reverse of a revision on top of another revision @@ -2159,13 +2152,8 @@ fn cmd_edit(ui: &mut Ui, command: &CommandHelper, args: &EditArgs) -> Result<(), fn resolve_destination_revs( workspace_command: &WorkspaceCommandHelper, revisions: &[RevisionArg], - allow_plural_revsets: bool, ) -> Result, CommandError> { - let commits = resolve_multiple_nonempty_revsets_flag_guarded( - workspace_command, - revisions, - allow_plural_revsets, - )?; + let commits = resolve_multiple_nonempty_revsets_default_single(workspace_command, revisions)?; let root_commit_id = workspace_command.repo().store().root_commit_id(); if commits.len() >= 2 && commits.iter().any(|c| c.id() == root_commit_id) { Err(user_error("Cannot merge with root revision")) @@ -2180,13 +2168,9 @@ fn cmd_new(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), C !args.revisions.is_empty(), "expected a non-empty list from clap" ); - let target_commits = resolve_destination_revs( - &workspace_command, - &args.revisions, - args.allow_large_revsets, - )? - .into_iter() - .collect_vec(); + let target_commits = resolve_destination_revs(&workspace_command, &args.revisions)? + .into_iter() + .collect_vec(); let target_ids = target_commits.iter().map(|c| c.id().clone()).collect_vec(); let mut tx = workspace_command.start_transaction("new empty commit"); let mut num_rebased = 0; @@ -3065,7 +3049,7 @@ don't make any changes, then the operation will be aborted. } fn cmd_merge(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), CommandError> { - if !args.allow_large_revsets && args.revisions.len() < 2 { + if args.revisions.len() < 2 { return Err(CommandError::CliError(String::from( "Merge requires at least two revisions", ))); @@ -3075,21 +3059,14 @@ fn cmd_merge(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), fn cmd_rebase(ui: &mut Ui, command: &CommandHelper, args: &RebaseArgs) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; - let new_parents = resolve_destination_revs( - &workspace_command, - &args.destination, - args.allow_large_revsets, - )? - .into_iter() - .collect_vec(); + let new_parents = resolve_destination_revs(&workspace_command, &args.destination)? + .into_iter() + .collect_vec(); if let Some(rev_str) = &args.revision { rebase_revision(ui, command, &mut workspace_command, &new_parents, rev_str)?; } else if !args.source.is_empty() { - let source_commits = resolve_multiple_nonempty_revsets_flag_guarded( - &workspace_command, - &args.source, - args.allow_large_revsets, - )?; + let source_commits = + resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.source)?; rebase_descendants( ui, command, @@ -3101,11 +3078,7 @@ fn cmd_rebase(ui: &mut Ui, command: &CommandHelper, args: &RebaseArgs) -> Result let branch_commits = if args.branch.is_empty() { IndexSet::from([workspace_command.resolve_single_rev("@")?]) } else { - resolve_multiple_nonempty_revsets_flag_guarded( - &workspace_command, - &args.branch, - args.allow_large_revsets, - )? + resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.branch)? }; rebase_branch( ui, diff --git a/tests/test_rebase_command.rs b/tests/test_rebase_command.rs index 62d766d0d4..27f2e81822 100644 --- a/tests/test_rebase_command.rs +++ b/tests/test_rebase_command.rs @@ -150,12 +150,9 @@ fn test_rebase_branch() { Hint: The revset "e|d" resolved to these revisions: e52756c82985 e 514fa6b265d4 d - If this was intentional, specify the `--allow-large-revsets` argument + Wrap the expression in 'multiple()' to allow any number of revisions (i.e. 'multiple(e|d)'). "###); - let stdout = test_env.jj_cmd_success( - &repo_path, - &["rebase", "-b=e|d", "-d=b", "--allow-large-revsets"], - ); + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-b=multiple(e|d)", "-d=b"]); insta::assert_snapshot!(stdout, @r###" Rebased 2 commits Working copy now at: 817e3fb0dc64 e @@ -377,12 +374,9 @@ fn test_rebase_multiple_destinations() { Hint: The revset "b|c" resolved to these revisions: fe2e8e8b50b3 c d370aee184ba b - If this was intentional, specify the `--allow-large-revsets` argument + Wrap the expression in 'multiple()' to allow any number of revisions (i.e. 'multiple(b|c)'). "###); - let stdout = test_env.jj_cmd_success( - &repo_path, - &["rebase", "--allow-large-revsets", "-r", "a", "-d", "b|c"], - ); + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-r", "a", "-d", "multiple(b|c)"]); insta::assert_snapshot!(stdout, @r###""###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ a @@ -398,19 +392,10 @@ fn test_rebase_multiple_destinations() { Error: More than one revset resolved to revision d370aee184ba "###); - // Same error with --allow-large-revsets if there is overlap. + // Same error with multiple() if there is overlap. let stderr = test_env.jj_cmd_failure( &repo_path, - &[ - "rebase", - "-r", - "a", - "-d", - "b|c", - "-d", - "b", - "--allow-large-revsets", - ], + &["rebase", "-r", "a", "-d", "multiple(b|c)", "-d", "b"], ); insta::assert_snapshot!(stderr, @r###" Error: More than one revset resolved to revision d370aee184ba @@ -515,12 +500,9 @@ fn test_rebase_with_descendants() { Hint: The revset "b|d" resolved to these revisions: df54a9fd85ae d d370aee184ba b - If this was intentional, specify the `--allow-large-revsets` argument + Wrap the expression in 'multiple()' to allow any number of revisions (i.e. 'multiple(b|d)'). "###); - let stdout = test_env.jj_cmd_success( - &repo_path, - &["rebase", "-s=b|d", "-d=a", "--allow-large-revsets"], - ); + let stdout = test_env.jj_cmd_success(&repo_path, &["rebase", "-s=multiple(b|d)", "-d=a"]); insta::assert_snapshot!(stdout, @r###" Rebased 3 commits Working copy now at: d17539f7ea7c d