cli: reload both repo and wc commit if working copy was updated

src/cli_util.rs

@@ -1023,22 +1023,37 @@ impl WorkspaceCommandHelper {
     }
 
     pub fn snapshot_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> {
-        let repo = self.repo().clone();
         let workspace_id = self.workspace_id().to_owned();
-        let wc_commit_id = match repo.view().get_wc_commit_id(&workspace_id) {
-            Some(wc_commit_id) => wc_commit_id.clone(),
-            None => {
-                // If the workspace has been deleted, it's unclear what to do, so we just skip
-                // committing the working copy.
-                return Ok(());
-            }
+        let get_wc_commit = |repo: &ReadonlyRepo| -> Result<Option<_>, _> {
+            repo.view()
+                .get_wc_commit_id(&workspace_id)
+                .map(|id| repo.store().get_commit(id))
+                .transpose()
+        };
+        let repo = self.repo().clone();
+        let wc_commit = if let Some(wc_commit) = get_wc_commit(&repo)? {
+            wc_commit
+        } else {
+            // If the workspace has been deleted, it's unclear what to do, so we just skip
+            // committing the working copy.
+            return Ok(());
         };
         let base_ignores = self.base_ignores();
+
+        // Compare working-copy tree and operation with repo's, and reload as needed.
         let mut locked_wc = self.workspace.working_copy_mut().start_mutation();
         let old_op_id = locked_wc.old_operation_id().clone();
-        let wc_commit = repo.store().get_commit(&wc_commit_id)?;
-        let repo = match check_stale_working_copy(&locked_wc, &wc_commit, repo.clone()) {
-            Ok(repo) => repo,
+        let (repo, wc_commit) = match check_stale_working_copy(&locked_wc, &wc_commit, &repo) {
+            Ok(None) => (repo, wc_commit),
+            Ok(Some(wc_operation)) => {
+                let repo = repo.reload_at(&wc_operation);
+                let wc_commit = if let Some(wc_commit) = get_wc_commit(&repo)? {
+                    wc_commit
+                } else {
+                    return Ok(()); // The workspace has been deleted (see above)
+                };
+                (repo, wc_commit)
+            }
             Err(StaleWorkingCopyError::WorkingCopyStale) => {
                 locked_wc.discard();
                 return Err(user_error_with_hint(
@@ -1430,12 +1445,13 @@ pub enum StaleWorkingCopyError {
 pub fn check_stale_working_copy(
     locked_wc: &LockedWorkingCopy,
     wc_commit: &Commit,
-    repo: Arc<ReadonlyRepo>,
-) -> Result<Arc<ReadonlyRepo>, StaleWorkingCopyError> {
+    repo: &ReadonlyRepo,
+) -> Result<Option<Operation>, StaleWorkingCopyError> {
     // Check if the working copy's tree matches the repo's view
     let wc_tree_id = locked_wc.old_tree_id().clone();
     if *wc_commit.tree_id() == wc_tree_id {
-        Ok(repo)
+        // The working copy isn't stale, and no need to reload the repo.
+        Ok(None)
     } else {
         let wc_operation_data = repo
             .op_store()
@@ -1455,9 +1471,9 @@ pub fn check_stale_working_copy(
         );
         if let Some(ancestor_op) = maybe_ancestor_op {
             if ancestor_op.id() == repo_operation.id() {
-                // The working copy was updated since we loaded the repo. We reload the repo
-                // at the working copy's operation.
-                Ok(repo.reload_at(&wc_operation))
+                // The working copy was updated since we loaded the repo. The repo must be
+                // reloaded at the working copy's operation.
+                Ok(Some(wc_operation))
             } else if ancestor_op.id() == wc_operation.id() {
                 // The working copy was not updated when some repo operation committed,
                 // meaning that it's stale compared to the repo view.

src/commands/mod.rs

@@ -3436,7 +3436,7 @@ fn cmd_workspace_update_stale(
     let repo = workspace_command.repo().clone();
     let (mut locked_wc, desired_wc_commit) =
         workspace_command.unsafe_start_working_copy_mutation()?;
-    match check_stale_working_copy(&locked_wc, &desired_wc_commit, repo.clone()) {
+    match check_stale_working_copy(&locked_wc, &desired_wc_commit, &repo) {
         Ok(_) => {
             locked_wc.discard();
             ui.write("Nothing to do (the working copy is not stale).\n")?;

tests/test_concurrent_operations.rs

@@ -14,6 +14,8 @@
 
 use std::path::Path;
 
+use itertools::Itertools as _;
+
 use crate::common::TestEnvironment;
 
 pub mod common;
@@ -137,6 +139,77 @@ fn test_concurrent_operations_wc_modified() {
     "###);
 }
 
+#[test]
+fn test_concurrent_snapshot_wc_reloadable() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+    let op_heads_dir = repo_path
+        .join(".jj")
+        .join("repo")
+        .join("op_heads")
+        .join("heads");
+
+    std::fs::write(repo_path.join("base"), "").unwrap();
+    test_env.jj_cmd_success(&repo_path, &["commit", "-m", "initial"]);
+
+    // Create new commit and checkout it.
+    std::fs::write(repo_path.join("child1"), "").unwrap();
+    test_env.jj_cmd_success(&repo_path, &["commit", "-m", "new child1"]);
+
+    let template = r#"id ++ "\n" ++ description ++ "\n" ++ tags"#;
+    let op_log_stdout = test_env.jj_cmd_success(&repo_path, &["op", "log", "-T", template]);
+    insta::assert_snapshot!(op_log_stdout, @r###"
+    @  bacc8f507ccede29c282bb1459b71ffd233a9e29f4ec11b027422923592c4ef949e4465fd101c99ee6cfd39af26f29cd5910ef3b16985538e50fd21523bcf3e1
+    │  commit 323b414dd255b51375d7f4392b7b2641ffe4289f
+    │  args: jj commit -m 'new child1'
+    ◉  6aa8b9099660e021813be72b4230692f782699448947cc76ffe5245d23108184dd60bb795fba350eb39abc2b0643169623cf59bb0c04130102388f8d87791070
+    │  snapshot working copy
+    │  args: jj commit -m 'new child1'
+    ◉  7a31786821de03db03d5b9b7ec97454d10b90eee33844a86ebd282124c8ab43babcf79091cd1c47796ed8cc6ff23febabb35a10e0aad1f96428f958eb834b30d
+    │  commit 3d918700494a9895696e955b85fa05eb0d314cc6
+    │  args: jj commit -m initial
+    ◉  36440b76f049c4999505fa1dd3b21bfdb1a1f93c64e04f0a3797e704145780df959afcd3babd59c536544c6e7a6b883594abe7b38e8c4be5fe6d66a3cd8f4f9d
+    │  snapshot working copy
+    │  args: jj commit -m initial
+    ◉  a99a3fd5c51e8f7ccb9ae2f9fb749612a23f0a7cf25d8c644f36c35c077449ce3c66f49d098a5a704ca5e47089a7f019563a5b8cbc7d451619e0f90c82241ceb
+    │  add workspace 'default'
+    ◉  56b94dfc38e7d54340377f566e96ab97dc6163ea7841daf49fb2e1d1ceb27e26274db1245835a1a421fb9d06e6e0fe1e4f4aa1b0258c6e86df676ad9111d0dab
+       initialize repo
+    "###);
+    let op_log_lines = op_log_stdout.lines().collect_vec();
+    let current_op_id = op_log_lines[0].split_once("  ").unwrap().1;
+    let previous_op_id = op_log_lines[6].split_once("  ").unwrap().1;
+
+    // Another process started from the "initial" operation, but snapshots after
+    // the "child1" checkout has been completed.
+    std::fs::rename(
+        op_heads_dir.join(current_op_id),
+        op_heads_dir.join(previous_op_id),
+    )
+    .unwrap();
+    std::fs::write(repo_path.join("child2"), "").unwrap();
+    let stdout = test_env.jj_cmd_success(&repo_path, &["describe", "-m", "new child2"]);
+    insta::assert_snapshot!(stdout, @r###"
+    Working copy now at: 4011424ea0a2 new child2
+    Parent commit      : e08863ee7a0d new child1
+    "###);
+
+    // Since the repo can be reloaded before snapshotting, "child2" should be
+    // a child of "child1", not of "initial".
+    let template = r#"commit_id ++ " " ++ description"#;
+    let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", template, "-s"]);
+    insta::assert_snapshot!(stdout, @r###"
+    @  4011424ea0a210a914f869ea3c47d76931598d1d new child2
+    │  A child2
+    ◉  e08863ee7a0df688755d3d3126498afdf4f580ad new child1
+    │  A child1
+    ◉  79989e62f8331e69a803058b57bacc264405cb65 initial
+    │  A base
+    ◉  0000000000000000000000000000000000000000
+    "###);
+}
+
 fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
     let template = r#"commit_id ++ " " ++ description"#;
     test_env.jj_cmd_success(cwd, &["log", "-T", template])