From b690ba7ce3d165c425cb6e2024b0ee8df5ed01da Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jan 2024 22:12:32 -0800 Subject: [PATCH] rewrite.rs: fix working copy position after `jj rebase --abandon-empty` Fixes #2869 --- lib/src/rewrite.rs | 29 +++++++++++++++++--------- lib/tests/test_rewrite.rs | 44 +++++++++++++++++++++------------------ 2 files changed, 43 insertions(+), 30 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 32d077a091..91de854dba 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -112,15 +112,17 @@ pub fn rebase_commit( new_parents, &Default::default(), ) + .map(|(commit, _abandoned_old_commit)| commit) } +/// Returns the new parent commit, and whether the old commit was abandoned pub fn rebase_commit_with_options( settings: &UserSettings, mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], options: &RebaseOptions, -) -> Result { +) -> Result<(Commit, bool), TreeMergeError> { let old_parents = old_commit.parents(); let old_parent_trees = old_parents .iter() @@ -164,23 +166,26 @@ pub fn rebase_commit_with_options( // 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((parent.clone(), true)); } } let new_parent_ids = new_parents .iter() .map(|commit| commit.id().clone()) .collect(); - Ok(mut_repo - .rewrite_commit(settings, old_commit) - .set_parents(new_parent_ids) - .set_tree_id(new_tree_id) - .write()?) + Ok(( + mut_repo + .rewrite_commit(settings, old_commit) + .set_parents(new_parent_ids) + .set_tree_id(new_tree_id) + .write()?, + false, + )) } pub fn rebase_to_dest_parent( @@ -568,7 +573,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .filter(|new_parent| head_set.contains(new_parent)) .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id)) .try_collect()?; - let new_commit = rebase_commit_with_options( + let (new_commit, abandoned_old_commit) = rebase_commit_with_options( self.settings, self.mut_repo, &old_commit, @@ -586,7 +591,11 @@ 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()], + abandoned_old_commit, + )?; return Ok(Some(RebasedDescendant { old_commit, new_commit, diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 4d6d59fcff..afea5ff7e8 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1625,21 +1625,15 @@ fn test_rebase_abandoning_empty() { // 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) - // | / | + // F G (empty) + // |/ + // E (WC, empty) D (empty) F' new_working_copy_commit (new, 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); @@ -1700,19 +1694,29 @@ 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()); + let new_working_copy_commit_id = tx + .mut_repo() + .view() + .get_wc_commit_id(&workspace) + .unwrap() + .clone(); + let new_working_copy_commit = repo + .store() + .get_commit(&new_working_copy_commit_id) + .unwrap() + .clone(); + + assert_ne!(new_working_copy_commit.change_id(), commit_c.change_id()); + assert_ne!(new_working_copy_commit.change_id(), commit_e.change_id()); + assert_ne!(new_working_copy_commit.change_id(), commit_f.change_id()); + assert_ne!(new_working_copy_commit.change_id(), commit_g.change_id()); assert_eq!( *tx.mut_repo().view().heads(), - hashset! {new_commit_f.id().clone()} - ); - - assert_eq!( - tx.mut_repo().view().get_wc_commit_id(&workspace), - Some(new_commit_e.id()) + hashset! {new_commit_f.id().clone(), new_working_copy_commit_id} ); }