From 68d8b6401236d0e31c77acb8240ee66e6e747c6f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 25 Jul 2023 10:17:36 -0700 Subject: [PATCH] rebase/new: make `--allow-large-revset` no longer also allow duplicates The `--allow-large-revset` option for `jj rebase` and `jj new` is used for allowing a single revset to resolve to more than one destination commit. It also means that duplicate commits between individual revsets are allowed (e.g. `jj rebase -d x -d 'x|y'`). I'm about to replace the first meaning of the flag by a revset function. I don't think it's worth keeping the flag only for the second meaning, so I'm just removing the feature instead. We can add it back under a different name (`--allow-duplicate-destinations`?) if people care about it. --- CHANGELOG.md | 4 +++ src/cli_util.rs | 23 ++++++++++++----- tests/test_rebase_command.rs | 50 ++++++++---------------------------- 3 files changed, 30 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5029fad835..de70d796bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ 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` 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'`). + ### New features * `jj log` output is now topologically grouped. diff --git a/src/cli_util.rs b/src/cli_util.rs index f9e221441e..dce668355b 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -1751,23 +1751,32 @@ pub fn resolve_multiple_nonempty_revsets_flag_guarded( revisions: &[RevisionArg], allow_plural_revsets: bool, ) -> Result, CommandError> { - if allow_plural_revsets { - resolve_multiple_nonempty_revsets(revisions, workspace_command) - } else { - let mut commits = IndexSet::new(); - for revision_str in revisions { + 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 commit_hash = short_commit_hash(commit.id()); - if !commits.insert(commit) { + if !all_commits.insert(commit) { return Err(user_error(format!( r#"More than one revset resolved to revision {commit_hash}"#, ))); } } - Ok(commits) } + Ok(all_commits) } fn append_large_revsets_hint_if_multiple_revisions(err: CommandError) -> CommandError { diff --git a/tests/test_rebase_command.rs b/tests/test_rebase_command.rs index 18df91388c..62d766d0d4 100644 --- a/tests/test_rebase_command.rs +++ b/tests/test_rebase_command.rs @@ -142,10 +142,9 @@ fn test_rebase_branch() { ◉ "###); - // Same test but with more than one revision per argument and same revision - // repeated in more than one argument + // Same test but with more than one revision per argument test_env.jj_cmd_success(&repo_path, &["undo"]); - let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b=e|d", "-b=d", "-d=b"]); + let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b=e|d", "-d=b"]); insta::assert_snapshot!(stderr, @r###" Error: Revset "e|d" resolved to more than one revision Hint: The revset "e|d" resolved to these revisions: @@ -155,7 +154,7 @@ fn test_rebase_branch() { "###); let stdout = test_env.jj_cmd_success( &repo_path, - &["rebase", "-b=e|d", "-b=d", "-d=b", "--allow-large-revsets"], + &["rebase", "-b=e|d", "-d=b", "--allow-large-revsets"], ); insta::assert_snapshot!(stdout, @r###" Rebased 2 commits @@ -399,30 +398,8 @@ fn test_rebase_multiple_destinations() { Error: More than one revset resolved to revision d370aee184ba "###); - // Adding --allow-large-revsets suppresses this error in addition to the large - // revsets error. - let stdout = test_env.jj_cmd_success( - &repo_path, - &[ - "rebase", - "-r", - "a", - "-d", - "b", - "-d", - "b", - "--allow-large-revsets", - ], - ); - insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ◉ a - ◉ b - │ @ c - ├─╯ - ◉ - "###); - let stdout = test_env.jj_cmd_success( + // Same error with --allow-large-revsets if there is overlap. + let stderr = test_env.jj_cmd_failure( &repo_path, &[ "rebase", @@ -435,14 +412,8 @@ fn test_rebase_multiple_destinations() { "--allow-large-revsets", ], ); - insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - ◉ a - ├─╮ - │ ◉ b - @ │ c - ├─╯ - ◉ + insta::assert_snapshot!(stderr, @r###" + Error: More than one revset resolved to revision d370aee184ba "###); let stderr = @@ -536,10 +507,9 @@ fn test_rebase_with_descendants() { ◉ "###); - // Same test as above, but with duplicate commits and multiple commits per - // argument + // Same test as above, but with multiple commits per argument test_env.jj_cmd_success(&repo_path, &["undo"]); - let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-s=b|d", "-s=d", "-d=a"]); + let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-s=b|d", "-d=a"]); insta::assert_snapshot!(stderr, @r###" Error: Revset "b|d" resolved to more than one revision Hint: The revset "b|d" resolved to these revisions: @@ -549,7 +519,7 @@ fn test_rebase_with_descendants() { "###); let stdout = test_env.jj_cmd_success( &repo_path, - &["rebase", "-s=b|d", "-s=d", "-d=a", "--allow-large-revsets"], + &["rebase", "-s=b|d", "-d=a", "--allow-large-revsets"], ); insta::assert_snapshot!(stdout, @r###" Rebased 3 commits