rewrite.rs: fix working copy position after jj rebase --abandon-empty

Fixes #2869

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<Commit, TreeMergeError> {
+) -> 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,

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