cli: make for_loaded_repo() callers specify whether repo/workspace ar…

…e 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

@@ -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<ReadonlyRepo>,
     ) -> Result<WorkspaceCommandHelper, CommandError> {
-        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<ReadonlyRepo>,
+    ) -> Result<WorkspaceCommandHelper, CommandError> {
+        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,

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();

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())?;

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.

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())?;

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)]