From eb332b1d08422fa64d39b8afdb62ba6e5e53843f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 22 Jul 2024 23:18:06 +0900 Subject: [PATCH] cli: make for_loaded_repo() callers specify whether repo/workspace are synced It's wrong to deduce loaded_at_head from command-line arguments if the repo was loaded at an arbitrary operation. Instead, the caller should specify whether the working copy state is synchronized with the repo view. I think .for_loaded_repo(ui, workspace, repo, true) would be bad for readability, so I added separate functions. I'm not happy with the name .for_temporary_repo(), but it seems okay for the current call sites. --- cli/src/cli_util.rs | 25 +++++++++++++++++++++---- cli/src/commands/git/clone.rs | 2 +- cli/src/commands/git/init.rs | 4 ++-- cli/src/commands/operation/diff.rs | 2 +- cli/src/commands/operation/show.rs | 3 ++- cli/src/commands/workspace.rs | 6 ++---- 6 files changed, 29 insertions(+), 13 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 8c00c4720a..0b2b27fe8e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -300,7 +300,8 @@ impl CommandHelper { let workspace = self.load_workspace()?; let op_head = self.resolve_operation(ui, workspace.repo_loader())?; let repo = workspace.repo_loader().load_at(&op_head)?; - self.for_loaded_repo(ui, workspace, repo) + let loaded_at_head = self.global_args.at_operation == "@"; + WorkspaceCommandHelper::new(ui, self, workspace, repo, loaded_at_head) } pub fn get_working_copy_factory(&self) -> Result<&dyn WorkingCopyFactory, CommandError> { @@ -371,14 +372,30 @@ impl CommandHelper { } } + /// Creates helper for the repo whose view is supposed to be in sync with + /// the working copy. If `--ignore-working-copy` is not specified, the + /// returned helper will attempt to update the working copy. #[instrument(skip_all)] - pub fn for_loaded_repo( + pub fn for_workable_repo( &self, ui: &mut Ui, workspace: Workspace, repo: Arc, ) -> Result { - let loaded_at_head = self.global_args.at_operation == "@"; + let loaded_at_head = true; + WorkspaceCommandHelper::new(ui, self, workspace, repo, loaded_at_head) + } + + /// Creates helper for the repo whose view might be out of sync with + /// the working copy. Therefore, the working copy should not be updated. + #[instrument(skip_all)] + pub fn for_temporary_repo( + &self, + ui: &mut Ui, + workspace: Workspace, + repo: Arc, + ) -> Result { + let loaded_at_head = false; WorkspaceCommandHelper::new(ui, self, workspace, repo, loaded_at_head) } } @@ -495,7 +512,7 @@ pub struct WorkspaceCommandHelper { impl WorkspaceCommandHelper { #[instrument(skip_all)] - pub fn new( + fn new( ui: &mut Ui, command: &CommandHelper, workspace: Workspace, diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 469a5172d1..b7cfdf8101 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -195,7 +195,7 @@ fn do_git_clone( r#"Fetching into new repo in "{}""#, wc_path.display() )?; - let mut workspace_command = command.for_loaded_repo(ui, workspace, repo)?; + let mut workspace_command = command.for_workable_repo(ui, workspace, repo)?; maybe_add_gitignore(&workspace_command)?; git_repo.remote(remote_name, source).unwrap(); let mut fetch_tx = workspace_command.start_transaction(); diff --git a/cli/src/commands/git/init.rs b/cli/src/commands/git/init.rs index f6d9a2e79d..4b463c8dc2 100644 --- a/cli/src/commands/git/init.rs +++ b/cli/src/commands/git/init.rs @@ -149,7 +149,7 @@ pub fn do_init( GitInitMode::Colocate => { let (workspace, repo) = Workspace::init_colocated_git(command.settings(), workspace_root)?; - let workspace_command = command.for_loaded_repo(ui, workspace, repo)?; + let workspace_command = command.for_workable_repo(ui, workspace, repo)?; maybe_add_gitignore(&workspace_command)?; } GitInitMode::External(git_repo_path) => { @@ -159,7 +159,7 @@ pub fn do_init( // chronological order. let colocated = is_colocated_git_workspace(&workspace, &repo); let repo = init_git_refs(ui, command, repo, colocated)?; - let mut workspace_command = command.for_loaded_repo(ui, workspace, repo)?; + let mut workspace_command = command.for_workable_repo(ui, workspace, repo)?; maybe_add_gitignore(&workspace_command)?; workspace_command.maybe_snapshot(ui)?; maybe_set_repository_level_trunk_alias(ui, workspace_command.repo())?; diff --git a/cli/src/commands/operation/diff.rs b/cli/src/commands/operation/diff.rs index 61a1f309a9..bd0b5f2241 100644 --- a/cli/src/commands/operation/diff.rs +++ b/cli/src/commands/operation/diff.rs @@ -91,7 +91,7 @@ pub fn cmd_op_diff( // Create a new transaction starting from `to_repo`. let mut workspace_command = - command.for_loaded_repo(ui, command.load_workspace()?, to_repo.clone())?; + command.for_temporary_repo(ui, command.load_workspace()?, to_repo.clone())?; let mut tx = workspace_command.start_transaction(); // Merge index from `from_repo` to `to_repo`, so commits in `from_repo` are // accessible. diff --git a/cli/src/commands/operation/show.rs b/cli/src/commands/operation/show.rs index 1546d1b53b..e6f18e9f03 100644 --- a/cli/src/commands/operation/show.rs +++ b/cli/src/commands/operation/show.rs @@ -59,7 +59,8 @@ pub fn cmd_op_show( let parent_repo = repo_loader.load_at(&parent_op)?; let repo = repo_loader.load_at(&op)?; - let workspace_command = command.for_loaded_repo(ui, command.load_workspace()?, repo.clone())?; + let workspace_command = + command.for_temporary_repo(ui, command.load_workspace()?, repo.clone())?; let commit_summary_template = workspace_command.commit_summary_template(); let with_content_format = LogContentFormat::new(ui, command.settings())?; diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index bf8cdcc178..fde6de92eb 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -173,9 +173,7 @@ fn cmd_workspace_add( )?; // Copy sparse patterns from workspace where the command was run - let loaded_at_head = true; - let mut new_workspace_command = - WorkspaceCommandHelper::new(ui, command, new_workspace, repo, loaded_at_head)?; + let mut new_workspace_command = command.for_workable_repo(ui, new_workspace, repo)?; let (mut locked_ws, _wc_commit) = new_workspace_command.start_working_copy_mutation()?; let sparse_patterns = old_workspace_command .working_copy() @@ -374,7 +372,7 @@ fn for_stale_working_copy( Err(e) => return Err(e.into()), } }; - Ok((command.for_loaded_repo(ui, workspace, repo)?, recovered)) + Ok((command.for_workable_repo(ui, workspace, repo)?, recovered)) } #[instrument(skip_all)]