From f614c96383930583efa53618b1f2e50ae58d555d Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 12 May 2024 11:24:50 +0900 Subject: [PATCH] diff_util: remove CommandError dependency from show functions Suppose we add commit.diff() template method, some of these show_*() functions will be called from there. CommandError shouldn't appear in that layer. --- cli/src/command_error.rs | 15 ++++++----- cli/src/commands/interdiff.rs | 3 ++- cli/src/commands/obslog.rs | 3 ++- cli/src/diff_util.rs | 47 +++++++++++++++++++---------------- 4 files changed, 39 insertions(+), 29 deletions(-) diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 7fb674b3b..6b0a826b4 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -36,10 +36,9 @@ use jj_lib::working_copy::{ResetError, SnapshotError, WorkingCopyStateError}; use jj_lib::workspace::WorkspaceInitError; use thiserror::Error; +use crate::diff_util::DiffRenderError; use crate::formatter::{FormatRecorder, Formatter}; -use crate::merge_tools::{ - ConflictResolveError, DiffEditError, DiffGenerateError, MergeToolConfigError, -}; +use crate::merge_tools::{ConflictResolveError, DiffEditError, MergeToolConfigError}; use crate::revset_util::UserRevsetEvaluationError; use crate::template_parser::{TemplateParseError, TemplateParseErrorKind}; use crate::ui::Ui; @@ -361,9 +360,13 @@ impl From for CommandError { } } -impl From for CommandError { - fn from(err: DiffGenerateError) -> Self { - user_error_with_message("Failed to generate diff", err) +impl From for CommandError { + fn from(err: DiffRenderError) -> Self { + match err { + DiffRenderError::DiffGenerate(_) => user_error(err), + DiffRenderError::Backend(err) => err.into(), + DiffRenderError::Io(err) => err.into(), + } } } diff --git a/cli/src/commands/interdiff.rs b/cli/src/commands/interdiff.rs index 1137656a3..b08b9af99 100644 --- a/cli/src/commands/interdiff.rs +++ b/cli/src/commands/interdiff.rs @@ -68,5 +68,6 @@ pub(crate) fn cmd_interdiff( &to_tree, matcher.as_ref(), &diff_formats, - ) + )?; + Ok(()) } diff --git a/cli/src/commands/obslog.rs b/cli/src/commands/obslog.rs index 11fc8f571..d677395b0 100644 --- a/cli/src/commands/obslog.rs +++ b/cli/src/commands/obslog.rs @@ -181,5 +181,6 @@ fn show_predecessor_patch( &tree, &EverythingMatcher, diff_formats, - ) + )?; + Ok(()) } diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 3baa33fb0..3d4cd2e3b 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -19,7 +19,7 @@ use std::ops::Range; use futures::{try_join, Stream, StreamExt}; use itertools::Itertools; -use jj_lib::backend::{BackendResult, TreeValue}; +use jj_lib::backend::{BackendError, BackendResult, TreeValue}; use jj_lib::commit::Commit; use jj_lib::conflicts::{materialize_tree_value, MaterializedTreeValue}; use jj_lib::diff::{Diff, DiffHunk}; @@ -34,14 +34,14 @@ use jj_lib::settings::{ConfigResultExt as _, UserSettings}; use jj_lib::store::Store; use jj_lib::{diff, files}; use pollster::FutureExt; +use thiserror::Error; use tracing::instrument; use unicode_width::UnicodeWidthStr as _; use crate::cli_util::WorkspaceCommandHelper; -use crate::command_error::CommandError; use crate::config::CommandNameAndArgs; use crate::formatter::Formatter; -use crate::merge_tools::{self, ExternalMergeTool}; +use crate::merge_tools::{self, DiffGenerateError, ExternalMergeTool}; use crate::text_util; use crate::ui::Ui; @@ -190,6 +190,16 @@ fn default_diff_format( } } +#[derive(Debug, Error)] +pub enum DiffRenderError { + #[error("Failed to generate diff")] + DiffGenerate(#[source] DiffGenerateError), + #[error(transparent)] + Backend(#[from] BackendError), + #[error(transparent)] + Io(#[from] io::Error), +} + pub fn show_diff( ui: &Ui, formatter: &mut dyn Formatter, @@ -198,7 +208,7 @@ pub fn show_diff( to_tree: &MergedTree, matcher: &dyn Matcher, formats: &[DiffFormat], -) -> Result<(), CommandError> { +) -> Result<(), DiffRenderError> { for format in formats { match format { DiffFormat::Summary => { @@ -222,7 +232,8 @@ pub fn show_diff( show_color_words_diff(formatter, workspace_command, *context, tree_diff)?; } DiffFormat::Tool(tool) => { - merge_tools::generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool)?; + merge_tools::generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool) + .map_err(DiffRenderError::DiffGenerate)?; } } } @@ -236,7 +247,7 @@ pub fn show_patch( commit: &Commit, matcher: &dyn Matcher, formats: &[DiffFormat], -) -> Result<(), CommandError> { +) -> Result<(), DiffRenderError> { let from_tree = commit.parent_tree(workspace_command.repo().as_ref())?; let to_tree = commit.tree()?; show_diff( @@ -400,10 +411,7 @@ fn file_content_for_diff(reader: &mut dyn io::Read) -> io::Result { }) } -fn diff_content( - path: &RepoPath, - value: MaterializedTreeValue, -) -> Result { +fn diff_content(path: &RepoPath, value: MaterializedTreeValue) -> io::Result { match value { MaterializedTreeValue::Absent => Ok(FileContent::empty()), MaterializedTreeValue::File { mut reader, .. } => { @@ -453,7 +461,7 @@ pub fn show_color_words_diff( workspace_command: &WorkspaceCommandHelper, num_context_lines: usize, tree_diff: TreeDiffStream, -) -> Result<(), CommandError> { +) -> Result<(), DiffRenderError> { formatter.push_label("diff")?; let mut diff_stream = materialized_diff_stream(workspace_command.repo().store(), tree_diff); async { @@ -561,7 +569,7 @@ pub fn show_color_words_diff( } } } - Ok::<(), CommandError>(()) + Ok::<(), DiffRenderError>(()) } .block_on()?; formatter.pop_label()?; @@ -574,10 +582,7 @@ struct GitDiffPart { content: Vec, } -fn git_diff_part( - path: &RepoPath, - value: MaterializedTreeValue, -) -> Result { +fn git_diff_part(path: &RepoPath, value: MaterializedTreeValue) -> io::Result { let mode; let hash; let mut contents: Vec; @@ -729,7 +734,7 @@ fn show_unified_diff_hunks( left_content: &[u8], right_content: &[u8], num_context_lines: usize, -) -> Result<(), CommandError> { +) -> io::Result<()> { for hunk in unified_diff_hunks(left_content, right_content, num_context_lines) { writeln!( formatter.labeled("hunk_header"), @@ -797,7 +802,7 @@ pub fn show_git_diff( workspace_command: &WorkspaceCommandHelper, num_context_lines: usize, tree_diff: TreeDiffStream, -) -> Result<(), CommandError> { +) -> Result<(), DiffRenderError> { formatter.push_label("diff")?; let mut diff_stream = materialized_diff_stream(workspace_command.repo().store(), tree_diff); @@ -857,7 +862,7 @@ pub fn show_git_diff( show_unified_diff_hunks(formatter, &left_part.content, &[], num_context_lines)?; } } - Ok::<(), CommandError>(()) + Ok::<(), DiffRenderError>(()) } .block_on()?; formatter.pop_label()?; @@ -938,7 +943,7 @@ pub fn show_diff_stat( formatter: &mut dyn Formatter, workspace_command: &WorkspaceCommandHelper, tree_diff: TreeDiffStream, -) -> Result<(), CommandError> { +) -> Result<(), DiffRenderError> { let mut stats: Vec = vec![]; let mut max_path_width = 0; let mut max_diffs = 0; @@ -955,7 +960,7 @@ pub fn show_diff_stat( max_diffs = max(max_diffs, stat.added + stat.removed); stats.push(stat); } - Ok::<(), CommandError>(()) + Ok::<(), DiffRenderError>(()) } .block_on()?;