workspace: abandon discardable working copy on forget

Forgetting a workspace removes its working-copy commit, so it makes
sense for it to be abandoned if it is discardable just like editing a
new commit will cause the old commit to be abandoned if it is
discardable.

CHANGELOG.md

@@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   default branch of the remote as repository settings for
   `revset-aliases."trunk()"`.`
 
+* `jj workspace forget` now abandons the workspace's working-copy commit if it
+  was empty.
+
 ### Fixed bugs
 
 ## [0.19.0] - 2024-07-03

cli/src/commands/workspace.rs

@@ -260,7 +260,8 @@ fn cmd_workspace_forget(
     // bundle every workspace forget into a single transaction, so that e.g.
     // undo correctly restores all of them at once.
     let mut tx = workspace_command.start_transaction();
-    wss.iter().for_each(|ws| tx.mut_repo().remove_wc_commit(ws));
+    wss.iter()
+        .try_for_each(|ws| tx.mut_repo().remove_wc_commit(ws))?;
     let description = if let [ws] = wss.as_slice() {
         format!("forget workspace {}", ws.as_str())
     } else {

cli/tests/test_workspaces.rs

@@ -628,14 +628,11 @@ fn test_workspaces_forget() {
     insta::assert_snapshot!(stderr, @"");
 
     // The old working copy doesn't get an "@" in the log output
-    // TODO: We should abandon the empty working copy commit
     // TODO: It seems useful to still have the "secondary@" marker here even though
     // there's only one workspace. We should show it when the command is not run
     // from that workspace.
     insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
     ◉  18463f438cc9
-    │ ◉  909d51b17292
-    ├─╯
     ◉  4e8f9d2be039
     ◉  000000000000
     "###);
@@ -695,7 +692,7 @@ fn test_workspaces_forget_multi_transaction() {
     // the op log should have multiple workspaces forgotten in a single tx
     let stdout = test_env.jj_cmd_success(&main_path, &["op", "log", "--limit", "1"]);
     insta::assert_snapshot!(stdout, @r###"
-    @  6c88cdee70e6 test-username@host.example.com 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00
+    @  f91852cb278f test-username@host.example.com 2001-02-03 04:05:12.000 +07:00 - 2001-02-03 04:05:12.000 +07:00
     │  forget workspaces second, third
     │  args: jj workspace forget second third
     "###);
@@ -712,6 +709,65 @@ fn test_workspaces_forget_multi_transaction() {
     "###);
 }
 
+#[test]
+fn test_workspaces_forget_abandon_commits() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "main"]);
+    let main_path = test_env.env_root().join("main");
+
+    std::fs::write(main_path.join("file"), "contents").unwrap();
+
+    test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../second"]);
+    test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../third"]);
+    test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../fourth"]);
+    let third_path = test_env.env_root().join("third");
+    test_env.jj_cmd_ok(&third_path, &["edit", "second@"]);
+    let fourth_path = test_env.env_root().join("fourth");
+    test_env.jj_cmd_ok(&fourth_path, &["edit", "second@"]);
+
+    // there should be four workspaces, three of which are at the same empty commit
+    let stdout = test_env.jj_cmd_success(&main_path, &["workspace", "list"]);
+    insta::assert_snapshot!(stdout, @r###"
+    default: qpvuntsm 4e8f9d2b (no description set)
+    fourth: uuqppmxq 57d63245 (empty) (no description set)
+    second: uuqppmxq 57d63245 (empty) (no description set)
+    third: uuqppmxq 57d63245 (empty) (no description set)
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
+    ◉  57d63245a308 fourth@ second@ third@
+    │ @  4e8f9d2be039 default@
+    ├─╯
+    ◉  000000000000
+    "###);
+
+    // delete the default workspace (should not abandon commit since not empty)
+    test_env.jj_cmd_success(&main_path, &["workspace", "forget", "default"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
+    ◉  57d63245a308 fourth@ second@ third@
+    │ ◉  4e8f9d2be039
+    ├─╯
+    ◉  000000000000
+    "###);
+
+    // delete the second workspace (should not abandon commit since other workspaces
+    // still have commit checked out)
+    test_env.jj_cmd_success(&main_path, &["workspace", "forget", "second"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
+    ◉  57d63245a308 fourth@ third@
+    │ ◉  4e8f9d2be039
+    ├─╯
+    ◉  000000000000
+    "###);
+
+    // delete the last 2 workspaces (commit should be abandoned now even though
+    // forgotten in same tx)
+    test_env.jj_cmd_success(&main_path, &["workspace", "forget", "third", "fourth"]);
+    insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
+    ◉  4e8f9d2be039
+    ◉  000000000000
+    "###);
+}
+
 /// Test context of commit summary template
 #[test]
 fn test_list_workspaces_template() {

lib/src/repo.rs

@@ -1290,8 +1290,10 @@ impl MutableRepo {
         Ok(())
     }
 
-    pub fn remove_wc_commit(&mut self, workspace_id: &WorkspaceId) {
+    pub fn remove_wc_commit(&mut self, workspace_id: &WorkspaceId) -> Result<(), EditCommitError> {
+        self.maybe_abandon_wc_commit(workspace_id)?;
         self.view_mut().remove_wc_commit(workspace_id);
+        Ok(())
     }
 
     pub fn check_out(

lib/tests/test_mut_repo.rs

@@ -598,3 +598,54 @@ fn test_rename_remote() {
         RemoteRef::absent()
     );
 }
+
+#[test]
+fn test_remove_wc_commit_previous_not_discardable() {
+    // Test that MutableRepo::remove_wc_commit() does not usually abandon the
+    // previous commit.
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    let mut tx = repo.start_transaction(&settings);
+    let mut_repo = tx.mut_repo();
+    let old_wc_commit = write_random_commit(mut_repo, &settings);
+    let ws_id = WorkspaceId::default();
+    mut_repo.edit(ws_id.clone(), &old_wc_commit).unwrap();
+    let repo = tx.commit("test");
+
+    let mut tx = repo.start_transaction(&settings);
+    let mut_repo = tx.mut_repo();
+    mut_repo.remove_wc_commit(&ws_id).unwrap();
+    mut_repo.rebase_descendants(&settings).unwrap();
+    assert!(mut_repo.view().heads().contains(old_wc_commit.id()));
+}
+
+#[test]
+fn test_remove_wc_commit_previous_discardable() {
+    // Test that MutableRepo::remove_wc_commit() abandons the previous commit
+    // if it was discardable.
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    let mut tx = repo.start_transaction(&settings);
+    let mut_repo = tx.mut_repo();
+    let old_wc_commit = mut_repo
+        .new_commit(
+            &settings,
+            vec![repo.store().root_commit_id().clone()],
+            repo.store().empty_merged_tree_id(),
+        )
+        .write()
+        .unwrap();
+    let ws_id = WorkspaceId::default();
+    mut_repo.edit(ws_id.clone(), &old_wc_commit).unwrap();
+    let repo = tx.commit("test");
+
+    let mut tx = repo.start_transaction(&settings);
+    let mut_repo = tx.mut_repo();
+    mut_repo.remove_wc_commit(&ws_id).unwrap();
+    mut_repo.rebase_descendants(&settings).unwrap();
+    assert!(!mut_repo.view().heads().contains(old_wc_commit.id()));
+}

lib/tests/test_view.rs

@@ -158,7 +158,7 @@ fn test_merge_views_checkout() {
     tx1.mut_repo()
         .set_wc_commit(ws2_id.clone(), commit2.id().clone())
         .unwrap();
-    tx1.mut_repo().remove_wc_commit(&ws4_id);
+    tx1.mut_repo().remove_wc_commit(&ws4_id).unwrap();
     tx1.mut_repo()
         .set_wc_commit(ws5_id.clone(), commit2.id().clone())
         .unwrap();
@@ -176,7 +176,7 @@ fn test_merge_views_checkout() {
     tx2.mut_repo()
         .set_wc_commit(ws4_id.clone(), commit3.id().clone())
         .unwrap();
-    tx2.mut_repo().remove_wc_commit(&ws5_id);
+    tx2.mut_repo().remove_wc_commit(&ws5_id).unwrap();
     tx2.mut_repo()
         .set_wc_commit(ws7_id.clone(), commit3.id().clone())
         .unwrap();