From 3e50844afa970ebd902334d00f4af0df2fae4849 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 15 Jan 2023 17:02:37 +0900 Subject: [PATCH] cli: proxy write_commit_summary() of immutable context through workspace helper I'm going to add 'format_commit_summary(commit) -> String' to replace short_commit_description(), and it's fundamentally the same as 'write_commit_summary()' except where the output will go. The problem of adding such method to WorkspaceCommandHelper is that it's super easy to use wrong repo ref while transaction is in progress. This problem could be avoided by passing repo ref explicitly, but other methods like resolve_revset() have the same problem. One idea to prevent such API misuse is to exclusively borrow the helper: 'start_transaction(&'a mut self) -> Wrapper<'a>'. That's doable, but I'm not certain that it is the right way to go. Anyway, this isn't the problem only applies to write_commit_summary(), so I decided to add a convenient wrapper to WorkspaceCommandHelper. --- src/cli_util.rs | 20 ++++++++++++++++ src/commands.rs | 62 +++++++------------------------------------------ 2 files changed, 29 insertions(+), 53 deletions(-) diff --git a/src/cli_util.rs b/src/cli_util.rs index ae55be670..c0a3c368a 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -726,6 +726,26 @@ impl WorkspaceCommandHelper { } } + // TODO: Any methods that depend on self.repo aren't aware of a mutable repo + // while transaction is in progress. That's fine if the caller expects to + // operate on tx.base_repo(), but WorkspaceCommandHelper API doesn't clarify + // that. + + /// Writes one-line summary of the given `commit`. + pub fn write_commit_summary( + &self, + formatter: &mut dyn Formatter, + commit: &Commit, + ) -> std::io::Result<()> { + write_commit_summary( + formatter, + self.repo.as_repo_ref(), + self.workspace.workspace_id(), + commit, + &self.settings, + ) + } + pub fn check_rewriteable(&self, commit: &Commit) -> Result<(), CommandError> { if commit.id() == self.repo.store().root_commit_id() { return Err(user_error("Cannot rewrite the root commit")); diff --git a/src/commands.rs b/src/commands.rs index 0ca18fa9c..34921571c 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1495,26 +1495,13 @@ fn cmd_status( let mut formatter = ui.stdout_formatter(); let formatter = formatter.as_mut(); if let Some(wc_commit) = &maybe_checkout { - let workspace_id = workspace_command.workspace_id(); for parent in wc_commit.parents() { formatter.write_str("Parent commit: ")?; - write_commit_summary( - formatter, - repo.as_repo_ref(), - &workspace_id, - &parent, - command.settings(), - )?; + workspace_command.write_commit_summary(formatter, &parent)?; formatter.write_str("\n")?; } formatter.write_str("Working copy : ")?; - write_commit_summary( - formatter, - repo.as_repo_ref(), - &workspace_id, - wc_commit, - command.settings(), - )?; + workspace_command.write_commit_summary(formatter, wc_commit)?; formatter.write_str("\n")?; } else { formatter.write_str("No working copy\n")?; @@ -3257,25 +3244,18 @@ fn cmd_branch( fn list_branches( ui: &mut Ui, - command: &CommandHelper, + _command: &CommandHelper, workspace_command: &WorkspaceCommandHelper, ) -> Result<(), CommandError> { let repo = workspace_command.repo(); - let workspace_id = workspace_command.workspace_id(); let print_branch_target = |formatter: &mut dyn Formatter, target: Option<&RefTarget>| -> Result<(), CommandError> { match target { Some(RefTarget::Normal(id)) => { write!(formatter, ": ")?; let commit = repo.store().get_commit(id)?; - write_commit_summary( - formatter, - repo.as_repo_ref(), - &workspace_id, - &commit, - command.settings(), - )?; + workspace_command.write_commit_summary(formatter, &commit)?; writeln!(formatter)?; } Some(RefTarget::Conflict { adds, removes }) => { @@ -3285,25 +3265,13 @@ fn list_branches( for id in removes { let commit = repo.store().get_commit(id)?; write!(formatter, " - ")?; - write_commit_summary( - formatter, - repo.as_repo_ref(), - &workspace_id, - &commit, - command.settings(), - )?; + workspace_command.write_commit_summary(formatter, &commit)?; writeln!(formatter)?; } for id in adds { let commit = repo.store().get_commit(id)?; write!(formatter, " + ")?; - write_commit_summary( - formatter, - repo.as_repo_ref(), - &workspace_id, - &commit, - command.settings(), - )?; + workspace_command.write_commit_summary(formatter, &commit)?; writeln!(formatter)?; } } @@ -3734,13 +3702,7 @@ fn cmd_workspace_list( for (workspace_id, checkout_id) in repo.view().wc_commit_ids().iter().sorted() { write!(ui, "{}: ", workspace_id.as_str())?; let commit = repo.store().get_commit(checkout_id)?; - write_commit_summary( - ui.stdout_formatter().as_mut(), - repo.as_repo_ref(), - &workspace_command.workspace_id(), - &commit, - command.settings(), - )?; + workspace_command.write_commit_summary(ui.stdout_formatter().as_mut(), &commit)?; writeln!(ui)?; } Ok(()) @@ -3754,7 +3716,6 @@ fn cmd_workspace_update_stale( let workspace = command.load_workspace()?; let mut workspace_command = command.resolve_operation(ui, workspace)?; let repo = workspace_command.repo().clone(); - let workspace_id = workspace_command.workspace_id(); 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()) { @@ -3775,13 +3736,8 @@ fn cmd_workspace_update_stale( })?; locked_wc.finish(repo.op_id().clone()); ui.write("Working copy now at: ")?; - write_commit_summary( - ui.stdout_formatter().as_mut(), - repo.as_repo_ref(), - &workspace_id, - &desired_wc_commit, - command.settings(), - )?; + workspace_command + .write_commit_summary(ui.stdout_formatter().as_mut(), &desired_wc_commit)?; ui.write("\n")?; print_checkout_stats(ui, stats)?; }