From 237914567093f3168185ea159e3809d5bd253cd9 Mon Sep 17 00:00:00 2001
From: Matt Stark <msta@google.com>
Date: Wed, 3 Jan 2024 13:56:53 +1100
Subject: [PATCH] Ensure you never drop the working commit with --skip-empty

See #2766 for discussions
---
 cli/src/commands/rebase.rs |  1 +
 lib/src/rewrite.rs         |  7 +++++-
 lib/tests/test_rewrite.rs  | 47 ++++++++++++++++++++++++++++++--------
 3 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs
index e733902bd9..6099d1f4ee 100644
--- a/cli/src/commands/rebase.rs
+++ b/cli/src/commands/rebase.rs
@@ -148,6 +148,7 @@ pub(crate) struct RebaseArgs {
     /// If true, when rebasing would produce an empty commit, the commit is
     /// skipped.
     /// Will never skip merge commits with multiple non-empty parents.
+    /// Will never skip the working commit.
     #[arg(long, conflicts_with = "revision")]
     skip_empty: bool,
 
diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs
index dba86dea29..b896ad81f2 100644
--- a/lib/src/rewrite.rs
+++ b/lib/src/rewrite.rs
@@ -158,7 +158,12 @@ pub fn rebase_commit_with_options(
             }
             EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id,
         };
-        if should_abandon {
+        // If the user runs `jj checkout foo`, then `jj rebase -s foo -d bar`, and we
+        // drop the checked out empty commit, then the user will unknowingly
+        // 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()) {
             // 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.
diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs
index 255d7a48b0..8aed433e49 100644
--- a/lib/tests/test_rewrite.rs
+++ b/lib/tests/test_rewrite.rs
@@ -1614,18 +1614,24 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) {
 }
 
 #[test]
-fn test_rebase_commit_via_rebase_api() {
+fn test_rebase_abandoning_empty() {
     let settings = testutils::user_settings();
     let test_repo = TestRepo::init();
     let repo = &test_repo.repo;
 
-    // Rebase B onto B2
+    // Rebase B onto B2, where B2 and B have the same tree, abandoning all empty commits.
     //
-    // C
-    // |
-    // B B2
-    // |/
-    // A
+    // We expect B and D to be skipped because they're empty, but not E because it's the working
+    // commit.
+    //
+    // D (empty)  E (WC, empty)       E' (WC, empty)
+    // |         /                    |
+    // C---------                     C'
+    // |                           => |
+    // B B2                           B2
+    // |/                             |
+    // A                              A
+
     let mut tx = repo.start_transaction(&settings);
     let commit_a = write_random_commit(tx.mut_repo(), &settings);
     let commit_b = create_random_commit(tx.mut_repo(), &settings)
@@ -1636,12 +1642,27 @@ fn test_rebase_commit_via_rebase_api() {
         .set_parents(vec![commit_b.id().clone()])
         .write()
         .unwrap();
+    let commit_d = create_random_commit(tx.mut_repo(), &settings)
+        .set_parents(vec![commit_c.id().clone()])
+        .set_tree_id(commit_c.tree_id().clone())
+        .write()
+        .unwrap();
+    let commit_e = create_random_commit(tx.mut_repo(), &settings)
+        .set_parents(vec![commit_c.id().clone()])
+        .set_tree_id(commit_c.tree_id().clone())
+        .write()
+        .unwrap();
     let commit_b2 = create_random_commit(tx.mut_repo(), &settings)
         .set_parents(vec![commit_a.id().clone()])
         .set_tree_id(commit_b.tree_id().clone())
         .write()
         .unwrap();
 
+    let workspace = WorkspaceId::new("ws".to_string());
+    tx.mut_repo()
+        .set_wc_commit(workspace.clone(), commit_e.id().clone())
+        .unwrap();
+
     let rebase_options = RebaseOptions {
         empty: EmptyBehaviour::AbandonAllEmpty,
     };
@@ -1657,12 +1678,20 @@ fn test_rebase_commit_via_rebase_api() {
         .mut_repo()
         .rebase_descendants_with_options_return_map(&settings, rebase_options)
         .unwrap();
-    assert_eq!(rebase_map.len(), 1);
+    assert_eq!(rebase_map.len(), 3);
     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_eq!(
         *tx.mut_repo().view().heads(),
-        hashset! {new_commit_c.id().clone()}
+        hashset! {new_commit_e.id().clone()}
+    );
+
+    assert_eq!(
+        tx.mut_repo().view().get_wc_commit_id(&workspace),
+        Some(new_commit_e.id())
     );
 }