ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

formatter: add a with_label() helper

Browse source 
There's a risk of forgetting to call `remove_label()` and I've wanted
to reduce that risk for a long time. I considered creating RAII
adapters that implement `Drop`, but I didn't like that that would
ignore errors (such as `BrokenPipe`) that can happen while emitting an
escape sequence in `remove_label()`. I would ideally have liked
Python's context managers here, but Rust doesn't have that. Instead,
we get to use closures. That works pretty well, except that we can't
return other errors than `io::Error` inside the closures. Even with
that limitation, we can use the new `with_label()` method in all but a
few cases.

We can't define the `with_label()` method directly in the trait
because `&mut self` is not a trait object, so we can't pass it on to
the closure (which expects a trait object). The solution is to use
`impl dyn Formatter` (thanks to @kupiakos for figuring that
out!). That unfortunately means that we can *only* call the function
on trait objects, so if `f` is a concrete formatter type
(e.g. `PlainTextFormatter`), then `f.with_label()` won't
compile. Since we only ever access the formatters as trait objects,
that's not really a problem, however.
This commit is contained in:
Martin von Zweigbergk 2022-10-12 11:53:37 -07:00 committed by Martin von Zweigbergk
parent a5f7e09cbb
commit a061ac022e
2 changed files with 201 additions and 198 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

196
src/commands.rs
View file

