From e38e2904bb08b5ec201904d17f2ee1f3fe52627b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 31 Mar 2024 09:08:39 -0700 Subject: [PATCH] ui: make `status()` return a `dyn Write`, add `status_formatter()` When the caller needs a formatter, it's because they're doing something non-trivial. When the user passed `--quiet` (see upcoming patch), we should ideally skip doing related work for print the formatting output. It helps if the `Ui` object doesn't even return a `Formatter` then, so the caller is forced to handle the quiet case differently. Thanks to Yuya for the suggestion. --- cli/src/cli_util.rs | 33 +++++++++++++++------------ cli/src/commands/abandon.rs | 43 ++++++++++++++++++----------------- cli/src/commands/diffedit.rs | 12 ++++++---- cli/src/commands/duplicate.rs | 10 ++++---- cli/src/commands/new.rs | 8 ++++--- cli/src/commands/rebase.rs | 20 +++++++++------- cli/src/commands/resolve.rs | 19 +++++++++------- cli/src/commands/restore.rs | 12 ++++++---- cli/src/commands/split.rs | 16 +++++++------ cli/src/commands/workspace.rs | 12 ++++++---- cli/src/git_util.rs | 6 +++-- cli/src/ui.rs | 18 ++++++++++----- 12 files changed, 120 insertions(+), 89 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 251140c86..c1f354106 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1147,16 +1147,17 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin new_commit, )?; if Some(new_commit) != maybe_old_commit { - let mut formatter = ui.status(); - let template = self.commit_summary_template(); - write!(formatter, "Working copy now at: ")?; - formatter.with_label("working_copy", |fmt| template.format(new_commit, fmt))?; - writeln!(formatter)?; - for parent in new_commit.parents() { - // "Working copy now at: " - write!(formatter, "Parent commit : ")?; - template.format(&parent, formatter.as_mut())?; + if let Some(mut formatter) = ui.status_formatter() { + let template = self.commit_summary_template(); + write!(formatter, "Working copy now at: ")?; + formatter.with_label("working_copy", |fmt| template.format(new_commit, fmt))?; writeln!(formatter)?; + for parent in new_commit.parents() { + // "Working copy now at: " + write!(formatter, "Parent commit : ")?; + template.format(&parent, formatter.as_mut())?; + writeln!(formatter)?; + } } } if let Some(stats) = stats { @@ -1236,6 +1237,9 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin ui: &mut Ui, old_repo: &Arc, ) -> Result<(), CommandError> { + let Some(mut fmt) = ui.status_formatter() else { + return Ok(()); + }; let old_view = old_repo.view(); let new_repo = self.repo().as_ref(); let new_view = new_repo.view(); @@ -1279,7 +1283,6 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin .retain(|change_id, _commits| !removed_conflicts_by_change_id.contains_key(change_id)); // TODO: Also report new divergence and maybe resolved divergence - let mut fmt = ui.status(); let template = self.commit_summary_template(); if !resolved_conflicts_by_change_id.is_empty() { writeln!( @@ -1642,12 +1645,12 @@ pub fn print_trackable_remote_branches(ui: &Ui, view: &View) -> io::Result<()> { ui.hint_default(), "The following remote branches aren't associated with the existing local branches:" )?; - let mut formatter = ui.status(); - for full_name in &remote_branch_names { - write!(formatter, " ")?; - writeln!(formatter.labeled("branch"), "{full_name}")?; + if let Some(mut formatter) = ui.status_formatter() { + for full_name in &remote_branch_names { + write!(formatter, " ")?; + writeln!(formatter.labeled("branch"), "{full_name}")?; + } } - drop(formatter); writeln!( ui.hint_default(), "Run `jj branch track {names}` to keep local branches updated on future pulls.", diff --git a/cli/src/commands/abandon.rs b/cli/src/commands/abandon.rs index c9c1e718b..e46a7898e 100644 --- a/cli/src/commands/abandon.rs +++ b/cli/src/commands/abandon.rs @@ -66,28 +66,29 @@ pub(crate) fn cmd_abandon( } let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; - if to_abandon.len() == 1 { - write!(ui.status(), "Abandoned commit ")?; - tx.base_workspace_helper() - .write_commit_summary(ui.status().as_mut(), &to_abandon[0])?; - writeln!(ui.status())?; - } else if !args.summary { - let mut formatter = ui.status(); - let template = tx.base_workspace_helper().commit_summary_template(); - writeln!(formatter, "Abandoned the following commits:")?; - for commit in &to_abandon { - write!(formatter, " ")?; - template.format(commit, formatter.as_mut())?; - writeln!(formatter)?; + if let Some(mut formatter) = ui.status_formatter() { + if to_abandon.len() == 1 { + write!(formatter, "Abandoned commit ")?; + tx.base_workspace_helper() + .write_commit_summary(formatter.as_mut(), &to_abandon[0])?; + writeln!(ui.status())?; + } else if !args.summary { + let template = tx.base_workspace_helper().commit_summary_template(); + writeln!(formatter, "Abandoned the following commits:")?; + for commit in &to_abandon { + write!(formatter, " ")?; + template.format(commit, formatter.as_mut())?; + writeln!(formatter)?; + } + } else { + writeln!(formatter, "Abandoned {} commits.", &to_abandon.len())?; + } + if num_rebased > 0 { + writeln!( + formatter, + "Rebased {num_rebased} descendant commits onto parents of abandoned commits" + )?; } - } else { - writeln!(ui.status(), "Abandoned {} commits.", &to_abandon.len())?; - } - if num_rebased > 0 { - writeln!( - ui.status(), - "Rebased {num_rebased} descendant commits onto parents of abandoned commits" - )?; } let transaction_description = if to_abandon.len() == 1 { format!("abandon commit {}", to_abandon[0].id().hex()) diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 1a76bfbd8..d97d6aed3 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -107,11 +107,13 @@ don't make any changes, then the operation will be aborted.", // rebase_descendants early; otherwise `new_commit` would always have // a conflicted change id at this point. let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; - write!(ui.status(), "Created ")?; - tx.write_commit_summary(ui.status().as_mut(), &new_commit)?; - writeln!(ui.status())?; - if num_rebased > 0 { - writeln!(ui.status(), "Rebased {num_rebased} descendant commits")?; + if let Some(mut formatter) = ui.status_formatter() { + write!(formatter, "Created ")?; + tx.write_commit_summary(formatter.as_mut(), &new_commit)?; + writeln!(formatter)?; + if num_rebased > 0 { + writeln!(formatter, "Rebased {num_rebased} descendant commits")?; + } } tx.finish(ui, format!("edit commit {}", target_commit.id().hex()))?; } diff --git a/cli/src/commands/duplicate.rs b/cli/src/commands/duplicate.rs index 248eff58e..0ebe30336 100644 --- a/cli/src/commands/duplicate.rs +++ b/cli/src/commands/duplicate.rs @@ -77,10 +77,12 @@ pub(crate) fn cmd_duplicate( duplicated_old_to_new.insert(original_commit_id, new_commit); } - for (old_id, new_commit) in &duplicated_old_to_new { - write!(ui.status(), "Duplicated {} as ", short_commit_hash(old_id))?; - tx.write_commit_summary(ui.status().as_mut(), new_commit)?; - writeln!(ui.status())?; + if let Some(mut formatter) = ui.status_formatter() { + for (old_id, new_commit) in &duplicated_old_to_new { + write!(formatter, "Duplicated {} as ", short_commit_hash(old_id))?; + tx.write_commit_summary(formatter.as_mut(), new_commit)?; + writeln!(formatter)?; + } } tx.finish(ui, format!("duplicate {} commit(s)", to_duplicate.len()))?; Ok(()) diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index 45700e04f..94b84a8ae 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -189,9 +189,11 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", } num_rebased += tx.mut_repo().rebase_descendants(command.settings())?; if args.no_edit { - write!(ui.status(), "Created new commit ")?; - tx.write_commit_summary(ui.status().as_mut(), &new_commit)?; - writeln!(ui.status())?; + if let Some(mut formatter) = ui.status_formatter() { + write!(formatter, "Created new commit ")?; + tx.write_commit_summary(formatter.as_mut(), &new_commit)?; + writeln!(formatter)?; + } } else { tx.edit(&new_commit).unwrap(); // The description of the new commit will be printed by tx.finish() diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index f9013720f..16613d282 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -33,6 +33,7 @@ use crate::cli_util::{ RevisionArg, WorkspaceCommandHelper, }; use crate::command_error::{user_error, CommandError}; +use crate::formatter::Formatter; use crate::ui::Ui; /// Move revisions to different parent(s) @@ -295,7 +296,9 @@ fn rebase_descendants( .iter() .partition::, _>(|commit| commit.parents() == new_parents); if !skipped_commits.is_empty() { - log_skipped_rebase_commits_message(ui, workspace_command, &skipped_commits)?; + if let Some(mut fmt) = ui.status_formatter() { + log_skipped_rebase_commits_message(fmt.as_mut(), workspace_command, &skipped_commits)?; + } } if old_commits.is_empty() { return Ok(()); @@ -447,9 +450,11 @@ fn rebase_revision( // longer have any children; they have all been rebased and the originals // have been abandoned. let skipped_commit_rebase = if old_commit.parents() == new_parents { - write!(ui.status(), "Skipping rebase of commit ")?; - tx.write_commit_summary(ui.status().as_mut(), &old_commit)?; - writeln!(ui.status())?; + if let Some(mut formatter) = ui.status_formatter() { + write!(formatter, "Skipping rebase of commit ")?; + tx.write_commit_summary(formatter.as_mut(), &old_commit)?; + writeln!(formatter)?; + } true } else { rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?; @@ -496,21 +501,20 @@ fn check_rebase_destinations( } fn log_skipped_rebase_commits_message( - ui: &Ui, + fmt: &mut dyn Formatter, workspace_command: &WorkspaceCommandHelper, commits: &[&Commit], ) -> Result<(), CommandError> { - let mut fmt = ui.status(); let template = workspace_command.commit_summary_template(); if commits.len() == 1 { write!(fmt, "Skipping rebase of commit ")?; - template.format(commits[0], fmt.as_mut())?; + template.format(commits[0], fmt)?; writeln!(fmt)?; } else { writeln!(fmt, "Skipping rebase of commits:")?; for commit in commits { write!(fmt, " ")?; - template.format(commit, fmt.as_mut())?; + template.format(commit, fmt)?; writeln!(fmt)?; } } diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index 3b9a9a0db..98ba6b1f7 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -115,15 +115,18 @@ pub(crate) fn cmd_resolve( format!("Resolve conflicts in commit {}", commit.id().hex()), )?; + // TODO: Delete local `--quiet` if !args.quiet { - let new_tree = new_commit.tree()?; - let new_conflicts = new_tree.conflicts().collect_vec(); - if !new_conflicts.is_empty() { - writeln!( - ui.status(), - "After this operation, some files at this revision still have conflicts:" - )?; - print_conflicted_paths(&new_conflicts, ui.status().as_mut(), &workspace_command)?; + if let Some(mut formatter) = ui.status_formatter() { + let new_tree = new_commit.tree()?; + let new_conflicts = new_tree.conflicts().collect_vec(); + if !new_conflicts.is_empty() { + writeln!( + formatter, + "After this operation, some files at this revision still have conflicts:" + )?; + print_conflicted_paths(&new_conflicts, formatter.as_mut(), &workspace_command)?; + } } }; Ok(()) diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index d486fac4c..b3a599096 100644 --- a/cli/src/commands/restore.rs +++ b/cli/src/commands/restore.rs @@ -112,11 +112,13 @@ pub(crate) fn cmd_restore( // rebase_descendants early; otherwise `new_commit` would always have // a conflicted change id at this point. let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; - write!(ui.status(), "Created ")?; - tx.write_commit_summary(ui.status().as_mut(), &new_commit)?; - writeln!(ui.status())?; - if num_rebased > 0 { - writeln!(ui.status(), "Rebased {num_rebased} descendant commits")?; + if let Some(mut formatter) = ui.status_formatter() { + write!(formatter, "Created ")?; + tx.write_commit_summary(formatter.as_mut(), &new_commit)?; + writeln!(formatter)?; + if num_rebased > 0 { + writeln!(formatter, "Rebased {num_rebased} descendant commits")?; + } } tx.finish(ui, format!("restore into commit {}", to_commit.id().hex()))?; } diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index fbb47300a..01465f5ac 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -149,14 +149,16 @@ don't make any changes, then the operation will be aborted. tx.mut_repo() .set_rewritten_commit(commit.id().clone(), second_commit.id().clone()); let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; - if num_rebased > 0 { - writeln!(ui.status(), "Rebased {num_rebased} descendant commits")?; + if let Some(mut formatter) = ui.status_formatter() { + if num_rebased > 0 { + writeln!(formatter, "Rebased {num_rebased} descendant commits")?; + } + write!(formatter, "First part: ")?; + tx.write_commit_summary(formatter.as_mut(), &first_commit)?; + write!(formatter, "\nSecond part: ")?; + tx.write_commit_summary(formatter.as_mut(), &second_commit)?; + writeln!(formatter)?; } - write!(ui.status(), "First part: ")?; - tx.write_commit_summary(ui.status().as_mut(), &first_commit)?; - write!(ui.status(), "\nSecond part: ")?; - tx.write_commit_summary(ui.status().as_mut(), &second_commit)?; - writeln!(ui.status())?; tx.finish(ui, format!("split commit {}", commit.id().hex()))?; Ok(()) } diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 81c67c63b..6c609b3de 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -424,11 +424,13 @@ fn cmd_workspace_update_stale( ) })?; locked_ws.finish(repo.op_id().clone())?; - write!(ui.status(), "Working copy now at: ")?; - ui.status().with_label("working_copy", |fmt| { - workspace_command.write_commit_summary(fmt, &desired_wc_commit) - })?; - writeln!(ui.status())?; + 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)?; } } diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs index bc74d5cb5..6642470ce 100644 --- a/cli/src/git_util.rs +++ b/cli/src/git_util.rs @@ -256,6 +256,9 @@ pub fn print_git_import_stats( stats: &GitImportStats, show_ref_stats: bool, ) -> Result<(), CommandError> { + let Some(mut formatter) = ui.status_formatter() else { + return Ok(()); + }; if show_ref_stats { let refs_stats = stats .changed_remote_refs @@ -274,7 +277,6 @@ pub fn print_git_import_stats( let max_width = refs_stats.iter().map(|x| x.ref_name.width()).max(); if let Some(max_width) = max_width { - let mut formatter = ui.status(); for status in refs_stats { status.output(max_width, has_both_ref_kinds, &mut *formatter)?; } @@ -283,7 +285,7 @@ pub fn print_git_import_stats( if !stats.abandoned_commits.is_empty() { writeln!( - ui.status(), + formatter, "Abandoned {} commits that are no longer reachable.", stats.abandoned_commits.len() )?; diff --git a/cli/src/ui.rs b/cli/src/ui.rs index 14fde6b94..e6e6a77ac 100644 --- a/cli/src/ui.rs +++ b/cli/src/ui.rs @@ -373,10 +373,16 @@ impl Ui { }) } - /// Writes a message that's a status update not part of the command's main - /// output. - pub fn status(&self) -> Box { - self.stderr_formatter() + /// Writer to print an update that's not part of the command's main output. + pub fn status(&self) -> Box { + Box::new(self.stderr()) + } + + /// A formatter to print an update that's not part of the command's main + /// output. Returns `None` if `--quiet` was requested. + // TODO: Actually support `--quiet` + pub fn status_formatter(&self) -> Option> { + Some(self.stderr_formatter()) } /// Writer to print hint with the default "Hint: " heading. @@ -388,7 +394,7 @@ impl Ui { /// Writer to print hint without the "Hint: " heading. pub fn hint_no_heading(&self) -> LabeledWriter, &'static str> { - LabeledWriter::new(self.status(), "hint") + LabeledWriter::new(self.stderr_formatter(), "hint") } /// Writer to print hint with the given heading. @@ -396,7 +402,7 @@ impl Ui { &self, heading: H, ) -> HeadingLabeledWriter, &'static str, H> { - HeadingLabeledWriter::new(self.status(), "hint", heading) + HeadingLabeledWriter::new(self.stderr_formatter(), "hint", heading) } /// Writer to print warning with the default "Warning: " heading.