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.
This commit is contained in:
Yuya Nishihara 2023-01-15 17:02:37 +09:00
parent 8d27bb0f27
commit 3e50844afa
2 changed files with 29 additions and 53 deletions

View file

@ -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> { pub fn check_rewriteable(&self, commit: &Commit) -> Result<(), CommandError> {
if commit.id() == self.repo.store().root_commit_id() { if commit.id() == self.repo.store().root_commit_id() {
return Err(user_error("Cannot rewrite the root commit")); return Err(user_error("Cannot rewrite the root commit"));

View file

@ -1495,26 +1495,13 @@ fn cmd_status(
let mut formatter = ui.stdout_formatter(); let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut(); let formatter = formatter.as_mut();
if let Some(wc_commit) = &maybe_checkout { if let Some(wc_commit) = &maybe_checkout {
let workspace_id = workspace_command.workspace_id();
for parent in wc_commit.parents() { for parent in wc_commit.parents() {
formatter.write_str("Parent commit: ")?; formatter.write_str("Parent commit: ")?;
write_commit_summary( workspace_command.write_commit_summary(formatter, &parent)?;
formatter,
repo.as_repo_ref(),
&workspace_id,
&parent,
command.settings(),
)?;
formatter.write_str("\n")?; formatter.write_str("\n")?;
} }
formatter.write_str("Working copy : ")?; formatter.write_str("Working copy : ")?;
write_commit_summary( workspace_command.write_commit_summary(formatter, wc_commit)?;
formatter,
repo.as_repo_ref(),
&workspace_id,
wc_commit,
command.settings(),
)?;
formatter.write_str("\n")?; formatter.write_str("\n")?;
} else { } else {
formatter.write_str("No working copy\n")?; formatter.write_str("No working copy\n")?;
@ -3257,25 +3244,18 @@ fn cmd_branch(
fn list_branches( fn list_branches(
ui: &mut Ui, ui: &mut Ui,
command: &CommandHelper, _command: &CommandHelper,
workspace_command: &WorkspaceCommandHelper, workspace_command: &WorkspaceCommandHelper,
) -> Result<(), CommandError> { ) -> Result<(), CommandError> {
let repo = workspace_command.repo(); let repo = workspace_command.repo();
let workspace_id = workspace_command.workspace_id();
let print_branch_target = let print_branch_target =
|formatter: &mut dyn Formatter, target: Option<&RefTarget>| -> Result<(), CommandError> { |formatter: &mut dyn Formatter, target: Option<&RefTarget>| -> Result<(), CommandError> {
match target { match target {
Some(RefTarget::Normal(id)) => { Some(RefTarget::Normal(id)) => {
write!(formatter, ": ")?; write!(formatter, ": ")?;
let commit = repo.store().get_commit(id)?; let commit = repo.store().get_commit(id)?;
write_commit_summary( workspace_command.write_commit_summary(formatter, &commit)?;
formatter,
repo.as_repo_ref(),
&workspace_id,
&commit,
command.settings(),
)?;
writeln!(formatter)?; writeln!(formatter)?;
} }
Some(RefTarget::Conflict { adds, removes }) => { Some(RefTarget::Conflict { adds, removes }) => {
@ -3285,25 +3265,13 @@ fn list_branches(
for id in removes { for id in removes {
let commit = repo.store().get_commit(id)?; let commit = repo.store().get_commit(id)?;
write!(formatter, " - ")?; write!(formatter, " - ")?;
write_commit_summary( workspace_command.write_commit_summary(formatter, &commit)?;
formatter,
repo.as_repo_ref(),
&workspace_id,
&commit,
command.settings(),
)?;
writeln!(formatter)?; writeln!(formatter)?;
} }
for id in adds { for id in adds {
let commit = repo.store().get_commit(id)?; let commit = repo.store().get_commit(id)?;
write!(formatter, " + ")?; write!(formatter, " + ")?;
write_commit_summary( workspace_command.write_commit_summary(formatter, &commit)?;
formatter,
repo.as_repo_ref(),
&workspace_id,
&commit,
command.settings(),
)?;
writeln!(formatter)?; writeln!(formatter)?;
} }
} }
@ -3734,13 +3702,7 @@ fn cmd_workspace_list(
for (workspace_id, checkout_id) in repo.view().wc_commit_ids().iter().sorted() { for (workspace_id, checkout_id) in repo.view().wc_commit_ids().iter().sorted() {
write!(ui, "{}: ", workspace_id.as_str())?; write!(ui, "{}: ", workspace_id.as_str())?;
let commit = repo.store().get_commit(checkout_id)?; let commit = repo.store().get_commit(checkout_id)?;
write_commit_summary( workspace_command.write_commit_summary(ui.stdout_formatter().as_mut(), &commit)?;
ui.stdout_formatter().as_mut(),
repo.as_repo_ref(),
&workspace_command.workspace_id(),
&commit,
command.settings(),
)?;
writeln!(ui)?; writeln!(ui)?;
} }
Ok(()) Ok(())
@ -3754,7 +3716,6 @@ fn cmd_workspace_update_stale(
let workspace = command.load_workspace()?; let workspace = command.load_workspace()?;
let mut workspace_command = command.resolve_operation(ui, workspace)?; let mut workspace_command = command.resolve_operation(ui, workspace)?;
let repo = workspace_command.repo().clone(); let repo = workspace_command.repo().clone();
let workspace_id = workspace_command.workspace_id();
let (mut locked_wc, desired_wc_commit) = let (mut locked_wc, desired_wc_commit) =
workspace_command.unsafe_start_working_copy_mutation()?; workspace_command.unsafe_start_working_copy_mutation()?;
match check_stale_working_copy(&locked_wc, &desired_wc_commit, repo.clone()) { 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()); locked_wc.finish(repo.op_id().clone());
ui.write("Working copy now at: ")?; ui.write("Working copy now at: ")?;
write_commit_summary( workspace_command
ui.stdout_formatter().as_mut(), .write_commit_summary(ui.stdout_formatter().as_mut(), &desired_wc_commit)?;
repo.as_repo_ref(),
&workspace_id,
&desired_wc_commit,
command.settings(),
)?;
ui.write("\n")?; ui.write("\n")?;
print_checkout_stats(ui, stats)?; print_checkout_stats(ui, stats)?;
} }