diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index f5e61e4f5c..b640afb3a6 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -112,15 +112,27 @@ 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 { +) -> Result<(Commit, bool), TreeMergeError> { let old_parents = old_commit.parents(); let old_parent_trees = old_parents .iter() @@ -164,23 +176,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( @@ -456,10 +471,10 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { &mut self, old_commit_id: CommitId, new_commit_ids: Vec, - edit: bool, + abandoned_old_commit: bool, ) -> Result<(), BackendError> { // We arbitrarily pick a new working-copy commit among the candidates. - self.update_wc_commits(&old_commit_id, &new_commit_ids[0], edit)?; + 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() { let mut branch_updates = vec![]; @@ -496,7 +511,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { &mut self, old_commit_id: &CommitId, new_commit_id: &CommitId, - edit: bool, + abandoned_old_commit: bool, ) -> Result<(), BackendError> { let workspaces_to_update = self .mut_repo @@ -507,7 +522,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } let new_commit = self.mut_repo.store().get_commit(new_commit_id)?; - let new_wc_commit = if edit { + let new_wc_commit = if !abandoned_old_commit { new_commit } else { self.mut_repo @@ -534,13 +549,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, true)?; + self.update_references(old_commit_id, new_parent_ids, false)?; continue; } 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, true)?; + self.update_references(old_commit_id, divergent_ids, false)?; continue; } let old_parent_ids = old_commit.parent_ids(); @@ -549,7 +564,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, false)?; + self.update_references(old_commit_id, new_parent_ids, true)?; continue; } else if new_parent_ids == old_parent_ids { // The commit is already in place. @@ -568,7 +583,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 +601,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()], true)?; + 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} ); }