From 54877e1f797b13527a597989976695fe1b0aaddf Mon Sep 17 00:00:00 2001 From: Scott Taylor Date: Thu, 27 Jun 2024 18:26:47 -0500 Subject: [PATCH] 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 | 3 ++ cli/src/commands/workspace.rs | 3 +- cli/tests/test_workspaces.rs | 64 ++++++++++++++++++++++++++++++++--- lib/src/repo.rs | 4 ++- lib/tests/test_mut_repo.rs | 51 ++++++++++++++++++++++++++++ lib/tests/test_view.rs | 4 +-- 6 files changed, 121 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2307f6bb91..8925794bd5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 222e6fa690..7add4413bc 100644 --- a/cli/src/commands/workspace.rs +++ b/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 { diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index 80f1398647..5f78ad4960 100644 --- a/cli/tests/test_workspaces.rs +++ b/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() { diff --git a/lib/src/repo.rs b/lib/src/repo.rs index a012420850..60165ffe4a 100644 --- a/lib/src/repo.rs +++ b/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( diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index b5f4c33ea8..baeb62bd8e 100644 --- a/lib/tests/test_mut_repo.rs +++ b/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())); +} diff --git a/lib/tests/test_view.rs b/lib/tests/test_view.rs index 91787b567a..029473cc41 100644 --- a/lib/tests/test_view.rs +++ b/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();