rewrite.rs: revert commits cfcc7c5 and becbc88

This mostly reverts martinvonz#2901 as well as its
fixup martinvonz#2903. The related bug is reopened,
see martinvonz#2869 (comment).

The problem is that while the fix did fix martinvonz#2869 in most cases, it did
reintroduce the more severe bug martinvonz#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 --skip-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 --skip-empty`, you'd have the
following result (before this commit), which shows the reintroduction of martinvonz#2760:

```
root -> A @ -> C
         \-- B
```

with the working copy at `A`. The reason for this is explained in
martinvonz#2901 (comment).

### After this commit

After this commit, both case 1 and case 2 will be wrong in the sense of martinvonz#2869,
but it will no longer exhibit the worse bug martinvonz#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

@@ -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<Commit, TreeMergeError> {
     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(())
     }
 

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