@ -1330,17 +1330,17 @@ fn show_color_words_diff_line(
diff_line: &DiffLine,
) -> io::Result<()> {
if diff_line.has_left_content {
formatter.add_label("removed")?;
formatter.write_bytes(format!("{:>4}", diff_line.left_line_number).as_bytes())?;
formatter.remove_label()?;
formatter.with_label("removed", |formatter| {
formatter.write_bytes(format!("{:>4}", diff_line.left_line_number).as_bytes())
})?;
formatter.write_bytes(b" ")?;
} else {
formatter.write_bytes(b" ")?;
}
if diff_line.has_right_content {
formatter.add_label("added")?;
formatter.write_bytes(format!("{:>4}", diff_line.right_line_number).as_bytes())?;
formatter.remove_label()?;
formatter.with_label("added", |formatter| {
formatter.write_bytes(format!("{:>4}", diff_line.right_line_number).as_bytes())
})?;
formatter.write_bytes(b": ")?;
} else {
formatter.write_bytes(b" : ")?;
@ -1354,14 +1354,10 @@ fn show_color_words_diff_line(
let before = data[0];
let after = data[1];
if !before.is_empty() {
formatter.add_label("removed")?;
formatter.write_bytes(before)?;
formatter.remove_label()?;
formatter.with_label("removed", |formatter| formatter.write_bytes(before))?;
}
if !after.is_empty() {
formatter.add_label("added")?;
formatter.write_bytes(after)?;
formatter.remove_label()?;
formatter.with_label("added", |formatter| formatter.write_bytes(after))?;
}
}
}
@ -1547,9 +1543,9 @@ fn show_color_words_diff(
tree::Diff::Added(right_value) => {
let right_content = diff_content(repo, &path, &right_value)?;
let description = basic_diff_file_type(&right_value);
formatter.add_label("header")?;
formatter.write_str(&format!("Added {} {}:\n", description, ui_path))?;
formatter.remove_label()?;
formatter.with_label("header", |formatter| {
formatter.write_str(&format!("Added {} {}:\n", description, ui_path))
})?;
show_color_words_diff_hunks(&[], &right_content, formatter)?;
}
tree::Diff::Modified(left_value, right_value) => {
@ -1596,17 +1592,17 @@ fn show_color_words_diff(
)
}
};
formatter.add_label("header")?;
formatter.write_str(&format!("{} {}:\n", description, ui_path))?;
formatter.remove_label()?;
formatter.with_label("header", |formatter| {
formatter.write_str(&format!("{} {}:\n", description, ui_path))
})?;
show_color_words_diff_hunks(&left_content, &right_content, formatter)?;
}
tree::Diff::Removed(left_value) => {
let left_content = diff_content(repo, &path, &left_value)?;
let description = basic_diff_file_type(&left_value);
formatter.add_label("header")?;
formatter.write_str(&format!("Removed {} {}:\n", description, ui_path))?;
formatter.remove_label()?;
formatter.with_label("header", |formatter| {
formatter.write_str(&format!("Removed {} {}:\n", description, ui_path))
})?;
show_color_words_diff_hunks(&left_content, &[], formatter)?;
}
}
@ -1771,7 +1767,7 @@ fn show_unified_diff_hunks(
right_content: &[u8],
) -> Result<(), CommandError> {
for hunk in unified_diff_hunks(left_content, right_content, 3) {
formatter.add_label("hunk_header")?;
formatter.with_label("hunk_header", |formatter| {
writeln!(
formatter,
"@@ -{},{} +{},{} @@",
@ -1779,27 +1775,27 @@ fn show_unified_diff_hunks(
hunk.left_line_range.len(),
hunk.right_line_range.start,
hunk.right_line_range.len()
)?;
formatter.remove_label()?;
)
})?;
for (line_type, content) in hunk.lines {
match line_type {
DiffLineType::Context => {
formatter.add_label("context")?;
formatter.with_label("context", |formatter| {
formatter.write_str(" ")?;
formatter.write_all(content)?;
formatter.remove_label()?;
formatter.write_all(content)
})?;
}
DiffLineType::Removed => {
formatter.add_label("removed")?;
formatter.with_label("removed", |formatter| {
formatter.write_str("-")?;
formatter.write_all(content)?;
formatter.remove_label()?;
formatter.write_all(content)
})?;
}
DiffLineType::Added => {
formatter.add_label("added")?;
formatter.with_label("added", |formatter| {
formatter.write_str("+")?;
formatter.write_all(content)?;
formatter.remove_label()?;
formatter.write_all(content)
})?;
}
}
if !content.ends_with(b"\n") {
@ -1819,21 +1815,23 @@ fn show_git_diff(
formatter.add_label("diff")?;
for (path, diff) in tree_diff {
let path_string = path.to_internal_file_string();
formatter.add_label("file_header")?;
writeln!(formatter, "diff --git a/{} b/{}", path_string, path_string)?;
match diff {
tree::Diff::Added(right_value) => {
let right_part = git_diff_part(repo, &path, &right_value)?;
formatter.with_label("file_header", |formatter| {
writeln!(formatter, "diff --git a/{} b/{}", path_string, path_string)?;
writeln!(formatter, "new file mode {}", &right_part.mode)?;
writeln!(formatter, "index 0000000000..{}", &right_part.hash)?;
writeln!(formatter, "--- /dev/null")?;
writeln!(formatter, "+++ b/{}", path_string)?;
formatter.remove_label()?;
writeln!(formatter, "+++ b/{}", path_string)
})?;
show_unified_diff_hunks(formatter, &[], &right_part.content)?;
}
tree::Diff::Modified(left_value, right_value) => {
let left_part = git_diff_part(repo, &path, &left_value)?;
let right_part = git_diff_part(repo, &path, &right_value)?;
formatter.with_label("file_header", |formatter| {
writeln!(formatter, "diff --git a/{} b/{}", path_string, path_string)?;
if left_part.mode != right_part.mode {
writeln!(formatter, "old mode {}", &left_part.mode)?;
writeln!(formatter, "new mode {}", &right_part.mode)?;
@ -1851,16 +1849,19 @@ fn show_git_diff(
writeln!(formatter, "--- a/{}", path_string)?;
writeln!(formatter, "+++ b/{}", path_string)?;
}
formatter.remove_label()?;
Ok(())
})?;
show_unified_diff_hunks(formatter, &left_part.content, &right_part.content)?;
}
tree::Diff::Removed(left_value) => {
let left_part = git_diff_part(repo, &path, &left_value)?;
formatter.with_label("file_header", |formatter| {
writeln!(formatter, "diff --git a/{} b/{}", path_string, path_string)?;
writeln!(formatter, "deleted file mode {}", &left_part.mode)?;
writeln!(formatter, "index {}..0000000000", &left_part.hash)?;
writeln!(formatter, "--- a/{}", path_string)?;
writeln!(formatter, "+++ /dev/null")?;
formatter.remove_label()?;
writeln!(formatter, "+++ /dev/null")
})?;
show_unified_diff_hunks(formatter, &left_part.content, &[])?;
}
}
@ -1874,40 +1875,40 @@ fn show_diff_summary(
workspace_command: &WorkspaceCommandHelper,
tree_diff: TreeDiffIterator,
) -> io::Result<()> {
formatter.add_label("diff")?;
formatter.with_label("diff", |formatter| {
for (repo_path, diff) in tree_diff {
match diff {
tree::Diff::Modified(_, _) => {
formatter.add_label("modified")?;
formatter.with_label("modified", |formatter| {
writeln!(
formatter,
"M {}",
workspace_command.format_file_path(&repo_path)
)?;
formatter.remove_label()?;
)
})?;
}
tree::Diff::Added(_) => {
formatter.add_label("added")?;
formatter.with_label("added", |formatter| {
writeln!(
formatter,
"A {}",
workspace_command.format_file_path(&repo_path)
)?;
formatter.remove_label()?;
)
})?;
}
tree::Diff::Removed(_) => {
formatter.add_label("removed")?;
formatter.with_label("removed", |formatter| {
writeln!(
formatter,
"R {}",
workspace_command.format_file_path(&repo_path)
)?;
formatter.remove_label()?;
)
})?;
}
}
}
formatter.remove_label()?;
Ok(())
})
}
fn cmd_status(
@ -1964,14 +1965,12 @@ fn cmd_status(
}
}
if !conflicted_local_branches.is_empty() {
formatter.add_label("conflict")?;
writeln!(formatter, "These branches have conflicts:")?;
formatter.remove_label()?;
formatter.with_label("conflict", |formatter| {
writeln!(formatter, "These branches have conflicts:")
})?;
for branch_name in conflicted_local_branches {
write!(formatter, " ")?;
formatter.add_label("branch")?;
write!(formatter, "{}", branch_name)?;
formatter.remove_label()?;
formatter.with_label("branch", |formatter| write!(formatter, "{}", branch_name))?;
writeln!(formatter)?;
}
writeln!(
@ -1981,14 +1980,14 @@ fn cmd_status(
)?;
}
if !conflicted_remote_branches.is_empty() {
formatter.add_label("conflict")?;
writeln!(formatter, "These remote branches have conflicts:")?;
formatter.remove_label()?;
formatter.with_label("conflict", |formatter| {
writeln!(formatter, "These remote branches have conflicts:")
})?;
for (branch_name, remote_name) in conflicted_remote_branches {
write!(formatter, " ")?;
formatter.add_label("branch")?;
write!(formatter, "{}@{}", branch_name, remote_name)?;
formatter.remove_label()?;
formatter.with_label("branch", |formatter| {
write!(formatter, "{}@{}", branch_name, remote_name)
})?;
writeln!(formatter)?;
}
writeln!(
@ -2013,9 +2012,9 @@ fn cmd_status(
let conflicts = tree.conflicts();
if !conflicts.is_empty() {
formatter.add_label("conflict")?;
writeln!(formatter, "There are unresolved conflicts at these paths:")?;
formatter.remove_label()?;
formatter.with_label("conflict", |formatter| {
writeln!(formatter, "There are unresolved conflicts at these paths:")
})?;
for (path, _) in conflicts {
writeln!(formatter, "{}", &workspace_command.format_file_path(&path))?;
}
@ -2126,11 +2125,11 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C
{
let mut formatter = ui.new_formatter(&mut buffer);
if is_checkout {
formatter.add_label("working_copy")?;
}
formatter.with_label("working_copy", |formatter| {
template.format(&commit, formatter)
})?;
} else {
template.format(&commit, formatter.as_mut())?;
if is_checkout {
formatter.remove_label()?;
}
}
if !buffer.ends_with(b"\n") {
@ -3411,8 +3410,9 @@ fn list_branches(
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> {
let print_branch_target = |formatter: &mut dyn Formatter,
target: Option<&RefTarget>|
-> Result<(), CommandError> {
match target {
Some(RefTarget::Normal(id)) => {
write!(formatter, ": ")?;
@ -3428,9 +3428,7 @@ fn list_branches(
}
Some(RefTarget::Conflict { adds, removes }) => {
write!(formatter, " ")?;
formatter.add_label("conflict")?;
write!(formatter, "(conflicted)")?;
formatter.remove_label()?;
formatter.with_label("conflict", |formatter| write!(formatter, "(conflicted)"))?;
writeln!(formatter, ":")?;
for id in removes {
let commit = repo.store().get_commit(id)?;
@ -3468,9 +3466,7 @@ fn list_branches(
let formatter = formatter.as_mut();
let index = repo.index();
for (name, branch_target) in repo.view().branches() {
formatter.add_label("branch")?;
write!(formatter, "{}", name)?;
formatter.remove_label()?;
formatter.with_label("branch", |formatter| write!(formatter, "{}", name))?;
print_branch_target(formatter, branch_target.local_target.as_ref())?;
for (remote, remote_target) in branch_target
@ -3482,9 +3478,7 @@ fn list_branches(
continue;
}
write!(formatter, " ")?;
formatter.add_label("branch")?;
write!(formatter, "@{}", remote)?;
formatter.remove_label()?;
formatter.with_label("branch", |formatter| write!(formatter, "@{}", remote))?;
if let Some(local_target) = branch_target.local_target.as_ref() {
let remote_ahead_count = index
.walk_revs(&remote_target.adds(), &local_target.adds())
@ -3617,30 +3611,28 @@ fn cmd_op_log(
impl Template<Operation> for OpTemplate {
fn format(&self, op: &Operation, formatter: &mut dyn Formatter) -> io::Result<()> {
// TODO: Make this templated
formatter.add_label("id")?;
formatter.write_str(&op.id().hex()[0..12])?;
formatter.remove_label()?;
formatter.with_label("id", |formatter| formatter.write_str(&op.id().hex()[0..12]))?;
formatter.write_str(" ")?;
let metadata = &op.store_operation().metadata;
formatter.add_label("user")?;
formatter.write_str(&format!("{}@{}", metadata.username, metadata.hostname))?;
formatter.remove_label()?;
formatter.with_label("user", |formatter| {
formatter.write_str(&format!("{}@{}", metadata.username, metadata.hostname))
})?;
formatter.write_str(" ")?;
formatter.add_label("time")?;
formatter.with_label("time", |formatter| {
formatter.write_str(&format!(
"{} - {}",
format_timestamp(&metadata.start_time),
format_timestamp(&metadata.end_time)
))?;
formatter.remove_label()?;
))
})?;
formatter.write_str("\n")?;
formatter.add_label("description")?;
formatter.write_str(&metadata.description)?;
formatter.remove_label()?;
formatter.with_label("description", |formatter| {
formatter.write_str(&metadata.description)
})?;
for (key, value) in &metadata.tags {
formatter.add_label("tags")?;
formatter.write_str(&format!("\n{}: {}", key, value))?;
formatter.remove_label()?;
formatter.with_label("tags", |formatter| {
formatter.write_str(&format!("\n{}: {}", key, value))
})?;
}
Ok(())
}
@ -3661,15 +3653,13 @@ fn cmd_op_log(
let mut buffer = vec![];
{
let mut formatter = ui.new_formatter(&mut buffer);
formatter.add_label("op-log")?;
formatter.with_label("op-log", |formatter| {
if is_head_op {
formatter.add_label("head")?;
formatter.with_label("head", |formatter| template.format(&op, formatter))
} else {
template.format(&op, formatter)
}
template.format(&op, formatter.as_mut())?;
if is_head_op {
formatter.remove_label()?;
}
formatter.remove_label()?;
})?;
}
if !buffer.ends_with(b"\n") {
buffer.push(b'\n');

13
src/formatter.rs
View file

@ -40,6 +40,19 @@ pub trait Formatter: Write {
fn remove_label(&mut self) -> io::Result<()>;
}
impl dyn Formatter + '_ {
pub fn with_label(
&mut self,
label: &str,
write_inner: impl FnOnce(&mut dyn Formatter) -> io::Result<()>,
) -> io::Result<()> {
self.add_label(label)?;
// Call `remove_label()` whether or not `write_inner()` fails, but don't let
// its error replace the one from `write_inner()`.
write_inner(self).and(self.remove_label())
}
}
/// Creates `Formatter` instances with preconfigured parameters.
#[derive(Clone, Debug)]
pub struct FormatterFactory {