cli_util: consolidate update_stale command fully into cli_util

cli/src/cli_util.rs

@@ -283,11 +283,6 @@ struct CommandHelperData {
     working_copy_factories: WorkingCopyFactories,
 }
 
-pub enum StaleWorkingCopy {
-    Recovered(WorkspaceCommandHelper),
-    Snapshotted((Arc<ReadonlyRepo>, Commit)),
-}
-
 impl CommandHelper {
     pub fn app(&self) -> &Command {
         &self.data.app
@@ -389,29 +384,7 @@ impl CommandHelper {
                 // auto-update-stale, so let's do that now. We need to do it up here, not at a
                 // lower level (e.g. inside snapshot_working_copy()) to avoid recursive locking
                 // of the working copy.
-                match self.load_stale_working_copy_commit(ui)? {
-                    StaleWorkingCopy::Recovered(workspace_command) => workspace_command,
-                    StaleWorkingCopy::Snapshotted((repo, stale_commit)) => {
-                        let mut workspace_command = self.workspace_helper_no_snapshot(ui)?;
-                        let (locked_ws, new_commit) =
-                            workspace_command.unchecked_start_working_copy_mutation()?;
-                        let stats = update_stale_working_copy(
-                            locked_ws,
-                            repo.op_id().clone(),
-                            &stale_commit,
-                            &new_commit,
-                        )?;
-                        writeln!(
-                            ui.warning_default(),
-                            "Automatically updated to fresh commit {}",
-                            short_commit_hash(stale_commit.id())
-                        )?;
-                        workspace_command.write_stale_commit_stats(ui, &new_commit, stats)?;
-
-                        workspace_command.user_repo = ReadonlyUserRepo::new(repo);
-                        workspace_command
-                    }
-                }
+                self.recover_stale_working_copy(ui)?
             }
         };
 
@@ -460,10 +433,10 @@ impl CommandHelper {
             })
     }
 
