From 82b635a5044e0aa40fad4d03bc996185bc751ead Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Fri, 2 Feb 2024 20:30:26 -0800 Subject: [PATCH] rewrite.rs: revert commits cfcc7c5e and becbc889 This mostly reverts https://github.com/martinvonz/jj/pull/2901 as well as its fixup https://github.com/martinvonz/jj/pull/2903. The related bug is reopened, see https://github.com/martinvonz/jj/issues/2869#issuecomment-1920367932. The problem is that while the fix did fix #2896 in most cases, it did reintroduce the more severe bug https://github.com/martinvonz/jj/issues/2760 in one case, if the working copy is the commit being rebased. For example, suppose you have the tree ``` root -> A -> B -> @ (empty) -> C ``` ### Before this commit #### Case 1 `jj rebase -s B -d root --abandon-empty` would work perfectly before this commit, resulting in ``` root -> A \-------B -> C \- @ (new, empty) ``` #### Case 2 Unfortunately, if you run `jj rebase -s @ -d A --abandon-empty`, you'd have the following result (before this commit), which shows the reintroduction of #2760: ``` root -> A @ -> C \-- B ``` with the working copy at `A`. The reason for this is explained in https://github.com/martinvonz/jj/pull/2901#issuecomment-1920043560. ### After this commit After this commit, both case 1 and case 2 will be wrong in the sense of #2896, but it will no longer exhibit the worse bug #2760 in the second case. Case 1 would result in: ``` root -> A \-------B -> @ (empty) -> C ``` Case 2 would result in: ``` root -> A -> @ -> C \-- B ``` with the working copy remaining a descendant of A --- lib/src/rewrite.rs | 39 ++++++++---------------------- lib/tests/test_rewrite.rs | 51 +++++++++++++++++---------------------- 2 files changed, 32 insertions(+), 58 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 1026e76cc0..17a8f606e0 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -112,27 +112,15 @@ pub fn rebase_commit( new_parents, &Default::default(), ) - .map(|(commit, abandoned_old_commit)| { - assert!( - !abandoned_old_commit, - "Old commit should never be abandoned since the default options include \ - EmptyBehavior::Keep" - ); - commit - }) } -/// Returns the new parent commit, and whether the old commit was abandoned -/// -/// It should only be possible for the old commit to be abandoned if -/// `options.empty != EmptyBehavior::Keep` pub fn rebase_commit_with_options( settings: &UserSettings, mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], options: &RebaseOptions, -) -> Result<(Commit, bool), TreeMergeError> { +) -> Result { let old_parents = old_commit.parents(); let old_parent_trees = old_parents .iter() @@ -176,26 +164,23 @@ 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 { + 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. mut_repo.record_rewritten_commit(old_commit.id().clone(), parent.id().clone()); - return Ok((parent.clone(), true)); + return Ok(parent.clone()); } } 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()?, - false, - )) + Ok(mut_repo + .rewrite_commit(settings, old_commit) + .set_parents(new_parent_ids) + .set_tree_id(new_tree_id) + .write()?) } pub fn rebase_to_dest_parent( @@ -580,7 +565,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, abandoned_old_commit) = rebase_commit_with_options( + let new_commit = rebase_commit_with_options( self.settings, self.mut_repo, &old_commit, @@ -598,11 +583,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()], - abandoned_old_commit, - )?; + self.update_references(old_commit_id, vec![new_commit.id().clone()], false)?; Ok(()) } diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 4af1f5dce4..4d6d59fcff 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -1622,21 +1622,24 @@ 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, E and G to be skipped because they're empty. F remains - // as it's not empty. Since the working copy commit E is skipped, a new working - // copy commit should be created in its stead, just as would have happened if we - // did a normal rebase and then manually abandoned the working copy with `jj - // abandon`. + // 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) - // |/ - // E (WC, empty) D (empty) F' new_working_copy_commit (new, WC, empty) - // | / | / + // F G (empty) F' + // |/ | + // E (WC, empty) D (empty) 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); @@ -1697,29 +1700,19 @@ 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()); - assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id()); + let new_commit_e = + assert_rebased_onto(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_c.id()]); - assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id()); + 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()); - 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(), new_working_copy_commit_id} + hashset! {new_commit_f.id().clone()} + ); + + assert_eq!( + tx.mut_repo().view().get_wc_commit_id(&workspace), + Some(new_commit_e.id()) ); }