From e1416981aa66f5414b93af9c3f711881e9e0686a Mon Sep 17 00:00:00 2001 From: dploch Date: Tue, 12 Nov 2024 11:27:39 -0500 Subject: [PATCH] cli_util: consolidate update_stale command fully into cli_util --- cli/src/cli_util.rs | 106 +++++++++++---------- cli/src/commands/workspace/update_stale.rs | 39 +------- cli/tests/test_workspaces.rs | 14 +-- 3 files changed, 64 insertions(+), 95 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 765de113f3..d5e0832f3e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -283,11 +283,6 @@ struct CommandHelperData { working_copy_factories: WorkingCopyFactories, } -pub enum StaleWorkingCopy { - Recovered(WorkspaceCommandHelper), - Snapshotted((Arc, 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 { + ) -> Result { 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, @@ -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, diff --git a/cli/src/commands/workspace/update_stale.rs b/cli/src/commands/workspace/update_stale.rs index 6db2b24466..e7a4a25730 100644 --- a/cli/src/commands/workspace/update_stale.rs +++ b/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(()) } diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index 706bd4b590..5c49fce68e 100644 --- a/cli/tests/test_workspaces.rs +++ b/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###"