-    pub fn load_stale_working_copy_commit(
+    pub fn recover_stale_working_copy(
         &self,
         ui: &Ui,
-    ) -> Result<StaleWorkingCopy, CommandError> {
+    ) -> Result<WorkspaceCommandHelper, CommandError> {
         let workspace = self.load_workspace()?;
         let op_id = workspace.working_copy().operation_id();
 
@@ -471,12 +444,61 @@ impl CommandHelper {
             Ok(op) => {
                 let repo = workspace.repo_loader().load_at(&op)?;
                 let mut workspace_command = self.for_workable_repo(ui, workspace, repo)?;
+
+                // Snapshot the current working copy on top of the last known working-copy
+                // operation, then merge the divergent 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.
                 workspace_command.maybe_snapshot(ui)?;
 
                 let wc_commit_id = workspace_command.get_wc_commit_id().unwrap();
                 let repo = workspace_command.repo().clone();
-                let wc_commit = repo.store().get_commit(wc_commit_id)?;
-                Ok(StaleWorkingCopy::Snapshotted((repo, wc_commit)))
+                let stale_wc_commit = repo.store().get_commit(wc_commit_id)?;
+
+                let mut workspace_command = self.workspace_helper_no_snapshot(ui)?;
+
+                let repo = workspace_command.repo().clone();
+                let (mut locked_ws, desired_wc_commit) =
+                    workspace_command.unchecked_start_working_copy_mutation()?;
+                match WorkingCopyFreshness::check_stale(
+                    locked_ws.locked_wc(),
+                    &desired_wc_commit,
+                    &repo,
+                )? {
+                    WorkingCopyFreshness::Fresh | WorkingCopyFreshness::Updated(_) => {
+                        writeln!(
+                            ui.status(),
+                            "Attempted recovery, but the working copy is not stale"
+                        )?;
+                    }
+                    WorkingCopyFreshness::WorkingCopyStale
+                    | WorkingCopyFreshness::SiblingOperation => {
+                        let stats = update_stale_working_copy(
+                            locked_ws,
+                            repo.op_id().clone(),
+                            &stale_wc_commit,
+                            &desired_wc_commit,
+                        )?;
+
+                        // TODO: Share this code with new/checkout somehow.
+                        if let Some(mut formatter) = ui.status_formatter() {
+                            write!(formatter, "Working copy now at: ")?;
+                            formatter.with_label("working_copy", |fmt| {
+                                workspace_command.write_commit_summary(fmt, &desired_wc_commit)
+                            })?;
+                            writeln!(formatter)?;
+                        }
+                        print_checkout_stats(ui, stats, &desired_wc_commit)?;
+
+                        writeln!(
+                            ui.status(),
+                            "Updated working copy to fresh commit {}",
+                            short_commit_hash(desired_wc_commit.id())
+                        )?;
+                    }
+                };
+
+                Ok(workspace_command)
             }
             Err(e @ OpStoreError::ObjectNotFound { .. }) => {
                 writeln!(
@@ -487,7 +509,7 @@ impl CommandHelper {
 
                 let mut workspace_command = self.workspace_helper_no_snapshot(ui)?;
                 workspace_command.create_and_check_out_recovery_commit(ui)?;
-                Ok(StaleWorkingCopy::Recovered(workspace_command))
+                Ok(workspace_command)
             }
             Err(e) => Err(e.into()),
         }
@@ -1643,22 +1665,6 @@ to the current parents may contain changes from multiple commits.
         self.commit_summary_template().format(commit, formatter)
     }
 
-    pub fn write_stale_commit_stats(
-        &self,
-        ui: &Ui,
-        commit: &Commit,
-        stats: CheckoutStats,
-    ) -> std::io::Result<()> {
-        if let Some(mut formatter) = ui.status_formatter() {
-            write!(formatter, "Working copy now at: ")?;
-            formatter.with_label("working_copy", |fmt| self.write_commit_summary(fmt, commit))?;
-            writeln!(formatter)?;
-        }
-        print_checkout_stats(ui, stats, commit)?;
-
-        Ok(())
-    }
-
     pub fn check_rewritable<'a>(
         &self,
         commits: impl IntoIterator<Item = &'a CommitId>,
@@ -2404,7 +2410,7 @@ pub fn start_repo_transaction(
     tx
 }
 
-pub fn update_stale_working_copy(
+fn update_stale_working_copy(
     mut locked_ws: LockedWorkspace,
     op_id: OperationId,
     stale_commit: &Commit,

cli/src/commands/workspace/update_stale.rs

@@ -12,12 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use jj_lib::working_copy::WorkingCopyFreshness;
 use tracing::instrument;
 
-use crate::cli_util::update_stale_working_copy;
 use crate::cli_util::CommandHelper;
-use crate::cli_util::StaleWorkingCopy;
 use crate::command_error::CommandError;
 use crate::ui::Ui;
 
@@ -34,41 +31,7 @@ pub 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 divergent 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 = match command.load_stale_working_copy_commit(ui)? {
-        StaleWorkingCopy::Recovered(_) => {
-            // We have already recovered from the situation that prompted the user to run
-            // this command, and it is known that the workspace is not stale
-            // (since we just updated it), so we can return early.
-            return Ok(());
-        }
-        StaleWorkingCopy::Snapshotted((_repo, commit)) => commit,
-    };
-    let mut workspace_command = command.workspace_helper_no_snapshot(ui)?;
+    command.recover_stale_working_copy(ui)?;
 
-    let repo = workspace_command.repo().clone();
-    let (mut locked_ws, desired_wc_commit) =
-        workspace_command.unchecked_start_working_copy_mutation()?;
-    match WorkingCopyFreshness::check_stale(locked_ws.locked_wc(), &desired_wc_commit, &repo)? {
-        WorkingCopyFreshness::Fresh | WorkingCopyFreshness::Updated(_) => {
-            writeln!(
-                ui.status(),
-                "Nothing to do (the working copy is not stale)."
-            )?;
-        }
-        WorkingCopyFreshness::WorkingCopyStale | WorkingCopyFreshness::SiblingOperation => {
-            let stats = update_stale_working_copy(
-                locked_ws,
-                repo.op_id().clone(),
-                &known_wc_commit,
-                &desired_wc_commit,
-            )?;
-
-            workspace_command.write_stale_commit_stats(ui, &desired_wc_commit, stats)?;
-        }
-    }
     Ok(())
 }

cli/tests/test_workspaces.rs

@@ -523,6 +523,7 @@ fn test_workspaces_conflicting_edits() {
     Rebased 1 descendant commits onto commits rewritten by other operation
     Working copy now at: pmmvwywv?? e82cd4ee (empty) (no description set)
     Added 0 files, modified 1 files, removed 0 files
+    Updated working copy to fresh commit e82cd4ee8faa
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &secondary_path),
     @r###"
@@ -600,6 +601,7 @@ fn test_workspaces_updated_by_other() {
     insta::assert_snapshot!(stderr, @r###"
     Working copy now at: pmmvwywv e82cd4ee (empty) (no description set)
     Added 0 files, modified 1 files, removed 0 files
+    Updated working copy to fresh commit e82cd4ee8faa
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &secondary_path),
     @r###"
@@ -657,13 +659,13 @@ fn test_workspaces_updated_by_other_automatic() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["st"]);
     insta::assert_snapshot!(stdout, @r###"
     The working copy is clean
-    Working copy : pmmvwywv 3224de8a (empty) (no description set)
-    Parent commit: qpvuntsm 506f4ec3 (no description set)
+    Working copy : pmmvwywv e82cd4ee (empty) (no description set)
+    Parent commit: qpvuntsm d4124476 (no description set)
     "###);
     insta::assert_snapshot!(stderr, @r###"
-    Warning: Automatically updated to fresh commit 3224de8ae048
     Working copy now at: pmmvwywv e82cd4ee (empty) (no description set)
     Added 0 files, modified 1 files, removed 0 files
+    Updated working copy to fresh commit e82cd4ee8faa
     "###);
 
     insta::assert_snapshot!(get_log_output(&test_env, &secondary_path),
@@ -848,9 +850,7 @@ fn test_workspaces_update_stale_noop() {
 
     let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["workspace", "update-stale"]);
     insta::assert_snapshot!(stdout, @"");
-    insta::assert_snapshot!(stderr, @r###"
-    Nothing to do (the working copy is not stale).
-    "###);
+    insta::assert_snapshot!(stderr, @"Attempted recovery, but the working copy is not stale");
 
     let stderr = test_env.jj_cmd_failure(
         &main_path,
@@ -890,7 +890,7 @@ fn test_workspaces_update_stale_snapshot() {
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
     Concurrent modification detected, resolving automatically.
-    Nothing to do (the working copy is not stale).
+    Attempted recovery, but the working copy is not stale
     "###);
 
     insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###"