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()} ); }