From 39528c7569d8c411c22c941d9278ffc9d4589874 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 24 Feb 2024 19:37:57 -0800 Subject: [PATCH 1/3] rewrite: move decision about abandoned commit into update_references() --- lib/src/rewrite.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 32e134828f..0633aa5089 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -458,9 +458,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { &mut self, old_commit_id: CommitId, new_commit_ids: Vec, - abandoned_old_commit: bool, ) -> Result<(), BackendError> { // We arbitrarily pick a new working-copy commit among the candidates. + let abandoned_old_commit = self.abandoned.contains(&old_commit_id); self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?; if let Some(branch_names) = self.branches.get(&old_commit_id).cloned() { @@ -533,13 +533,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { // (i.e. it's part of the input for this rebase). We don't need // to rebase it, but we still want to update branches pointing // to the old commit. - self.update_references(old_commit_id, new_parent_ids, false)?; + self.update_references(old_commit_id, new_parent_ids)?; return Ok(()); } if let Some(divergent_ids) = self.divergent.get(&old_commit_id).cloned() { // Leave divergent commits in place. Don't update `parent_mapping` since we // don't want to rebase descendants either. - self.update_references(old_commit_id, divergent_ids, false)?; + self.update_references(old_commit_id, divergent_ids)?; return Ok(()); } let old_parent_ids = old_commit.parent_ids(); @@ -548,7 +548,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { // Update the `new_parents` map so descendants are rebased correctly. self.parent_mapping .insert(old_commit_id.clone(), new_parent_ids.clone()); - self.update_references(old_commit_id, new_parent_ids, true)?; + self.update_references(old_commit_id, new_parent_ids)?; return Ok(()); } else if new_parent_ids == old_parent_ids { // The commit is already in place. @@ -587,7 +587,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { (None, None), "Trying to rebase the same commit {old_commit_id:?} in two different ways", ); - self.update_references(old_commit_id, vec![new_commit.id().clone()], false)?; + self.update_references(old_commit_id, vec![new_commit.id().clone()])?; Ok(()) } From 4f66f6ff1cd6df67d87dec4b8155917b56e15eb2 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 25 Feb 2024 09:41:07 -0800 Subject: [PATCH 2/3] cli: make `rebase --skip-empty` keep already empty commits I think the user usually wants to abandon only newly empty commits. I think they should use `jj abandon` if they want to get rid of already empty commits. By keeping already empty commits, we don't need to special-case the working copy and merge commits. --- CHANGELOG.md | 3 +++ cli/src/commands/rebase.rs | 8 +++--- cli/tests/cli-reference@.md.snap | 2 +- cli/tests/test_rebase_command.rs | 43 ++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20d5ab4f22..85676984e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Dropped support for the deprecated `:` revset operator. Use `::` instead. +* `jj rebase --skip-empty` no longer abandons commits that were already empty + before the rebase. + ### New features * Added support for commit signing and verification. This comes with diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index a987a9352a..8e02bf2dcf 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -155,9 +155,9 @@ pub(crate) struct RebaseArgs { destination: Vec, /// If true, when rebasing would produce an empty commit, the commit is - /// skipped. - /// Will never skip merge commits with multiple non-empty parents. - /// Will never skip the working commit. + /// 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 = "revision")] skip_empty: bool, @@ -181,7 +181,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets let rebase_options = RebaseOptions { empty: match args.skip_empty { - true => EmptyBehaviour::AbandonAllEmpty, + true => EmptyBehaviour::AbandonNewlyEmpty, false => EmptyBehaviour::Keep, }, simplify_ancestor_merge: false, diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index ef8c9699f1..f72a92a33d 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -1461,7 +1461,7 @@ J J * `-s`, `--source ` — Rebase specified revision(s) together their tree of descendants (can be repeated) * `-r`, `--revision ` — Rebase only this revision, rebasing descendants onto this revision's parent(s) * `-d`, `--destination ` — The revision(s) to rebase onto (can be repeated to create a merge commit) -* `--skip-empty` — If true, when rebasing would produce an empty commit, the commit is skipped. Will never skip merge commits with multiple non-empty parents. Will never skip the working commit +* `--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 Possible values: `true`, `false` diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 3729c65d26..b820139c80 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -1038,3 +1038,46 @@ fn test_rebase_with_child_and_descendant_bug_2600() { ◉ "###); } + +#[test] +fn test_rebase_skip_empty() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + create_commit(&test_env, &repo_path, "a", &[]); + create_commit(&test_env, &repo_path, "b", &["a"]); + test_env.jj_cmd_ok(&repo_path, &["new", "a", "-m", "will become empty"]); + test_env.jj_cmd_ok(&repo_path, &["restore", "--from=b"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "already empty"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "also already empty"]); + + // Test the setup + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###" + @ also already empty + ◉ already empty + ◉ will become empty + │ ◉ b + ├─╯ + ◉ a + ◉ + "###); + + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-empty"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 3 commits + Working copy now at: yostqsxw 6b74c840 (empty) also already empty + Parent commit : vruxwmqv 48a31526 (empty) already empty + "###); + + // The parent commit became empty and was dropped, but the already empty commits + // were kept + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###" + @ also already empty + ◉ already empty + ◉ b + ◉ a + ◉ + "###); +} From 4258d9160c18c0ad158279d993eaf9dd0f415029 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 24 Feb 2024 19:55:34 -0800 Subject: [PATCH 3/3] rewrite: allow working-copy to be abandoned This removes the special handling of the working-copy commit. By recording when an empty/emptied commit was abanoned, we rebase descendants correctly and create a new empty working-copy commit on top. --- lib/src/rewrite.rs | 40 +++++++++++++++++++++++++------------ lib/tests/test_rewrite.rs | 42 ++++++++++++++++++--------------------- 2 files changed, 46 insertions(+), 36 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 0633aa5089..47e9113fca 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -105,13 +105,22 @@ pub fn rebase_commit( old_commit: &Commit, new_parents: &[Commit], ) -> Result { - rebase_commit_with_options( + let rebased_commit = rebase_commit_with_options( settings, mut_repo, old_commit, new_parents, &Default::default(), - ) + )?; + match rebased_commit { + RebasedCommit::Rewritten(new_commit) => Ok(new_commit), + RebasedCommit::Abandoned { parent: _ } => panic!("Commit was unexpectedly abandoned"), + } +} + +pub enum RebasedCommit { + Rewritten(Commit), + Abandoned { parent: Commit }, } pub fn rebase_commit_with_options( @@ -120,7 +129,7 @@ pub fn rebase_commit_with_options( old_commit: &Commit, new_parents: &[Commit], options: &RebaseOptions, -) -> Result { +) -> Result { let old_parents = old_commit.parents(); let old_parent_trees = old_parents .iter() @@ -159,28 +168,26 @@ pub fn rebase_commit_with_options( } EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id, }; - // If the user runs `jj checkout foo`, then `jj rebase -s foo -d bar`, and we - // drop the checked out empty commit, then the user will unknowingly - // have done the equivalent of `jj edit foo` instead of `jj checkout - // foo`. Thus, we never allow dropping the working commit. See #2766 and - // #2760 for discussions. - if should_abandon && !mut_repo.view().is_wc_commit_id(old_commit.id()) { + if should_abandon { // Record old_commit as being succeeded by the parent. // This ensures that when we stack commits, the second commit knows to // rebase on top of the parent commit, rather than the abandoned commit. mut_repo.record_rewritten_commit(old_commit.id().clone(), parent.id().clone()); - return Ok(parent.clone()); + return Ok(RebasedCommit::Abandoned { + parent: parent.clone(), + }); } } let new_parent_ids = new_parents .iter() .map(|commit| commit.id().clone()) .collect(); - Ok(mut_repo + let new_commit = mut_repo .rewrite_commit(settings, old_commit) .set_parents(new_parent_ids) .set_tree_id(new_tree_id) - .write()?) + .write()?; + Ok(RebasedCommit::Rewritten(new_commit)) } pub fn rebase_to_dest_parent( @@ -569,13 +576,20 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .iter() .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id)) .try_collect()?; - let new_commit = rebase_commit_with_options( + let rebased_commit: RebasedCommit = rebase_commit_with_options( self.settings, self.mut_repo, &old_commit, &new_parents, &self.options, )?; + let new_commit = match rebased_commit { + RebasedCommit::Rewritten(new_commit) => new_commit, + RebasedCommit::Abandoned { parent } => { + self.abandoned.insert(old_commit.id().clone()); + parent + } + }; let previous_rebased_value = self .rebased .insert(old_commit_id.clone(), new_commit.id().clone()); diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 1301972bfb..7285c070d3 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1663,24 +1663,17 @@ fn test_rebase_abandoning_empty() { // Rebase B onto B2, where B2 and B have the same tree, abandoning all empty // commits. // - // We expect B, D, and G to be skipped because they're empty, but not E because - // it's the working commit. F also remains as it's not empty. - // - // F G (empty) F' - // |/ | - // E (WC, empty) D (empty) E' (WC, empty) - // | / | + // We expect B, D, E, and G to be skipped because they're empty. F remains + // as it's not empty. + // F G (empty) + // |/ + // E (WC, empty) D (empty) F' E' (WC, empty) + // | / |/ // C------------- C' // | => | // B B2 B2 // |/ | // A A - // - // TODO(#2869): There is a minor bug here. We'd like the result to be - // equivalent to rebasing and then `jj abandon`-ing all the empty commits. - // In case of the working copy, this would mean that F' would be a direct - // child of C', and the working copy would be a new commit that's also a - // direct child of C'. let mut tx = repo.start_transaction(&settings); let commit_a = write_random_commit(tx.mut_repo(), &settings); @@ -1742,19 +1735,22 @@ fn test_rebase_abandoning_empty() { let new_commit_c = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_b2.id()]); assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, new_commit_c.id()); - let new_commit_e = - assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[new_commit_c.id()]); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id()); let new_commit_f = - assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[new_commit_e.id()]); - assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_e.id()); + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[new_commit_c.id()]); + assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id()); - assert_eq!( - *tx.mut_repo().view().heads(), - hashset! {new_commit_f.id().clone()} - ); + let new_wc_commit_id = tx + .mut_repo() + .view() + .get_wc_commit_id(&workspace) + .unwrap() + .clone(); + let new_wc_commit = tx.mut_repo().store().get_commit(&new_wc_commit_id).unwrap(); + assert_eq!(new_wc_commit.parent_ids(), &[new_commit_c.id().clone()]); assert_eq!( - tx.mut_repo().view().get_wc_commit_id(&workspace), - Some(new_commit_e.id()) + *tx.mut_repo().view().heads(), + hashset! {new_commit_f.id().clone(), new_wc_commit_id.clone()} ); }