From b7f36e1e013bf4c31da1fe310dd85531f543b72f Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Tue, 2 Jan 2024 11:20:52 +1100 Subject: [PATCH 1/2] Rewrite instead of abandoning empty commits. Fixes #2760 Given the tree: ``` A-B-C \ B2 ``` And the command `jj rebase -s B -d B2` We were previously marking B as abandoned, despite the comment stating that we were marking it as being succeeded by B2. This resulted in a call to `rewrite(rewrites={}, abandoned={B})` instead of `rewrite(rewrites={B=>B2}, abandoned={})`, which then made the new parent of `C` into `A` instead of `B2` --- lib/src/rewrite.rs | 5 ++-- lib/tests/test_rewrite.rs | 56 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 0a786ac9e5..dba86dea29 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -159,11 +159,10 @@ pub fn rebase_commit_with_options( EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id, }; if should_abandon { - mut_repo.record_abandoned_commit(old_commit.id().clone()); - // Record old_commit as being succeeded by the parent for the purposes of - // the rebase. + // 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()); } } diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 67882a1ba2..255d7a48b0 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -19,7 +19,7 @@ use jj_lib::merged_tree::MergedTree; use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId}; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; -use jj_lib::rewrite::{restore_tree, EmptyBehaviour, RebaseOptions}; +use jj_lib::rewrite::{rebase_commit_with_options, restore_tree, EmptyBehaviour, RebaseOptions}; use maplit::{hashmap, hashset}; use test_case::test_case; use testutils::{ @@ -1612,3 +1612,57 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) { } ); } + +#[test] +fn test_rebase_commit_via_rebase_api() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Rebase B onto B2 + // + // C + // | + // B B2 + // |/ + // A + let mut tx = repo.start_transaction(&settings); + let commit_a = write_random_commit(tx.mut_repo(), &settings); + let commit_b = create_random_commit(tx.mut_repo(), &settings) + .set_parents(vec![commit_a.id().clone()]) + .write() + .unwrap(); + let commit_c = create_random_commit(tx.mut_repo(), &settings) + .set_parents(vec![commit_b.id().clone()]) + .write() + .unwrap(); + let commit_b2 = create_random_commit(tx.mut_repo(), &settings) + .set_parents(vec![commit_a.id().clone()]) + .set_tree_id(commit_b.tree_id().clone()) + .write() + .unwrap(); + + let rebase_options = RebaseOptions { + empty: EmptyBehaviour::AbandonAllEmpty, + }; + rebase_commit_with_options( + &settings, + tx.mut_repo(), + &commit_b, + &[commit_b2.clone()], + &rebase_options, + ) + .unwrap(); + let rebase_map = tx + .mut_repo() + .rebase_descendants_with_options_return_map(&settings, rebase_options) + .unwrap(); + assert_eq!(rebase_map.len(), 1); + let new_commit_c = + assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[&commit_b2.id()]); + + assert_eq!( + *tx.mut_repo().view().heads(), + hashset! {new_commit_c.id().clone()} + ); +} From 5f753847528aa48f64f0010a4138603e779c896b Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Wed, 3 Jan 2024 13:56:53 +1100 Subject: [PATCH 2/2] Ensure you never drop the working commit with --skip-empty See #2766 for discussions --- cli/src/commands/rebase.rs | 1 + lib/src/rewrite.rs | 7 +++++- lib/tests/test_rewrite.rs | 48 +++++++++++++++++++++++++++++++------- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index e733902bd9..6099d1f4ee 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -148,6 +148,7 @@ pub(crate) struct RebaseArgs { /// 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. #[arg(long, conflicts_with = "revision")] skip_empty: bool, diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index dba86dea29..b896ad81f2 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -158,7 +158,12 @@ pub fn rebase_commit_with_options( } EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id, }; - if should_abandon { + // 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()) { // 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. diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 255d7a48b0..24ddd8098d 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1614,18 +1614,25 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) { } #[test] -fn test_rebase_commit_via_rebase_api() { +fn test_rebase_abandoning_empty() { let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; - // Rebase B onto B2 + // Rebase B onto B2, where B2 and B have the same tree, abandoning all empty + // commits. // - // C - // | - // B B2 - // |/ - // A + // We expect B and D to be skipped because they're empty, but not E because it's + // the working commit. + // + // D (empty) E (WC, empty) E' (WC, empty) + // | / | + // C--------- C' + // | => | + // B B2 B2 + // |/ | + // A A + let mut tx = repo.start_transaction(&settings); let commit_a = write_random_commit(tx.mut_repo(), &settings); let commit_b = create_random_commit(tx.mut_repo(), &settings) @@ -1636,12 +1643,27 @@ fn test_rebase_commit_via_rebase_api() { .set_parents(vec![commit_b.id().clone()]) .write() .unwrap(); + let commit_d = create_random_commit(tx.mut_repo(), &settings) + .set_parents(vec![commit_c.id().clone()]) + .set_tree_id(commit_c.tree_id().clone()) + .write() + .unwrap(); + let commit_e = create_random_commit(tx.mut_repo(), &settings) + .set_parents(vec![commit_c.id().clone()]) + .set_tree_id(commit_c.tree_id().clone()) + .write() + .unwrap(); let commit_b2 = create_random_commit(tx.mut_repo(), &settings) .set_parents(vec![commit_a.id().clone()]) .set_tree_id(commit_b.tree_id().clone()) .write() .unwrap(); + let workspace = WorkspaceId::new("ws".to_string()); + tx.mut_repo() + .set_wc_commit(workspace.clone(), commit_e.id().clone()) + .unwrap(); + let rebase_options = RebaseOptions { empty: EmptyBehaviour::AbandonAllEmpty, }; @@ -1657,12 +1679,20 @@ fn test_rebase_commit_via_rebase_api() { .mut_repo() .rebase_descendants_with_options_return_map(&settings, rebase_options) .unwrap(); - assert_eq!(rebase_map.len(), 1); + assert_eq!(rebase_map.len(), 3); 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_eq!( *tx.mut_repo().view().heads(), - hashset! {new_commit_c.id().clone()} + hashset! {new_commit_e.id().clone()} + ); + + assert_eq!( + tx.mut_repo().view().get_wc_commit_id(&workspace), + Some(new_commit_e.id()) ); }