From 93a9a292dc18c0db1ad9fe6f241f59b76e42590c Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Wed, 3 Jul 2024 11:54:36 +1000 Subject: [PATCH] feat(rebase): Rename --skip-empty to --skip-emptied. This is based on @martinvonz's comment in #3830 about the inconsistency between squash --keep-emptied and rebase --skip-empty. --- CHANGELOG.md | 36 +++++++------- cli/src/commands/rebase.rs | 82 ++++++++++++++++++-------------- cli/tests/cli-reference@.md.snap | 3 +- cli/tests/test_rebase_command.rs | 4 +- 4 files changed, 70 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cc374473a..786ecf9f6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). move` to ensure that the target branch already exists. [#3584](https://github.com/martinvonz/jj/issues/3584) +* `jj rebase --skip-empty` has been renamed to `jj rebase --skip-emptied` + ### Deprecations -* Replacing `-l` shorthand for `--limit` with `-n` in `jj log`, `jj op log` and `jj obslog`. +* Replacing `-l` shorthand for `--limit` with `-n` in `jj log`, `jj op log` + and `jj obslog`. * `jj split --siblings` is deprecated in favor of `jj split --parallel` (to match `jj parallelize`). @@ -88,7 +91,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). to-be-pushed commit has conflicts, or has no description / committer / author set. [#3029](https://github.com/martinvonz/jj/issues/3029) -* `jj` will look for divergent changes outside the short prefix set even if it finds the change id inside the short prefix set. [#2476](https://github.com/martinvonz/jj/issues/2476) +* `jj` will look for divergent changes outside the short prefix set even if it + finds the change id inside the short prefix + set. [#2476](https://github.com/martinvonz/jj/issues/2476) ## [0.18.0] - 2024-06-05 @@ -121,7 +126,6 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). were global flags and specifying them once would insert the new commit before/ after all the specified commits. - ### Deprecations * Attempting to alias a built-in command now gives a warning, rather than being @@ -233,12 +237,12 @@ Thanks to the people who made this release happen! ### Breaking changes * The default template aliases were replaced as follows: - * `builtin_op_log_root(op_id: OperationId)` -> - `format_root_operation(root: Operation)` - * `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` -> - `format_root_commit(root: Commit)` - * `builtin_change_id_with_hidden_and_divergent_info` -> - `format_short_change_id_with_hidden_and_divergent_info(commit: Commit)` + * `builtin_op_log_root(op_id: OperationId)` -> + `format_root_operation(root: Operation)` + * `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` -> + `format_root_commit(root: Commit)` + * `builtin_change_id_with_hidden_and_divergent_info` -> + `format_short_change_id_with_hidden_and_divergent_info(commit: Commit)` * The `--revision` option of `jj rebase` is renamed to `--revisions`. The short alias `-r` is still supported. @@ -362,8 +366,8 @@ Thanks to the people who made this release happen! default `jj` commands. * Graph node symbols are now configurable via templates - * `templates.log_node` - * `templates.op_log_node` + * `templates.log_node` + * `templates.op_log_node` * `jj log` now includes synthetic nodes in the graph where some revisions were elided. @@ -465,8 +469,8 @@ No code changes (fixing Rust `Cargo.toml` stuff). commits when they are created. This comes with out-of-the-box support for the following backends: - * GnuPG - * SSH + * GnuPG + * SSH Signature verification and an explicit sign command will hopefully come soon. @@ -812,9 +816,9 @@ Thanks to the people who made this release happen! `git.auto-local-branch` setting is applied only to newly fetched remote branches. Existing remote branches are migrated as follows: - * If local branch exists, the corresponding remote branches are considered - tracking branches. - * Otherwise, the remote branches are non-tracking branches. + * If local branch exists, the corresponding remote branches are considered + tracking branches. + * Otherwise, the remote branches are non-tracking branches. If the deduced tracking flags are wrong, use `jj branch track`/`untrack` commands to fix them up. diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index bd92e4feb3..9270f5831a 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -125,8 +125,8 @@ use crate::ui::Ui; /// commit. This is true in general; it is not specific to this command. #[derive(clap::Args, Clone, Debug)] #[command(verbatim_doc_comment)] -#[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revisions"])))] -#[command(group(ArgGroup::new("target").args(&["destination", "insert_after", "insert_before"]).multiple(true).required(true)))] +#[command(group(ArgGroup::new("to_rebase").args(& ["branch", "source", "revisions"])))] +#[command(group(ArgGroup::new("target").args(& ["destination", "insert_after", "insert_before"]).multiple(true).required(true)))] pub(crate) struct RebaseArgs { /// Rebase the whole branch relative to destination's ancestors (can be /// repeated) @@ -188,12 +188,16 @@ pub(crate) struct RebaseArgs { )] insert_before: Vec, + /// Deprecated. Use --skip-emptied instead. + #[arg(long, conflicts_with = "revisions")] + skip_empty: bool, + /// If true, when rebasing would produce an empty commit, the commit is /// abandoned. It will not be abandoned if it was already empty before the /// rebase. Will never skip merge commits with multiple non-empty /// parents. #[arg(long, conflicts_with = "revisions")] - skip_empty: bool, + skip_emptied: bool, /// Deprecated. Please prefix the revset with `all:` instead. #[arg(long, short = 'L', hide = true)] @@ -213,8 +217,14 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets )); } + if args.skip_empty { + return Err(user_error( + "--skip-empty is deprecated, and has been renamed to --skip-emptied.", + )); + } + let rebase_options = RebaseOptions { - empty: match args.skip_empty { + empty: match args.skip_emptied { true => EmptyBehaviour::AbandonNewlyEmpty, false => EmptyBehaviour::Keep, }, @@ -869,42 +879,42 @@ fn move_commits( .iter() .map(|(commit_id, commit)| { let new_parents = - // New child of the rebased target commits. - if let Some(new_child_parents) = new_children_parents.get(commit_id) { - new_child_parents.clone() - } - // Commits in the target set should persist only rebased parents from the target - // sets. - else if let Some(target_commit_parents) = - target_commits_internal_parents.get(commit_id) - { - // If the commit does not have any parents in the target set, it is one of the - // commits in the root set, and should be rebased onto the new destination. - if target_commit_parents.is_empty() { - new_parent_ids.clone() - } else { - target_commit_parents.clone() + // New child of the rebased target commits. + if let Some(new_child_parents) = new_children_parents.get(commit_id) { + new_child_parents.clone() } - } - // Commits outside the target set should have references to commits inside the set - // replaced. - else if commit - .parent_ids() - .iter() - .any(|id| target_commits_external_parents.contains_key(id)) - { - let mut new_parents = vec![]; - for parent in commit.parent_ids() { - if let Some(parents) = target_commits_external_parents.get(parent) { - new_parents.extend(parents.iter().cloned()); + // Commits in the target set should persist only rebased parents from the target + // sets. + else if let Some(target_commit_parents) = + target_commits_internal_parents.get(commit_id) + { + // If the commit does not have any parents in the target set, it is one of the + // commits in the root set, and should be rebased onto the new destination. + if target_commit_parents.is_empty() { + new_parent_ids.clone() } else { - new_parents.push(parent.clone()); + target_commit_parents.clone() } } - new_parents - } else { - commit.parent_ids().iter().cloned().collect_vec() - }; + // Commits outside the target set should have references to commits inside the set + // replaced. + else if commit + .parent_ids() + .iter() + .any(|id| target_commits_external_parents.contains_key(id)) + { + let mut new_parents = vec![]; + for parent in commit.parent_ids() { + if let Some(parents) = target_commits_external_parents.get(parent) { + new_parents.extend(parents.iter().cloned()); + } else { + new_parents.push(parent.clone()); + } + } + new_parents + } else { + commit.parent_ids().iter().cloned().collect_vec() + }; (commit_id.clone(), new_parents) }) diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 6d23499cc5..1a9b2e675d 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1540,7 +1540,8 @@ commit. This is true in general; it is not specific to this command. * `-B`, `--insert-before ` — The revision(s) to insert before (can be repeated to create a merge commit) Only works with `-r`. -* `--skip-empty` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents +* `--skip-empty` — Deprecated. Use --skip-emptied instead +* `--skip-emptied` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index b7ae47fbc3..8e4ca1c858 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -2236,7 +2236,7 @@ fn test_rebase_revisions_after_before() { } #[test] -fn test_rebase_skip_empty() { +fn test_rebase_skip_emptied() { let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); @@ -2259,7 +2259,7 @@ fn test_rebase_skip_empty() { ◉ "###); - let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-empty"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-emptied"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Rebased 3 commits