From bab13e19824e91e7f0e22acf3a7541f45b1b416d Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 26 Feb 2023 20:52:06 +0900 Subject: [PATCH] cli: snapshot stale working copy before updating Since there's no easy API to snapshot the stale working copy without releasing the lock, we have to compare the tree ids after reacquiring the lock. We could instead manually snapshot and rebase the working-copy commit, but that would require more copy-paste codes. Closes #1310 --- CHANGELOG.md | 3 +++ src/cli_util.rs | 16 +++++++++++++ src/commands/mod.rs | 17 ++++++++++++- tests/test_workspaces.rs | 52 +++++++++++++++++++++++++++++++++++----- 4 files changed, 81 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03aa854f4..bf80b4f64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the new name. If a new remote with the old name and containing the same branches was added, the remote branches may not be recreated in some cases. +* `jj workspace update-stale` now snapshots the working-copy changes before + updating to the new working-copy commit. + ## [0.7.0] - 2023-02-16 ### Breaking changes diff --git a/src/cli_util.rs b/src/cli_util.rs index 5ab59faf9..3ee6c50b1 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -459,6 +459,22 @@ impl CommandHelper { repo, ) } + + /// Loads workspace that will diverge from the last working-copy operation. + pub fn for_stale_working_copy( + &self, + ui: &mut Ui, + ) -> Result { + let workspace = self.load_workspace()?; + let op_store = workspace.repo_loader().op_store(); + let op_id = workspace.working_copy().operation_id(); + let op_data = op_store + .read_operation(op_id) + .map_err(|e| CommandError::InternalError(format!("Failed to read operation: {e}")))?; + let operation = Operation::new(op_store.clone(), op_id.clone(), op_data); + let repo = workspace.repo_loader().load_at(&operation); + self.for_loaded_repo(ui, workspace, repo) + } } // Provides utilities for writing a command that works on a workspace (like most diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 8bfedae19..7c23af544 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -3227,7 +3227,18 @@ fn cmd_workspace_update_stale( command: &CommandHelper, _args: &WorkspaceUpdateStaleArgs, ) -> Result<(), CommandError> { + // Snapshot the current working copy on top of the last known working-copy + // operation, then merge the concurrent operations. The wc_commit_id of the + // merged repo wouldn't change because the old one wins, but it's probably + // fine if we picked the new wc_commit_id. + let known_wc_commit = { + let mut workspace_command = command.for_stale_working_copy(ui)?; + workspace_command.snapshot(ui)?; + let wc_commit_id = workspace_command.get_wc_commit_id().unwrap(); + workspace_command.repo().store().get_commit(wc_commit_id)? + }; let mut workspace_command = command.workspace_helper_no_snapshot(ui)?; + let repo = workspace_command.repo().clone(); let (mut locked_wc, desired_wc_commit) = workspace_command.unsafe_start_working_copy_mutation()?; @@ -3237,7 +3248,11 @@ fn cmd_workspace_update_stale( ui.write("Nothing to do (the working copy is not stale).\n")?; } Err(_) => { - // TODO: First commit the working copy + // The same check as start_working_copy_mutation(), but with the stale + // working-copy commit. + if known_wc_commit.tree_id() != locked_wc.old_tree_id() { + return Err(user_error("Concurrent working copy operation. Try again.")); + } let stats = locked_wc .check_out(&desired_wc_commit.tree()) .map_err(|err| { diff --git a/tests/test_workspaces.rs b/tests/test_workspaces.rs index 795ffcce7..6446e8dc5 100644 --- a/tests/test_workspaces.rs +++ b/tests/test_workspaces.rs @@ -122,17 +122,21 @@ fn test_workspaces_conflicting_edits() { Hint: Run `jj workspace update-stale` to update it. "###); let stdout = test_env.jj_cmd_success(&secondary_path, &["workspace", "update-stale"]); - // It was detected that the working copy is now stale - // TODO: Since there was an uncommitted change in the working copy, it should + // It was detected that the working copy is now stale. + // Since there was an uncommitted change in the working copy, it should // have been committed first (causing divergence) insta::assert_snapshot!(stdout, @r###" + Concurrent modification detected, resolving automatically. + Rebased 1 descendant commits onto commits rewritten by other operation Working copy now at: a1896a17282f (no description set) Added 0 files, modified 1 files, removed 0 files "###); insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###" - o fe8f41ed01d693b2d4365cd89e42ad9c531a939b default@ - │ @ a1896a17282f19089a5cec44358d6609910e0513 secondary@ + o 8d90dc175c874af0dff032d611029dc722d4e108 (divergent) + │ o fe8f41ed01d693b2d4365cd89e42ad9c531a939b default@ + ├─╯ + │ @ a1896a17282f19089a5cec44358d6609910e0513 secondary@ (divergent) ├─╯ o c0d4a99ef98ada7da8dc73a778bbb747c4178385 o 0000000000000000000000000000000000000000 @@ -141,8 +145,10 @@ fn test_workspaces_conflicting_edits() { let stdout = get_log_output(&test_env, &secondary_path); assert!(!stdout.starts_with("The working copy is stale")); insta::assert_snapshot!(stdout, @r###" - o fe8f41ed01d693b2d4365cd89e42ad9c531a939b default@ - │ @ a1896a17282f19089a5cec44358d6609910e0513 secondary@ + o 8d90dc175c874af0dff032d611029dc722d4e108 (divergent) + │ o fe8f41ed01d693b2d4365cd89e42ad9c531a939b default@ + ├─╯ + │ @ a1896a17282f19089a5cec44358d6609910e0513 secondary@ (divergent) ├─╯ o c0d4a99ef98ada7da8dc73a778bbb747c4178385 o 0000000000000000000000000000000000000000 @@ -235,6 +241,40 @@ fn test_workspaces_update_stale_noop() { "###); } +/// Test "update-stale" in a dirty, but not stale working copy. +#[test] +fn test_workspaces_update_stale_snapshot() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "--git", "main"]); + let main_path = test_env.env_root().join("main"); + let secondary_path = test_env.env_root().join("secondary"); + + std::fs::write(main_path.join("file"), "changed in main\n").unwrap(); + test_env.jj_cmd_success(&main_path, &["new"]); + test_env.jj_cmd_success(&main_path, &["workspace", "add", "../secondary"]); + + // Record new operation in one workspace. + test_env.jj_cmd_success(&main_path, &["new"]); + + // Snapshot the other working copy, which unfortunately results in concurrent + // operations, but should be resolved cleanly. + std::fs::write(secondary_path.join("file"), "changed in second\n").unwrap(); + let stdout = test_env.jj_cmd_success(&secondary_path, &["workspace", "update-stale"]); + insta::assert_snapshot!(stdout, @r###" + Concurrent modification detected, resolving automatically. + Nothing to do (the working copy is not stale). + "###); + + insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###" + @ 4976dfa88529814c4dd8c06253fbd82d076b79f8 secondary@ + │ o 8357b22214ba8adb6d2d378fa5b85274f1c7967c default@ + │ o 1a769966ed69fa7abadbd2d899e2be1025cb04fb + ├─╯ + o b4a6c25e777817db67fdcbd50f1dd3b74b46b5f1 + o 0000000000000000000000000000000000000000 + "###); +} + /// Test forgetting workspaces #[test] fn test_workspaces_forget() {