Skip to content

Commit

Permalink
cli: replace --allow-large-revsets by use of all: prefix
Browse files Browse the repository at this point in the history 
See the earlier commit introducing the `::` operator for reasoning.
  • Loading branch information
@martinvonz
martinvonz committed Jul 28, 2023
1 parent 2b919d0 commit a0a2437
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 99 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* The `:` revset operator is deprecated. Use `::` instead. We plan to delete the
`:` form in jj 0.15+.

* The `--allow-large-revsets` flag for `jj rebase` and `jj new` was replaced by
a `all:` before the revset. For example, use `jj rebase -d 'all: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 'all:x|y'`).

### New features

Expand Down
71 changes: 36 additions & 35 deletions src/cli_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,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,
Expand Down Expand Up @@ -991,6 +993,7 @@ impl WorkspaceCommandHelper {
}
}

/// Resolve a revset any number of revisions (including 0).
pub fn resolve_revset(
&self,
revision_str: &str,
Expand All @@ -1001,6 +1004,35 @@ impl WorkspaceCommandHelper {
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 prefixing the
/// expression with `all:`.
pub fn resolve_revset_default_single(
&self,
revision_str: &str,
ui: &mut Ui,
) -> Result<Vec<Commit>, CommandError> {
// TODO: Let pest parse the prefix too once we've dropped support for `:`
if let Some(revision_str) = revision_str.strip_prefix("all:") {
self.resolve_revset(revision_str, ui)
} else {
self.resolve_single_rev(revision_str, ui)
.map_err(|err| match err {
CommandError::UserError { message, hint } => user_error_with_hint(
message,
format!(
"{old_hint}Prefix the expression with 'all' to allow any number of \
revisions (i.e. 'all:{}').",
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,
Expand Down Expand Up @@ -1842,29 +1874,16 @@ 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,
ui: &mut Ui,
revisions: &[RevisionArg],
allow_plural_revsets: bool,
) -> Result<IndexSet<Commit>, CommandError> {
let mut all_commits = IndexSet::new();
for revision_str in revisions {
if allow_plural_revsets {
let commits = workspace_command.resolve_revset(revision_str, ui)?;
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, ui)
.map_err(append_large_revsets_hint_if_multiple_revisions)?;
let commits = workspace_command.resolve_revset_default_single(revision_str, ui)?;
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!(
Expand All @@ -1876,24 +1895,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<ReadonlyRepo>,
wc: &mut WorkingCopy,
Expand Down
62 changes: 25 additions & 37 deletions src/commands/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ use tracing::instrument;

use crate::cli_util::{
check_stale_working_copy, get_new_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,
Expand Down Expand Up @@ -2183,14 +2183,9 @@ fn resolve_destination_revs(
workspace_command: &WorkspaceCommandHelper,
ui: &mut Ui,
revisions: &[RevisionArg],
allow_plural_revsets: bool,
) -> Result<IndexSet<Commit>, CommandError> {
let commits = resolve_multiple_nonempty_revsets_flag_guarded(
workspace_command,
ui,
revisions,
allow_plural_revsets,
)?;
let commits =
resolve_multiple_nonempty_revsets_default_single(workspace_command, ui, 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"))
Expand All @@ -2201,19 +2196,20 @@ fn resolve_destination_revs(

#[instrument(skip_all)]
fn cmd_new(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(), CommandError> {
if args.allow_large_revsets {
return Err(user_error(
"--allow-large-revsets has been deprecated.
Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
));
}
let mut workspace_command = command.workspace_helper(ui)?;
assert!(
!args.revisions.is_empty(),
"expected a non-empty list from clap"
);
let target_commits = resolve_destination_revs(
&workspace_command,
ui,
&args.revisions,
args.allow_large_revsets,
)?
.into_iter()
.collect_vec();
let target_commits = resolve_destination_revs(&workspace_command, ui, &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;
Expand Down Expand Up @@ -3103,7 +3099,7 @@ don't make any changes, then the operation will be aborted.

#[instrument(skip_all)]
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",
)));
Expand All @@ -3113,24 +3109,21 @@ fn cmd_merge(ui: &mut Ui, command: &CommandHelper, args: &NewArgs) -> Result<(),

#[instrument(skip_all)]
fn cmd_rebase(ui: &mut Ui, command: &CommandHelper, args: &RebaseArgs) -> Result<(), CommandError> {
if args.allow_large_revsets {
return Err(user_error(
"--allow-large-revsets has been deprecated.
Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets -d x -d y`.",
));
}
let mut workspace_command = command.workspace_helper(ui)?;
let new_parents = resolve_destination_revs(
&workspace_command,
ui,
&args.destination,
args.allow_large_revsets,
)?
.into_iter()
.collect_vec();
let new_parents = resolve_destination_revs(&workspace_command, ui, &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,
ui,
&args.source,
args.allow_large_revsets,
)?;
let source_commits =
resolve_multiple_nonempty_revsets_default_single(&workspace_command, ui, &args.source)?;
rebase_descendants(
ui,
command,
Expand All @@ -3142,12 +3135,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("@", ui)?])
} else {
resolve_multiple_nonempty_revsets_flag_guarded(
&workspace_command,
ui,
&args.branch,
args.allow_large_revsets,
)?
resolve_multiple_nonempty_revsets_default_single(&workspace_command, ui, &args.branch)?
};
rebase_branch(
ui,
Expand Down
34 changes: 8 additions & 26 deletions tests/test_rebase_command.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all: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=all:e|d", "-d=b"]);
insta::assert_snapshot!(stdout, @r###"
Rebased 2 commits
Working copy now at: 817e3fb0dc64 e
Expand Down Expand Up @@ -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
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all: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", "all:b|c"]);
insta::assert_snapshot!(stdout, @r###""###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
◉ a
Expand All @@ -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 'all:' 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", "all:b|c", "-d", "b"],
);
insta::assert_snapshot!(stderr, @r###"
Error: More than one revset resolved to revision d370aee184ba
Expand Down Expand Up @@ -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
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all: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=all:b|d", "-d=a"]);
insta::assert_snapshot!(stdout, @r###"
Rebased 3 commits
Working copy now at: d17539f7ea7c d
Expand Down

0 comments on commit a0a2437

Please sign in to comment.